Fix wrong key queue for addon controllers (#1152)

The key queue for clustermanagementaddon informer is not correct for
several controllers, fix it by introducing a new queuekey func

Signed-off-by: Jian Qiu <jqiu@redhat.com>

pkg/addon/controllers/addonowner/controller.go

@@ -16,6 +16,7 @@ import (
 	addoninformerv1alpha1 "open-cluster-management.io/api/client/addon/informers/externalversions/addon/v1alpha1"
 	addonlisterv1alpha1 "open-cluster-management.io/api/client/addon/listers/addon/v1alpha1"
 
+	addonindex "open-cluster-management.io/ocm/pkg/addon/index"
 	"open-cluster-management.io/ocm/pkg/common/queue"
 )
 
@@ -26,6 +27,7 @@ const UnsupportedConfigurationType = "UnsupportedConfiguration"
 type addonOwnerController struct {
 	addonClient                  addonv1alpha1client.Interface
 	managedClusterAddonLister    addonlisterv1alpha1.ManagedClusterAddOnLister
+	managedClusterAddonIndexer   cache.Indexer
 	clusterManagementAddonLister addonlisterv1alpha1.ClusterManagementAddOnLister
 	addonFilterFunc              factory.EventFilterFunc
 }
@@ -40,6 +42,7 @@ func NewAddonOwnerController(
 	c := &addonOwnerController{
 		addonClient:                  addonClient,
 		managedClusterAddonLister:    addonInformers.Lister(),
+		managedClusterAddonIndexer:   addonInformers.Informer().GetIndexer(),
 		clusterManagementAddonLister: clusterManagementAddonInformers.Lister(),
 		addonFilterFunc:              addonFilterFunc,
 	}
@@ -47,7 +50,11 @@ func NewAddonOwnerController(
 	return factory.New().
 		WithFilteredEventsInformersQueueKeysFunc(
 			queue.QueueKeyByMetaNamespaceName,
-			c.addonFilterFunc, clusterManagementAddonInformers.Informer()).
+			c.addonFilterFunc).
+		WithInformersQueueKeysFunc(
+			addonindex.ManagedClusterAddonByNameQueueKey(addonInformers),
+			clusterManagementAddonInformers.Informer(),
+		).
 		WithInformersQueueKeysFunc(
 			queue.QueueKeyByMetaNamespaceName,
 			addonInformers.Informer()).

pkg/addon/controllers/addonprogressing/controller.go

@@ -29,6 +29,7 @@ import (
 	workapiv1 "open-cluster-management.io/api/work/v1"
 	"open-cluster-management.io/sdk-go/pkg/patcher"
 
+	addonindex "open-cluster-management.io/ocm/pkg/addon/index"
 	"open-cluster-management.io/ocm/pkg/common/queue"
 )
 
@@ -59,8 +60,11 @@ func NewAddonProgressingController(
 	}
 
 	return factory.New().WithInformersQueueKeysFunc(
-		queue.QueueKeyByMetaNamespaceName,
-		addonInformers.Informer(), clusterManagementAddonInformers.Informer()).
+		queue.QueueKeyByMetaNamespaceName, addonInformers.Informer()).
+		WithInformersQueueKeysFunc(
+			addonindex.ManagedClusterAddonByNameQueueKey(addonInformers),
+			clusterManagementAddonInformers.Informer(),
+		).
 		WithFilteredEventsInformersQueueKeysFunc(
 			func(obj runtime.Object) []string {
 				accessor, _ := meta.Accessor(obj)

pkg/addon/index/index.go

@@ -102,3 +102,25 @@ func ClusterManagementAddonByPlacementDecisionQueueKey(
 		return keys
 	}
 }
+
+// ManagedClusterAddonByNameQueueKey finds all the addon using the name of the object and return all their keys.
+func ManagedClusterAddonByNameQueueKey(addonInformers addoninformerv1alpha1.ManagedClusterAddOnInformer) func(obj runtime.Object) []string {
+	return func(obj runtime.Object) []string {
+		accessor, err := meta.Accessor(obj)
+		if err != nil {
+			utilruntime.HandleError(err)
+			return []string{}
+		}
+		addons, err := addonInformers.Informer().GetIndexer().ByIndex(ManagedClusterAddonByName, accessor.GetName())
+		if err != nil {
+			utilruntime.HandleError(err)
+			return []string{}
+		}
+		keys := make([]string, 0, len(addons))
+		for _, addon := range addons {
+			key, _ := cache.MetaNamespaceKeyFunc(addon)
+			keys = append(keys, key)
+		}
+		return keys
+	}
+}

pkg/addon/index/index_test.go

@@ -0,0 +1,446 @@
+package index
+
+import (
+	"reflect"
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/tools/cache"
+
+	addonv1alpha1 "open-cluster-management.io/api/addon/v1alpha1"
+	fakeaddon "open-cluster-management.io/api/client/addon/clientset/versioned/fake"
+	addoninformers "open-cluster-management.io/api/client/addon/informers/externalversions"
+	clusterv1beta1 "open-cluster-management.io/api/cluster/v1beta1"
+)
+
+func TestIndexClusterManagementAddonByPlacement(t *testing.T) {
+	cases := []struct {
+		name     string
+		obj      interface{}
+		expected []string
+		wantErr  bool
+	}{
+		{
+			name:     "invalid object type",
+			obj:      "invalid",
+			expected: []string{},
+			wantErr:  true,
+		},
+		{
+			name: "empty install strategy",
+			obj: &addonv1alpha1.ClusterManagementAddOn{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-addon",
+				},
+				Spec: addonv1alpha1.ClusterManagementAddOnSpec{},
+			},
+			expected: nil,
+			wantErr:  false,
+		},
+		{
+			name: "manual install strategy",
+			obj: &addonv1alpha1.ClusterManagementAddOn{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-addon",
+				},
+				Spec: addonv1alpha1.ClusterManagementAddOnSpec{
+					InstallStrategy: addonv1alpha1.InstallStrategy{
+						Type: addonv1alpha1.AddonInstallStrategyManual,
+					},
+				},
+			},
+			expected: nil,
+			wantErr:  false,
+		},
+		{
+			name: "placements install strategy with single placement",
+			obj: &addonv1alpha1.ClusterManagementAddOn{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-addon",
+				},
+				Spec: addonv1alpha1.ClusterManagementAddOnSpec{
+					InstallStrategy: addonv1alpha1.InstallStrategy{
+						Type: addonv1alpha1.AddonInstallStrategyPlacements,
+						Placements: []addonv1alpha1.PlacementStrategy{
+							{
+								PlacementRef: addonv1alpha1.PlacementRef{
+									Name:      "test-placement",
+									Namespace: "test-namespace",
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: []string{"test-namespace/test-placement"},
+			wantErr:  false,
+		},
+		{
+			name: "placements install strategy with multiple placements",
+			obj: &addonv1alpha1.ClusterManagementAddOn{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-addon",
+				},
+				Spec: addonv1alpha1.ClusterManagementAddOnSpec{
+					InstallStrategy: addonv1alpha1.InstallStrategy{
+						Type: addonv1alpha1.AddonInstallStrategyPlacements,
+						Placements: []addonv1alpha1.PlacementStrategy{
+							{
+								PlacementRef: addonv1alpha1.PlacementRef{
+									Name:      "test-placement-1",
+									Namespace: "test-namespace-1",
+								},
+							},
+							{
+								PlacementRef: addonv1alpha1.PlacementRef{
+									Name:      "test-placement-2",
+									Namespace: "test-namespace-2",
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: []string{"test-namespace-1/test-placement-1", "test-namespace-2/test-placement-2"},
+			wantErr:  false,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			keys, err := IndexClusterManagementAddonByPlacement(tc.obj)
+
+			if tc.wantErr && err == nil {
+				t.Errorf("expected error but got none")
+				return
+			}
+			if !tc.wantErr && err != nil {
+				t.Errorf("unexpected error: %v", err)
+				return
+			}
+
+			if !reflect.DeepEqual(keys, tc.expected) {
+				t.Errorf("expected keys %v, got %v", tc.expected, keys)
+			}
+		})
+	}
+}
+
+func TestIndexManagedClusterAddonByName(t *testing.T) {
+	cases := []struct {
+		name     string
+		obj      interface{}
+		expected []string
+		wantErr  bool
+	}{
+		{
+			name:     "invalid object type",
+			obj:      "invalid",
+			expected: []string{},
+			wantErr:  true,
+		},
+		{
+			name: "valid ManagedClusterAddon",
+			obj: &addonv1alpha1.ManagedClusterAddOn{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-addon",
+					Namespace: "cluster1",
+				},
+			},
+			expected: []string{"test-addon"},
+			wantErr:  false,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			keys, err := IndexManagedClusterAddonByName(tc.obj)
+
+			if tc.wantErr && err == nil {
+				t.Errorf("expected error but got none")
+				return
+			}
+			if !tc.wantErr && err != nil {
+				t.Errorf("unexpected error: %v", err)
+				return
+			}
+
+			if !reflect.DeepEqual(keys, tc.expected) {
+				t.Errorf("expected keys %v, got %v", tc.expected, keys)
+			}
+		})
+	}
+}
+
+func TestClusterManagementAddonByPlacementQueueKey(t *testing.T) {
+	cma1 := &addonv1alpha1.ClusterManagementAddOn{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-addon-1",
+		},
+		Spec: addonv1alpha1.ClusterManagementAddOnSpec{
+			InstallStrategy: addonv1alpha1.InstallStrategy{
+				Type: addonv1alpha1.AddonInstallStrategyPlacements,
+				Placements: []addonv1alpha1.PlacementStrategy{
+					{
+						PlacementRef: addonv1alpha1.PlacementRef{
+							Name:      "test-placement",
+							Namespace: "test-namespace",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	cma2 := &addonv1alpha1.ClusterManagementAddOn{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-addon-2",
+		},
+		Spec: addonv1alpha1.ClusterManagementAddOnSpec{
+			InstallStrategy: addonv1alpha1.InstallStrategy{
+				Type: addonv1alpha1.AddonInstallStrategyPlacements,
+				Placements: []addonv1alpha1.PlacementStrategy{
+					{
+						PlacementRef: addonv1alpha1.PlacementRef{
+							Name:      "test-placement",
+							Namespace: "test-namespace",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	placement := &clusterv1beta1.Placement{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-placement",
+			Namespace: "test-namespace",
+		},
+	}
+
+	fakeClient := fakeaddon.NewSimpleClientset(cma1, cma2)
+	informerFactory := addoninformers.NewSharedInformerFactory(fakeClient, 0)
+	cmaInformer := informerFactory.Addon().V1alpha1().ClusterManagementAddOns()
+
+	cmaInformer.Informer().GetIndexer().AddIndexers(cache.Indexers{
+		ClusterManagementAddonByPlacement: IndexClusterManagementAddonByPlacement,
+	})
+
+	cmaInformer.Informer().GetStore().Add(cma1)
+	cmaInformer.Informer().GetStore().Add(cma2)
+
+	queueKeyFunc := ClusterManagementAddonByPlacementQueueKey(cmaInformer)
+
+	cases := []struct {
+		name     string
+		obj      runtime.Object
+		expected []string
+	}{
+		{
+			name:     "placement object",
+			obj:      placement,
+			expected: []string{"test-addon-1", "test-addon-2"},
+		},
+		{
+			name: "unrelated object",
+			obj: &clusterv1beta1.Placement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "unrelated-placement",
+					Namespace: "test-namespace",
+				},
+			},
+			expected: []string{},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			keys := queueKeyFunc(tc.obj)
+
+			if len(keys) != len(tc.expected) {
+				t.Errorf("expected %d keys, got %d", len(tc.expected), len(keys))
+			}
+
+			for _, expectedKey := range tc.expected {
+				found := false
+				for _, key := range keys {
+					if key == expectedKey {
+						found = true
+						break
+					}
+				}
+				if !found {
+					t.Errorf("expected key %s not found in result %v", expectedKey, keys)
+				}
+			}
+		})
+	}
+}
+
+func TestClusterManagementAddonByPlacementDecisionQueueKey(t *testing.T) {
+	cma1 := &addonv1alpha1.ClusterManagementAddOn{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-addon-1",
+		},
+		Spec: addonv1alpha1.ClusterManagementAddOnSpec{
+			InstallStrategy: addonv1alpha1.InstallStrategy{
+				Type: addonv1alpha1.AddonInstallStrategyPlacements,
+				Placements: []addonv1alpha1.PlacementStrategy{
+					{
+						PlacementRef: addonv1alpha1.PlacementRef{
+							Name:      "test-placement",
+							Namespace: "test-namespace",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	placementDecision := &clusterv1beta1.PlacementDecision{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-placement-decision",
+			Namespace: "test-namespace",
+			Labels: map[string]string{
+				clusterv1beta1.PlacementLabel: "test-placement",
+			},
+		},
+	}
+
+	fakeClient := fakeaddon.NewSimpleClientset(cma1)
+	informerFactory := addoninformers.NewSharedInformerFactory(fakeClient, 0)
+	cmaInformer := informerFactory.Addon().V1alpha1().ClusterManagementAddOns()
+
+	cmaInformer.Informer().GetIndexer().AddIndexers(cache.Indexers{
+		ClusterManagementAddonByPlacement: IndexClusterManagementAddonByPlacement,
+	})
+
+	cmaInformer.Informer().GetStore().Add(cma1)
+
+	queueKeyFunc := ClusterManagementAddonByPlacementDecisionQueueKey(cmaInformer)
+
+	cases := []struct {
+		name     string
+		obj      runtime.Object
+		expected []string
+	}{
+		{
+			name:     "placement decision with placement label",
+			obj:      placementDecision,
+			expected: []string{"test-addon-1"},
+		},
+		{
+			name: "placement decision without placement label",
+			obj: &clusterv1beta1.PlacementDecision{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-placement-decision-no-label",
+					Namespace: "test-namespace",
+				},
+			},
+			expected: []string{},
+		},
+		{
+			name: "placement decision with wrong placement label",
+			obj: &clusterv1beta1.PlacementDecision{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-placement-decision-wrong",
+					Namespace: "test-namespace",
+					Labels: map[string]string{
+						clusterv1beta1.PlacementLabel: "wrong-placement",
+					},
+				},
+			},
+			expected: nil,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			keys := queueKeyFunc(tc.obj)
+
+			if !reflect.DeepEqual(keys, tc.expected) {
+				t.Errorf("expected keys %v, got %v", tc.expected, keys)
+			}
+		})
+	}
+}
+
+func TestManagedClusterAddonByNameQueueKey(t *testing.T) {
+	mca1 := &addonv1alpha1.ManagedClusterAddOn{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-addon",
+			Namespace: "cluster1",
+		},
+	}
+
+	mca2 := &addonv1alpha1.ManagedClusterAddOn{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-addon",
+			Namespace: "cluster2",
+		},
+	}
+
+	testObj := &addonv1alpha1.ClusterManagementAddOn{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-addon",
+		},
+	}
+
+	fakeClient := fakeaddon.NewSimpleClientset(mca1, mca2)
+	informerFactory := addoninformers.NewSharedInformerFactory(fakeClient, 0)
+	mcaInformer := informerFactory.Addon().V1alpha1().ManagedClusterAddOns()
+
+	mcaInformer.Informer().GetIndexer().AddIndexers(cache.Indexers{
+		ManagedClusterAddonByName: IndexManagedClusterAddonByName,
+	})
+
+	mcaInformer.Informer().GetStore().Add(mca1)
+	mcaInformer.Informer().GetStore().Add(mca2)
+
+	queueKeyFunc := ManagedClusterAddonByNameQueueKey(mcaInformer)
+
+	cases := []struct {
+		name     string
+		obj      runtime.Object
+		expected []string
+	}{
+		{
+			name:     "object with name matching ManagedClusterAddons",
+			obj:      testObj,
+			expected: []string{"cluster1/test-addon", "cluster2/test-addon"},
+		},
+		{
+			name: "object with name not matching any ManagedClusterAddons",
+			obj: &addonv1alpha1.ClusterManagementAddOn{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "non-existent-addon",
+				},
+			},
+			expected: []string{},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			keys := queueKeyFunc(tc.obj)
+
+			if len(keys) != len(tc.expected) {
+				t.Errorf("expected %d keys, got %d", len(tc.expected), len(keys))
+			}
+
+			for _, expectedKey := range tc.expected {
+				found := false
+				for _, key := range keys {
+					if key == expectedKey {
+						found = true
+						break
+					}
+				}
+				if !found {
+					t.Errorf("expected key %s not found in result %v", expectedKey, keys)
+				}
+			}
+		})
+	}
+}

test/integration/addon/suite_test.go

@@ -13,6 +13,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes"
+	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 
 	"open-cluster-management.io/addon-framework/pkg/addonmanager"
@@ -50,6 +51,11 @@ var cancel context.CancelFunc
 var mgrContext context.Context
 var addonManager addonmanager.AddonManager
 
+func init() {
+	klog.InitFlags(nil)
+	klog.SetOutput(ginkgo.GinkgoWriter)
+}
+
 func TestIntegration(t *testing.T) {
 	gomega.RegisterFailHandler(ginkgo.Fail)
 	ginkgo.RunSpecs(t, "Integration Suite")