From b3928aaa3efaec3393a6f54795e4c3b7df7c86af Mon Sep 17 00:00:00 2001
From: Claudio Lorina <claudio.lorina@gmail.com>
Date: Tue, 4 Feb 2025 18:31:57 +0100
Subject: [PATCH] fix deletion routine

---
 .../deletion-routine.go                       | 62 ++++++++++++++-----
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/pkg/liqo-controller-manager/offloading/virtualnode-controller/deletion-routine.go b/pkg/liqo-controller-manager/offloading/virtualnode-controller/deletion-routine.go
index 2c9135479c..b9114cbd17 100644
--- a/pkg/liqo-controller-manager/offloading/virtualnode-controller/deletion-routine.go
+++ b/pkg/liqo-controller-manager/offloading/virtualnode-controller/deletion-routine.go
@@ -137,6 +137,7 @@ func (dr *DeletionRoutine) handle(ctx context.Context, key string) (err error) {
 				Status: offloadingv1beta1.DrainingConditionStatusType,
 			}})
 
+	// Check if the Node resource exists to make sure that we are not in a case in which it should not exist.
 	var node *corev1.Node
 	node, err = getters.GetNodeFromVirtualNode(ctx, dr.vnr.Client, vn)
 	if client.IgnoreNotFound(err) != nil {
@@ -146,7 +147,10 @@ func (dr *DeletionRoutine) handle(ctx context.Context, key string) (err error) {
 
 	if node != nil {
 		if !*vn.Spec.CreateNode {
-			// We need to ensure that the current pods will no recreate the node after deleting it.
+			// There might be some cases in which a Node exists even when the creation of a Node resource is disabled in the VirtualNode.
+			// This could happen when this option has been recently changed and some new pods are replacing the ones with the old configuration.
+			// So we wait for the new pods to replace all the old pods before actually deleting the Node resource, otherwise, if we delete the Node
+			// the old pods might recreate them.
 			var found bool
 			if found, err = vkutils.CheckVirtualKubeletFlagsConsistence(
 				ctx, dr.vnr.Client, vn, createNodeFalseFlag); err != nil || !found {
@@ -158,10 +162,11 @@ func (dr *DeletionRoutine) handle(ctx context.Context, key string) (err error) {
 				return err
 			}
 		}
-		if err = dr.deleteNode(ctx, node, vn); err != nil {
-			err = fmt.Errorf("error deleting node: %w", err)
-			return err
-		}
+	}
+
+	if err = dr.deleteNode(ctx, node, vn); err != nil {
+		err = fmt.Errorf("error deleting node and its subresources: %w", err)
+		return err
 	}
 
 	if !vn.DeletionTimestamp.IsZero() {
@@ -188,19 +193,21 @@ func (dr *DeletionRoutine) handle(ctx context.Context, key string) (err error) {
 	return nil
 }
 
-// deleteNode deletes the Node created by VirtualNode.
+// deleteNode deletes the Node created by VirtualNode. If node is nill it just tries to remove the VirtualKubelet deployment.
 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)
-	}
+	if node != nil {
+		if err := cordonNode(ctx, dr.vnr.Client, node); err != nil {
+			return fmt.Errorf("error cordoning node: %w", err)
+		}
 
-	klog.Infof("Node %s cordoned", node.Name)
+		klog.Infof("Node %s cordoned", node.Name)
 
-	if err := client.IgnoreNotFound(drainNode(ctx, dr.vnr.Client, vn)); err != nil {
-		return fmt.Errorf("error draining 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 drained", node.Name)
+		klog.Infof("Node %s drained", node.Name)
+	}
 
 	if !vn.DeletionTimestamp.IsZero() {
 		ForgeCondition(vn,
@@ -214,18 +221,39 @@ 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 {
+		var err error
+		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
 }