Repository: submariner-io/submariner-operator
Pull request #1848 (2022-02-16 02:11:32)

Ensure uninstall components aren't recreated
If one component completes before another or one times out and is deleted, ensure they are not recreated on the next reconcile loop.



submariner-bot: 🤖 Created branch: z_pr1848/tpantelis/uninstall_recreate_issue 
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

Patch:
diff --git a/controllers/uninstall/uninstall.go b/controllers/uninstall/uninstall.go
index de1cf4c3..de16a3bc 100644
--- a/controllers/uninstall/uninstall.go
+++ b/controllers/uninstall/uninstall.go
@@ -58,6 +58,15 @@ func (c *Component) isInstalled() bool {
 }
 
 func (i *Info) Run(ctx context.Context) (bool, error) {
+	timedOut := time.Since(i.StartTime) >= ComponentReadyTimeout
+	if timedOut {
+		i.Log.Info("Timed out waiting for components to complete - aborting")
+
+		i.cleanup(ctx)
+
+		return false, nil
+	}
+
 	// First, delete the regular DaemonSets/Deployments.
 	for _, c := range i.Components {
 		if !c.isInstalled() {
@@ -71,9 +80,9 @@ func (i *Info) Run(ctx context.Context) (bool, error) {
 	}
 
 	// Next, ensure all their pods are cleaned up.
-	requeue, err := i.ensurePodsDeleted(ctx)
-	if requeue || err != nil {
-		return requeue, err
+	podsIncomplete, err := i.ensurePodsDeleted(ctx)
+	if err != nil {
+		return false, err
 	}
 
 	err = i.createUninstallResources(ctx)
@@ -81,7 +90,18 @@ func (i *Info) Run(ctx context.Context) (bool, error) {
 		return false, err
 	}
 
-	return i.ensureUninstallResourcesComplete(ctx)
+	uninstallIncomplete, err := i.ensureUninstallResourcesComplete(ctx)
+	if err != nil {
+		return false, err
+	}
+
+	if podsIncomplete || uninstallIncomplete {
+		return true, nil
+	}
+
+	i.cleanup(ctx)
+
+	return false, nil
 }
 
 func (i *Info) ensureDeleted(ctx context.Context, obj client.Object) error {
@@ -98,6 +118,8 @@ func (i *Info) ensureDeleted(ctx context.Context, obj client.Object) error {
 }
 
 func (i *Info) ensurePodsDeleted(ctx context.Context) (bool, error) {
+	requeue := false
+
 	for _, c := range i.Components {
 		if !c.isInstalled() {
 			continue
@@ -114,11 +136,12 @@ func (i *Info) ensurePodsDeleted(ctx context.Context) (bool, error) {
 		if numPods > 0 {
 			i.Log.Info(fmt.Sprintf("%T still has pods - requeueing: ", c.Resource), "name", c.Resource.GetName(),
 				"namespace", c.Resource.GetNamespace(), "numPods", numPods)
-			return true, nil
+
+			requeue = true
 		}
 	}
 
-	return false, nil
+	return requeue, nil
 }
 
 func (i *Info) createUninstallResources(ctx context.Context) error {
@@ -178,7 +201,7 @@ func (i *Info) createUninstallDaemonSetFrom(ctx context.Context, daemonSet *apps
 }
 
 func (i *Info) ensureUninstallResourcesComplete(ctx context.Context) (bool, error) {
-	timedOut := time.Since(i.StartTime) >= ComponentReadyTimeout
+	anyRequeue := false
 
 	for _, c := range i.Components {
 		if !c.isInstalled() {
@@ -190,9 +213,9 @@ func (i *Info) ensureUninstallResourcesComplete(ctx context.Context) (bool, erro
 
 		switch d := c.UninstallResource.(type) {
 		case *appsv1.DaemonSet:
-			requeue, err = i.ensureDaemonSetReady(ctx, client.ObjectKeyFromObject(d), timedOut)
+			requeue, err = i.ensureDaemonSetReady(ctx, client.ObjectKeyFromObject(d))
 		case *appsv1.Deployment:
-			requeue, err = i.ensureDeploymentReady(ctx, client.ObjectKeyFromObject(d), timedOut)
+			requeue, err = i.ensureDeploymentReady(ctx, client.ObjectKeyFromObject(d))
 		default:
 			panic(fmt.Sprintf("Unknown type: %T", d))
 		}
@@ -201,15 +224,13 @@ func (i *Info) ensureUninstallResourcesComplete(ctx context.Context) (bool, erro
 			return false, err
 		}
 
-		if requeue {
-			return true, nil
-		}
+		anyRequeue = anyRequeue || requeue
 	}
 
-	return false, nil
+	return anyRequeue, nil
 }
 
-func (i *Info) ensureDaemonSetReady(ctx context.Context, key client.ObjectKey, timedOut bool) (bool, error) {
+func (i *Info) ensureDaemonSetReady(ctx context.Context, key client.ObjectKey) (bool, error) {
 	daemonSet := &appsv1.DaemonSet{}
 
 	err := i.Client.Get(ctx, key, daemonSet)
@@ -228,20 +249,15 @@ func (i *Info) ensureDaemonSetReady(ctx context.Context, key client.ObjectKey, t
 	} else if daemonSet.Status.DesiredNumberScheduled != daemonSet.Status.NumberReady {
 		i.Log.Info("DaemonSet not ready yet:", "name", daemonSet.Name, "namespace", daemonSet.Namespace,
 			"DesiredNumberScheduled", daemonSet.Status.DesiredNumberScheduled, "NumberReady", daemonSet.Status.NumberReady)
-
-		if !timedOut {
-			return true, nil
-		}
-
-		i.Log.Info("DaemonSet timed out - deleting:", "name", daemonSet.Name, "namespace", daemonSet.Namespace)
+		return true, nil
 	} else {
 		i.Log.Info("DaemonSet is ready:", "name", daemonSet.Name, "namespace", daemonSet.Namespace)
 	}
 
-	return false, i.ensureDeleted(ctx, daemonSet)
+	return false, nil
 }
 
-func (i *Info) ensureDeploymentReady(ctx context.Context, key client.ObjectKey, timedOut bool) (bool, error) {
+func (i *Info) ensureDeploymentReady(ctx context.Context, key client.ObjectKey) (bool, error) {
 	deployment := &appsv1.Deployment{}
 
 	err := i.Client.Get(ctx, key, deployment)
@@ -257,17 +273,22 @@ func (i *Info) ensureDeploymentReady(ctx context.Context, key client.ObjectKey,
 	if deployment.Status.AvailableReplicas != replicas {
 		i.Log.Info("Deployment not ready yet:", "name", deployment.Name, "namespace", deployment.Namespace,
 			"AvailableReplicas", deployment.Status.AvailableReplicas, "DesiredReplicas", replicas)
-
-		if !timedOut {
-			return true, nil
-		}
-
-		i.Log.Info("Deployment timed out - deleting:", "name", deployment.Name, "namespace", deployment.Namespace)
+		return true, nil
 	}
 
 	i.Log.Info("Deployment is ready:", "name", deployment.Name, "namespace", deployment.Namespace)
 
-	return false, i.ensureDeleted(ctx, deployment)
+	return false, nil
+}
+
+func (i *Info) cleanup(ctx context.Context) {
+	for _, c := range i.Components {
+		err := i.ensureDeleted(ctx, c.UninstallResource)
+		if err != nil {
+			i.Log.Error(err, "Unable to delete uninstall resource", "name", c.UninstallResource.GetName(),
+				"namespace", c.UninstallResource.GetNamespace())
+		}
+	}
 }
 
 func convertPodSpecContainersToUninstall(podSpec *corev1.PodSpec) {


Test patch:
diff --git a/controllers/submariner/submariner_controller_test.go b/controllers/submariner/submariner_controller_test.go
index bd637c8a..1d52926d 100644
--- a/controllers/submariner/submariner_controller_test.go
+++ b/controllers/submariner/submariner_controller_test.go
@@ -277,19 +277,18 @@ func testDeletion() {
 			t.AssertNoDaemonSet(names.GlobalnetComponent)
 			t.AssertNoDeployment(names.NetworkPluginSyncerComponent)
 
-			// Simulate the DaemonSet controller cleaning up its pods.
+			// Simulate the gateway DaemonSet controller cleaning up its pods.
 			t.DeletePods("app", names.GatewayComponent)
 
 			// Next, the controller should create the corresponding uninstall DaemonSets/Deployments.
 			t.AssertReconcileRequeue()
 
-			// For the gateway DaemonSet, we'll only update it to nodes available but not yet ready at this point.
-			gatewayDS := t.assertUninstallGatewayDaemonSet()
-			t.UpdateDaemonSetToScheduled(gatewayDS)
-
-			// For the globalnet DaemonSet, we'll update it to observed but no nodes available - this will cause it to be deleted.
+			// For the globalnet DaemonSet, we'll only update it to nodes available but not yet ready at this point.
 			globalnetDS := t.assertUninstallGlobalnetDaemonSet()
-			t.UpdateDaemonSetToObserved(globalnetDS)
+			t.UpdateDaemonSetToScheduled(globalnetDS)
+
+			// For the gateway DaemonSet, we'll update it to observed but no nodes available - this will cause it to be deleted.
+			t.UpdateDaemonSetToObserved(t.assertUninstallGatewayDaemonSet())
 
 			t.UpdateDaemonSetToReady(t.assertUninstallRouteAgentDaemonSet())
 			t.UpdateDeploymentToReady(t.assertUninstallNetworkPluginSyncerDeployment())
@@ -297,8 +296,8 @@ func testDeletion() {
 			// Next, the controller should again requeue b/c the gateway DaemonSet isn't ready yet.
 			t.AssertReconcileRequeue()
 
-			// Now update the gateway DaemonSet to ready.
-			t.UpdateDaemonSetToReady(gatewayDS)
+			// Now update the globalnet DaemonSet to ready.
+			t.UpdateDaemonSetToReady(globalnetDS)
 
 			// Ensure the finalizer is still present.
 			t.awaitFinalizer()