Less aggressive orphaned resource cleanup (#236)

It's unlikely but possible that namespace deletion hits the informer cache before CR deletion. So we either need to make non-caching calls before recreating the namespace of orphaned resources, or just wait for informers to catch up. This is already such a rare case, I think the risk of making apiserver calls is unnecessary and just waiting on the informers is sufficient. Co-authored-by: Jordan Olshevski <[email protected]>
Azure · Oct 24, 2024 · 6f9e27b · 6f9e27b
1 parent 6f93932
commit 6f9e27b
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 20 deletions.
diff --git a/cmd/eno-reconciler/main.go b/cmd/eno-reconciler/main.go
@@ -89,7 +89,7 @@ func run() error {
 	}
 
 	if namespaceCleanup {
-		err = liveness.NewNamespaceController(mgr, namespaceCreationGracePeriod)
+		err = liveness.NewNamespaceController(mgr, 5, namespaceCreationGracePeriod)
 		if err != nil {
 			return fmt.Errorf("constructing namespace liveness controller: %w", err)
 		}

diff --git a/internal/controllers/liveness/namespace.go b/internal/controllers/liveness/namespace.go
@@ -23,11 +23,12 @@ var orphanableKinds = []string{"Symphony", "Composition", "ResourceSlice"}
 // This can happen if clients get tricky with the /finalize API.
 // Without this controller Eno resources will never be deleted since updates to remove the finalizers will fail.
 type namespaceController struct {
-	client              client.Client
-	creationGracePeriod time.Duration
+	client                client.Client
+	creationGracePeriod   time.Duration
+	orphanCheckIterations int
 }
 
-func NewNamespaceController(mgr ctrl.Manager, creationGracePeriod time.Duration) error {
+func NewNamespaceController(mgr ctrl.Manager, checks int, creationGracePeriod time.Duration) error {
 	b := ctrl.NewControllerManagedBy(mgr).For(&corev1.Namespace{})
 
 	for _, kind := range orphanableKinds {
@@ -43,8 +44,9 @@ func NewNamespaceController(mgr ctrl.Manager, creationGracePeriod time.Duration)
 
 	return b.WithLogConstructor(manager.NewLogConstructor(mgr, "namespaceLivenessController")).
 		Complete(&namespaceController{
-			client:              mgr.GetClient(),
-			creationGracePeriod: creationGracePeriod,
+			client:                mgr.GetClient(),
+			creationGracePeriod:   creationGracePeriod,
+			orphanCheckIterations: checks,
 		})
 }
 
@@ -79,21 +81,29 @@ func (c *namespaceController) Reconcile(ctx context.Context, req ctrl.Request) (
 	}
 
 	// Avoid recreating the namespace when it doesn't have any orphaned resources
-	var foundOrphans bool
-	for _, kind := range orphanableKinds {
-		hasOrphans, res, err := c.findOrphans(ctx, ns.Name, kind)
-		if err != nil {
-			return ctrl.Result{}, err
+	for i := 1; true; i++ {
+		var foundOrphans bool
+		for _, kind := range orphanableKinds {
+			hasOrphans, res, err := c.findOrphans(ctx, ns.Name, kind)
+			if err != nil {
+				return ctrl.Result{}, err
+			}
+			if res != nil {
+				return *res, nil
+			}
+			if hasOrphans {
+				foundOrphans = true
+			}
 		}
-		if res != nil {
-			return *res, nil
+		if !foundOrphans {
+			return ctrl.Result{}, nil
 		}
-		if hasOrphans {
-			foundOrphans = true
+		if i >= c.orphanCheckIterations {
+			break
 		}
-	}
-	if !foundOrphans {
-		return ctrl.Result{}, nil
+
+		// Sleep a bit before the next check to let informers catch up.
+		time.Sleep(time.Second / 2)
 	}
 
 	// Recreate the namespace briefly so we can remove the finalizers.

diff --git a/internal/controllers/liveness/namespace_test.go b/internal/controllers/liveness/namespace_test.go
@@ -48,7 +48,7 @@ func testMissingNamespace(t *testing.T, orphan client.Object) {
 	mgr := testutil.NewManager(t, testutil.WithCompositionNamespace(ns.Name))
 	cli := mgr.GetClient()
 
-	require.NoError(t, NewNamespaceController(mgr.Manager, time.Second))
+	require.NoError(t, NewNamespaceController(mgr.Manager, 2, time.Second))
 	mgr.Start(t)
 
 	require.NoError(t, cli.Create(ctx, ns))

diff --git a/internal/controllers/reconciliation/helpers_test.go b/internal/controllers/reconciliation/helpers_test.go
@@ -30,7 +30,7 @@ func registerControllers(t *testing.T, mgr *testutil.Manager) {
 	require.NoError(t, rollout.NewController(mgr.Manager, time.Millisecond))
 	require.NoError(t, rollout.NewSynthesizerController(mgr.Manager))
 	require.NoError(t, flowcontrol.NewSynthesisConcurrencyLimiter(mgr.Manager, 10, 0))
-	require.NoError(t, liveness.NewNamespaceController(mgr.Manager, time.Second))
+	require.NoError(t, liveness.NewNamespaceController(mgr.Manager, 3, time.Second))
 	require.NoError(t, watch.NewController(mgr.Manager))
 }