Merge pull request #5083 from muraee/rosa-fix-diff-detection

🐛 ROSA: Fix ROSAMchinePool spec.Taints diff detection

Author: k8s-ci-robot

exp/controllers/rosamachinepool_controller.go

@@ -312,7 +312,7 @@ func (r *ROSAMachinePoolReconciler) reconcileDelete(
 		if err := ocmClient.DeleteNodePool(machinePoolScope.ControlPlane.Status.ID, nodePool.ID()); err != nil {
 			return err
 		}
-		machinePoolScope.Info("Successfully deleted NodePool %s", nodePool.ID())
+		machinePoolScope.Info("Successfully deleted NodePool")
 	}
 
 	controllerutil.RemoveFinalizer(machinePoolScope.RosaMachinePool, expinfrav1.RosaMachinePoolFinalizer)
@@ -360,21 +360,14 @@ func (r *ROSAMachinePoolReconciler) updateNodePool(machinePoolScope *scope.RosaM
 	machinePool := machinePoolScope.RosaMachinePool.DeepCopy()
 	// default all fields before comparing, so that nil/unset fields don't cause an unnecessary update call.
 	machinePool.Default()
-
 	desiredSpec := machinePool.Spec
-	currentSpec := nodePoolToRosaMachinePoolSpec(nodePool)
 
-	ignoredFields := []string{
-		"ProviderIDList", // providerIDList is set by the controller.
-		"Version",        // Version changes are reconciled separately.
-		"AdditionalTags", // AdditionalTags day2 changes not supported.
-	}
-	if cmp.Equal(desiredSpec, currentSpec,
-		cmpopts.EquateEmpty(), // ensures empty non-nil slices and nil slices are considered equal.
-		cmpopts.IgnoreFields(currentSpec, ignoredFields...)) {
+	specDiff := computeSpecDiff(desiredSpec, nodePool)
+	if specDiff == "" {
 		// no changes detected.
 		return nodePool, nil
 	}
+	machinePoolScope.Info("MachinePool spec diff detected", "diff", specDiff)
 
 	// zero-out fields that shouldn't be part of the update call.
 	desiredSpec.Version = ""
@@ -400,6 +393,21 @@ func (r *ROSAMachinePoolReconciler) updateNodePool(machinePoolScope *scope.RosaM
 	return updatedNodePool, nil
 }
 
+func computeSpecDiff(desiredSpec expinfrav1.RosaMachinePoolSpec, nodePool *cmv1.NodePool) string {
+	currentSpec := nodePoolToRosaMachinePoolSpec(nodePool)
+
+	ignoredFields := []string{
+		"ProviderIDList",           // providerIDList is set by the controller.
+		"Version",                  // Version changes are reconciled separately.
+		"AdditionalTags",           // AdditionalTags day2 changes not supported.
+		"AdditionalSecurityGroups", // AdditionalSecurityGroups day2 changes not supported.
+	}
+
+	return cmp.Diff(desiredSpec, currentSpec,
+		cmpopts.EquateEmpty(), // ensures empty non-nil slices and nil slices are considered equal.
+		cmpopts.IgnoreFields(currentSpec, ignoredFields...))
+}
+
 func validateMachinePoolSpec(machinePoolScope *scope.RosaMachinePoolScope) (*string, error) {
 	if machinePoolScope.RosaMachinePool.Spec.Version == "" {
 		return nil, nil
@@ -517,7 +525,7 @@ func nodePoolToRosaMachinePoolSpec(nodePool *cmv1.NodePool) expinfrav1.RosaMachi
 		}
 	}
 	if nodePool.Taints() != nil {
-		rosaTaints := make([]expinfrav1.RosaTaint, len(nodePool.Taints()))
+		rosaTaints := make([]expinfrav1.RosaTaint, 0, len(nodePool.Taints()))
 		for _, taint := range nodePool.Taints() {
 			rosaTaints = append(rosaTaints, expinfrav1.RosaTaint{
 				Key:    taint.Key(),

exp/controllers/rosamachinepool_controller_test.go

@@ -4,8 +4,8 @@ import (
 	"testing"
 	"time"
 
-	"github.com/google/go-cmp/cmp/cmpopts"
 	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/utils/ptr"
@@ -33,18 +33,30 @@ func TestNodePoolToRosaMachinePoolSpec(t *testing.T) {
 				MaxUnavailable: ptr.To(intstr.FromInt32(5)),
 			},
 		},
+		AdditionalSecurityGroups: []string{
+			"id-1",
+			"id-2",
+		},
+		Labels: map[string]string{
+			"label1": "value1",
+			"label2": "value2",
+		},
+		Taints: []expinfrav1.RosaTaint{
+			{
+				Key:    "myKey",
+				Value:  "myValue",
+				Effect: corev1.TaintEffectNoExecute,
+			},
+		},
 	}
 
 	machinePoolSpec := expclusterv1.MachinePoolSpec{
 		Replicas: ptr.To[int32](2),
 	}
 
 	nodePoolBuilder := nodePoolBuilder(rosaMachinePoolSpec, machinePoolSpec)
-
 	nodePoolSpec, err := nodePoolBuilder.Build()
 	g.Expect(err).ToNot(HaveOccurred())
 
-	expectedSpec := nodePoolToRosaMachinePoolSpec(nodePoolSpec)
-
-	g.Expect(rosaMachinePoolSpec).To(BeComparableTo(expectedSpec, cmpopts.EquateEmpty()))
+	g.Expect(computeSpecDiff(rosaMachinePoolSpec, nodePoolSpec)).To(BeEmpty())
 }