Skip to content

🐛 Fix ManagedClusterAddons not removed when ClusterManagementAddon is deleted #1160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Sep 10, 2025
+330 −55
Merged

🐛 Fix ManagedClusterAddons not removed when ClusterManagementAddon is deleted #1160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1f2fa2b
Fix ManagedClusterAddons not removed when ClusterManagementAddon is d…
zhujian7 Sep 3, 2025
78b6016
add e2e test
zhujian7 Sep 3, 2025
de16035
return err when stopUnusedManagers failed
zhujian7 Sep 3, 2025
1ecd849
Address review comments for addon manager deletion fix
zhujian7 Sep 4, 2025
88b5213
Fix e2e test timeout for configmap deletion check
zhujian7 Sep 4, 2025
63f10c2
Improve error logging in template agent
zhujian7 Sep 4, 2025
78edb40
Use ManagedClusterAddonByName index for efficient lookup
zhujian7 Sep 4, 2025
e60a374
Remove unused mcaLister field
zhujian7 Sep 4, 2025
18983f4
Replace inefficient list-and-filter with indexed lookup in runController
zhujian7 Sep 4, 2025
cffc8eb
Fix review comments for addon manager deletion
zhujian7 Sep 8, 2025
4649d1b
Optimize ManagedClusterAddOn event handling in addon template controller
zhujian7 Sep 8, 2025
171ebbc
Revert "Optimize ManagedClusterAddOn event handling in addon template…
zhujian7 Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 77 additions & 39 deletions pkg/addon/controllers/addontemplate/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,17 @@ import (
"github.com/openshift/library-go/pkg/operator/events"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/client-go/dynamic/dynamicinformer"
kubeinformers "k8s.io/client-go/informers"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/cache"
"k8s.io/klog/v2"

"open-cluster-management.io/addon-framework/pkg/addonfactory"
"open-cluster-management.io/addon-framework/pkg/addonmanager"
"open-cluster-management.io/addon-framework/pkg/utils"
addonapiv1alpha1 "open-cluster-management.io/api/addon/v1alpha1"
addonv1alpha1 "open-cluster-management.io/api/addon/v1alpha1"
addonv1alpha1client "open-cluster-management.io/api/client/addon/clientset/versioned"
addoninformers "open-cluster-management.io/api/client/addon/informers/externalversions"
Expand All @@ -29,6 +28,7 @@ import (
workv1informers "open-cluster-management.io/api/client/work/informers/externalversions"
clusterv1 "open-cluster-management.io/api/cluster/v1"

addonindex "open-cluster-management.io/ocm/pkg/addon/index"
"open-cluster-management.io/ocm/pkg/addon/templateagent"
"open-cluster-management.io/ocm/pkg/common/queue"
)
Expand All @@ -40,17 +40,18 @@ type addonTemplateController struct {
// The key is the name of the template type addon.
addonManagers map[string]context.CancelFunc

kubeConfig *rest.Config
addonClient addonv1alpha1client.Interface
workClient workv1client.Interface
kubeClient kubernetes.Interface
cmaLister addonlisterv1alpha1.ClusterManagementAddOnLister
addonInformers addoninformers.SharedInformerFactory
clusterInformers clusterv1informers.SharedInformerFactory
dynamicInformers dynamicinformer.DynamicSharedInformerFactory
workInformers workv1informers.SharedInformerFactory
runControllerFunc runController
eventRecorder events.Recorder
kubeConfig *rest.Config
addonClient addonv1alpha1client.Interface
workClient workv1client.Interface
kubeClient kubernetes.Interface
cmaLister addonlisterv1alpha1.ClusterManagementAddOnLister
managedClusterAddonIndexer cache.Indexer
addonInformers addoninformers.SharedInformerFactory
clusterInformers clusterv1informers.SharedInformerFactory
dynamicInformers dynamicinformer.DynamicSharedInformerFactory
workInformers workv1informers.SharedInformerFactory
runControllerFunc runController
eventRecorder events.Recorder
}

type runController func(ctx context.Context, addonName string) error
Expand All @@ -69,17 +70,18 @@ func NewAddonTemplateController(
runController ...runController,
) factory.Controller {
c := &addonTemplateController{
kubeConfig: hubKubeconfig,
kubeClient: hubKubeClient,
addonClient: addonClient,
workClient: workClient,
cmaLister: addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Lister(),
addonManagers: make(map[string]context.CancelFunc),
addonInformers: addonInformers,
clusterInformers: clusterInformers,
dynamicInformers: dynamicInformers,
workInformers: workInformers,
eventRecorder: recorder,
kubeConfig: hubKubeconfig,
kubeClient: hubKubeClient,
addonClient: addonClient,
workClient: workClient,
cmaLister: addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Lister(),
managedClusterAddonIndexer: addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Informer().GetIndexer(),
addonManagers: make(map[string]context.CancelFunc),
Comment on lines +78 to +79
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Register the ManagedClusterAddonByName index before using ByIndex.

ByIndex will error if the indexer wasn’t registered; in production this will cause stop/start paths to misbehave or leak managers. Tests add the indexer, but the controller doesn’t.

Apply:

 controller := factory.New().
   WithSyncContext(syncCtx).
@@
   WithSync(c.sync).
   ToController("addon-template-controller", recorder)

+ // Ensure the MCA by-name index is registered (required for ByIndex calls).
+ if err := addonInformers.Addon().V1alpha1().ManagedClusterAddOns().
+   Informer().AddIndexers(cache.Indexers{
+     addonindex.ManagedClusterAddonByName: addonindex.IndexManagedClusterAddonByName,
+   }); err != nil {
+   utilruntime.HandleError(err)
+ }

Also applies to: 101-109

🤖 Prompt for AI Agents 
In pkg/addon/controllers/addontemplate/controller.go around lines 84-85 (and
similarly around 101-109), the ManagedClusterAddon indexer is used via ByIndex
without ensuring the index was registered; add code during controller
construction to call the indexer's AddIndexers to register the
ManagedClusterAddonByName indexer (provide a function that returns the addon
name key from the ManagedClusterAddOn object or its metadata), do this
registration before any call to ByIndex, and handle and log/return any error
from AddIndexers so the controller fails fast if registration fails.

addonInformers: addonInformers,
clusterInformers: clusterInformers,
dynamicInformers: dynamicInformers,
workInformers: workInformers,
eventRecorder: recorder,
}

if len(runController) > 0 {
Expand All @@ -88,9 +90,28 @@ func NewAddonTemplateController(
// easy to mock in unit tests
c.runControllerFunc = c.runController
}
return factory.New().WithInformersQueueKeysFunc(
queue.QueueKeyByMetaNamespaceName,
addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Informer()).
return factory.New().
WithInformersQueueKeysFunc(
queue.QueueKeyByMetaNamespaceName,
addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Informer()).
WithFilteredEventsInformersQueueKeysFunc(
queue.QueueKeyByMetaName,
func(obj interface{}) bool {
mca, ok := obj.(*addonv1alpha1.ManagedClusterAddOn)
if !ok {
return false
}

// Only process ManagedClusterAddOns that reference AddOnTemplates
for _, configRef := range mca.Status.ConfigReferences {
if configRef.ConfigGroupResource.Group == "addon.open-cluster-management.io" &&
configRef.ConfigGroupResource.Resource == "addontemplates" {
return true
}
}
return false
},
addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Informer()).
Comment on lines +97 to +114
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Handle tombstones in the filter predicate and use constants instead of hard-coded strings.

Delete events can arrive as cache.DeletedFinalStateUnknown; current cast drops those. Also prefer utils.AddOnTemplateGVR to avoid drift.

Apply:

-        queue.QueueKeyByMetaName,
-        func(obj interface{}) bool {
-          mca, ok := obj.(*addonv1alpha1.ManagedClusterAddOn)
-          if !ok {
-            return false
-          }
-
-          // Only process ManagedClusterAddOns that reference AddOnTemplates
-          for _, configRef := range mca.Status.ConfigReferences {
-            if configRef.ConfigGroupResource.Group == "addon.open-cluster-management.io" &&
-              configRef.ConfigGroupResource.Resource == "addontemplates" {
-              return true
-            }
-          }
-          return false
-        },
+        queue.QueueKeyByMetaName,
+        func(obj interface{}) bool {
+          // unwrap tombstone for delete events
+          if ts, ok := obj.(cache.DeletedFinalStateUnknown); ok {
+            obj = ts.Obj
+          }
+          mca, ok := obj.(*addonv1alpha1.ManagedClusterAddOn)
+          if !ok {
+            return false
+          }
+          // Only process MCAs that reference AddOnTemplates
+          for _, ref := range mca.Status.ConfigReferences {
+            if ref.ConfigGroupResource.Group == utils.AddOnTemplateGVR.Group &&
+               ref.ConfigGroupResource.Resource == utils.AddOnTemplateGVR.Resource {
+              return true
+            }
+          }
+          return false
+        },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
WithFilteredEventsInformersQueueKeysFunc(
queue.QueueKeyByMetaName,
func(obj interface{}) bool {
mca, ok := obj.(*addonv1alpha1.ManagedClusterAddOn)
if !ok {
return false
}
// Only process ManagedClusterAddOns that reference AddOnTemplates
for _, configRef := range mca.Status.ConfigReferences {
if configRef.ConfigGroupResource.Group == "addon.open-cluster-management.io" &&
configRef.ConfigGroupResource.Resource == "addontemplates" {
return true
}
}
return false
},
addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Informer()).
WithFilteredEventsInformersQueueKeysFunc(
queue.QueueKeyByMetaName,
func(obj interface{}) bool {
// unwrap tombstone for delete events
if ts, ok := obj.(cache.DeletedFinalStateUnknown); ok {
obj = ts.Obj
}
mca, ok := obj.(*addonv1alpha1.ManagedClusterAddOn)
if !ok {
return false
}
// Only process MCAs that reference AddOnTemplates
for _, ref := range mca.Status.ConfigReferences {
if ref.ConfigGroupResource.Group == utils.AddOnTemplateGVR.Group &&
ref.ConfigGroupResource.Resource == utils.AddOnTemplateGVR.Resource {
return true
}
}
return false
},
addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Informer()).
🤖 Prompt for AI Agents 
In pkg/addon/controllers/addontemplate/controller.go around lines 97 to 114, the
filter predicate currently casts obj directly to
*addonv1alpha1.ManagedClusterAddOn which drops tombstone-wrapped delete events
(cache.DeletedFinalStateUnknown) and uses hard-coded group/resource strings;
update the predicate to unwrap cache.DeletedFinalStateUnknown by extracting the
tombstone.Obj when needed and then assert the target type, and replace the
literal "addon.open-cluster-management.io"/"addontemplates" checks with the
shared constant (e.g., utils.AddOnTemplateGVR or the project’s AddOnTemplateGVR)
comparing Group and Resource against those constant values so delete events are
handled and the GVR strings are not hard-coded.

WithBareInformers(
// do not need to queue, just make sure the controller reconciles after the addonTemplate cache is synced
// otherwise, there will be "xx-addon-template" not found" errors in the log as the controller uses the
Expand All @@ -101,14 +122,32 @@ func NewAddonTemplateController(
}

func (c *addonTemplateController) stopUnusedManagers(
ctx context.Context, syncCtx factory.SyncContext, addOnName string) {
// TODO: check if all managed cluster addon instances are deleted
ctx context.Context, syncCtx factory.SyncContext, addOnName string) error {
logger := klog.FromContext(ctx)

// Check if all managed cluster addon instances are deleted before stopping the manager
// This ensures the manager continues running while ManagedClusterAddOns exist
addons, err := c.managedClusterAddonIndexer.ByIndex(addonindex.ManagedClusterAddonByName, addOnName)
if err != nil {
return err
}

// Check if there are still ManagedClusterAddOns for this addon
if len(addons) > 0 {
logger.Info("ManagedClusterAddOn still exists, waiting for deletion",
"addonName", addOnName, "count", len(addons))
// No need to requeue since we now watch ManagedClusterAddon events
return nil
}

stopFunc, ok := c.addonManagers[addOnName]
if ok {
logger.Info("Start to stop the manager for addon", "addonName", addOnName)
stopFunc()
delete(c.addonManagers, addOnName)
klog.FromContext(ctx).Info("Stopping the manager for addon", "addonName", addOnName)
logger.Info("The manager for addon stopped", "addonName", addOnName)
}
return nil
}

func (c *addonTemplateController) sync(ctx context.Context, syncCtx factory.SyncContext) error {
Expand All @@ -118,15 +157,13 @@ func (c *addonTemplateController) sync(ctx context.Context, syncCtx factory.Sync
cma, err := c.cmaLister.Get(addonName)
if err != nil {
if errors.IsNotFound(err) {
c.stopUnusedManagers(ctx, syncCtx, addonName)
return nil
return c.stopUnusedManagers(ctx, syncCtx, addonName)
}
return err
}

if !templateagent.SupportAddOnTemplate(cma) {
c.stopUnusedManagers(ctx, syncCtx, cma.Name)
return nil
return c.stopUnusedManagers(ctx, syncCtx, cma.Name)
}

_, exist := c.addonManagers[addonName]
Expand Down Expand Up @@ -188,7 +225,7 @@ func (c *addonTemplateController) runController(ctx context.Context, addonName s
listOptions.LabelSelector = metav1.FormatLabelSelector(selector)
}),
)
getValuesClosure := func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) (addonfactory.Values, error) {
getValuesClosure := func(cluster *clusterv1.ManagedCluster, addon *addonv1alpha1.ManagedClusterAddOn) (addonfactory.Values, error) {
return templateagent.GetAddOnRegistriesPrivateValuesFromClusterAnnotation(klog.FromContext(ctx), cluster, addon)
}
agentAddon := templateagent.NewCRDTemplateAgentAddon(
Expand Down Expand Up @@ -224,13 +261,14 @@ func (c *addonTemplateController) runController(ctx context.Context, addonName s
kubeInformers.Start(ctx.Done())

// trigger the manager to reconcile for the existing managed cluster addons
mcas, err := c.addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Lister().List(labels.Everything())
mcas, err := c.managedClusterAddonIndexer.ByIndex(addonindex.ManagedClusterAddonByName, addonName)
if err != nil {
logger.Info("Failed to list ManagedClusterAddOns", "error", err)
logger.Info("Failed to list ManagedClusterAddOns by index", "error", err)
} else {
for _, mca := range mcas {
if mca.Name == addonName {
mgr.Trigger(mca.Namespace, addonName)
addon, ok := mca.(*addonv1alpha1.ManagedClusterAddOn)
if ok {
mgr.Trigger(addon.Namespace, addonName)
}
}
}
Expand Down
Loading
Loading