From 062ae225bb7834c7c0ef500d9e8555020e2e47f5 Mon Sep 17 00:00:00 2001 From: Morven Cao Date: Thu, 29 Jan 2026 15:28:29 +0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20enhance=20clusterprofile=20lifec?= =?UTF-8?q?ycle=20controller=20(#1359)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * check namespace existence and state in clusterprofile lifecycle controller. Signed-off-by: Morven Cao * optimize the queue key and log for clusterprofile controller. Signed-off-by: Morven Cao --------- Signed-off-by: Morven Cao --- .../clusterprofile/lifecycle_controller.go | 55 +++++------ .../lifecycle_controller_sync_test.go | 97 ++++++++++++++++++- pkg/registration/hub/manager.go | 1 + 3 files changed, 118 insertions(+), 35 deletions(-) diff --git a/pkg/registration/hub/clusterprofile/lifecycle_controller.go b/pkg/registration/hub/clusterprofile/lifecycle_controller.go index 973cce3b8..fe1d477e1 100644 --- a/pkg/registration/hub/clusterprofile/lifecycle_controller.go +++ b/pkg/registration/hub/clusterprofile/lifecycle_controller.go @@ -12,6 +12,7 @@ import ( utilerrors "k8s.io/apimachinery/pkg/util/errors" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/cache" "k8s.io/klog/v2" cpv1alpha1 "sigs.k8s.io/cluster-inventory-api/apis/v1alpha1" @@ -45,6 +46,7 @@ const ( // // Key constraint: ManagedClusterSetBinding.Name MUST equal ManagedClusterSetBinding.Spec.ClusterSet type clusterProfileLifecycleController struct { + kubeClient kubernetes.Interface clusterLister listerv1.ManagedClusterLister clusterSetLister clusterlisterv1beta2.ManagedClusterSetLister clusterSetBindingLister clusterlisterv1beta2.ManagedClusterSetBindingLister @@ -55,6 +57,7 @@ type clusterProfileLifecycleController struct { // NewClusterProfileLifecycleController creates a controller that manages ClusterProfile lifecycle func NewClusterProfileLifecycleController( + kubeClient kubernetes.Interface, clusterInformer informerv1.ManagedClusterInformer, clusterSetInformer clusterinformerv1beta2.ManagedClusterSetInformer, clusterSetBindingInformer clusterinformerv1beta2.ManagedClusterSetBindingInformer, @@ -65,6 +68,7 @@ func NewClusterProfileLifecycleController( // so we don't need to add it again here. Informers are shared across controllers. c := &clusterProfileLifecycleController{ + kubeClient: kubeClient, clusterLister: clusterInformer.Lister(), clusterSetLister: clusterSetInformer.Lister(), clusterSetBindingLister: clusterSetBindingInformer.Lister(), @@ -219,16 +223,12 @@ func (c *clusterProfileLifecycleController) clusterSetToQueueKeys(obj runtime.Ob } // Collect unique namespaces that have bindings to this clusterset - namespaces := make(map[string]bool) + namespaces := sets.New[string]() for _, binding := range bindings { - namespaces[binding.Namespace] = true + namespaces.Insert(binding.Namespace) } - keys := make([]string, 0, len(namespaces)) - for ns := range namespaces { - keys = append(keys, ns) - } - return keys + return namespaces.UnsortedList() } // clusterToQueueKeys maps a ManagedCluster to all namespaces that should have its profile @@ -246,7 +246,7 @@ func (c *clusterProfileLifecycleController) clusterToQueueKeys(obj runtime.Objec } // For each clusterset, use indexer to efficiently find namespaces with bindings to it - namespaces := make(map[string]bool) + namespaces := sets.New[string]() for _, clusterSet := range clusterSets { bindings, err := c.getBindingsByClusterSet(clusterSet.Name) if err != nil { @@ -254,15 +254,11 @@ func (c *clusterProfileLifecycleController) clusterToQueueKeys(obj runtime.Objec continue } for _, binding := range bindings { - namespaces[binding.Namespace] = true + namespaces.Insert(binding.Namespace) } } - keys := make([]string, 0, len(namespaces)) - for ns := range namespaces { - keys = append(keys, ns) - } - return keys + return namespaces.UnsortedList() } // profileToQueueKey maps a ClusterProfile to its namespace @@ -286,6 +282,20 @@ func (c *clusterProfileLifecycleController) sync(ctx context.Context, syncCtx fa logger.V(4).Info("Reconciling ClusterProfiles in namespace") + // 0. Check if namespace exists and is not terminating + ns, err := c.kubeClient.CoreV1().Namespaces().Get(ctx, namespace, metav1.GetOptions{}) + if errors.IsNotFound(err) { + logger.V(4).Info("Namespace not found, skipping reconciliation") + return nil + } + if err != nil { + return err + } + if !ns.DeletionTimestamp.IsZero() { + logger.V(4).Info("Namespace is terminating, skipping reconciliation") + return nil + } + // 1. Get all bindings in this namespace allBindings, err := c.clusterSetBindingLister.ManagedClusterSetBindings(namespace).List(labels.Everything()) if err != nil { @@ -364,8 +374,6 @@ func (c *clusterProfileLifecycleController) sync(ctx context.Context, syncCtx fa // Clusters to delete = existing - desired clustersToDelete := existingClusters.Difference(desiredClusters) - profilesCreated := 0 - profilesDeleted := 0 var errs []error // Create missing profiles @@ -374,8 +382,6 @@ func (c *clusterProfileLifecycleController) sync(ctx context.Context, syncCtx fa if err != nil { logger.Error(err, "Failed to create ClusterProfile", "cluster", clusterName) errs = append(errs, fmt.Errorf("failed to create ClusterProfile %s/%s: %w", namespace, clusterName, err)) - } else { - profilesCreated++ } } @@ -388,21 +394,10 @@ func (c *clusterProfileLifecycleController) sync(ctx context.Context, syncCtx fa logger.Error(err, "Failed to delete ClusterProfile", "cluster", clusterName) errs = append(errs, fmt.Errorf("failed to delete ClusterProfile %s/%s: %w", namespace, clusterName, err)) } else if err == nil { - profilesDeleted++ - logger.V(2).Info("Deleted ClusterProfile", "namespace", namespace, "name", clusterName) + logger.V(2).Info("Deleted ClusterProfile", "name", clusterName) } } - if profilesCreated > 0 || profilesDeleted > 0 { - logger.Info("Namespace reconciliation complete", - "profilesCreated", profilesCreated, - "profilesDeleted", profilesDeleted, - "totalDesired", desiredClusters.Len()) - syncCtx.Recorder().Eventf(ctx, "ClusterProfilesReconciled", - "reconciled namespace %s: created %d, deleted %d profiles", - namespace, profilesCreated, profilesDeleted) - } - return utilerrors.NewAggregate(errs) } diff --git a/pkg/registration/hub/clusterprofile/lifecycle_controller_sync_test.go b/pkg/registration/hub/clusterprofile/lifecycle_controller_sync_test.go index 3ce5cd111..484f5e408 100644 --- a/pkg/registration/hub/clusterprofile/lifecycle_controller_sync_test.go +++ b/pkg/registration/hub/clusterprofile/lifecycle_controller_sync_test.go @@ -4,8 +4,10 @@ import ( "context" "testing" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + kubefake "k8s.io/client-go/kubernetes/fake" clienttesting "k8s.io/client-go/testing" cpv1alpha1 "sigs.k8s.io/cluster-inventory-api/apis/v1alpha1" cpfake "sigs.k8s.io/cluster-inventory-api/client/clientset/versioned/fake" @@ -531,10 +533,29 @@ func TestLifecycleControllerSync(t *testing.T) { }, } + // ========== Helper function ========== + createNamespace := func(name string) *corev1.Namespace { + return &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + } + } + + // ========== Namespaces ========== + ns1 := createNamespace("ns1") + nsTerminating := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ns-terminating", + DeletionTimestamp: &metav1.Time{Time: metav1.Now().Time}, + }, + } + // ========== Test Cases ========== cases := []struct { name string key string + namespace *corev1.Namespace // if nil, namespace doesn't exist clusters []runtime.Object clusterSets []runtime.Object bindings []runtime.Object @@ -543,9 +564,50 @@ func TestLifecycleControllerSync(t *testing.T) { expectedDeletes []string // cluster names that should be deleted expectedNumActions int }{ + { + name: "namespace not found - skip reconciliation", + key: "ns-not-found", + namespace: nil, // namespace doesn't exist + clusters: []runtime.Object{cluster1, cluster2}, + clusterSets: []runtime.Object{defaultClusterSet}, + bindings: []runtime.Object{boundBindingDefault}, + expectedCreates: nil, + expectedDeletes: nil, + expectedNumActions: 0, // should skip reconciliation + }, + { + name: "namespace terminating - skip reconciliation", + key: "ns-terminating", + namespace: nsTerminating, + clusters: []runtime.Object{cluster1, cluster2}, + clusterSets: []runtime.Object{defaultClusterSet}, + bindings: []runtime.Object{ + &v1beta2.ManagedClusterSetBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "ns-terminating", + }, + Spec: v1beta2.ManagedClusterSetBindingSpec{ + ClusterSet: "default", + }, + Status: v1beta2.ManagedClusterSetBindingStatus{ + Conditions: []metav1.Condition{ + { + Type: v1beta2.ClusterSetBindingBoundType, + Status: metav1.ConditionTrue, + }, + }, + }, + }, + }, + expectedCreates: nil, + expectedDeletes: nil, + expectedNumActions: 0, // should skip reconciliation + }, { name: "create profiles for bound binding", key: "ns1", + namespace: ns1, clusters: []runtime.Object{cluster1, cluster2}, clusterSets: []runtime.Object{defaultClusterSet}, bindings: []runtime.Object{boundBindingDefault}, @@ -555,6 +617,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "no action for unbound binding", key: "ns2", + namespace: createNamespace("ns2"), clusters: []runtime.Object{cluster1, cluster2}, clusterSets: []runtime.Object{defaultClusterSet}, bindings: []runtime.Object{unboundBindingDefault}, @@ -565,6 +628,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "update existing profile (no creates)", key: "ns1", + namespace: ns1, clusters: []runtime.Object{cluster1, cluster2}, clusterSets: []runtime.Object{defaultClusterSet}, bindings: []runtime.Object{boundBindingDefault}, @@ -575,6 +639,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "delete stale profile", key: "ns1", + namespace: ns1, clusters: []runtime.Object{cluster1, cluster2}, clusterSets: []runtime.Object{defaultClusterSet}, bindings: []runtime.Object{boundBindingDefault}, @@ -586,6 +651,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "namespace with no bindings", key: "empty-ns", + namespace: createNamespace("empty-ns"), clusters: []runtime.Object{cluster1}, clusterSets: []runtime.Object{defaultClusterSet}, bindings: []runtime.Object{}, @@ -595,6 +661,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "multiple bindings in same namespace - non-overlapping", key: "ns1", + namespace: ns1, clusters: []runtime.Object{cluster1, cluster2, clusterBoundSet}, clusterSets: []runtime.Object{defaultClusterSet, clusterSetbound}, bindings: []runtime.Object{boundBindingDefault, boundBindingSet}, @@ -604,6 +671,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "multiple bindings with clusters in deletion state", key: "ns1", + namespace: ns1, clusters: []runtime.Object{cluster1, clusterDeleting, cluster2}, clusterSets: []runtime.Object{defaultClusterSet}, bindings: []runtime.Object{boundBindingDefault}, @@ -613,6 +681,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "multiple bindings, one unbound", key: "ns1", + namespace: ns1, clusters: []runtime.Object{cluster1, cluster2, clusterUnboundSet}, clusterSets: []runtime.Object{defaultClusterSet, clusterSetUnbound}, bindings: []runtime.Object{boundBindingDefault, unboundBindingSetUnbound}, @@ -623,6 +692,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "LabelSelector - create profiles for clusters matching label selector", key: "ns-label", + namespace: createNamespace("ns-label"), clusters: []runtime.Object{clusterProdUSWest, clusterDevUSWest, clusterMixed}, clusterSets: []runtime.Object{clusterSetProdLabelSelector}, bindings: []runtime.Object{boundBindingProdLabelSelector}, @@ -632,6 +702,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "LabelSelector with MatchExpressions - select multiple environments", key: "ns-expr", + namespace: createNamespace("ns-expr"), clusters: []runtime.Object{clusterProdUSWest, clusterDevUSWest, cluster1}, clusterSets: []runtime.Object{clusterSetEnvMatchExpressions}, bindings: []runtime.Object{boundBindingEnvMatchExpr}, @@ -641,6 +712,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "overlap - ExclusiveClusterSetLabel and LabelSelector selecting same cluster", key: "ns-overlap", + namespace: createNamespace("ns-overlap"), clusters: []runtime.Object{cluster1, clusterMixed, clusterProdUSWest}, clusterSets: []runtime.Object{defaultClusterSet, clusterSetProdLabelSelector}, bindings: []runtime.Object{boundBindingDefaultOverlap, boundBindingProdLabelSelectorOverlap}, @@ -650,6 +722,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "overlap - multiple LabelSelectors selecting overlapping clusters", key: "ns-label-overlap", + namespace: createNamespace("ns-label-overlap"), clusters: []runtime.Object{clusterProdUSWest, clusterProdEUWest}, clusterSets: []runtime.Object{clusterSetProdLabelSelector, clusterSetUSWestLabelSelector}, bindings: []runtime.Object{boundBindingProdLabelSelectorLabelOverlap, boundBindingUSWestLabelSelectorLabelOverlap}, @@ -659,6 +732,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "global clusterset - matches all clusters", key: "ns-global", + namespace: createNamespace("ns-global"), clusters: []runtime.Object{cluster1, clusterProdUSWest, clusterDevUSWest, clusterNoLabels}, clusterSets: []runtime.Object{globalClusterSet}, bindings: []runtime.Object{boundBindingGlobal}, @@ -668,6 +742,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "global clusterset - skips clusters in deletion", key: "ns-global", + namespace: createNamespace("ns-global"), clusters: []runtime.Object{cluster1, clusterProdUSWest, clusterDeleting}, clusterSets: []runtime.Object{globalClusterSet}, bindings: []runtime.Object{boundBindingGlobal}, @@ -677,6 +752,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "global clusterset - overlap with default clusterset", key: "ns-global-overlap", + namespace: createNamespace("ns-global-overlap"), clusters: []runtime.Object{cluster1, cluster2, clusterProdUSWest}, clusterSets: []runtime.Object{globalClusterSet, defaultClusterSet}, bindings: []runtime.Object{boundBindingGlobalOverlap, boundBindingDefaultGlobalOverlap}, @@ -686,6 +762,7 @@ func TestLifecycleControllerSync(t *testing.T) { { name: "complex overlap - ExclusiveClusterSetLabel, LabelSelector with MatchLabels, and MatchExpressions", key: "ns-complex", + namespace: createNamespace("ns-complex"), clusters: []runtime.Object{cluster1, clusterMixed, clusterProdUSWest, clusterDevUSWest}, clusterSets: []runtime.Object{defaultClusterSet, clusterSetProdLabelSelector, clusterSetEnvMatchExpressions}, bindings: []runtime.Object{boundBindingDefaultComplex, boundBindingProdLabelSelectorComplex, boundBindingEnvMatchExprComplex}, @@ -696,6 +773,14 @@ func TestLifecycleControllerSync(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { + // Create kubeClient with namespace if specified + var kubeClient *kubefake.Clientset + if c.namespace != nil { + kubeClient = kubefake.NewSimpleClientset(c.namespace) + } else { + kubeClient = kubefake.NewSimpleClientset() + } + clusterObjects := append(c.clusters, c.clusterSets...) clusterObjects = append(clusterObjects, c.bindings...) clusterClient := clusterfake.NewSimpleClientset(clusterObjects...) @@ -719,11 +804,13 @@ func TestLifecycleControllerSync(t *testing.T) { } ctrl := &clusterProfileLifecycleController{ - clusterLister: clusterInformers.Cluster().V1().ManagedClusters().Lister(), - clusterSetLister: clusterInformers.Cluster().V1beta2().ManagedClusterSets().Lister(), - clusterSetBindingLister: clusterInformers.Cluster().V1beta2().ManagedClusterSetBindings().Lister(), - clusterProfileClient: cpClient, - clusterProfileLister: cpInformers.Apis().V1alpha1().ClusterProfiles().Lister(), + kubeClient: kubeClient, + clusterLister: clusterInformers.Cluster().V1().ManagedClusters().Lister(), + clusterSetLister: clusterInformers.Cluster().V1beta2().ManagedClusterSets().Lister(), + clusterSetBindingLister: clusterInformers.Cluster().V1beta2().ManagedClusterSetBindings().Lister(), + clusterSetBindingIndexer: clusterInformers.Cluster().V1beta2().ManagedClusterSetBindings().Informer().GetIndexer(), + clusterProfileClient: cpClient, + clusterProfileLister: cpInformers.Apis().V1alpha1().ClusterProfiles().Lister(), } syncCtx := testingcommon.NewFakeSyncContext(t, c.key) diff --git a/pkg/registration/hub/manager.go b/pkg/registration/hub/manager.go index f5455f645..9569a1d8c 100644 --- a/pkg/registration/hub/manager.go +++ b/pkg/registration/hub/manager.go @@ -327,6 +327,7 @@ func (m *HubManagerOptions) RunControllerManagerWithInformers( var clusterProfileStatusController factory.Controller if features.HubMutableFeatureGate.Enabled(ocmfeature.ClusterProfile) { clusterProfileLifecycleController = clusterprofile.NewClusterProfileLifecycleController( + kubeClient, clusterInformers.Cluster().V1().ManagedClusters(), clusterInformers.Cluster().V1beta2().ManagedClusterSets(), clusterInformers.Cluster().V1beta2().ManagedClusterSetBindings(),