MachineDeployment should support spec.deletePolicy (like MS)

[rhockenbury]

What steps did you take and what happened:

deletePolicy was implemented in #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

[detiber]

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

[vincepri]

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

/milestone v0.3.x

[erstaples]

/assign

[vincepri]

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

[erstaples]

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

[erstaples]

@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?

[vincepri]

@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?

[erstaples]

@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...?

[vincepri]

@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]

/help

[k8s-ci-robot]

@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.

[vincepri]

/area api
/milestone v0.4.0
/priority important-soon

[vincepri]

/kind feature

[vincepri]

/retitle MachineDeployment should support spec.deletePolicy (like MS)

[shysank]

@vincepri is this still being worked on by someone? If not, can I take this one?

[vincepri]

Sure yeah!

[shysank]

/assign

[fabriziopandini]

/kind api-change