Merge pull request #628 from darkdoc/improve_acm_check

feat: haveACMHub and usage refactor

Author: openshift-merge-bot[bot]

internal/controller/acm.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"strings"
 
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -30,12 +31,16 @@ import (
 func haveACMHub(r *PatternReconciler) bool {
 	gvrMCH := schema.GroupVersionResource{Group: "operator.open-cluster-management.io", Version: "v1", Resource: "multiclusterhubs"}
 
-	_, err := r.dynamicClient.Resource(gvrMCH).Namespace("open-cluster-management").Get(context.Background(), "multiclusterhub", metav1.GetOptions{})
+	mch, err := r.dynamicClient.Resource(gvrMCH).Namespace("open-cluster-management").Get(context.Background(), "multiclusterhub", metav1.GetOptions{})
 	if err != nil {
 		log.Printf("Error obtaining hub: %s\n", err)
 		return false
 	}
-	return true
+
+	return strings.EqualFold(
+		mch.GetAnnotations()["patterns.gitops.validatedpatterns.io/managed"],
+		"true",
+	)
 }
 
 // listManagedClusters lists all ManagedCluster resources (excluding local-cluster)

internal/controller/acm_test.go

@@ -41,7 +41,7 @@ var _ = Describe("HaveACMHub", func() {
 
 	})
 
-	Context("when the ACM Hub exists", func() {
+	Context("when the ACM Hub exists, and owned by the pattern operator", func() {
 		BeforeEach(func() {
 			hub := &unstructured.Unstructured{
 				Object: map[string]any{
@@ -50,6 +50,9 @@ var _ = Describe("HaveACMHub", func() {
 					"metadata": map[string]any{
 						"name":      "multiclusterhub",
 						"namespace": "open-cluster-management",
+						"annotations": map[string]any{
+							"patterns.gitops.validatedpatterns.io/managed": "true",
+						},
 					},
 				},
 			}
@@ -63,21 +66,30 @@ var _ = Describe("HaveACMHub", func() {
 		})
 	})
 
-	Context("when the ACM Hub does not exist", func() {
+	Context("when the ACM Hub exists, and NOT owned by the pattern operator", func() {
+		BeforeEach(func() {
+			hub := &unstructured.Unstructured{
+				Object: map[string]any{
+					"apiVersion": "operator.open-cluster-management.io/v1",
+					"kind":       "MultiClusterHub",
+					"metadata": map[string]any{
+						"name":      "multiclusterhub",
+						"namespace": "open-cluster-management",
+					},
+				},
+			}
+			_, err := dynamicClient.Resource(gvrMCH).Namespace("open-cluster-management").Create(context.Background(), hub, metav1.CreateOptions{})
+			Expect(err).ToNot(HaveOccurred())
+		})
+
 		It("should return false", func() {
 			result := haveACMHub(patternReconciler)
 			Expect(result).To(BeFalse())
 		})
 	})
 
-	Context("when there is an error listing ConfigMaps", func() {
-		BeforeEach(func() {
-			kubeClient.PrependReactor("list", "configmaps", func(testing.Action) (handled bool, ret runtime.Object, err error) {
-				return true, nil, fmt.Errorf("config map error")
-			})
-		})
-
-		It("should return false and log the error", func() {
+	Context("when the ACM Hub does not exist", func() {
+		It("should return false", func() {
 			result := haveACMHub(patternReconciler)
 			Expect(result).To(BeFalse())
 		})

internal/controller/pattern_controller.go

@@ -721,17 +721,21 @@ func (r *PatternReconciler) deleteHubApps(targetApp, app *argoapi.Application, n
 	// Delete managed clusters (excluding local-cluster)
 	// These must be removed before hub deletion can proceed because ACM won't delete properly if they exist
 	// we do not care about the error, since we might be on a standalone cluster
-	managedClusters, _ := r.listManagedClusters(context.Background())
+	// Only do this if the pattern is in charge of the acm hub
 
-	if len(managedClusters) > 0 {
-		deletedCount, err := r.deleteManagedClusters(context.TODO())
-		if err != nil {
-			return fmt.Errorf("failed to delete managed clusters: %w", err)
-		}
+	if haveACMHub(r) {
+		managedClusters, _ := r.listManagedClusters(context.Background())
+
+		if len(managedClusters) > 0 {
+			deletedCount, err := r.deleteManagedClusters(context.TODO())
+			if err != nil {
+				return fmt.Errorf("failed to delete managed clusters: %w", err)
+			}
 
-		if deletedCount > 0 {
-			log.Printf("Deleted %d managed cluster(s), waiting for them to be fully removed", deletedCount)
-			return fmt.Errorf("deleted %d managed cluster(s), waiting for removal to complete before proceeding with hub deletion", deletedCount)
+			if deletedCount > 0 {
+				log.Printf("Deleted %d managed cluster(s), waiting for them to be fully removed", deletedCount)
+				return fmt.Errorf("deleted %d managed cluster(s), waiting for removal to complete before proceeding with hub deletion", deletedCount)
+			}
 		}
 	}
 
@@ -784,17 +788,9 @@ func (r *PatternReconciler) finalizeObject(instance *api.Pattern) error {
 		// Initialize deletion phase if not set
 		if qualifiedInstance.Status.DeletionPhase == api.InitializeDeletion {
 			log.Printf("Initializing deletion phase")
-			if haveACMHub(r) {
-				if err := r.updateDeletionPhase(qualifiedInstance, api.DeleteSpokeChildApps); err != nil {
-					return err
-				}
-			} else {
-				// There is no acm/spoke, we can directly start cleaning up child apps (from hub)
-				if err := r.updateDeletionPhase(qualifiedInstance, api.DeleteHubChildApps); err != nil {
-					return err
-				}
+			if err := r.updateDeletionPhase(qualifiedInstance, api.DeleteSpokeChildApps); err != nil {
+				return err
 			}
-
 			return fmt.Errorf("initialized deletion phase, requeueing now")
 		}