Merge pull request #5016 from muraee/fix-rosa-update-config

🐛 ROSA: fix ROSAMachinePool.Spec.UpdateConfig causing unnecessary update calls
kubernetes-sigs · Jun 13, 2024 · 059a51a · 059a51a
2 parents be020fd + 599eb74
commit 059a51a
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 15 deletions.
diff --git a/exp/api/v1beta2/rosamachinepool_webhook.go b/exp/api/v1beta2/rosamachinepool_webhook.go
@@ -5,8 +5,11 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	runtime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -127,4 +130,17 @@ func validateImmutable(old, updated interface{}, name string) field.ErrorList {
 
 // Default implements admission.Defaulter.
 func (r *ROSAMachinePool) Default() {
+	if r.Spec.NodeDrainGracePeriod == nil {
+		r.Spec.NodeDrainGracePeriod = &metav1.Duration{}
+	}
+
+	if r.Spec.UpdateConfig == nil {
+		r.Spec.UpdateConfig = &RosaUpdateConfig{}
+	}
+	if r.Spec.UpdateConfig.RollingUpdate == nil {
+		r.Spec.UpdateConfig.RollingUpdate = &RollingUpdate{
+			MaxUnavailable: ptr.To(intstr.FromInt32(0)),
+			MaxSurge:       ptr.To(intstr.FromInt32(1)),
+		}
+	}
 }
diff --git a/exp/controllers/rosamachinepool_controller.go b/exp/controllers/rosamachinepool_controller.go
@@ -357,14 +357,12 @@ 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()
-	currentSpec := nodePoolToRosaMachinePoolSpec(nodePool)
+	machinePool := machinePoolScope.RosaMachinePool.DeepCopy()
+	// default all fields before comparing, so that nil/unset fields don't cause an unnecessary update call.
+	machinePool.Default()
 
-	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{}
-	}
+	desiredSpec := machinePool.Spec
+	currentSpec := nodePoolToRosaMachinePoolSpec(nodePool)
 
 	ignoredFields := []string{
 		"ProviderIDList", // providerIDList is set by the controller.
@@ -481,12 +479,12 @@ func nodePoolBuilder(rosaMachinePoolSpec expinfrav1.RosaMachinePoolSpec, machine
 	if rosaMachinePoolSpec.UpdateConfig != nil {
 		configMgmtBuilder := cmv1.NewNodePoolManagementUpgrade()
 
-		if rosaMachinePoolSpec.UpdateConfig.RollingUpdate != nil {
-			if rosaMachinePoolSpec.UpdateConfig.RollingUpdate.MaxSurge != nil {
-				configMgmtBuilder = configMgmtBuilder.MaxSurge(rosaMachinePoolSpec.UpdateConfig.RollingUpdate.MaxSurge.String())
+		if rollingUpdate := rosaMachinePoolSpec.UpdateConfig.RollingUpdate; rollingUpdate != nil {
+			if rollingUpdate.MaxSurge != nil {
+				configMgmtBuilder = configMgmtBuilder.MaxSurge(rollingUpdate.MaxSurge.String())
 			}
-			if rosaMachinePoolSpec.UpdateConfig.RollingUpdate.MaxUnavailable != nil {
-				configMgmtBuilder = configMgmtBuilder.MaxUnavailable(rosaMachinePoolSpec.UpdateConfig.RollingUpdate.MaxUnavailable.String())
+			if rollingUpdate.MaxUnavailable != nil {
+				configMgmtBuilder = configMgmtBuilder.MaxUnavailable(rollingUpdate.MaxUnavailable.String())
 			}
 		}
 
@@ -542,7 +540,7 @@ func nodePoolToRosaMachinePoolSpec(nodePool *cmv1.NodePool) expinfrav1.RosaMachi
 			spec.UpdateConfig.RollingUpdate.MaxSurge = ptr.To(intstr.Parse(nodePool.ManagementUpgrade().MaxSurge()))
 		}
 		if nodePool.ManagementUpgrade().MaxUnavailable() != "" {
-			spec.UpdateConfig.RollingUpdate.MaxSurge = ptr.To(intstr.Parse(nodePool.ManagementUpgrade().MaxUnavailable()))
+			spec.UpdateConfig.RollingUpdate.MaxUnavailable = ptr.To(intstr.Parse(nodePool.ManagementUpgrade().MaxUnavailable()))
 		}
 	}
 

diff --git a/exp/controllers/rosamachinepool_controller_test.go b/exp/controllers/rosamachinepool_controller_test.go
@@ -4,6 +4,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp/cmpopts"
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -28,7 +29,8 @@ func TestNodePoolToRosaMachinePoolSpec(t *testing.T) {
 		},
 		UpdateConfig: &expinfrav1.RosaUpdateConfig{
 			RollingUpdate: &expinfrav1.RollingUpdate{
-				MaxSurge: &intstr.IntOrString{IntVal: 3},
+				MaxSurge:       ptr.To(intstr.FromInt32(3)),
+				MaxUnavailable: ptr.To(intstr.FromInt32(5)),
 			},
 		},
 	}
@@ -44,5 +46,5 @@ func TestNodePoolToRosaMachinePoolSpec(t *testing.T) {
 
 	expectedSpec := nodePoolToRosaMachinePoolSpec(nodePoolSpec)
 
-	g.Expect(expectedSpec).To(Equal(rosaMachinePoolSpec))
+	g.Expect(rosaMachinePoolSpec).To(BeComparableTo(expectedSpec, cmpopts.EquateEmpty()))
 }