🐛 enhance clusterprofile lifecycle controller (#1359)

* check namespace existence and state in clusterprofile lifecycle controller.

Signed-off-by: Morven Cao <lcao@redhat.com>

* optimize the queue key and log for clusterprofile controller.

Signed-off-by: Morven Cao <lcao@redhat.com>

---------

Signed-off-by: Morven Cao <lcao@redhat.com>

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)
 }
 

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)

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(),