From cafaab0bc92b63f6feeae8683308728c5d705429 Mon Sep 17 00:00:00 2001
From: Mulham Raee <mraee@redhat.com>
Date: Fri, 10 May 2024 18:02:56 +0200
Subject: [PATCH] fix ROSAMachinePool changes detection logic

---
 exp/controllers/rosamachinepool_controller.go | 32 +++++++++++++++----
 .../rosamachinepool_controller_test.go        |  4 ---
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/exp/controllers/rosamachinepool_controller.go b/exp/controllers/rosamachinepool_controller.go
index f809f83b3f..90f46a3c6c 100644
--- a/exp/controllers/rosamachinepool_controller.go
+++ b/exp/controllers/rosamachinepool_controller.go
@@ -9,6 +9,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/blang/semver"
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
 	"github.com/openshift/rosa/pkg/ocm"
 	"github.com/pkg/errors"
@@ -220,6 +221,11 @@ func (r *ROSAMachinePoolReconciler) reconcileNormal(ctx context.Context,
 	}
 
 	if found {
+		if rosaMachinePool.Spec.AvailabilityZone == "" {
+			// reflect the current AvailabilityZone in the spec if not set.
+			rosaMachinePool.Spec.AvailabilityZone = nodePool.AvailabilityZone()
+		}
+
 		nodePool, err := r.updateNodePool(machinePoolScope, ocmClient, nodePool)
 		if err != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to ensure rosaMachinePool: %w", err)
@@ -349,13 +355,23 @@ func (r *ROSAMachinePoolReconciler) reconcileMachinePoolVersion(machinePoolScope
 }
 
 func (r *ROSAMachinePoolReconciler) updateNodePool(machinePoolScope *scope.RosaMachinePoolScope, ocmClient *ocm.Client, nodePool *cmv1.NodePool) (*cmv1.NodePool, error) {
-	desiredSpec := machinePoolScope.RosaMachinePool.Spec.DeepCopy()
-
+	desiredSpec := *machinePoolScope.RosaMachinePool.Spec.DeepCopy()
 	currentSpec := nodePoolToRosaMachinePoolSpec(nodePool)
-	currentSpec.ProviderIDList = desiredSpec.ProviderIDList // providerIDList is set by the controller and shouldn't be compared here.
-	currentSpec.Version = desiredSpec.Version               // Version changes are reconciled separately and shouldn't be compared here.
 
-	if cmp.Equal(desiredSpec, currentSpec) {
+	if desiredSpec.NodeDrainGracePeriod == nil {
+		// currentSpec.NodeDrainGracePeriod is always non-nil.
+		// if desiredSpec.NodeDrainGracePeriod is nil, set to 0 so we update the nodePool, otherewise the current value will be preserved.
+		desiredSpec.NodeDrainGracePeriod = &metav1.Duration{}
+	}
+
+	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...)) {
 		// no changes detected.
 		return nodePool, nil
 	}
@@ -365,7 +381,7 @@ func (r *ROSAMachinePoolReconciler) updateNodePool(machinePoolScope *scope.RosaM
 	desiredSpec.AdditionalSecurityGroups = nil
 	desiredSpec.AdditionalTags = nil
 
-	npBuilder := nodePoolBuilder(*desiredSpec, machinePoolScope.MachinePool.Spec)
+	npBuilder := nodePoolBuilder(desiredSpec, machinePoolScope.MachinePool.Spec)
 	nodePoolSpec, err := npBuilder.Build()
 	if err != nil {
 		return nil, fmt.Errorf("failed to build nodePool spec: %w", err)
@@ -470,11 +486,13 @@ func nodePoolToRosaMachinePoolSpec(nodePool *cmv1.NodePool) expinfrav1.RosaMachi
 		AvailabilityZone:         nodePool.AvailabilityZone(),
 		Subnet:                   nodePool.Subnet(),
 		Labels:                   nodePool.Labels(),
-		AdditionalTags:           nodePool.AWSNodePool().Tags(),
 		AutoRepair:               nodePool.AutoRepair(),
 		InstanceType:             nodePool.AWSNodePool().InstanceType(),
 		TuningConfigs:            nodePool.TuningConfigs(),
 		AdditionalSecurityGroups: nodePool.AWSNodePool().AdditionalSecurityGroupIds(),
+		// nodePool.AWSNodePool().Tags() returns all tags including "system" tags if "fetchUserTagsOnly" parameter is not specified.
+		// TODO: enable when AdditionalTags day2 changes is supported.
+		// AdditionalTags:           nodePool.AWSNodePool().Tags(),
 	}
 
 	if nodePool.Autoscaling() != nil {
diff --git a/exp/controllers/rosamachinepool_controller_test.go b/exp/controllers/rosamachinepool_controller_test.go
index 58f1963ed4..dd5c02c889 100644
--- a/exp/controllers/rosamachinepool_controller_test.go
+++ b/exp/controllers/rosamachinepool_controller_test.go
@@ -8,7 +8,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
 
-	infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
 	expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2"
 	expclusterv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
 )
@@ -26,9 +25,6 @@ func TestNodePoolToRosaMachinePoolSpec(t *testing.T) {
 		NodeDrainGracePeriod: &metav1.Duration{
 			Duration: time.Minute * 10,
 		},
-		AdditionalTags: infrav1.Tags{
-			"tag1": "value1",
-		},
 	}
 
 	machinePoolSpec := expclusterv1.MachinePoolSpec{