IBGU: Ensure managed cluster labels before creating CGUs (#861)

- Make sure managed cluster labels are synced with ibgu progress before creating CGUs. - Update managed cluster labels in best effort manner. Signed-off-by: Saeid Askari <[email protected]>
openshift-kni · Dec 13, 2024 · a57a58a · a57a58a
1 parent d0e0823
commit a57a58a
Show file tree

Hide file tree

Showing 2 changed files with 112 additions and 7 deletions.
diff --git a/controllers/imagebasedgroupupgrade.go b/controllers/imagebasedgroupupgrade.go
@@ -97,6 +97,12 @@ func (r *IBGUReconciler) Reconcile(ctx context.Context, req ctrl.Request) (nextR
 		r.Log.Error(err, "Failed to get IBGU")
 		return requeueWithError(err)
 	}
+	err = r.ensureClusterLabels(ctx, ibgu)
+	if err != nil {
+		r.Log.Error(err, "error ensuring cluster labels")
+		return requeueWithError(err)
+	}
+
 	err = r.ensureManifests(ctx, ibgu)
 	if err != nil {
 		r.Log.Error(err, "error ensure manifests")
@@ -125,6 +131,7 @@ func (r *IBGUReconciler) Reconcile(ctx context.Context, req ctrl.Request) (nextR
 }
 
 func (r *IBGUReconciler) ensureClusterLabels(ctx context.Context, ibgu *ibguv1alpha1.ImageBasedGroupUpgrade) error {
+	failedClusters := []string{}
 	for _, clusterState := range ibgu.Status.Clusters {
 		labelsToAdd := make(map[string]string)
 		for _, action := range clusterState.FailedActions {
@@ -140,37 +147,52 @@ func (r *IBGUReconciler) ensureClusterLabels(ctx context.Context, ibgu *ibguv1al
 			labelsToAdd[fmt.Sprintf(utils.IBGUActionCompletedLabelTemplate, strings.ToLower(action.Action))] = ""
 		}
 		if !removeLabels && len(labelsToAdd) == 0 {
-			return nil
+			continue
 		}
 		cluster := &clusterv1.ManagedCluster{}
 		if err := r.Get(ctx, types.NamespacedName{Name: clusterState.Name}, cluster); err != nil {
-			return fmt.Errorf("failed to get managed cluster: %w", err)
+			r.Log.Error(err, "failed to get managed cluster")
+			failedClusters = append(failedClusters, clusterState.Name)
+			continue
 		}
 		currentLabels := cluster.GetLabels()
 		if currentLabels == nil {
 			currentLabels = make(map[string]string)
 		}
 
+		needToUpdate := false
 		if removeLabels {
 			pattern := `lcm\.openshift\.io/ibgu-[a-zA-Z]+-(completed|failed)`
 			re := regexp.MustCompile(pattern)
 			for key := range currentLabels {
 				if re.MatchString(key) {
+					needToUpdate = true
 					delete(currentLabels, key)
 				}
 			}
 		} else {
 			for key, value := range labelsToAdd {
+				if _, exist := currentLabels[key]; exist {
+					continue
+				}
 				currentLabels[key] = value
+				needToUpdate = true
 			}
 		}
 
-		cluster.SetLabels(currentLabels)
-		if err := r.Update(ctx, cluster); err != nil {
-			return fmt.Errorf("failed to update labels for cluster: %s, err: %w", cluster.Name, err)
+		if needToUpdate {
+			cluster.SetLabels(currentLabels)
+			if err := r.Update(ctx, cluster); err != nil {
+				r.Log.Error(err, "failed to update labels for cluster")
+				failedClusters = append(failedClusters, cluster.Name)
+				continue
+			}
 		}
 	}
-	return nil
+	if len(failedClusters) == 0 {
+		return nil
+	}
+	return fmt.Errorf("failed to ensure cluster labels for %v", failedClusters)
 }
 
 func (r *IBGUReconciler) syncStatusWithCGUs(ctx context.Context, ibgu *ibguv1alpha1.ImageBasedGroupUpgrade) error {

diff --git a/controllers/imagebasedgroupupgrade_test.go b/controllers/imagebasedgroupupgrade_test.go
@@ -3,6 +3,7 @@ package controllers
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"testing"
 
 	"github.com/go-logr/logr"
@@ -485,6 +486,7 @@ func TestEnsureClusterLabels(t *testing.T) {
 		managedClusters         []clusterv1.ManagedCluster
 		expectedManagedClusters []clusterv1.ManagedCluster
 		clusterStates           []ibguv1alpha1.ClusterState
+		expectedErr             error
 	}{
 		{
 			name: "failed prep, upgrade and abort",
@@ -623,6 +625,83 @@ func TestEnsureClusterLabels(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "two cluster, first throws an error",
+			managedClusters: []clusterv1.ManagedCluster{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Name: "cluster2",
+					},
+				},
+			},
+			clusterStates: []ibguv1alpha1.ClusterState{
+				{
+					Name: "cluster1",
+					CompletedActions: []ibguv1alpha1.ActionMessage{
+						{
+							Action: ibguv1alpha1.Prep,
+						},
+					},
+				},
+				{
+					Name: "cluster2",
+					CompletedActions: []ibguv1alpha1.ActionMessage{
+						{
+							Action: ibguv1alpha1.Prep,
+						},
+					},
+				},
+			},
+			expectedManagedClusters: []clusterv1.ManagedCluster{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Name: "cluster2",
+						Labels: map[string]string{
+							"lcm.openshift.io/ibgu-prep-completed": "",
+						},
+					},
+				},
+			},
+			expectedErr: fmt.Errorf("failed to ensure cluster labels for %v", []string{"cluster-1"}),
+		},
+		{
+			name: "two cluster, first nothing to do",
+			managedClusters: []clusterv1.ManagedCluster{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Name: "cluster1",
+					},
+				},
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Name: "cluster2",
+					},
+				},
+			},
+			clusterStates: []ibguv1alpha1.ClusterState{
+				{
+					Name: "cluster1",
+				},
+				{
+					Name: "cluster2",
+					CompletedActions: []ibguv1alpha1.ActionMessage{
+						{
+							Action: ibguv1alpha1.Prep,
+						},
+					},
+				},
+			},
+			expectedManagedClusters: []clusterv1.ManagedCluster{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Name: "cluster2",
+						Labels: map[string]string{
+							"lcm.openshift.io/ibgu-prep-completed": "",
+						},
+					},
+				},
+			},
+		},
 	}
 
 	ibgu := &ibguv1alpha1.ImageBasedGroupUpgrade{
@@ -663,7 +742,11 @@ func TestEnsureClusterLabels(t *testing.T) {
 			}
 			reconciler := IBGUReconciler{Client: fakeClient, Scheme: testscheme, Log: logr.Discard()}
 			err = reconciler.ensureClusterLabels(context.TODO(), ibgu)
-			assert.NoError(t, err)
+			if test.expectedErr == nil {
+				assert.NoError(t, err)
+			} else {
+				assert.Error(t, test.expectedErr)
+			}
 			for _, expected := range test.expectedManagedClusters {
 				got := &clusterv1.ManagedCluster{}
 				err := fakeClient.Get(context.TODO(), types.NamespacedName{Namespace: expected.Namespace, Name: expected.Name}, got)