Cluster-api: MachineDeployment should support spec.deletePolicy (like MS)

Created on 8 Mar 2020 · 18Comments · Source: kubernetes-sigs/cluster-api

What steps did you take and what happened:

deletePolicy was implemented in https://github.com/kubernetes-sigs/cluster-api/pull/726 by adding a property to the machineSet to define the deletion policy (random, oldest, newest).

How is this intended to be leveraged since most users are only directly creating the machineDeployment api objects? I would have expected the machineDeployment to also expose a deletePolicy property.

Environment:

Cluster-api version:
Minikube/KIND version:
Kubernetes version: (use kubectl version):
OS (e.g. from /etc/os-release):

/kind bug

areapi help wanted kinapi-change kinfeature prioritimportant-soon

Source

rhockenbury

Most helpful comment

It looks like we'll likely need to expose this through the MachineRollingUpdateDeployment type for MachineDeployments

detiber on 9 Mar 2020

👍2

All 18 comments

It looks like we'll likely need to expose this through the MachineRollingUpdateDeployment type for MachineDeployments

detiber on 9 Mar 2020

👍2

This seems like it can be made in a future release as a non-breaking change

/milestone v0.3.x

vincepri on 9 Mar 2020

/assign

erstaples on 10 Mar 2020

@erstaples are you still interested in working on this feature?

vincepri on 31 Mar 2020

Hi @vincepri , I am indeed. Apologies for my self-assign-and-disappear act :) I can start work on it, at the latest, tomorrow.

erstaples on 1 Apr 2020

@vincepri OK, having taken a look at it, it looks like its a matter of surfacing DeletePolicy on the MachineRollingUpdateDeployment type, setting a default in PopulateDefaultsMachineDeployment, and making sure to set DeletePolicy on the MachineSet in machinedeployment_sync.go.

What might the concerns be wrt backwards compatibility?

erstaples on 1 Apr 2020

@erstaples If the MachineSet already has a default I don't think we need to set one in MachineDeployment, so it could effectively be empty (nil), maybe?

vincepri on 1 Apr 2020

👍1

@vincepri I have this complete and ready for a PR, aside from one issue. I ran make generate, which deleted the autogenerated function Convert_v1alpha3_MachineRollingUpdateDeployment_To_v1alpha2_MachineRollingUpdateDeployment.

After that, make test fails with the error

setting up [email protected] env vars
# sigs.k8s.io/cluster-api/api/v1alpha2
api/v1alpha2/zz_generated.conversion.go:162:10: undefined: Convert_v1alpha3_MachineRollingUpdateDeployment_To_v1alpha2_MachineRollingUpdateDeployment
api/v1alpha2/zz_generated.conversion.go:700:13: undefined: Convert_v1alpha3_MachineRollingUpdateDeployment_To_v1alpha2_MachineRollingUpdateDeployment
# sigs.k8s.io/cluster-api/api/v1alpha2 [sigs.k8s.io/cluster-api/api/v1alpha2.test]
api/v1alpha2/zz_generated.conversion.go:162:10: undefined: Convert_v1alpha3_MachineRollingUpdateDeployment_To_v1alpha2_MachineRollingUpdateDeployment
api/v1alpha2/zz_generated.conversion.go:700:13: undefined: Convert_v1alpha3_MachineRollingUpdateDeployment_To_v1alpha2_MachineRollingUpdateDeployment
FAIL    sigs.k8s.io/cluster-api/api/v1alpha2 [build failed]

Am I running the wrong version of a dependency used to generate the files, or...?

erstaples on 15 Apr 2020

@erstaples sorry for the late reply, those errors usually means that the auto-conversion generation wasn't able to figure out how to convert from the old type to the new one, so it needs manual conversion

vincepri on 1 May 2020

/help

vincepri on 10 Jun 2020

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

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.