From 5282eee5ea1bc733554a9ee06e72678a5ed893ef Mon Sep 17 00:00:00 2001 From: Coleen Iona Quadros Date: Tue, 24 Sep 2024 17:51:50 +0200 Subject: [PATCH 1/4] make sure local cluster is part of list all the time Signed-off-by: Coleen Iona Quadros --- .../placementrule/placementrule_controller.go | 14 +++++++++++++- .../controllers/placementrule/predicate_func.go | 6 ++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go index 088c1dfd1..ed169caa6 100644 --- a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go +++ b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go @@ -127,9 +127,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" { + 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 !deleteAll && !mco.Spec.ObservabilityAddonSpec.EnableMetrics { reqLogger.Info("EnableMetrics is set to false. Delete Observability addons") deleteAll = true diff --git a/operators/multiclusterobservability/controllers/placementrule/predicate_func.go b/operators/multiclusterobservability/controllers/placementrule/predicate_func.go index 8e8d5a1a3..4fb70e3ce 100644 --- a/operators/multiclusterobservability/controllers/placementrule/predicate_func.go +++ b/operators/multiclusterobservability/controllers/placementrule/predicate_func.go @@ -6,6 +6,7 @@ package placementrule import ( "fmt" + clusterv1 "open-cluster-management.io/api/cluster/v1" "reflect" "strings" @@ -61,6 +62,11 @@ func getClusterPreds() predicate.Funcs { } 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 } From aff406c3c9ad1bdc07c57a27bee35f50fafa68a5 Mon Sep 17 00:00:00 2001 From: Coleen Iona Quadros Date: Tue, 24 Sep 2024 19:51:10 +0200 Subject: [PATCH 2/4] fix tests Signed-off-by: Coleen Iona Quadros --- .../placementrule/placementrule_controller.go | 32 ++++++-------- .../placementrule_controller_test.go | 13 +++--- .../placementrule/predicate_func.go | 22 ++++++---- .../placementrule/predicate_func_test.go | 42 ++++++++++--------- 4 files changed, 56 insertions(+), 53 deletions(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go index ed169caa6..6d598bf8e 100644 --- a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go +++ b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go @@ -61,7 +61,7 @@ var ( clusterAddon = &addonv1alpha1.ClusterManagementAddOn{} defaultAddonDeploymentConfig = &addonv1alpha1.AddOnDeploymentConfig{} isplacementControllerRunnning = false - managedClusterList = map[string]string{} + managedClusterList = sync.Map{} managedClusterListMutex = &sync.RWMutex{} installMetricsWithoutAddon = false ) @@ -110,7 +110,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,8 +127,8 @@ 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 - delete(managedClusterList, "local-cluster") - if _, ok := managedClusterList["local-cluster"]; !ok { + managedClusterList.Delete("local-cluster") + if _, ok := managedClusterList.Load("local-cluster"); !ok { obj := &clusterv1.ManagedCluster{ ObjectMeta: metav1.ObjectMeta{ Name: "local-cluster", @@ -387,8 +387,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", @@ -429,22 +430,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) @@ -607,18 +607,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/operators/multiclusterobservability/controllers/placementrule/placementrule_controller_test.go b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller_test.go index 80207e4a7..6bba39945 100644 --- a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller_test.go +++ b/operators/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 index 4fb70e3ce..ad5287c81 100644 --- a/operators/multiclusterobservability/controllers/placementrule/predicate_func.go +++ b/operators/multiclusterobservability/controllers/placementrule/predicate_func.go @@ -6,10 +6,11 @@ package placementrule import ( "fmt" - clusterv1 "open-cluster-management.io/api/cluster/v1" "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" @@ -32,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 } @@ -49,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() @@ -60,7 +61,10 @@ 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)) { @@ -78,9 +82,9 @@ func getClusterPreds() predicate.Funcs { return false } - managedClusterListMutex.Lock() - delete(managedClusterList, e.Object.GetName()) - managedClusterListMutex.Unlock() + if e.Object.GetName() != localClusterName { + updateManagedClusterList(e.Object) + } 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 index 9be504e58..e8160d8dc 100644 --- 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,7 +231,8 @@ func TestManagedClusterLabelReady(t *testing.T) { Labels: c.labels, }, }, - ObjectOld: &appsv1.Deployment{ + + ObjectOld: &clusterv1.ManagedCluster{ ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: c.namespace, @@ -237,7 +240,6 @@ func TestManagedClusterLabelReady(t *testing.T) { }, }, } - if c.expectedUpdate { if !pred.UpdateFunc(update_event) { t.Fatalf("pre func return false on applied update event in case: (%v)", c.caseName) From b785a601ecfe4de0cc757d84e8e2a9136e446310 Mon Sep 17 00:00:00 2001 From: Coleen Iona Quadros Date: Wed, 25 Sep 2024 13:28:51 +0200 Subject: [PATCH 3/4] use sync map Signed-off-by: Coleen Iona Quadros --- .../controllers/placementrule/predicate_func.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/predicate_func.go b/operators/multiclusterobservability/controllers/placementrule/predicate_func.go index ad5287c81..61027c878 100644 --- a/operators/multiclusterobservability/controllers/placementrule/predicate_func.go +++ b/operators/multiclusterobservability/controllers/placementrule/predicate_func.go @@ -83,7 +83,7 @@ func getClusterPreds() predicate.Funcs { } if e.Object.GetName() != localClusterName { - updateManagedClusterList(e.Object) + managedClusterList.Delete(e.Object.GetName()) } managedClusterImageRegistryMutex.Lock() delete(managedClusterImageRegistry, e.Object.GetName()) From 328727f684c4e5a2d86435143230572fe65ee0b1 Mon Sep 17 00:00:00 2001 From: Coleen Iona Quadros Date: Wed, 25 Sep 2024 13:37:40 +0200 Subject: [PATCH 4/4] lint Signed-off-by: Coleen Iona Quadros --- .../controllers/placementrule/placementrule_controller.go | 1 - 1 file changed, 1 deletion(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go index 6d598bf8e..93eebc5bd 100644 --- a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go +++ b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go @@ -62,7 +62,6 @@ var ( defaultAddonDeploymentConfig = &addonv1alpha1.AddOnDeploymentConfig{} isplacementControllerRunnning = false managedClusterList = sync.Map{} - managedClusterListMutex = &sync.RWMutex{} installMetricsWithoutAddon = false )