[release-2.10] Make sure local cluster is always in the managedcluster list

operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go

@@ -63,7 +63,7 @@ var (
 	clusterAddon                  = &addonv1alpha1.ClusterManagementAddOn{}
 	defaultAddonDeploymentConfig  = &addonv1alpha1.AddOnDeploymentConfig{}
 	isplacementControllerRunnning = false
-	managedClusterList            = map[string]string{}
+	managedClusterList            = sync.Map{}
 	managedClusterListMutex       = &sync.RWMutex{}
 	installMetricsWithoutAddon    = false
 )
@@ -94,25 +94,6 @@ func (r *PlacementRuleReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	reqLogger := log.WithValues("Request.Namespace", req.Namespace, "Request.Name", req.Name)
 	reqLogger.Info("Reconciling PlacementRule")
 
-	// ACM 8509: Special case for hub/local cluster metrics collection
-	// We want to ensure that the local-cluster is always in the managedClusterList
-	// In the case when hubSelfManagement is enabled, we will delete it from the list and modify the object
-	// to cater to the use case of deploying in open-cluster-management-observability namespace
-	delete(managedClusterList, "local-cluster")
-	if _, ok := managedClusterList["local-cluster"]; !ok {
-		obj := &clusterv1.ManagedCluster{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "local-cluster",
-				Namespace: config.GetDefaultNamespace(),
-				Labels: map[string]string{
-					"openshiftVersion": "mimical",
-				},
-			},
-		}
-		installMetricsWithoutAddon = true
-		updateManagedClusterList(obj)
-	}
-
 	if config.GetMonitoringCRName() == "" {
 		reqLogger.Info("multicluster observability resource is not available")
 		return ctrl.Result{}, nil
@@ -128,7 +109,7 @@ func (r *PlacementRuleReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	if err != nil {
 		if k8serrors.IsNotFound(err) {
 			deleteAll = true
-			delete(managedClusterList, "local-cluster")
+			managedClusterList.Delete("local-cluster")
 		} else {
 			// Error reading the object - requeue the request.
 			return ctrl.Result{}, err
@@ -141,6 +122,25 @@ func (r *PlacementRuleReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		return ctrl.Result{}, nil
 	}
 
+	// ACM 8509: Special case for hub/local cluster metrics collection
+	// We want to ensure that the local-cluster is always in the managedClusterList
+	// In the case when hubSelfManagement is enabled, we will delete it from the list and modify the object
+	// to cater to the use case of deploying in open-cluster-management-observability namespace
+	managedClusterList.Delete("local-cluster")
+	if _, ok := managedClusterList.Load("local-cluster"); !ok {
+		obj := &clusterv1.ManagedCluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "local-cluster",
+				Namespace: config.GetDefaultNamespace(),
+				Labels: map[string]string{
+					"openshiftVersion": "mimical",
+				},
+			},
+		}
+		installMetricsWithoutAddon = true
+		updateManagedClusterList(obj)
+	}
+
 	if !deleteAll && !mco.Spec.ObservabilityAddonSpec.EnableMetrics {
 		reqLogger.Info("EnableMetrics is set to false. Delete Observability addons")
 		deleteAll = true
@@ -196,9 +196,6 @@ func (r *PlacementRuleReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 			return ctrl.Result{}, err
 		}
 	}
-	if operatorconfig.IsMCOTerminating {
-		delete(managedClusterList, "local-cluster")
-	}
 
 	if !deleteAll {
 		if err := createAllRelatedRes(
@@ -398,7 +395,10 @@ func createAllRelatedRes(
 
 	failedCreateManagedClusterRes := false
 	managedClusterListMutex.RLock()
-	for managedCluster, openshiftVersion := range managedClusterList {
+
+	managedClusterList.Range(func(key, value interface{}) bool {
+		managedCluster := key.(string)
+		openshiftVersion := value.(string)
 		currentClusters = commonutil.Remove(currentClusters, managedCluster)
 		if isReconcileRequired(request, managedCluster) {
 			log.Info(
@@ -440,20 +440,28 @@ func createAllRelatedRes(
 				log.Error(err, "Failed to create managedcluster resources", "namespace", managedCluster)
 			}
 			if request.Namespace == managedCluster {
-				break
+				return false
 			}
 		}
+		return true
+	})
+
+	// Look through the obsAddonList items and find clusters
+	// which are no longer to be managed and therefore needs deletion
+	clustersToCleanup := []string{}
+	for _, ep := range obsAddonList.Items {
+		if _, ok := managedClusterList.Load(ep.Namespace); !ok {
+			clustersToCleanup = append(clustersToCleanup, ep.Namespace)
+		}
 	}
 	managedClusterListMutex.RUnlock()
 
 	failedDeleteOba := false
-	for _, cluster := range currentClusters {
-		if cluster != config.GetDefaultNamespace() {
-			err = deleteObsAddon(c, cluster)
-			if err != nil {
-				failedDeleteOba = true
-				log.Error(err, "Failed to delete observabilityaddon", "namespace", cluster)
-			}
+	for _, cluster := range clustersToCleanup {
+		err = deleteObsAddon(c, cluster)
+		if err != nil {
+			failedDeleteOba = true
+			log.Error(err, "Failed to delete observabilityaddon", "namespace", cluster)
 		}
 	}
 
@@ -613,9 +621,9 @@ func updateManagedClusterList(obj client.Object) {
 	managedClusterListMutex.Lock()
 	defer managedClusterListMutex.Unlock()
 	if version, ok := obj.GetLabels()["openshiftVersion"]; ok {
-		managedClusterList[obj.GetName()] = version
+		managedClusterList.Store(obj.GetName(), version)
 	} else {
-		managedClusterList[obj.GetName()] = nonOCP
+		managedClusterList.Store(obj.GetName(), nonOCP)
 	}
 }
 

operators/multiclusterobservability/controllers/placementrule/placementrule_controller_test.go

@@ -193,10 +193,9 @@ func TestObservabilityAddonController(t *testing.T) {
 		},
 	}
 
-	managedClusterList = map[string]string{
-		namespace:  "4",
-		namespace2: "4",
-	}
+	managedClusterList.Store(namespace, "4")
+	managedClusterList.Store(namespace2, "4")
+
 	_, err := r.Reconcile(context.TODO(), req)
 	if err != nil {
 		t.Fatalf("reconcile: (%v)", err)
@@ -216,7 +215,11 @@ func TestObservabilityAddonController(t *testing.T) {
 		t.Fatalf("Deprecated role not removed")
 	}
 
-	managedClusterList = map[string]string{namespace: "4"}
+	managedClusterList.Range(func(key, value interface{}) bool {
+		managedClusterList.Delete(key)
+		return true
+	})
+	managedClusterList.Store(namespace, "4")
 	_, err = r.Reconcile(context.TODO(), req)
 	if err != nil {
 		t.Fatalf("reconcile: (%v)", err)

operators/multiclusterobservability/controllers/placementrule/predicate_func.go

@@ -24,32 +24,26 @@ func getClusterPreds() predicate.Funcs {
 	createFunc := func(e event.CreateEvent) bool {
 		log.Info("CreateFunc", "managedCluster", e.Object.GetName())
 
-		//ACM 8509: Special case for local-cluster, we do not react changes to
-		//local-cluster as it is expected to be always present in the managed cluster list
-		//whether hubSelfManagement is enabled or not
-		if e.Object.GetName() == "local-cluster" {
-			return false
-		}
-
 		if isAutomaticAddonInstallationDisabled(e.Object) {
 			return false
 		}
 		updateManagedClusterImageRegistry(e.Object)
 		if !areManagedClusterLabelsReady(e.Object) {
 			return false
 		}
-		updateManagedClusterList(e.Object)
+		//ACM 8509: Special case for local-cluster, we do not react changes to
+		//local-cluster as it is expected to be always present in the managed cluster list
+		//whether hubSelfManagement is enabled or not
+		if e.Object.GetName() != localClusterName {
+			updateManagedClusterList(e.Object)
+		}
 
 		return true
 	}
 
 	updateFunc := func(e event.UpdateEvent) bool {
 		log.Info("UpdateFunc", "managedCluster", e.ObjectNew.GetName())
 
-		if e.ObjectNew.GetName() == "local-cluster" {
-			return false
-		}
-
 		if e.ObjectNew.GetResourceVersion() == e.ObjectOld.GetResourceVersion() {
 			return false
 		}
@@ -59,9 +53,7 @@ func getClusterPreds() predicate.Funcs {
 
 		if e.ObjectNew.GetDeletionTimestamp() != nil {
 			log.Info("managedcluster is in terminating state", "managedCluster", e.ObjectNew.GetName())
-			managedClusterListMutex.Lock()
-			delete(managedClusterList, e.ObjectNew.GetName())
-			managedClusterListMutex.Unlock()
+			managedClusterList.Delete(e.ObjectNew.GetName())
 			managedClusterImageRegistryMutex.Lock()
 			delete(managedClusterImageRegistry, e.ObjectNew.GetName())
 			managedClusterImageRegistryMutex.Unlock()
@@ -70,7 +62,13 @@ func getClusterPreds() predicate.Funcs {
 			if !areManagedClusterLabelsReady(e.ObjectNew) {
 				return false
 			}
-			updateManagedClusterList(e.ObjectNew)
+			//ACM 8509: Special case for local-cluster, we do not react changes to
+			//local-cluster as it is expected to be always present in the managed cluster list
+			//whether hubSelfManagement is enabled or not
+			if e.ObjectNew.GetName() != localClusterName {
+				updateManagedClusterList(e.ObjectNew)
+			}
+
 		}
 
 		return true
@@ -79,17 +77,16 @@ func getClusterPreds() predicate.Funcs {
 	deleteFunc := func(e event.DeleteEvent) bool {
 		log.Info("DeleteFunc", "managedCluster", e.Object.GetName())
 
-		if e.Object.GetName() == "local-cluster" {
-			return false
-		}
-
 		if isAutomaticAddonInstallationDisabled(e.Object) {
 			return false
 		}
 
-		managedClusterListMutex.Lock()
-		delete(managedClusterList, e.Object.GetName())
-		managedClusterListMutex.Unlock()
+		//ACM 8509: Special case for local-cluster, we do not react changes to
+		//local-cluster as it is expected to be always present in the managed cluster list
+		//whether hubSelfManagement is enabled or not
+		if e.Object.GetName() != localClusterName {
+			managedClusterList.Delete(e.Object.GetName())
+		}
 		managedClusterImageRegistryMutex.Lock()
 		delete(managedClusterImageRegistry, e.Object.GetName())
 		managedClusterImageRegistryMutex.Unlock()

operators/multiclusterobservability/controllers/placementrule/predicate_func_test.go

@@ -8,9 +8,10 @@ import (
 	"testing"
 	"time"
 
+	clusterv1 "open-cluster-management.io/api/cluster/v1"
+
 	addonv1alpha1 "open-cluster-management.io/api/addon/v1alpha1"
 
-	appsv1 "k8s.io/api/apps/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 )
@@ -66,15 +67,16 @@ func TestClusterPred(t *testing.T) {
 		t.Run(c.caseName, func(t *testing.T) {
 			pred := getClusterPreds()
 			create_event := event.CreateEvent{
-				Object: &appsv1.Deployment{
+				Object: &clusterv1.ManagedCluster{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:        name,
-						Namespace:   c.namespace,
-						Annotations: c.annotations,
-						Labels:      map[string]string{"vendor": "OpenShift", "openshiftVersion": "4.6.0"},
+						Name:            name,
+						Namespace:       c.namespace,
+						Annotations:     c.annotations,
+						Labels:          map[string]string{"vendor": "OpenShift", "openshiftVersion": "4.6.0"},
+						ResourceVersion: "1",
 					},
-					Spec: appsv1.DeploymentSpec{
-						Replicas: int32Ptr(2),
+					Spec: clusterv1.ManagedClusterSpec{
+						HubAcceptsClient: true,
 					},
 				},
 			}
@@ -90,7 +92,7 @@ func TestClusterPred(t *testing.T) {
 			}
 
 			update_event := event.UpdateEvent{
-				ObjectNew: &appsv1.Deployment{
+				ObjectNew: &clusterv1.ManagedCluster{
 					ObjectMeta: metav1.ObjectMeta{
 						Name:              name,
 						Namespace:         c.namespace,
@@ -100,7 +102,7 @@ func TestClusterPred(t *testing.T) {
 						Labels:            map[string]string{"vendor": "OpenShift", "openshiftVersion": "4.6.0"},
 					},
 				},
-				ObjectOld: &appsv1.Deployment{
+				ObjectOld: &clusterv1.ManagedCluster{
 					ObjectMeta: metav1.ObjectMeta{
 						Name:            name,
 						Namespace:       c.namespace,
@@ -125,14 +127,14 @@ func TestClusterPred(t *testing.T) {
 			}
 
 			delete_event := event.DeleteEvent{
-				Object: &appsv1.Deployment{
+				Object: &clusterv1.ManagedCluster{
 					ObjectMeta: metav1.ObjectMeta{
 						Name:        name,
 						Namespace:   c.namespace,
 						Annotations: c.annotations,
 					},
-					Spec: appsv1.DeploymentSpec{
-						Replicas: int32Ptr(2),
+					Spec: clusterv1.ManagedClusterSpec{
+						HubAcceptsClient: true,
 					},
 				},
 			}
@@ -196,19 +198,18 @@ func TestManagedClusterLabelReady(t *testing.T) {
 		t.Run(c.caseName, func(t *testing.T) {
 			pred := getClusterPreds()
 			create_event := event.CreateEvent{
-				Object: &appsv1.Deployment{
+				Object: &clusterv1.ManagedCluster{
 					ObjectMeta: metav1.ObjectMeta{
 						Name:        name,
 						Namespace:   c.namespace,
 						Annotations: c.annotations,
 						Labels:      c.labels,
 					},
-					Spec: appsv1.DeploymentSpec{
-						Replicas: int32Ptr(2),
+					Spec: clusterv1.ManagedClusterSpec{
+						HubAcceptsClient: true,
 					},
 				},
 			}
-
 			if c.expectedCreate {
 				if !pred.CreateFunc(create_event) {
 					t.Fatalf("pre func return false on applied createevent in case: (%v)", c.caseName)
@@ -218,8 +219,9 @@ func TestManagedClusterLabelReady(t *testing.T) {
 					t.Fatalf("pre func return true on non-applied createevent in case: (%v)", c.caseName)
 				}
 			}
+
 			update_event := event.UpdateEvent{
-				ObjectNew: &appsv1.Deployment{
+				ObjectNew: &clusterv1.ManagedCluster{
 					ObjectMeta: metav1.ObjectMeta{
 						Name:              name,
 						Namespace:         c.namespace,
@@ -229,15 +231,15 @@ func TestManagedClusterLabelReady(t *testing.T) {
 						Labels:            c.labels,
 					},
 				},
-				ObjectOld: &appsv1.Deployment{
+
+				ObjectOld: &clusterv1.ManagedCluster{
 					ObjectMeta: metav1.ObjectMeta{
 						Name:            name,
 						Namespace:       c.namespace,
 						ResourceVersion: "1",
 					},
 				},
 			}
-
 			if c.expectedUpdate {
 				if !pred.UpdateFunc(update_event) {
 					t.Fatalf("pre func return false on applied update event in case: (%v)", c.caseName)