Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support new topology/zones labels #70

Conversation

prashanth26
Copy link

@prashanth26 prashanth26 commented Feb 4, 2021

The old failure-domain label was deprecated in favour of topology/zone labels. This PR addresses this issue.
See: https://v1-18.docs.kubernetes.io/docs/reference/kubernetes-api/labels-annotations-taints/#failure-domainbetakubernetesiozone

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #68

Special notes for your reviewer:

Release note:

Support the latest zone label `topology.kubernetes.io/zone` in addition to the existing `failure-domain.beta.kubernetes.io/zone` while determining the zone for AWS machines.
Allow scaling up from zero using the latest stable zone, region, arch, OS, instanceType labels on node objects. 

@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 4, 2021
@prashanth26 prashanth26 mentioned this pull request Feb 4, 2021
2 tasks
Copy link
Member

@hardikdr hardikdr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @prashanth26 .

I think, buildGenericLabels needs to be adapted as well here: https://github.com/gardener/autoscaler/blob/machine-controller-manager-provider/cluster-autoscaler/cloudprovider/mcm/mcm_manager.go#L593.

Essentially, mcm-provider fetches the zone "value" from the MachineClass, and builds the node-object, where "key" of the zone-label on the node, should also be adapted for both topology/failure-domain. Both variations could be kept, for backward compatibility, you can take a call on that.

Also, can you please the test exact end-user case with this change, to ensure it solves the issue as expected?

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Feb 5, 2021
@hardikdr
Copy link
Member

hardikdr commented Feb 5, 2021

If I am not wrong, with this change, where both variations of the label are kept on the node, the original issue should be resolved, but I'd really suggest testing it once.

  • Although the worker-extension should anyways be updated for consistency.

@prashanth26
Copy link
Author

I think, buildGenericLabels needs to be adapted as well here: https://github.com/gardener/autoscaler/blob/machine-controller-manager-provider/cluster-autoscaler/cloudprovider/mcm/mcm_manager.go#L593.

Nice catch @hardikdr . Thanks for this. I shall update it to have both labels. Probably will also look into the region label.

Also, can you please the test exact end-user case with this change, to ensure it solves the issue as expected?

Yes, hardik. I will need the changes on extensions also to create the machineClass with the new labels. Will make the changes on extension and test it out together. Will keep this PR open until then change has been tested with the usecase.

If I am not wrong, with this change, where both variations of the label are kept on the node, the original issue should be resolved, but I'd really suggest testing it once.

Yes shall do that.

Thanks for this review. Appreciate it. :)

// labelFailureDomainZone is the deprecated label for failure domain zone used by kubernetes.io
// It has been deprecated in favor of (below) topology.kubernetes.io/zone label
// See: https://v1-18.docs.kubernetes.io/docs/reference/kubernetes-api/labels-annotations-taints/#failure-domainbetakubernetesiozone
labelFailureDomainZone = "failure-domain.beta.kubernetes.io/zone"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment, with rebase these labels now can be fetched from apiv1 "k8s.io/api/core/v1" as well.

@hardikdr
Copy link
Member

hardikdr commented Feb 5, 2021

Yes, hardik. I will need the changes on extensions also to create the machineClass with the new labels.

If the changes of the "buildGenericLabels" are also included, the changes at extension might not be needed to test it out, the extension should always be updated though.

@prashanth26 prashanth26 force-pushed the enhancement/update-zone-labels branch from 2cd801c to e3943ff Compare February 10, 2021 05:11
@gardener-robot gardener-robot added the size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) label Feb 10, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 10, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 10, 2021
@prashanth26
Copy link
Author

Hi @hardikdr ,

I have taken in your suggestions and made the necessary changes here - e3943ff. Hope this looks good now.

Will be testing these changes locally today/tomorrow.

@hardikdr
Copy link
Member

Sure, thank you.

@prashanth26
Copy link
Author

prashanth26 commented Feb 11, 2021

Just tested these changes locally. It works backward compatible with both the deprecated and current zone and region labels.

I think for the first cut let's just release the cluster-autoscaler with this change and it should take care of the rest. We can update the provider-aws labels in a couple of releases.

@hardikdr - Let me know if it looks good to merge.

@prashanth26 prashanth26 merged commit 0da90c1 into gardener:machine-controller-manager-provider Feb 12, 2021
@prashanth26 prashanth26 deleted the enhancement/update-zone-labels branch February 12, 2021 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support new zones label
6 participants