Make sure local cluster is always in the managedcluster list (#1637)

* make sure local cluster is part of list all the time Signed-off-by: Coleen Iona Quadros <[email protected]> * fix tests Signed-off-by: Coleen Iona Quadros <[email protected]> * use sync map Signed-off-by: Coleen Iona Quadros <[email protected]> * lint Signed-off-by: Coleen Iona Quadros <[email protected]> --------- Signed-off-by: Coleen Iona Quadros <[email protected]>
stolostron · Sep 30, 2024 · 4ade8e0 · 4ade8e0
1 parent 4f2fb5f
commit 4ade8e0
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 52 deletions.
diff --git a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go
@@ -61,8 +61,7 @@ var (
 	clusterAddon                  = &addonv1alpha1.ClusterManagementAddOn{}
 	defaultAddonDeploymentConfig  = &addonv1alpha1.AddOnDeploymentConfig{}
 	isplacementControllerRunnning = false
-	managedClusterList            = map[string]string{}
-	managedClusterListMutex       = &sync.RWMutex{}
+	managedClusterList            = sync.Map{}
 	installMetricsWithoutAddon    = false
 )
 
@@ -110,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
@@ -127,9 +126,21 @@ func (r *PlacementRuleReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	// 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
-	if req.Name == "local-cluster" {
+	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
@@ -375,8 +386,9 @@ func createAllRelatedRes(
 	}
 
 	failedCreateManagedClusterRes := false
-	managedClusterListMutex.RLock()
-	for managedCluster, openshiftVersion := range managedClusterList {
+	managedClusterList.Range(func(key, value interface{}) bool {
+		managedCluster := key.(string)
+		openshiftVersion := value.(string)
 		if isReconcileRequired(request, managedCluster) {
 			log.Info(
 				"Monitoring operator should be installed in cluster",
@@ -417,22 +429,21 @@ 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[ep.Namespace]; !ok {
+		if _, ok := managedClusterList.Load(ep.Namespace); !ok {
 			clustersToCleanup = append(clustersToCleanup, ep.Namespace)
 		}
 	}
 
-	managedClusterListMutex.RUnlock()
-
 	failedDeleteOba := false
 	for _, cluster := range clustersToCleanup {
 		err = deleteObsAddon(c, cluster)
@@ -595,18 +606,12 @@ func areManagedClusterLabelsReady(obj client.Object) bool {
 }
 
 func updateManagedClusterList(obj client.Object) {
-	managedClusterListMutex.Lock()
-	defer managedClusterListMutex.Unlock()
 	//ACM 8509: Special case for local-cluster, we deploy endpoint and metrics collector in the hub
 	//whether hubSelfManagement is enabled or not
-	if obj.GetName() == localClusterName {
-		managedClusterList[obj.GetName()] = "mimical"
-		return
-	}
 	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)
 	}
 }
 

diff --git a/...tors/multiclusterobservability/controllers/placementrule/placementrule_controller_test.go b/...tors/multiclusterobservability/controllers/placementrule/placementrule_controller_test.go
@@ -205,10 +205,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)
@@ -228,7 +227,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)

diff --git a/operators/multiclusterobservability/controllers/placementrule/predicate_func.go b/operators/multiclusterobservability/controllers/placementrule/predicate_func.go
@@ -9,6 +9,8 @@ import (
 	"reflect"
 	"strings"
 
+	clusterv1 "open-cluster-management.io/api/cluster/v1"
+
 	"github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/pkg/config"
 	operatorconfig "github.com/stolostron/multicluster-observability-operator/operators/pkg/config"
 
@@ -31,7 +33,9 @@ func getClusterPreds() predicate.Funcs {
 		if !areManagedClusterLabelsReady(e.Object) {
 			return false
 		}
-		updateManagedClusterList(e.Object)
+		if e.Object.GetName() != localClusterName {
+			updateManagedClusterList(e.Object)
+		}
 
 		return true
 	}
@@ -48,9 +52,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()
@@ -59,7 +61,15 @@ func getClusterPreds() predicate.Funcs {
 			if !areManagedClusterLabelsReady(e.ObjectNew) {
 				return false
 			}
-			updateManagedClusterList(e.ObjectNew)
+			if e.ObjectNew.GetName() != localClusterName {
+				updateManagedClusterList(e.ObjectNew)
+			}
+
+		}
+		//log the diff in managedccluster object
+		if !reflect.DeepEqual(e.ObjectNew.(*clusterv1.ManagedCluster), e.ObjectOld.(*clusterv1.ManagedCluster)) {
+			log.Info("managedcluster object New diff", "managedCluster", e.ObjectNew.GetName(), "diff", fmt.Sprintf("%+v", e.ObjectNew.(*clusterv1.ManagedCluster)))
+			log.Info("managedcluster object Old diff", "managedCluster", e.ObjectOld.GetName(), "diff", fmt.Sprintf("%+v", e.ObjectOld.(*clusterv1.ManagedCluster)))
 		}
 
 		return true
@@ -72,9 +82,9 @@ func getClusterPreds() predicate.Funcs {
 			return false
 		}
 
-		managedClusterListMutex.Lock()
-		delete(managedClusterList, e.Object.GetName())
-		managedClusterListMutex.Unlock()
+		if e.Object.GetName() != localClusterName {
+			managedClusterList.Delete(e.Object.GetName())
+		}
 		managedClusterImageRegistryMutex.Lock()
 		delete(managedClusterImageRegistry, e.Object.GetName())
 		managedClusterImageRegistryMutex.Unlock()

diff --git a/operators/multiclusterobservability/controllers/placementrule/predicate_func_test.go b/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)