ACM-10828: sort resources for microshift install (stolostron#1395)

* sort resources Signed-off-by: Thibault Mange <[email protected]> * add addon finalizers perm Signed-off-by: Thibault Mange <[email protected]> * fix permissions and watcher Signed-off-by: Thibault Mange <[email protected]> * add roles exception for ownerref Signed-off-by: Thibault Mange <[email protected]> * fix cache noisy logs Signed-off-by: Thibault Mange <[email protected]> * fix globalRes Signed-off-by: Thibault Mange <[email protected]> * clean some logs Signed-off-by: Thibault Mange <[email protected]> * update depoyer update logs Signed-off-by: Thibault Mange <[email protected]> --------- Signed-off-by: Thibault Mange <[email protected]>
coleenquadros · Apr 29, 2024 · 8e82494 · 8e82494
1 parent 4e38380
commit 8e82494
Show file tree

Hide file tree

Showing 7 changed files with 83 additions and 28 deletions.
diff --git a/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go b/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go
@@ -240,14 +240,24 @@ func (r *ObservabilityAddonReconciler) Reconcile(ctx context.Context, req ctrl.R
 		}
 		deployer := deploying.NewDeployer(r.Client)
 		for _, res := range toDeploy {
+			if res.GetNamespace() != namespace {
+				globalRes = append(globalRes, res)
+			}
+
 			if !isHubMetricsCollector {
 				// For kind tests we need to deploy prometheus in hub but cannot set controller
 				// reference as there is no observabilityaddon
-				if err := controllerutil.SetControllerReference(obsAddon, res, r.Scheme); err != nil {
-					log.Info("Failed to set controller reference", "resource", res.GetName())
-					globalRes = append(globalRes, res)
+
+				// skip setting controller reference for resources that don't need it
+				// and for which we lack permission to set it
+				skipResources := []string{"Role", "RoleBinding", "ClusterRole", "ClusterRoleBinding"}
+				if !slices.Contains(skipResources, res.GetKind()) {
+					if err := controllerutil.SetControllerReference(obsAddon, res, r.Scheme); err != nil {
+						log.Info("Failed to set controller reference", "resource", res.GetName(), "kind", res.GetKind(), "error", err.Error())
+					}
 				}
 			}
+
 			if err := deployer.Deploy(res); err != nil {
 				return ctrl.Result{}, fmt.Errorf("failed to deploy %s %s/%s: %w", res.GetKind(), namespace, res.GetName(), err)
 			}
@@ -475,15 +485,20 @@ func (r *ObservabilityAddonReconciler) SetupWithManager(mgr ctrl.Manager) error
 	if os.Getenv("NAMESPACE") != "" {
 		namespace = os.Getenv("NAMESPACE")
 	}
-	return ctrl.NewControllerManagedBy(mgr).
-		For(
-			&oav1beta1.ObservabilityAddon{},
-			builder.WithPredicates(getPred(obAddonName, namespace, true, true, true)),
-		).
-		Watches(
+
+	ctrlBuilder := ctrl.NewControllerManagedBy(mgr).For(
+		&oav1beta1.ObservabilityAddon{},
+		builder.WithPredicates(getPred(obAddonName, namespace, true, true, true)),
+	)
+
+	if isHubMetricsCollector {
+		ctrlBuilder = ctrlBuilder.Watches(
 			&source.Kind{Type: &oav1beta2.MultiClusterObservability{}},
 			&handler.EnqueueRequestForObject{},
-		).
+		)
+	}
+
+	return ctrlBuilder.
 		Watches(
 			&source.Kind{Type: &corev1.Secret{}},
 			&handler.EnqueueRequestForObject{},

diff --git a/operators/endpointmetrics/main.go b/operators/endpointmetrics/main.go
@@ -99,9 +99,13 @@ func main() {
 		oav1beta1.GroupVersion.WithKind("ObservabilityAddon"): {
 			{FieldSelector: namespaceSelector},
 		},
-		oav1beta2.GroupVersion.WithKind("MultiClusterObservability"): {
+	}
+
+	// Only watch MCO CRs in the hub cluster to avoid noisy log messages
+	if os.Getenv("HUB_ENDPOINT_OPERATOR") == "true" {
+		gvkLabelMap[oav1beta2.GroupVersion.WithKind("MultiClusterObservability")] = []filteredcache.Selector{
 			{FieldSelector: "metadata.name!=null"},
-		},
+		}
 	}
 
 	mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{

diff --git a/operators/endpointmetrics/manifests/prometheus/prometheus-operator-role.yaml b/operators/endpointmetrics/manifests/prometheus/prometheus-operator-role.yaml
@@ -16,6 +16,7 @@ rules:
       - alertmanagerconfigs
       - prometheuses
       - prometheuses/finalizers
+      - prometheuses/status
       - thanosrulers
       - thanosrulers/finalizers
       - servicemonitors

diff --git a/operators/endpointmetrics/pkg/rendering/renderer.go b/operators/endpointmetrics/pkg/rendering/renderer.go
@@ -12,6 +12,7 @@ import (
 	"strings"
 
 	prometheusv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
+	"golang.org/x/exp/slices"
 	v1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -250,9 +251,27 @@ func Render(
 		}
 	}
 
+	// Ordering resources to ensure they are applied in the correct order
+	slices.SortFunc(resources, func(a, b *unstructured.Unstructured) bool {
+		return (resourcePriority(a) - resourcePriority(b)) < 0
+	})
+
 	return resources, nil
 }
 
+func resourcePriority(resource *unstructured.Unstructured) int {
+	switch resource.GetKind() {
+	case "Role", "ClusterRole":
+		return 1
+	case "RoleBinding", "ClusterRoleBinding":
+		return 2
+	case "CustomResourceDefinition":
+		return 3
+	default:
+		return 4
+	}
+}
+
 func getDisabledMetrics(c runtimeclient.Client) (string, error) {
 	cm := &corev1.ConfigMap{}
 	err := c.Get(context.TODO(), types.NamespacedName{Name: operatorconfig.AllowlistConfigMapName,

diff --git a/operators/endpointmetrics/pkg/rendering/renderer_test.go b/operators/endpointmetrics/pkg/rendering/renderer_test.go
@@ -58,6 +58,14 @@ func TestRender(t *testing.T) {
 	}
 
 	printObjs(t, objs)
+
+	// ensure that objects are sorted
+	for i := 0; i < len(objs)-1; i++ {
+		if resourcePriority(objs[i]) > resourcePriority(objs[i+1]) {
+			t.Errorf("objects are not sorted")
+		}
+	}
+
 }
 
 func printObjs(t *testing.T, objs []*unstructured.Unstructured) {

diff --git a/operators/multiclusterobservability/manifests/endpoint-observability/role.yaml b/operators/multiclusterobservability/manifests/endpoint-observability/role.yaml
@@ -63,6 +63,12 @@ rules:
   verbs:
   - get
   - update
+- apiGroups:
+  - observability.open-cluster-management.io
+  resources:
+  - observabilityaddons/finalizers
+  verbs:
+  - update
 - apiGroups:
   - config.openshift.io
   resources:

diff --git a/operators/pkg/deploying/deployer.go b/operators/pkg/deploying/deployer.go
@@ -66,7 +66,7 @@ func (d *Deployer) Deploy(obj *unstructured.Unstructured) error {
 	)
 	if err != nil {
 		if errors.IsNotFound(err) {
-			log.Info("Create", "Kind:", obj.GroupVersionKind(), "Name:", obj.GetName())
+			log.Info("Create", "Kind", obj.GroupVersionKind(), "Name", obj.GetName())
 			return d.client.Create(context.TODO(), obj)
 		}
 		return err
@@ -78,7 +78,7 @@ func (d *Deployer) Deploy(obj *unstructured.Unstructured) error {
 		annotations, ok := metadata["annotations"].(map[string]interface{})
 		if ok && annotations != nil && annotations[config.AnnotationSkipCreation] != nil {
 			if strings.ToLower(annotations[config.AnnotationSkipCreation].(string)) == "true" {
-				log.Info("Skip creation", "Kind:", obj.GroupVersionKind(), "Name:", obj.GetName())
+				log.Info("Skip creation", "Kind", obj.GroupVersionKind(), "Name", obj.GetName())
 				return nil
 			}
 		}
@@ -88,7 +88,7 @@ func (d *Deployer) Deploy(obj *unstructured.Unstructured) error {
 	if ok {
 		return deployerFn(obj, found)
 	} else {
-		log.Info("deployerFn not found", "kind:", found.GetKind())
+		log.Info("deployerFn not found", "kind", found.GetKind())
 	}
 	return nil
 }
@@ -109,7 +109,7 @@ func (d *Deployer) updateDeployment(desiredObj, runtimeObj *unstructured.Unstruc
 	}
 
 	if !apiequality.Semantic.DeepDerivative(desiredDepoly.Spec, runtimeDepoly.Spec) {
-		log.Info("Update", "Kind:", runtimeObj.GroupVersionKind(), "Name:", runtimeObj.GetName())
+		logUpdateInfo(runtimeObj)
 		return d.client.Update(context.TODO(), desiredDepoly)
 	}
 
@@ -133,7 +133,7 @@ func (d *Deployer) updateStatefulSet(desiredObj, runtimeObj *unstructured.Unstru
 
 	if !apiequality.Semantic.DeepDerivative(desiredDepoly.Spec.Template, runtimeDepoly.Spec.Template) ||
 		!apiequality.Semantic.DeepDerivative(desiredDepoly.Spec.Replicas, runtimeDepoly.Spec.Replicas) {
-		log.Info("Update", "Kind:", runtimeObj.GroupVersionKind(), "Name:", runtimeObj.GetName())
+		logUpdateInfo(runtimeObj)
 		runtimeDepoly.Spec.Replicas = desiredDepoly.Spec.Replicas
 		runtimeDepoly.Spec.Template = desiredDepoly.Spec.Template
 		return d.client.Update(context.TODO(), runtimeDepoly)
@@ -160,7 +160,7 @@ func (d *Deployer) updateService(desiredObj, runtimeObj *unstructured.Unstructur
 	if !apiequality.Semantic.DeepDerivative(desiredService.Spec, runtimeService.Spec) {
 		desiredService.ObjectMeta.ResourceVersion = runtimeService.ObjectMeta.ResourceVersion
 		desiredService.Spec.ClusterIP = runtimeService.Spec.ClusterIP
-		log.Info("Update", "Kind:", runtimeObj.GroupVersionKind(), "Name:", runtimeObj.GetName())
+		logUpdateInfo(runtimeObj)
 		return d.client.Update(context.TODO(), desiredService)
 	}
 
@@ -183,7 +183,7 @@ func (d *Deployer) updateConfigMap(desiredObj, runtimeObj *unstructured.Unstruct
 	}
 
 	if !apiequality.Semantic.DeepDerivative(desiredConfigMap.Data, runtimeConfigMap.Data) {
-		log.Info("Update", "Kind:", runtimeObj.GroupVersionKind(), "Name:", runtimeObj.GetName())
+		logUpdateInfo(runtimeObj)
 		return d.client.Update(context.TODO(), desiredConfigMap)
 	}
 
@@ -207,7 +207,7 @@ func (d *Deployer) updateSecret(desiredObj, runtimeObj *unstructured.Unstructure
 
 	if desiredSecret.Data == nil ||
 		!apiequality.Semantic.DeepDerivative(desiredSecret.Data, runtimeSecret.Data) {
-		log.Info("Update", "Kind:", desiredObj.GroupVersionKind(), "Name:", desiredObj.GetName())
+		logUpdateInfo(desiredObj)
 		return d.client.Update(context.TODO(), desiredSecret)
 	}
 	return nil
@@ -230,7 +230,7 @@ func (d *Deployer) updateClusterRole(desiredObj, runtimeObj *unstructured.Unstru
 
 	if !apiequality.Semantic.DeepDerivative(desiredClusterRole.Rules, runtimeClusterRole.Rules) ||
 		!apiequality.Semantic.DeepDerivative(desiredClusterRole.AggregationRule, runtimeClusterRole.AggregationRule) {
-		log.Info("Update", "Kind:", desiredObj.GroupVersionKind(), "Name:", desiredObj.GetName())
+		logUpdateInfo(desiredObj)
 		return d.client.Update(context.TODO(), desiredClusterRole)
 	}
 	return nil
@@ -253,7 +253,7 @@ func (d *Deployer) updateClusterRoleBinding(desiredObj, runtimeObj *unstructured
 
 	if !apiequality.Semantic.DeepDerivative(desiredClusterRoleBinding.Subjects, runtimeClusterRoleBinding.Subjects) ||
 		!apiequality.Semantic.DeepDerivative(desiredClusterRoleBinding.RoleRef, runtimeClusterRoleBinding.RoleRef) {
-		log.Info("Update", "Kind:", desiredObj.GroupVersionKind(), "Name:", desiredObj.GetName())
+		logUpdateInfo(desiredObj)
 		return d.client.Update(context.TODO(), desiredClusterRoleBinding)
 	}
 	return nil
@@ -276,7 +276,7 @@ func (d *Deployer) updateCRD(desiredObj, runtimeObj *unstructured.Unstructured)
 	desiredCRD.ObjectMeta.ResourceVersion = runtimeCRD.ObjectMeta.ResourceVersion
 
 	if !apiequality.Semantic.DeepDerivative(desiredCRD.Spec, runtimeCRD.Spec) {
-		log.Info("Update", "Kind:", runtimeObj.GroupVersionKind(), "Name:", runtimeObj.GetName())
+		logUpdateInfo(runtimeObj)
 		return d.client.Update(context.TODO(), desiredCRD)
 	}
 
@@ -306,8 +306,6 @@ func (d *Deployer) updatePrometheus(desiredObj, runtimeObj *unstructured.Unstruc
 	// 2. delete endpoint operator pod
 
 	// inherit resource version if not specified
-	log.Info("Desired Prometheus", "resourceVersion", desiredPrometheus.ResourceVersion)
-	log.Info("Runtime Prometheus", "resourceVersion", runtimePrometheus.ResourceVersion)
 	if desiredPrometheus.ResourceVersion != runtimePrometheus.ResourceVersion {
 		desiredPrometheus.ResourceVersion = runtimePrometheus.ResourceVersion
 	}
@@ -327,7 +325,7 @@ func (d *Deployer) updatePrometheus(desiredObj, runtimeObj *unstructured.Unstruc
 	}
 
 	if !apiequality.Semantic.DeepDerivative(desiredPrometheus.Spec, runtimePrometheus.Spec) {
-		log.Info("Update", "Kind:", runtimeObj.GroupVersionKind(), "Name:", runtimeObj.GetName())
+		logUpdateInfo(runtimeObj)
 		return d.client.Update(context.TODO(), desiredPrometheus)
 	} else {
 		log.Info("Runtime Prometheus and Desired Prometheus are semantically equal!")
@@ -351,7 +349,7 @@ func (d *Deployer) updatePrometheusRule(desiredObj, runtimeObj *unstructured.Uns
 	}
 
 	if !apiequality.Semantic.DeepDerivative(desiredPrometheusRule.Spec, runtimePrometheusRule.Spec) {
-		log.Info("Update", "Kind:", runtimeObj.GroupVersionKind(), "Name:", runtimeObj.GetName())
+		logUpdateInfo(runtimeObj)
 		if desiredPrometheusRule.ResourceVersion != runtimePrometheusRule.ResourceVersion {
 			desiredPrometheusRule.ResourceVersion = runtimePrometheusRule.ResourceVersion
 		}
@@ -377,9 +375,13 @@ func (d *Deployer) updateIngress(desiredObj, runtimeObj *unstructured.Unstructur
 	}
 
 	if !apiequality.Semantic.DeepDerivative(desiredIngress.Spec, runtimeIngress.Spec) {
-		log.Info("Update", "Kind:", runtimeObj.GroupVersionKind(), "Name:", runtimeObj.GetName())
+		logUpdateInfo(runtimeObj)
 		return d.client.Update(context.TODO(), desiredIngress)
 	}
 
 	return nil
 }
+
+func logUpdateInfo(obj *unstructured.Unstructured) {
+	log.Info("Update", "kind", obj.GroupVersionKind().Kind, "kindVersion", obj.GroupVersionKind().Version, "name", obj.GetName())
+}