-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
1.29: Fix PersistentVolumeLabel admission plugin on Azure #124528
1.29: Fix PersistentVolumeLabel admission plugin on Azure #124528
Conversation
Do not provide any topology labels for PVs when the Azure cloud provider is not initialized. Providing `topology.kubernetes.io/region: ""` is wrong, because such region does not exist. This affects only in-tree PVs. We have CSI migration in place that makes sure newly provisioned PVs already have correct nodeAffinity, so topology labes are not necessary. And for manually provisioned PVs, no topology label is better that a wrong one, because users can add the label later, but they can't update PV spec.nodeAffinity that would be created from PV labels during PersistentVolumeLabel admission. Note that the whole Azure cloud provider is removed in v1.30, so I'm using just a minimal patch.
fc73095
to
e5561a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
LGTM label has been added. Git tree hash: ea211e15833b2759cc5c94df440870e5e6a20b7b
|
/approve |
/lgtm |
@cubxxw: changing LGTM is restricted to collaborators In response to this:
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. |
@jsafrane: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
/retest-required |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx, dims, jsafrane The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test pull-kubernetes-e2e-kind-cloud-provider-loadbalancer |
@Verolop: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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-sigs/prow repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/retest-required
…4528-upstream-release-1.27 Automated cherry pick of #124528: Fix PersistentVolumeLabel admission plugin on Azure
…4528-upstream-release-1.28 Automated cherry pick of #124528: Fix PersistentVolumeLabel admission plugin on Azure
@Verolop all required tests passed, it really misses just cherry pick approval |
/retest-required |
/test pull-kubernetes-e2e-kind-cloud-provider-loadbalancer |
@bridgetkromhout: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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-sigs/prow repository. |
/retest |
/test all |
/cc kubernetes/release-managers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/retest-required
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
3 similar comments
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
/skip |
This is a manual patch for v1.29. The whole Azure cloud provider was removed in v1.30 and master, so I cannot fix it there and cherry pick it to older branches. I will backport it to 1.28 and older (if they're still supported).
What type of PR is this?
/kind bug
What this PR does / why we need it:
Do not provide any topology labels for PVs when the Azure cloud provider is not initialized. Providing
topology.kubernetes.io/region: ""
is wrong, because such region does not exist and PVs with this label would not be usable by any node.This affects only in-tree PVs. We have CSI migration in place that makes sure newly provisioned PVs already have correct nodeAffinity, so topology labels are not necessary. And for manually provisioned PVs, no topology label is better that a wrong one, because users can add the label later, but they can't update PV spec.nodeAffinity that would be created from wrong PV label during PersistentVolumeLabel admission.
Note that the whole Azure cloud provider is removed in v1.30, so I'm using just a minimal patch.
Which issue(s) this PR fixes:
Fixes #124525
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: