-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[KEP 3458]: 1.29 milestone bump - stable #4257
[KEP 3458]: 1.29 milestone bump - stable #4257
Conversation
6d34417
to
08a599b
Compare
status: implementable | ||
stage: beta | ||
latest-milestone: "v1.28" | ||
stage: stable |
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.
Few comment about the KEP itself:
- In mitigations section:
For point 3. we could implement the following change in the service controller [...]
has that happened? If not, why we believe we don't need it? can you update the KEP?
Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested
has this happened? Can you please update the KEP with the test description and findings?
A new metric load_balancer_sync_count will be added
Has this happened?
A new metric nodesync_error_rate will be added
Has this happened?
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.
has that happened? If not, why we believe we don't need it? can you update the KEP?
Yes. The service controller still employs the predicate which excludes nodes with the taint ToBeDeletedByClusterAutoscaler
, see: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/cloud-provider/controllers/service/controller.go#L1009-L1014C26
has this happened? Can you please update the KEP with the test description and findings?
No
Has this happened?
Has this happened?
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.
For the "yes" thing - please update the KEP to reflect that (with links).
For the no thing [the test thing], can we try running something? @thockin - FYI
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.
For the "yes" thing - please update the KEP to reflect that (with links).
Done.
For the no thing [the test thing], can we try running something?
I am not sitting on infrastructure which allows me to create cluster from HEAD. That being said: I quite sure the upgrade path works because this has been enabled by default since 1.27 and my company has successfully upgraded clusters from 1.26 -> 1.27, with services existing on the cluster that are impacted by this change.
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 can help get access to infra to do this manual testing, if that's what we need. That said, I'm a little skeptical that we will find anyhing by just doing it, without a) knowing what we are looking for; and b) tickling the edge cases specifically.
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.
OK - I buy that.
But then let's update the Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested?
PRR question to something like
The owners of the KEP missed running the manual test when promoting the feature to Beta. Since then it was implicitly tested by many users that upgraded their clusters to 1.27+ versions without any bug reports, so running additional tests now wouldn't provide additional value.
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've updated the answer to that question.
feb318a
to
3f94e09
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
/approve
/hold
to discuss manual testing
status: implementable | ||
stage: beta | ||
latest-milestone: "v1.28" | ||
stage: stable |
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 can help get access to infra to do this manual testing, if that's what we need. That said, I'm a little skeptical that we will find anyhing by just doing it, without a) knowing what we are looking for; and b) tickling the edge cases specifically.
3f94e09
to
2edbe35
Compare
Thanks! /lgtm |
/lgtm Thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexanderConstantinescu, thockin, wojtek-t 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 |
/sig network
/assign @thockin
/assign @wojtek-t