-
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-1860 - Propose beta graduation and add missing PRR #4509
Conversation
f8b6a96
to
430f4a7
Compare
/assign @RyanAoh Feel free to refuse it, or otherwise use any of the additions here on your original KEP proposal. Thanks! |
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.
You need to update kep.yaml and keps/prod-readiness/sig-network/1860.yaml
too
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.
Hm... some of my comments before may have been slightly confused because I was thinking of this as a feature that the user opted into, rather than a feature that the CloudProvider decides whether or not to use.
/lgtm |
@wojtek-t as Dan approved it, can we now take a look into the PRR (once you have time!). Thanks!! |
I'm sorry for delay - will get to it tomorrow morning. |
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.
@rikatz - sorry for delay, added few comment - other than that LGTM
Longer term, we may want to require automated upgrade/rollback tests, but we | ||
are missing a bunch of machinery and tooling and can't do that now. | ||
--> | ||
No. |
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.
We should test that before enabling Beta by default.
Can you please describe the scenario here now (and ensure that you actually run it - manual run is fine) before enabling beta?
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.
Will do next here, will keep this thread open and close once I have results
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.
@wojtek-t did the test here. Just a node (that I have added to the KEP), because this feature is mostly related to cloud providers, the test I did was, on a 3 node KinD cluster, enabled the feature flag, installed MetalLB as the LB, ran some tests, and disabled the Feature flag to check how kube-proxy and apiserver would behave.
It is registered on the steps, let me know if you need something else.
f9f537e
to
c5ccc0c
Compare
* Confirmed that both iptables rules are present, even if the `ipMode` field was still | ||
set as `Proxy`, confirming the feature is disabled. Both accesses are working | ||
|
||
Additionally, an apiserver and kube-proxy upgrade test was executed as the following: |
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.
@wojtek-t added the additional test of v1.28 -> v1.29 -> v1.28 and worked fine. All tests were executed with KinD and metallb.
Thank you - this looks great now! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, rikatz, 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 |
One-line PR description: Add missing PRR descriptions and propose beta graduation for KEP 1860
Issue link: Make Kubernetes aware of the LoadBalancer behaviour #1860
Other comments: Will try to achieve PRR review and exception for v1.30, sorry for missing this train!