MD controller sync getNewMachineSet should not return nil as newMS after updating existingNewMS

[chingchiaw]

/kind bug

In pkg/controller/machinedeployments/sync.go getNewMachineSet, when there's an existing new MS, and its annotations or minReadySeconds need to be updated, the controller update the MS and return nil as new MS, and the error from r.Update as follows:

minReadySecondsNeedsUpdate := msCopy.Spec.MinReadySeconds != *d.Spec.MinReadySeconds
if annotationsUpdated || minReadySecondsNeedsUpdate {
	msCopy.Spec.MinReadySeconds = *d.Spec.MinReadySeconds
	return nil, r.Update(context.Background(), msCopy)
}

IMO it should not return nil, it should return the updated MS (msCopy).

https://github.com/kubernetes-sigs/cluster-api/blame/master/pkg/controller/machinedeployment/sync.go#L111

This was introduced in the major refactoring:
Oct 4 2019: Migrate to kubebuilder #494 (#494)
revision: 80129c0

I looked up the old code before the PR and originally the line was this:

minReadySecondsNeedsUpdate := msCopy.Spec.MinReadySeconds != *d.Spec.MinReadySeconds
		if annotationsUpdated || minReadySecondsNeedsUpdate {
			msCopy.Spec.MinReadySeconds = *d.Spec.MinReadySeconds
			return dc.machineClient.ClusterV1alpha1().MachineSets(msCopy.ObjectMeta.Namespace).Update(msCopy)
	}

And this is the original Update method called:

// Update takes the representation of a machineSet and updates it. Returns the server's representation of the machineSet, and an error, if there is any.
func (c *machineSets) Update(machineSet *v1alpha1.MachineSet) (result *v1alpha1.MachineSet, err error) {
	result = &v1alpha1.MachineSet{}
	err = c.client.Put().
		Namespace(c.ns).
		Resource("machinesets").
		Name(machineSet.Name).
		Body(machineSet).
		Do().
		Into(result)
	return
}

So originally newMS was the updated MS.

I don't have a concrete repro of any issue resulted from this bug, but naively this seems to me an issue to fix.

What did you expect to happen:
The controller method should return msCopy and error from r.Update(..., msCopy) instead

Anything else you would like to add:
NA

Environment:

• Cluster-api version: all revisions after Oct 4, 2018's 80129c0
• Minikube/KIND version: NA
• Kubernetes version: (use kubectl version): NA
• OS (e.g. from /etc/os-release): NA

[dlipovetsky]

@calebamiles and I looked at the code together.

From the doc string of getNewMachineSet:

// Returns a machine set that matches the intent of the given deployment. Returns nil if the new machine set doesn't exist yet.

Of course, L113 returns nil even though the new machine set does exist.

But none of the machine deployment tests fail because of this. So either those tests don't catch the issue, or it's not an issue (e.g. the caller of getNewMachineSet doesn't care that it returns a nil when it's supposed to return a MachineSet).

First thing to do might be to write a test for getNewMachineSet, and then to look at the machine deployment tests.

[calebamiles]

@detiber, maybe a short walkthrough of the code might be helpful. don't know if it makes sense to do so before or after the general cluster api code walkthrough

[calebamiles]

@dlipovetsky I got two examples of testing to look at

• one in kubebuilder
• one in controller runtime

[dlipovetsky]

MachineDeployment from the CAPI book: https://cluster-api.sigs.k8s.io/common_code/machinedeployment_controller.html

[calebamiles]

/unassign

[vincepri]

We'll go through some redesign/refactor of these controllers in the future #, I think it's safe to close this for now.

Ref: #1689

/close

[k8s-ci-robot]

@vincepri: Closing this issue.

In response to this:

We'll go through some redesign/refactor of these controllers in the future #, I think it's safe to close this for now.

Ref: #1689

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.