FailureDomain selection order possibly incorrect

[richardcase]

/kind bug

What steps did you take and what happened:
With #439 we added support for reporting failure domains (i.e. Azure AZs) back to CAPI so that it could then try to spread machines across failure domains (by populating Machine.Spec.FailureDomain). Logic was added to the machine scope to get the failure domain for use during machine reconciliation based on the precedence:

1. Machine.Spec.FailureDomain
2. AzureMachine.Spec.FailureDomain
3. AzureMachine.Spec.AvailabilityZone.ID (This is DEPRECATED)
4. No AZ

If a region has availability zones these will be reported back to CAPI, CAPI will then choose one of the AZs and set Machine.Spec.FailureDomain (if that field isn't already set). This means that if AZs are explicitly set in AzureMachine.Spec.FailureDomain or AzureMachine.Spec.AvailabilityZone.ID they will be ignored.

This behavior doesn't feel correct.

What did you expect to happen:
I would expect the user to be able to explicitly set an AZ for machine and for that to be honored.

We could change the order to:

1. AzureMachine.Spec.AvailabilityZone.ID (This is DEPRECATED)
2. AzureMachine.Spec.FailureDomain
3. Machine.Spec.FailureDomain

This would allow us to support old definitions and also take advantage of automatic AZ selection if no AZ is explicitly set.

I also think that AzureMachine.Spec.FailureDomain feels a little redundant and we could change the precedence to:

1. AzureMachine.Spec.AvailabilityZone.ID (This is DEPRECATED)
2. Machine.Spec.FailureDomain

So if a user wants to explicitly set the AZ for a machine then they use Machine.Spec.FailureDomain

Environment:

• cluster-api-provider-azure version: 0.43

[richardcase]

/assign

[richardcase]

@CecileRobertMichon @devigned @detiber - be good to get your thoughts on this.

[detiber]

@richardcase both AzureMachine.Spec.AvailabilityZone.ID and
AzureMachine.Spec.FailureDomain should be considered deprecated in favor of Machine.Spec.FailureDomain.

The addition of AzureMachine.Spec.FailureDomain was required to allow for migrating existing values that were previously set in AzureMachine.Spec.AvailabilityZone.ID to Machine.Spec.FailureDomain automatically for existing users.

The Machine controller will pull the value of .Spec.FailureDomain to populate Machine.Spec.FailureDomain (if it is not already set).

The reason for this was to facilitate the treatment of failure domains as first class across common Cluster API resources. Previously they were only available as an infrastructure provider specific with no real commonality across providers to standardize with.

[richardcase]

Ah yes, thanks @detiber. I forgot we discussed and added this to the machine controller:

cluster-api-provider-azure/controllers/azuremachine_controller.go

Line 218 in c0d09b2

if machineScope.AzureMachine.Spec.FailureDomain == nil {

[CecileRobertMichon]

@richardcase does that mean there is nothing to do here for now? Maybe just make this more clear in docs? And then eventually remove the AzureMachine fields when we move to v1alpha4?

[richardcase]

@CecileRobertMichon - yes i think a small docs update for now and then removal of field in v1alpha4 would work.

I'll do the docs update and reference this issue. Should i then raise an issue for removal in v1alpha4 or will it be covered by #618?

[CecileRobertMichon]

I commented #618 (comment) I think that should be enough. Thanks!

[alexeldeib]

/kind documentation
/kind cleanup
/remove-kind bug

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[CecileRobertMichon]

/remove-lifecycle stale

@richardcase did you still want to update the docs for this one?

[richardcase]

Sorry @CecileRobertMichon i completely forgot about this, yes i will update the dcos for this.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[richardcase]

I've still not got around to this, sorry. With v1alpha4 not far off is there any benefit in doing this?

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

[CecileRobertMichon]

/close

availabilityZones were removed in #1233

[k8s-ci-robot]

@CecileRobertMichon: Closing this issue.

In response to this:

/close

availabilityZones were removed in #1233

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.