From a45c00b9a96cf76c3f02cdebc0a18abbb56e7ab6 Mon Sep 17 00:00:00 2001 From: Himanshu Sharma Date: Wed, 29 Mar 2023 00:21:36 +0530 Subject: [PATCH 1/6] edge cases dealt with, unit tests added --- pkg/controller/controller_suite_test.go | 131 ++++++- pkg/controller/deployment_sync.go | 5 +- pkg/controller/deployment_sync_test.go | 489 +++++++++++++++++++++++- 3 files changed, 622 insertions(+), 3 deletions(-) diff --git a/pkg/controller/controller_suite_test.go b/pkg/controller/controller_suite_test.go index 6f1287014..82c6b5e11 100644 --- a/pkg/controller/controller_suite_test.go +++ b/pkg/controller/controller_suite_test.go @@ -29,6 +29,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/watch" coreinformers "k8s.io/client-go/informers" v1core "k8s.io/client-go/kubernetes/typed/core/v1" @@ -47,7 +48,7 @@ import ( ) func TestMachineControllerManagerSuite(t *testing.T) { - //for filtering out warning logs. Reflector short watch warning logs won't print now + // for filtering out warning logs. Reflector short watch warning logs won't print now klog.SetOutput(io.Discard) flags := &flag.FlagSet{} klog.InitFlags(flags) @@ -64,6 +65,99 @@ var ( TestMachineClass = "machineClass-0" ) +func newMachineDeployment( + specTemplate *v1alpha1.MachineTemplateSpec, + replicas int32, + minReadySeconds int32, + maxSurge int, + maxUnavailable int, + statusTemplate *v1alpha1.MachineDeploymentStatus, + owner *metav1.OwnerReference, + annotations map[string]string, + labels map[string]string, +) *v1alpha1.MachineDeployment { + md := newMachineDeployments(1, specTemplate, replicas, minReadySeconds, statusTemplate, owner, annotations, labels)[0] + intStrMaxSurge := intstr.FromInt(maxSurge) + intStrMaxUnavailable := intstr.FromInt(maxUnavailable) + md.Spec.Strategy.RollingUpdate.MaxSurge = &intStrMaxSurge + md.Spec.Strategy.RollingUpdate.MaxUnavailable = &intStrMaxUnavailable + + return md +} + +func newMachineDeployments( + machineDeploymentCount int, + specTemplate *v1alpha1.MachineTemplateSpec, + replicas int32, + minReadySeconds int32, + statusTemplate *v1alpha1.MachineDeploymentStatus, + owner *metav1.OwnerReference, + annotations map[string]string, + labels map[string]string, +) []*v1alpha1.MachineDeployment { + + intStr1 := intstr.FromInt(1) + machineDeployments := make([]*v1alpha1.MachineDeployment, machineDeploymentCount) + for i := range machineDeployments { + machineDeployment := &v1alpha1.MachineDeployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "machine.sapcloud.io", + Kind: "MachineDeployment", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("machinedeployment-%d", i), + Namespace: testNamespace, + Labels: labels, + }, + Spec: v1alpha1.MachineDeploymentSpec{ + MinReadySeconds: minReadySeconds, + Replicas: replicas, + Selector: &metav1.LabelSelector{ + MatchLabels: deepCopy(specTemplate.ObjectMeta.Labels), + }, + Strategy: v1alpha1.MachineDeploymentStrategy{ + Type: v1alpha1.RollingUpdateMachineDeploymentStrategyType, + RollingUpdate: &v1alpha1.RollingUpdateMachineDeployment{ + MaxSurge: &intStr1, + MaxUnavailable: &intStr1, + }, + }, + Template: *specTemplate.DeepCopy(), + }, + } + + if statusTemplate != nil { + machineDeployment.Status = *statusTemplate.DeepCopy() + } + + if owner != nil { + machineDeployment.OwnerReferences = append(machineDeployment.OwnerReferences, *owner.DeepCopy()) + } + + if annotations != nil { + machineDeployment.Annotations = annotations + } + + machineDeployments[i] = machineDeployment + } + return machineDeployments +} + +func newMachineSet( + specTemplate *v1alpha1.MachineTemplateSpec, + name string, + replicas int32, + minReadySeconds int32, + statusTemplate *v1alpha1.MachineSetStatus, + owner *metav1.OwnerReference, + annotations map[string]string, + labels map[string]string, +) *v1alpha1.MachineSet { + ms := newMachineSets(1, specTemplate, replicas, minReadySeconds, statusTemplate, owner, annotations, labels)[0] + ms.Name = name + return ms +} + func newMachineSets( machineSetCount int, specTemplate *v1alpha1.MachineTemplateSpec, @@ -115,6 +209,41 @@ func newMachineSets( return machineSets } +func modifyMachineSet(ms *v1alpha1.MachineSet, specTemplate *v1alpha1.MachineTemplateSpec, + replicas int32, + minReadySeconds int32, + statusTemplate *v1alpha1.MachineSetStatus, + owner *metav1.OwnerReference, + annotations map[string]string, + labels map[string]string) *v1alpha1.MachineSet { + + if specTemplate != nil { + ms.Spec.Template = *specTemplate.DeepCopy() + } + + Expect(ms).To(Not(BeNil())) + fmt.Println(*ms) + Expect(ms.Spec).To(Not(BeEmpty())) + ms.Spec.Replicas = replicas + ms.Spec.MinReadySeconds = minReadySeconds + if statusTemplate != nil { + ms.Status = *statusTemplate.DeepCopy() + + } + if owner != nil { + ms.OwnerReferences = append(ms.OwnerReferences, *owner.DeepCopy()) + } + if annotations != nil { + ms.Annotations = annotations + } + + if labels != nil { + ms.Labels = labels + } + + return ms +} + func deepCopy(m map[string]string) map[string]string { r := make(map[string]string, len(m)) for k := range m { diff --git a/pkg/controller/deployment_sync.go b/pkg/controller/deployment_sync.go index 306dfcf1e..bb9c9c7d3 100644 --- a/pkg/controller/deployment_sync.go +++ b/pkg/controller/deployment_sync.go @@ -414,6 +414,7 @@ func (dc *controller) scale(ctx context.Context, deployment *v1alpha1.MachineDep // If the new machine set is saturated, old machine sets should be fully scaled down. // This case handles machine set adoption during a saturated new machine set. if IsSaturated(deployment, newIS) { + fmt.Printf("Scaling old active machineSets as new machineSet %s is saturated", newIS.Name) klog.V(3).Infof("Scaling old active machineSets as new machineSet %s is saturated", newIS.Name) for _, old := range FilterActiveMachineSets(oldISs) { if _, _, err := dc.scaleMachineSetAndRecordEvent(ctx, old, 0, deployment); err != nil { @@ -429,6 +430,7 @@ func (dc *controller) scale(ctx context.Context, deployment *v1alpha1.MachineDep // - Scale down ? -> scale down the old machineSets proportionally if IsRollingUpdate(deployment) { klog.V(3).Infof("Scaling all active machineSets proportionally for scale-in, while scaling up latest machineSet only for scale-out, machineDeployment %s", deployment.Name) + fmt.Printf("Scaling all active machineSets proportionally for scale-in, while scaling up latest machineSet only for scale-out, machineDeployment %s", deployment.Name) allISs := FilterActiveMachineSets(append(oldISs, newIS)) allISsReplicas := GetReplicaCountForMachineSets(allISs) @@ -451,7 +453,7 @@ func (dc *controller) scale(ctx context.Context, deployment *v1alpha1.MachineDep nameToSize := make(map[string]int32) deploymentReplicasAdded := int32(0) switch { - case deploymentReplicasToAdd > 0: + case deploymentReplicasToAdd >= 0: scalingOperation = "up" nameToSize = dc.scaleNewMachineSet(newIS, allISs, deploymentReplicasToAdd, deployment) deploymentReplicasAdded = deploymentReplicasToAdd @@ -673,6 +675,7 @@ func (dc *controller) isScalingEvent(ctx context.Context, d *v1alpha1.MachineDep continue } if desired != (d.Spec.Replicas) { + klog.V(2).Infof("Desired replicas annotation value: %d on machineSet %s, Spec Desired Replicas value: %d on corresponding machineDeployment, so scaling has happened.", desired, is.Name, d.Spec.Replicas) return true, nil } } diff --git a/pkg/controller/deployment_sync_test.go b/pkg/controller/deployment_sync_test.go index 3b2a7d921..ad0629b73 100644 --- a/pkg/controller/deployment_sync_test.go +++ b/pkg/controller/deployment_sync_test.go @@ -104,7 +104,7 @@ var _ = Describe("deployment_sync", func() { }, } - It("when new MachinSet is not found in isList", func() { + It("when new MachineSet is not found in isList", func() { stop := make(chan struct{}) defer close(stop) @@ -355,4 +355,491 @@ var _ = Describe("deployment_sync", func() { }) + // scale(ctx context.Context, deployment *v1alpha1.MachineDeployment, newIS *v1alpha1.MachineSet, oldISs []*v1alpha1.MachineSet) error + Describe("#scale", func() { + const ( + nameMachineSet1 = "ms1" + nameMachineSet2 = "ms2" + nameMachineSet3 = "ms3" + ) + var ( + mSetSpecTemplate = &machinev1.MachineTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "test-label": "test-label", + }, + }, + Spec: machinev1.MachineSpec{ + Class: machinev1.ClassSpec{ + Kind: "AWSMachineClass", + Name: "test-machine-class", + }, + NodeTemplateSpec: machinev1.NodeTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "key1": "value1", + }, + Annotations: map[string]string{ + "anno1": "anno1", + }, + }, + Spec: corev1.NodeSpec{ + Taints: []corev1.Taint{{ + Key: "taintKey", + Value: "taintValue", + Effect: "NoSchedule", + }, + }, + }, + }, + }, + } + + mDeploymentSpecTemplate = &machinev1.MachineTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "test-label": "test-label", + }, + }, + Spec: machinev1.MachineSpec{ + Class: machinev1.ClassSpec{ + Kind: "AWSMachineClass", + Name: "test-machine-class", + }, + NodeTemplateSpec: machinev1.NodeTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "key1": "value1", + }, + Annotations: map[string]string{ + "anno1": "anno1", + }, + }, + Spec: corev1.NodeSpec{ + Taints: []corev1.Taint{{ + Key: "taintKey", + Value: "taintValue", + Effect: "NoSchedule", + }, + }, + }, + }, + }, + } + ) + + type setup struct { + machineDeployment *machinev1.MachineDeployment + oldISs []*machinev1.MachineSet + newIS *machinev1.MachineSet + } + type expect struct { + machineSets []*machinev1.MachineSet + err bool + } + type data struct { + setup setup + expect expect + } + + DescribeTable("##table", + func(data *data) { + stop := make(chan struct{}) + defer close(stop) + + controlObjects := []runtime.Object{} + Expect(data.setup.machineDeployment).To(Not(BeNil())) + controlObjects = append(controlObjects, data.setup.machineDeployment) + + for i := range data.setup.oldISs { + controlObjects = append(controlObjects, data.setup.oldISs[i]) + } + + controlObjects = append(controlObjects, data.setup.newIS) + + c, trackers := createController(stop, testNamespace, controlObjects, nil, nil) + defer trackers.Stop() + waitForCacheSync(stop, c) + + err := c.scale( + context.TODO(), + data.setup.machineDeployment, + data.setup.newIS, + data.setup.oldISs, + ) + + waitForCacheSync(stop, c) + + if !data.expect.err { + Expect(err).To(BeNil()) + } else { + Expect(err).To(HaveOccurred()) + } + + currMachineSetsList, err := c.controlMachineClient.MachineSets(testNamespace).List(context.TODO(), metav1.ListOptions{}) + + Expect(err).To(BeNil()) + + for _, expectedMachineSet := range data.expect.machineSets { + flag := false + for _, ms := range currMachineSetsList.Items { + if ms.Name == expectedMachineSet.Name { + flag = true + Expect(ms.Spec.Replicas).To(Equal(expectedMachineSet.Spec.Replicas)) + Expect(ms.Annotations[DesiredReplicasAnnotation]).To(Equal(expectedMachineSet.Annotations[DesiredReplicasAnnotation])) + Expect(ms.Annotations[MaxReplicasAnnotation]).To(Equal(expectedMachineSet.Annotations[MaxReplicasAnnotation])) + break + } + } + Expect(flag).To(BeTrue()) + } + }, + + // Cases: + // 1. deploymentReplicasToAdd > 0 + // 2. deploymentReplicasToAdd =0 + // 3. deploymentReplicasToAdd < 0 + + // In the below tests the machineDeployment passed in setup is the machineDeployment after scale-up + // To determine the previous machineDeployment, look at the oldIS and newIS replicas passed + // in setup , maxSurge has been set to 2 , both for old and new machineDeployment. + + Entry("if scale-up, then only new machineSet should be scaled-up", &data{ + setup: setup{ + machineDeployment: newMachineDeployment(mDeploymentSpecTemplate, 10, 500, 2, 0, nil, nil, nil, nil), + oldISs: []*machinev1.MachineSet{newMachineSet( + mSetSpecTemplate, + nameMachineSet1, + 1, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "7", + MaxReplicasAnnotation: "9", + }, + nil, + ), newMachineSet( + mSetSpecTemplate, + nameMachineSet2, + 3, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "7", + MaxReplicasAnnotation: "9", + }, + nil, + )}, + newIS: newMachineSet( + mSetSpecTemplate, + nameMachineSet3, + 5, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "7", + MaxReplicasAnnotation: "9", + }, + nil, + ), + }, + expect: expect{ + machineSets: []*machinev1.MachineSet{ + newMachineSet(mSetSpecTemplate, + nameMachineSet1, + 1, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "10", + MaxReplicasAnnotation: "12", + }, + nil, + ), + newMachineSet(mSetSpecTemplate, + nameMachineSet2, + 3, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "10", + MaxReplicasAnnotation: "12", + }, + nil, + ), + newMachineSet(mSetSpecTemplate, + nameMachineSet3, + 8, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "10", + MaxReplicasAnnotation: "12", + }, + nil, + ), + }, + err: false, + }, + }), + Entry("if scale-down such that leftover replicas to add >0, then proportional scale-down, left over to be adjusted in latest machineSet", &data{ + setup: setup{ + machineDeployment: newMachineDeployment(mDeploymentSpecTemplate, 3, 500, 2, 0, nil, nil, nil, nil), + oldISs: []*machinev1.MachineSet{newMachineSet( + mSetSpecTemplate, + nameMachineSet1, + 1, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "7", + MaxReplicasAnnotation: "9", + }, + nil, + ), newMachineSet( + mSetSpecTemplate, + nameMachineSet2, + 3, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "7", + MaxReplicasAnnotation: "9", + }, + nil, + )}, + newIS: newMachineSet( + mSetSpecTemplate, + nameMachineSet3, + 5, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "7", + MaxReplicasAnnotation: "9", + }, + nil, + ), + }, + expect: expect{ + machineSets: []*machinev1.MachineSet{ + newMachineSet(mSetSpecTemplate, + nameMachineSet1, + 1, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "3", + MaxReplicasAnnotation: "5", + }, + nil, + ), + newMachineSet(mSetSpecTemplate, + nameMachineSet2, + 2, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "3", + MaxReplicasAnnotation: "5", + }, + nil, + ), + newMachineSet(mSetSpecTemplate, + nameMachineSet3, + 2, // leftover adjusted, so 2 instead of 3 + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "3", + MaxReplicasAnnotation: "5", + }, + nil, + ), + }, + err: false, + }, + }), + Entry("if scale-down such that leftover replicas to add =0, then also proportional scale-down", &data{ + setup: setup{ + machineDeployment: newMachineDeployment(mDeploymentSpecTemplate, 5, 500, 2, 0, nil, nil, nil, nil), + oldISs: []*machinev1.MachineSet{newMachineSet( + mSetSpecTemplate, + nameMachineSet1, + 1, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "8", + MaxReplicasAnnotation: "10", + }, + nil, + ), newMachineSet( + mSetSpecTemplate, + nameMachineSet2, + 3, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "8", + MaxReplicasAnnotation: "10", + }, + nil, + )}, + newIS: newMachineSet( + mSetSpecTemplate, + nameMachineSet3, + 6, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "8", + MaxReplicasAnnotation: "10", + }, + nil, + ), + }, + expect: expect{ + machineSets: []*machinev1.MachineSet{ + newMachineSet(mSetSpecTemplate, + nameMachineSet1, + 1, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "5", + MaxReplicasAnnotation: "7", + }, + nil, + ), + newMachineSet(mSetSpecTemplate, + nameMachineSet2, + 2, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "5", + MaxReplicasAnnotation: "7", + }, + nil, + ), + newMachineSet(mSetSpecTemplate, + nameMachineSet3, + 4, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "5", + MaxReplicasAnnotation: "7", + }, + nil, + ), + }, + err: false, + }, + }), + Entry("if no scaling happened, but still for edge cases, machineSet replicas shouldn't change", &data{ + setup: setup{ + machineDeployment: newMachineDeployment(mDeploymentSpecTemplate, 7, 500, 2, 0, nil, nil, nil, nil), + oldISs: []*machinev1.MachineSet{newMachineSet( + mSetSpecTemplate, + nameMachineSet1, + 1, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "7", + MaxReplicasAnnotation: "9", + }, + nil, + ), newMachineSet( + mSetSpecTemplate, + nameMachineSet2, + 3, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "7", + MaxReplicasAnnotation: "9", + }, + nil, + )}, + newIS: newMachineSet( + mSetSpecTemplate, + nameMachineSet3, + 5, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "7", + MaxReplicasAnnotation: "9", + }, + nil, + ), + }, + expect: expect{ + machineSets: []*machinev1.MachineSet{ + newMachineSet(mSetSpecTemplate, + nameMachineSet1, + 1, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "7", + MaxReplicasAnnotation: "9", + }, + nil, + ), + newMachineSet(mSetSpecTemplate, + nameMachineSet2, + 3, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "7", + MaxReplicasAnnotation: "9", + }, + nil, + ), + newMachineSet(mSetSpecTemplate, + nameMachineSet3, + 5, + 500, + nil, + nil, + map[string]string{ + DesiredReplicasAnnotation: "7", + MaxReplicasAnnotation: "9", + }, + nil, + ), + }, + err: false, + }, + }), + ) + }) }) From fac7ade0be79d35858e61b07bba7994c225d352d Mon Sep 17 00:00:00 2001 From: Himanshu Sharma Date: Wed, 29 Mar 2023 12:02:52 +0530 Subject: [PATCH 2/6] removed printf statements --- pkg/controller/deployment_sync.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/controller/deployment_sync.go b/pkg/controller/deployment_sync.go index bb9c9c7d3..05a9d595b 100644 --- a/pkg/controller/deployment_sync.go +++ b/pkg/controller/deployment_sync.go @@ -414,7 +414,6 @@ func (dc *controller) scale(ctx context.Context, deployment *v1alpha1.MachineDep // If the new machine set is saturated, old machine sets should be fully scaled down. // This case handles machine set adoption during a saturated new machine set. if IsSaturated(deployment, newIS) { - fmt.Printf("Scaling old active machineSets as new machineSet %s is saturated", newIS.Name) klog.V(3).Infof("Scaling old active machineSets as new machineSet %s is saturated", newIS.Name) for _, old := range FilterActiveMachineSets(oldISs) { if _, _, err := dc.scaleMachineSetAndRecordEvent(ctx, old, 0, deployment); err != nil { @@ -430,7 +429,6 @@ func (dc *controller) scale(ctx context.Context, deployment *v1alpha1.MachineDep // - Scale down ? -> scale down the old machineSets proportionally if IsRollingUpdate(deployment) { klog.V(3).Infof("Scaling all active machineSets proportionally for scale-in, while scaling up latest machineSet only for scale-out, machineDeployment %s", deployment.Name) - fmt.Printf("Scaling all active machineSets proportionally for scale-in, while scaling up latest machineSet only for scale-out, machineDeployment %s", deployment.Name) allISs := FilterActiveMachineSets(append(oldISs, newIS)) allISsReplicas := GetReplicaCountForMachineSets(allISs) From ac15a5bdbac6f75ffc6337345a7659d697fbf25f Mon Sep 17 00:00:00 2001 From: Himanshu Sharma Date: Wed, 29 Mar 2023 12:04:38 +0530 Subject: [PATCH 3/6] removed unused method --- pkg/controller/controller_suite_test.go | 35 ------------------------- 1 file changed, 35 deletions(-) diff --git a/pkg/controller/controller_suite_test.go b/pkg/controller/controller_suite_test.go index 82c6b5e11..4b0fa7631 100644 --- a/pkg/controller/controller_suite_test.go +++ b/pkg/controller/controller_suite_test.go @@ -209,41 +209,6 @@ func newMachineSets( return machineSets } -func modifyMachineSet(ms *v1alpha1.MachineSet, specTemplate *v1alpha1.MachineTemplateSpec, - replicas int32, - minReadySeconds int32, - statusTemplate *v1alpha1.MachineSetStatus, - owner *metav1.OwnerReference, - annotations map[string]string, - labels map[string]string) *v1alpha1.MachineSet { - - if specTemplate != nil { - ms.Spec.Template = *specTemplate.DeepCopy() - } - - Expect(ms).To(Not(BeNil())) - fmt.Println(*ms) - Expect(ms.Spec).To(Not(BeEmpty())) - ms.Spec.Replicas = replicas - ms.Spec.MinReadySeconds = minReadySeconds - if statusTemplate != nil { - ms.Status = *statusTemplate.DeepCopy() - - } - if owner != nil { - ms.OwnerReferences = append(ms.OwnerReferences, *owner.DeepCopy()) - } - if annotations != nil { - ms.Annotations = annotations - } - - if labels != nil { - ms.Labels = labels - } - - return ms -} - func deepCopy(m map[string]string) map[string]string { r := make(map[string]string, len(m)) for k := range m { From fb7f7daaaf2aa33410197d13c70cd7e499ed804a Mon Sep 17 00:00:00 2001 From: Madhav Bhargava Date: Wed, 29 Mar 2023 12:28:41 +0530 Subject: [PATCH 4/6] minor fix --- pkg/controller/deployment_sync_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/controller/deployment_sync_test.go b/pkg/controller/deployment_sync_test.go index ad0629b73..5045fe062 100644 --- a/pkg/controller/deployment_sync_test.go +++ b/pkg/controller/deployment_sync_test.go @@ -447,7 +447,7 @@ var _ = Describe("deployment_sync", func() { stop := make(chan struct{}) defer close(stop) - controlObjects := []runtime.Object{} + var controlObjects []runtime.Object Expect(data.setup.machineDeployment).To(Not(BeNil())) controlObjects = append(controlObjects, data.setup.machineDeployment) @@ -588,7 +588,7 @@ var _ = Describe("deployment_sync", func() { err: false, }, }), - Entry("if scale-down such that leftover replicas to add >0, then proportional scale-down, left over to be adjusted in latest machineSet", &data{ + Entry("if scale-down such that leftover replicas to add > 0, then proportional scale-down, left over to be adjusted in latest machineSet", &data{ setup: setup{ machineDeployment: newMachineDeployment(mDeploymentSpecTemplate, 3, 500, 2, 0, nil, nil, nil, nil), oldISs: []*machinev1.MachineSet{newMachineSet( @@ -672,7 +672,7 @@ var _ = Describe("deployment_sync", func() { err: false, }, }), - Entry("if scale-down such that leftover replicas to add =0, then also proportional scale-down", &data{ + Entry("if scale-down such that leftover replicas after proportional scaling =0, then proportional scale-down should be done", &data{ setup: setup{ machineDeployment: newMachineDeployment(mDeploymentSpecTemplate, 5, 500, 2, 0, nil, nil, nil, nil), oldISs: []*machinev1.MachineSet{newMachineSet( From 2d5a5b75e6d45f548dae70778aa32ee311d17fb9 Mon Sep 17 00:00:00 2001 From: Himanshu Sharma Date: Wed, 29 Mar 2023 12:41:58 +0530 Subject: [PATCH 5/6] error logging corrected --- pkg/controller/machineset.go | 19 ++++++++++--------- pkg/test/integration/common/framework.go | 2 +- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/pkg/controller/machineset.go b/pkg/controller/machineset.go index 548033b03..1f8da9432 100644 --- a/pkg/controller/machineset.go +++ b/pkg/controller/machineset.go @@ -26,15 +26,16 @@ import ( "context" "errors" "fmt" + "reflect" + "sort" + "sync" + "time" + v1alpha1client "github.com/gardener/machine-controller-manager/pkg/client/clientset/versioned/typed/machine/v1alpha1" v1alpha1listers "github.com/gardener/machine-controller-manager/pkg/client/listers/machine/v1alpha1" apiequality "k8s.io/apimachinery/pkg/api/equality" errorsutil "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/util/retry" - "reflect" - "sort" - "sync" - "time" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -347,7 +348,7 @@ func (c *controller) manageReplicas(ctx context.Context, allMachines []*v1alpha1 } if err := c.terminateMachines(ctx, staleMachines, machineSet); err != nil { // TODO: proper error handling needs to happen here - klog.Errorf("failed to terminate stale machines for machineset %s: %w", machineSet.Name, err) + klog.Errorf("failed to terminate stale machines for machineset %s: %v", machineSet.Name, err) } diff := len(activeMachines) - int(machineSet.Spec.Replicas) @@ -371,7 +372,7 @@ func (c *controller) manageReplicas(ctx context.Context, allMachines []*v1alpha1 // beforehand and store it via ExpectCreations. if err := c.expectations.ExpectCreations(machineSetKey, diff); err != nil { // TODO: proper error handling needs to happen here - klog.Errorf("failed expect creations for machineset %s: %w", machineSet.Name, err) + klog.Errorf("failed expect creations for machineset %s: %v", machineSet.Name, err) } klog.V(2).Infof("Too few replicas for MachineSet %s, need %d, creating %d", machineSet.Name, (machineSet.Spec.Replicas), diff) // Batch the machine creates. Batch sizes start at SlowStartInitialBatchSize @@ -433,12 +434,12 @@ func (c *controller) manageReplicas(ctx context.Context, allMachines []*v1alpha1 // expired even if other machines are deleted. if err := c.expectations.ExpectDeletions(machineSetKey, getMachineKeys(machinesToDelete)); err != nil { // TODO: proper error handling needs to happen here - klog.Errorf("failed expect deletions for machineset %s: %w", machineSet.Name, err) + klog.Errorf("failed expect deletions for machineset %s: %v", machineSet.Name, err) } if err := c.terminateMachines(ctx, machinesToDelete, machineSet); err != nil { // TODO: proper error handling needs to happen here - klog.Errorf("failed to terminate machines for machineset %s: %w", machineSet.Name, err) + klog.Errorf("failed to terminate machines for machineset %s: %v", machineSet.Name, err) } } @@ -691,7 +692,7 @@ func (c *controller) prepareMachineForDeletion(ctx context.Context, targetMachin } if _, err := c.updateMachineStatus(ctx, targetMachine, lastOperation, currentStatus); err != nil { // TODO: proper error handling needs to happen here - klog.Errorf("failed to update machine status for machine %s: %w", targetMachine.Name, err) + klog.Errorf("failed to update machine status for machine %s: %v", targetMachine.Name, err) } klog.V(2).Infof("Delete machine from machineset %q", targetMachine.Name) } diff --git a/pkg/test/integration/common/framework.go b/pkg/test/integration/common/framework.go index b983b4bbe..c275cb488 100644 --- a/pkg/test/integration/common/framework.go +++ b/pkg/test/integration/common/framework.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "fmt" - "github.com/gardener/machine-controller-manager/pkg/test/utils/matchers" "io" "log" "os" @@ -16,6 +15,7 @@ import ( "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" "github.com/gardener/machine-controller-manager/pkg/test/integration/common/helpers" + "github.com/gardener/machine-controller-manager/pkg/test/utils/matchers" "github.com/onsi/ginkgo" "github.com/onsi/gomega" "github.com/onsi/gomega/gexec" From 4822e4322e3aa3a7e6d993f33c3ce32b8ac183b1 Mon Sep 17 00:00:00 2001 From: Himanshu Sharma Date: Wed, 29 Mar 2023 13:01:22 +0530 Subject: [PATCH 6/6] added warning log --- pkg/controller/deployment_sync.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/controller/deployment_sync.go b/pkg/controller/deployment_sync.go index 05a9d595b..5014ebe1a 100644 --- a/pkg/controller/deployment_sync.go +++ b/pkg/controller/deployment_sync.go @@ -477,6 +477,7 @@ func (dc *controller) scale(ctx context.Context, deployment *v1alpha1.MachineDep // TODO: Use transactions when we have them. if _, _, err := dc.scaleMachineSet(ctx, is, nameToSize[is.Name], deployment, scalingOperation); err != nil { // Return as soon as we fail, the deployment is requeued + klog.Warningf("updating machineSet %s failed while scaling. This could lead to desired replicas annotation not being updated. err: %v", is.Name, err) return err } }