-
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-3705: update CloudDualStackNodeIPs to beta #3975
Conversation
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.
Couple small comments from me - but overall this looks great
|
||
So assuming no bugs, the only visible change after (just) enabling the | ||
feature is the duplicate annotation. | ||
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.
Isn't https://github.com/kubernetes/enhancements/pull/3898/files effectively a change in default behavior?
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 guess you're dropping the proposal from that PR, right?
Assuming I'm right here, I actually like it especially given backward compatibility here...
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'm not sure which part of that PR you're referring to, but it was never proposing a change in default behavior.
The original KEP-3705 said that we would
- add dual-stack
--node-ips
for external clouds - add
--node-ip IPv4
and--node-ip IPv6
etc - rename the node IP annotation
#3898 just dropped parts 2 and 3 from alpha, and now this PR drops them from beta and GA too.
There was some discussion in #3898 about stuff we might do in the future, but even then, the assumption was that we'd do it in a backward-compatible way (eg, leave --node-ip
with its current bad semantics and add --node-ips
with new-and-improved semantics.)
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 guess I was too much biased by the "late-breaking change" header. I've read it more carefully now and it all makes sense now.
Conveniently, we can do this just by passing the dual-stack | ||
`--node-ip` value in the existing annotation; the old cloud provider | ||
will try to parse it as a single IP address, fail, log an error, and | ||
leave the node in the tainted state, which is exactly what we wanted | ||
it to do if it can't interpret the `--node-ip` value correctly. | ||
|
||
Therefore, we do not _need_ a new annotation for the new `--node-ip` | ||
Therefore, we do not need a new annotation for the new `--node-ip` |
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 removed the part about the fact that annotation name is alpha.kubernetes.io/provided-node-ip
- what we're going to do with it?
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.
As you may have noticed later, that text is still in the "Alternatives" section.
I'm not planning to do anything about the annotation as part of this KEP. I suspect I'm going to end up doing a new KEP with all of the leftover parts of this and #1665.
are missing a bunch of machinery and tooling and can't do that now. | ||
--> | ||
No. The behavior during upgrade and rollback is not especially | ||
different from the ordinary runtime behavior. |
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 agree with this statement, but still, given that IPs are propagated to cloud-provider via the annotation (so technically we have some persistent state related to this feature), I think we should test it.
Manual test is good enough here (and can be done after feature freeze and updated with the description of the test then - https://github.com/kubernetes/enhancements/pull/3658/files is a great example).
Does that make sense to you? If so, can you please switch to a TODO for running it and updating this question before promoting the feature to beta?
0e6ee92
to
37fa611
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.
This is great - just one small minor comment and this LGTM.
37fa611
to
fef6fc9
Compare
/lgtm thanks! |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, 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 |
One-line PR description: update KEP-3705
CloudDualStackNodeIPs
to beta, dropping the parts that were not implemented in alphaIssue link: Cloud Dual-Stack --node-ip Handling #3705
Other comments:
This drops the parts of the KEP that weren't implemented for alpha (
--node-ip IPv4
/--node-ip IPv6
, and renaming thealpha.kubernetes.io/provided-node-ip
annotation), and moves some of the old text to the "Alternatives" section explaining what we didn't do and why./assign @thockin