fix: bug causing the virtual kubelet not to be deleted on unpeer

This patch fixes a bug that caused the VirtualKubelet not to be deleted
when the Node resource was missing. The deletion routine now always
removes the VK deployment and double checks whether there are Node
resources to delete them.

Additionally a check on the VK arguments on deletion has been removed,
as when the deployment is deleted we make sure that there are no more
pods running of that deployment, only once the deployment is deleted the
Node is deleted, so there is no risk of a VK pod to recreate the node
after its deletion.

Author: claudiolor

pkg/liqo-controller-manager/offloading/virtualnode-controller/deletion-routine.go

@@ -137,33 +137,11 @@ func (dr *DeletionRoutine) handle(ctx context.Context, key string) (err error) {
 				Status: offloadingv1beta1.DrainingConditionStatusType,
 			}})
 
-	var node *corev1.Node
-	node, err = getters.GetNodeFromVirtualNode(ctx, dr.vnr.Client, vn)
-	if client.IgnoreNotFound(err) != nil {
-		err = fmt.Errorf("error getting node: %w", err)
+	if err = dr.deleteVirtualKubelet(ctx, vn); err != nil {
+		err = fmt.Errorf("error deleting node and Virtual Kubelet: %w", err)
 		return err
 	}
 
-	if node != nil {
-		if !*vn.Spec.CreateNode {
-			// We need to ensure that the current pods will no recreate the node after deleting it.
-			var found bool
-			if found, err = vkutils.CheckVirtualKubeletFlagsConsistence(
-				ctx, dr.vnr.Client, vn, createNodeFalseFlag); err != nil || !found {
-				if err == nil {
-					err = fmt.Errorf("virtual kubelet pods are still running with arg %s", createNodeFalseFlag.String())
-					return err
-				}
-				err = fmt.Errorf("error checking virtual kubelet pods: %w", err)
-				return err
-			}
-		}
-		if err = dr.deleteNode(ctx, node, vn); err != nil {
-			err = fmt.Errorf("error deleting node: %w", err)
-			return err
-		}
-	}
-
 	if !vn.DeletionTimestamp.IsZero() {
 		// VirtualNode resource is being deleted.
 		if err = dr.vnr.ensureNamespaceMapAbsence(ctx, vn); err != nil {
@@ -188,19 +166,28 @@ func (dr *DeletionRoutine) handle(ctx context.Context, key string) (err error) {
 	return nil
 }
 
-// deleteNode deletes the Node created by VirtualNode.
-func (dr *DeletionRoutine) deleteNode(ctx context.Context, node *corev1.Node, vn *offloadingv1beta1.VirtualNode) error {
-	if err := cordonNode(ctx, dr.vnr.Client, node); err != nil {
-		return fmt.Errorf("error cordoning node: %w", err)
+// deleteVirtualKubelet deletes the Node and the VirtualKubelet deployment related to the given VirtualNode.
+func (dr *DeletionRoutine) deleteVirtualKubelet(ctx context.Context, vn *offloadingv1beta1.VirtualNode) error {
+	// Check if the Node resource exists to make sure that we are not in a case in which it should not exist.
+	node, err := getters.GetNodeFromVirtualNode(ctx, dr.vnr.Client, vn)
+	if client.IgnoreNotFound(err) != nil {
+		err = fmt.Errorf("error getting node: %w", err)
+		return err
 	}
 
-	klog.Infof("Node %s cordoned", node.Name)
+	if node != nil {
+		if err := cordonNode(ctx, dr.vnr.Client, node); err != nil {
+			return fmt.Errorf("error cordoning node: %w", err)
+		}
 
-	if err := client.IgnoreNotFound(drainNode(ctx, dr.vnr.Client, vn)); err != nil {
-		return fmt.Errorf("error draining node: %w", err)
-	}
+		klog.Infof("Node %s cordoned", node.Name)
 
-	klog.Infof("Node %s drained", node.Name)
+		if err := client.IgnoreNotFound(drainNode(ctx, dr.vnr.Client, vn)); err != nil {
+			return fmt.Errorf("error draining node: %w", err)
+		}
+
+		klog.Infof("Node %s drained", node.Name)
+	}
 
 	if !vn.DeletionTimestamp.IsZero() {
 		ForgeCondition(vn,
@@ -214,18 +201,38 @@ func (dr *DeletionRoutine) deleteNode(ctx context.Context, node *corev1.Node, vn
 			return fmt.Errorf("error deleting virtual kubelet deployment: %w", err)
 		}
 	}
+
+	// Even node is nil we make sure that no Node resource has been created before the deletion of the VK deployment.
 	klog.Infof("VirtualKubelet deployment %s deleted", vn.Name)
 
+	var nodeToDelete *corev1.Node
+
+	if node != nil {
+		nodeToDelete = node
+	} else {
+		nodeToDelete, err = getters.GetNodeFromVirtualNode(ctx, dr.vnr.Client, vn)
+		if client.IgnoreNotFound(err) != nil {
+			err = fmt.Errorf("error getting node before deletion: %w", err)
+			return err
+		}
+	}
+
 	ForgeCondition(vn,
 		VnConditionMap{
 			offloadingv1beta1.NodeConditionType: VnCondition{
 				Status: offloadingv1beta1.DeletingConditionStatusType,
 			},
 		})
-	if err := client.IgnoreNotFound(dr.vnr.Client.Delete(ctx, node, &client.DeleteOptions{})); err != nil {
-		return fmt.Errorf("error deleting node: %w", err)
+
+	if nodeToDelete != nil {
+		if err := client.IgnoreNotFound(dr.vnr.Client.Delete(ctx, nodeToDelete, &client.DeleteOptions{})); err != nil {
+			return fmt.Errorf("error deleting node: %w", err)
+		}
+
+		klog.Infof("Node %s deleted", node.Name)
+	} else {
+		klog.Infof("Node of VirtualNode %s already deleted", vn.Name)
 	}
 
-	klog.Infof("Node %s deleted", node.Name)
 	return nil
 }

pkg/vkMachinery/utils/utils.go

@@ -17,15 +17,13 @@ package utils
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	appsv1 "k8s.io/api/apps/v1"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	offloadingv1beta1 "github.com/liqotech/liqo/apis/offloading/v1beta1"
 	"github.com/liqotech/liqo/pkg/utils/getters"
-	"github.com/liqotech/liqo/pkg/vkMachinery"
 	vkforge "github.com/liqotech/liqo/pkg/vkMachinery/forge"
 )
 
@@ -76,32 +74,3 @@ type Flag struct {
 func (f Flag) String() string {
 	return fmt.Sprintf("%s=%s", f.Name, f.Value)
 }
-
-// CheckVirtualKubeletFlagsConsistence checks if the VirtualKubelet args are consistent with the flag list provided.
-// It returns true if all the flags are consistent, false otherwise.
-// A flag is not consistent if it is present in the VirtualKubelet args with a different value.
-func CheckVirtualKubeletFlagsConsistence(
-	ctx context.Context, cl client.Client, vn *offloadingv1beta1.VirtualNode, flags ...Flag) (bool, error) {
-	list, err := getters.ListVirtualKubeletPodsFromVirtualNode(ctx, cl, vn)
-	if err != nil {
-		return false, err
-	}
-	for i := range list.Items {
-		for j := range list.Items[i].Spec.Containers {
-			if list.Items[i].Spec.Containers[j].Name != vkMachinery.ContainerName {
-				continue
-			}
-			for _, arg := range list.Items[i].Spec.Containers[j].Args {
-				for _, flag := range flags {
-					if strings.HasPrefix(arg, flag.Name) {
-						if flag.String() != arg {
-							return false, nil
-						}
-						break
-					}
-				}
-			}
-		}
-	}
-	return true, nil
-}