From 0f6777ed3b8d9e9518367672031d35033f901d7f Mon Sep 17 00:00:00 2001 From: Mulham Raee Date: Wed, 12 Jun 2024 16:25:19 +0200 Subject: [PATCH 1/2] fix ROSAMachinePool.Spec.UpdateConfig causing unncessary update calls --- exp/api/v1beta2/rosamachinepool_webhook.go | 16 +++++++++++++ exp/controllers/rosamachinepool_controller.go | 24 +++++++++---------- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/exp/api/v1beta2/rosamachinepool_webhook.go b/exp/api/v1beta2/rosamachinepool_webhook.go index acae78576e..d4fdaf00a5 100644 --- 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 index 5beb67ec5a..659793a857 100644 --- 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())) } } From 599eb74a791f72a37d2f1a6c756ab99ffc09cc1d Mon Sep 17 00:00:00 2001 From: Mulham Raee Date: Thu, 13 Jun 2024 09:37:51 +0200 Subject: [PATCH 2/2] updated TestNodePoolToRosaMachinePoolSpec unit test --- exp/controllers/rosamachinepool_controller_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/exp/controllers/rosamachinepool_controller_test.go b/exp/controllers/rosamachinepool_controller_test.go index 4ab87cbcf7..a17dd0c922 100644 --- 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())) }