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
Bug 1975379: Only use kubernetes.io/hostname
for workload anti-affinity
#566
Conversation
@jhadvig: This pull request references Bugzilla bug 1975379, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
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.
This fixes deployment on OpenStack. Please fix the unit tests and let's merge ASAP.
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.
Code LGTM, but we need to fix the unit tests
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
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.
I don't want it to hold up this patch, but the use of timezone
in the subject confused me! It's an availability zone/failure domain.
Yeah I noticed that too. It really should be availability zone not timezone. |
I would suggest that we should actually entirely remove topology from the scheduling requirements here. From the original bug report:
The only requirement here is that the console is not impacted during cluster upgrades, which is covered by My immediate priority, though, is to un-break the OpenStack jobs, so if that's landing this patch then land away! However, I recommend you remove this entirely. We also need to get our CI voting here! |
kubernetes.io/hostname
for workload anti-affinity
/hold cancel |
Updated, @mdbooth PTAL |
/retest |
Thanks for updating the patch. I personally didn't have major concerns for the soft anti-affinity for availability zones, however the commit message bothered me a bit. Glad it's now fixed. Let's unblock openstack CI. |
/test e2e-agnostic-upgrade |
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.
@spadgett thanks for updating the PR 👍
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, mandre, mdbooth, spadgett 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 |
/retest |
/retest-required |
@jhadvig: All pull requests linked via external trackers have merged: Bugzilla bug 1975379 has been moved to the MODIFIED state. 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. |
/cherry-pick release-4.8 |
@jhadvig: #566 failed to apply on top of branch "release-4.8":
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. |
No description provided.