🌱 Add unit test coverage for machineDeployments. reconcileNewMachineSet #4495

enxebre · 2021-04-20T09:34:17Z

What this PR does / why we need it:
Increase unit test coverage fo machineDeployments rolling upgrades.

First PR related to #4457

JoelSpeed · 2021-04-20T11:01:21Z

controllers/machinedeployment_rolling.go

@@ -72,11 +73,11 @@ func (r *MachineDeploymentReconciler) rolloutRolling(ctx context.Context, d *clu

 func (r *MachineDeploymentReconciler) reconcileNewMachineSet(ctx context.Context, allMSs []*clusterv1.MachineSet, newMS *clusterv1.MachineSet, deployment *clusterv1.MachineDeployment) error {
 	if deployment.Spec.Replicas == nil {
-		return errors.Errorf("spec replicas for deployment set %v is nil, this is unexpected", deployment.Name)
+		return errors.Errorf("spec replicas for machineDeployment %s is nil, this is unexpected", client.ObjectKeyFromObject(deployment).String())


Other logging/errors predominantly seem to capitalise as MachineDeployment, we should try make all the logging consistent case wise

Suggested change

return errors.Errorf("spec replicas for machineDeployment %s is nil, this is unexpected", client.ObjectKeyFromObject(deployment).String())

return errors.Errorf("spec replicas for MachineDeployment %s is nil, this is unexpected", client.ObjectKeyFromObject(deployment).String())

JoelSpeed · 2021-04-20T11:03:05Z

controllers/machinedeployment_rolling.go

 	}

 	if newMS.Spec.Replicas == nil {
-		return errors.Errorf("spec replicas for machine set %v is nil, this is unexpected", newMS.Name)
+		return errors.Errorf("spec replicas for machineSet %s is nil, this is unexpected", client.ObjectKeyFromObject(newMS).String())


Other logging/errors predominantly seem to capitalise as MachineSet, we should try make all the logging consistent case wise

Suggested change

return errors.Errorf("spec replicas for machineSet %s is nil, this is unexpected", client.ObjectKeyFromObject(newMS).String())

return errors.Errorf("spec replicas for MachineSet %s is nil, this is unexpected", client.ObjectKeyFromObject(newMS).String())

JoelSpeed · 2021-04-20T11:08:51Z

controllers/machinedeployment_rolling_test.go

+			if tc.error != nil {
+				g.Expect(err.Error()).To(BeEquivalentTo(tc.error.Error()))
+			}


Nit: If you were to return here, you wouldn't need the next if block and could unindent the rest of the test

Suggested change

if tc.error != nil {

g.Expect(err.Error()).To(BeEquivalentTo(tc.error.Error()))

}

if tc.error != nil {

g.Expect(err.Error()).To(BeEquivalentTo(tc.error.Error()))

return

}

Might make the test a bit more readable? 🤷 Seems odd that we basically have if/else here

JoelSpeed · 2021-04-20T11:10:16Z

Looks great, added a few nits but nothing major to add from me

JoelSpeed

/lgtm

alexander-demicev

looks like a nice test improvement

vincepri

/approve

k8s-ci-robot · 2021-04-20T18:04:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sbueringer · 2021-04-20T18:21:09Z

controllers/machinedeployment_rolling_test.go

+					Replicas: pointer.Int32Ptr(2),
+				},
+			},
+			newMachineSet: &clusterv1.MachineSet{


I'm pretty unfamiliar with this code, so I might be missing something entirely.

To me it looks like allMS is always append(oldMSs, newMS)

cluster-api/controllers/machinedeployment_rolling.go

Lines 45 to 48 in e949440

allMSs := append(oldMSs, newMS)

// Scale up, if we can.

if err := r.reconcileNewMachineSet(ctx, allMSs, newMS, d); err != nil {

I think we should also have test cases for this. As far as I can see the current tests all have disjunct newMS/allMS, is this intended?

This is actually already being ensured for every testCase, see https://github.com/kubernetes-sigs/cluster-api/pull/4495/files#diff-a70da8885988c24ea4a35f7c1291f24a99f8c5ffdedf2f27864b90575afc2312R188

I just renamed the field allMachineSets -> OldMachineSets to make this more obvious.

Oh, shouldn't have missed that :/

you raised a good point which resulted in better naming, thanks!

JoelSpeed · 2021-04-21T09:48:02Z

/lgtm

/hold @sbueringer I think @enxebre has your comments covered, just wanted to get your ack before we merge

sbueringer · 2021-04-21T10:42:54Z

@JoelSpeed
Thx, fine fore me
/lgtm

JoelSpeed · 2021-04-21T10:43:46Z

/hold cancel

Thanks @sbueringer

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 20, 2021

k8s-ci-robot requested review from detiber and JoelSpeed April 20, 2021 09:34

enxebre changed the title ~~Add unit test coverage for machineDeployments. reconcileNewMachineSet~~ 🌱 Add unit test coverage for machineDeployments. reconcileNewMachineSet Apr 20, 2021

enxebre force-pushed the unit-test-md-rolling branch 2 times, most recently from 8c64621 to cabf352 Compare April 20, 2021 09:41

JoelSpeed reviewed Apr 20, 2021

View reviewed changes

enxebre force-pushed the unit-test-md-rolling branch from cabf352 to e949440 Compare April 20, 2021 11:34

JoelSpeed approved these changes Apr 20, 2021

View reviewed changes

k8s-ci-robot assigned JoelSpeed Apr 20, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 2021

enxebre mentioned this pull request Apr 20, 2021

🌱 Add unit test coverage for machineDeployments. reconcileOldMachineSets #4498

Merged

alexander-demicev approved these changes Apr 20, 2021

View reviewed changes

vincepri approved these changes Apr 20, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2021

sbueringer reviewed Apr 20, 2021

View reviewed changes

Add unit test coverage for machineDeployments. reconcileNewMachineSet

a641e30

enxebre force-pushed the unit-test-md-rolling branch from e949440 to a641e30 Compare April 21, 2021 07:19

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 21, 2021

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 21, 2021

k8s-ci-robot assigned sbueringer Apr 21, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2021

k8s-ci-robot merged commit a0c20fe into kubernetes-sigs:master Apr 21, 2021

k8s-ci-robot added this to the v0.4 milestone Apr 21, 2021

enxebre mentioned this pull request Apr 21, 2021

🌱 Refactor MachineSet scaling code and add more tests #4506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Add unit test coverage for machineDeployments. reconcileNewMachineSet #4495

🌱 Add unit test coverage for machineDeployments. reconcileNewMachineSet #4495

enxebre commented Apr 20, 2021

JoelSpeed Apr 20, 2021

JoelSpeed Apr 20, 2021

JoelSpeed Apr 20, 2021

JoelSpeed commented Apr 20, 2021

JoelSpeed left a comment

alexander-demicev left a comment

vincepri left a comment

k8s-ci-robot commented Apr 20, 2021

sbueringer Apr 20, 2021 •

edited

Loading

enxebre Apr 21, 2021

enxebre Apr 21, 2021

sbueringer Apr 21, 2021

enxebre Apr 21, 2021

JoelSpeed commented Apr 21, 2021

sbueringer commented Apr 21, 2021

JoelSpeed commented Apr 21, 2021

	return errors.Errorf("spec replicas for machineDeployment %s is nil, this is unexpected", client.ObjectKeyFromObject(deployment).String())
	return errors.Errorf("spec replicas for MachineDeployment %s is nil, this is unexpected", client.ObjectKeyFromObject(deployment).String())

	allMSs := append(oldMSs, newMS)

	// Scale up, if we can.
	if err := r.reconcileNewMachineSet(ctx, allMSs, newMS, d); err != nil {

🌱 Add unit test coverage for machineDeployments. reconcileNewMachineSet #4495

🌱 Add unit test coverage for machineDeployments. reconcileNewMachineSet #4495

Conversation

enxebre commented Apr 20, 2021

JoelSpeed Apr 20, 2021

Choose a reason for hiding this comment

JoelSpeed Apr 20, 2021

Choose a reason for hiding this comment

JoelSpeed Apr 20, 2021

Choose a reason for hiding this comment

JoelSpeed commented Apr 20, 2021

JoelSpeed left a comment

Choose a reason for hiding this comment

alexander-demicev left a comment

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 20, 2021

sbueringer Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

enxebre Apr 21, 2021

Choose a reason for hiding this comment

enxebre Apr 21, 2021

Choose a reason for hiding this comment

sbueringer Apr 21, 2021

Choose a reason for hiding this comment

enxebre Apr 21, 2021

Choose a reason for hiding this comment

JoelSpeed commented Apr 21, 2021

sbueringer commented Apr 21, 2021

JoelSpeed commented Apr 21, 2021

sbueringer Apr 20, 2021 •

edited

Loading