Fix ManagedClusterAddons not removed when ClusterManagementAddon is deleted

The addon template controller was stopping addon managers immediately when
ClusterManagementAddon was deleted, without waiting for pre-delete jobs
to complete or ManagedClusterAddons to be cleaned up via owner reference
cascading deletion.

This change implements the TODO at line 105 by checking if all
ManagedClusterAddons are deleted before stopping the manager. The controller
now uses field selectors to efficiently query for remaining ManagedClusterAddons
and requeues after 10 seconds if any still exist, allowing time for proper
cleanup.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Signed-off-by: zhujian <[email protected]>

Author: zhujian7

pkg/addon/controllers/addontemplate/controller.go

@@ -102,12 +102,33 @@ func NewAddonTemplateController(
 
 func (c *addonTemplateController) stopUnusedManagers(
 	ctx context.Context, syncCtx factory.SyncContext, addOnName string) {
-	// TODO: check if all managed cluster addon instances are deleted
+	logger := klog.FromContext(ctx)
+
+	// Check if all managed cluster addon instances are deleted before stopping the manager
+	// This allows pre-delete jobs to complete
+	listOptions := metav1.ListOptions{
+		FieldSelector: "metadata.name=" + addOnName,
+	}
+	mcaList, err := c.addonClient.AddonV1alpha1().ManagedClusterAddOns("").List(ctx, listOptions)
+	if err != nil {
+		logger.Error(err, "Failed to list ManagedClusterAddOns", "addonName", addOnName)
+		return
+	}
+
+	// Check if there are still ManagedClusterAddOns for this addon
+	if len(mcaList.Items) > 0 {
+		logger.Info("ManagedClusterAddOn still exists, waiting for deletion",
+			"addonName", addOnName, "count", len(mcaList.Items))
+		// Requeue to check again later
+		syncCtx.Queue().AddAfter(addOnName, 10*time.Second)
+		return
+	}
+
 	stopFunc, ok := c.addonManagers[addOnName]
 	if ok {
 		stopFunc()
 		delete(c.addonManagers, addOnName)
-		klog.FromContext(ctx).Info("Stopping the manager for addon", "addonName", addOnName)
+		logger.Info("Stopping the manager for addon", "addonName", addOnName)
 	}
 }
 

pkg/addon/controllers/addontemplate/controller_test.go

@@ -25,6 +25,7 @@ import (
 	workinformers "open-cluster-management.io/api/client/work/informers/externalversions"
 
 	testingcommon "open-cluster-management.io/ocm/pkg/common/testing"
+	testinghelpers "open-cluster-management.io/ocm/pkg/registration/helpers/testing"
 )
 
 func TestReconcile(t *testing.T) {
@@ -268,3 +269,123 @@ func TestRunController(t *testing.T) {
 		}
 	}
 }
+
+func TestStopUnusedManagers(t *testing.T) {
+	cases := []struct {
+		name                   string
+		addonName              string
+		managedClusterAddons   []runtime.Object
+		existingManagers       map[string]context.CancelFunc
+		expectedManagerStopped bool
+		expectedRequeue        bool
+	}{
+		{
+			name:                 "no managed cluster addons, manager should be stopped",
+			addonName:            "test-addon",
+			managedClusterAddons: []runtime.Object{},
+			existingManagers: map[string]context.CancelFunc{
+				"test-addon": func() {},
+			},
+			expectedManagerStopped: true,
+			expectedRequeue:        false,
+		},
+		{
+			name:      "managed cluster addons exist, manager should not be stopped",
+			addonName: "test-addon",
+			managedClusterAddons: []runtime.Object{
+				testinghelpers.NewManagedClusterAddons("test-addon", "cluster1", nil, nil),
+			},
+			existingManagers: map[string]context.CancelFunc{
+				"test-addon": func() {},
+			},
+			expectedManagerStopped: false,
+			expectedRequeue:        true,
+		},
+		{
+			name:      "multiple managed cluster addons exist, manager should not be stopped",
+			addonName: "test-addon",
+			managedClusterAddons: []runtime.Object{
+				testinghelpers.NewManagedClusterAddons("test-addon", "cluster1", nil, nil),
+				testinghelpers.NewManagedClusterAddons("test-addon", "cluster2", nil, nil),
+			},
+			existingManagers: map[string]context.CancelFunc{
+				"test-addon": func() {},
+			},
+			expectedManagerStopped: false,
+			expectedRequeue:        true,
+		},
+		{
+			name:                   "no manager exists, should not error",
+			addonName:              "test-addon",
+			managedClusterAddons:   []runtime.Object{},
+			existingManagers:       map[string]context.CancelFunc{},
+			expectedManagerStopped: false,
+			expectedRequeue:        false,
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			fakeAddonClient := fakeaddon.NewSimpleClientset(c.managedClusterAddons...)
+			addonInformers := addoninformers.NewSharedInformerFactory(fakeAddonClient, 10*time.Minute)
+
+			fakeDynamicClient := dynamicfake.NewSimpleDynamicClient(runtime.NewScheme())
+			dynamicInformerFactory := dynamicinformer.NewDynamicSharedInformerFactory(fakeDynamicClient, 0)
+			fakeClusterClient := fakecluster.NewSimpleClientset()
+			clusterInformers := clusterv1informers.NewSharedInformerFactory(fakeClusterClient, 10*time.Minute)
+			fakeWorkClient := fakework.NewSimpleClientset()
+			workInformers := workinformers.NewSharedInformerFactory(fakeWorkClient, 10*time.Minute)
+			hubKubeClient := fakekube.NewSimpleClientset()
+
+			managerStopped := false
+			existingManagers := make(map[string]context.CancelFunc)
+			for name, stopFunc := range c.existingManagers {
+				existingManagers[name] = func() {
+					if name == c.addonName {
+						managerStopped = true
+					}
+					stopFunc()
+				}
+			}
+
+			controller := &addonTemplateController{
+				kubeConfig:       &rest.Config{},
+				kubeClient:       hubKubeClient,
+				addonClient:      fakeAddonClient,
+				workClient:       fakeWorkClient,
+				cmaLister:        addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Lister(),
+				addonManagers:    existingManagers,
+				addonInformers:   addonInformers,
+				clusterInformers: clusterInformers,
+				dynamicInformers: dynamicInformerFactory,
+				workInformers:    workInformers,
+				eventRecorder:    eventstesting.NewTestingEventRecorder(t),
+			}
+
+			ctx := context.TODO()
+			syncContext := testingcommon.NewFakeSyncContext(t, c.addonName)
+
+			controller.stopUnusedManagers(ctx, syncContext, c.addonName)
+
+			// Check if manager was stopped
+			if c.expectedManagerStopped {
+				assert.True(t, managerStopped, "expected manager to be stopped")
+				_, exists := controller.addonManagers[c.addonName]
+				assert.False(t, exists, "expected manager to be removed from map")
+			} else {
+				assert.False(t, managerStopped, "expected manager not to be stopped")
+				if len(c.existingManagers) > 0 {
+					_, exists := controller.addonManagers[c.addonName]
+					assert.True(t, exists, "expected manager to still exist in map")
+				}
+			}
+
+			// Check if requeue was called
+			if c.expectedRequeue {
+				// We can't easily test the exact requeue behavior with the fake sync context
+				// but we can verify the manager wasn't stopped when it should be requeued
+				assert.False(t, managerStopped, "manager should not be stopped when requeue is expected")
+			}
+		})
+	}
+}