Skip to content
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.

Sign up for GitHub

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

Jump to bottom

update CloudDualStackNodeIPs to beta #42875

Merged
merged 1 commit into from
Sep 26, 2023
Merged

update CloudDualStackNodeIPs to beta #42875

merged 1 commit into from
Sep 26, 2023

Conversation

danwinship
Copy link
Contributor

KEP-3705 CloudDualStackNodeIPs is beta in 1.29

@k8s-ci-robot k8s-ci-robot added this to the 1.29 milestone Sep 4, 2023
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 4, 2023
@netlify
Copy link

netlify bot commented Sep 4, 2023 •
edited
Loading

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit ba28234
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6508f4813b2abb00098d74cd

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 4, 2023
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Sep 4, 2023
@k8s-ci-robot k8s-ci-robot requested a review from sftim September 4, 2023 13:43
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Sep 4, 2023
@danwinship danwinship mentioned this pull request Sep 4, 2023
Closed
12 tasks
@Shubham82
Copy link
Contributor

/sig network

@k8s-ci-robot k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Sep 4, 2023
sftim
sftim reviewed Sep 4, 2023
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually wouldn't use a feature-state shortcode now. The “feature” is very hard to name.

content/en/docs/concepts/services-networking/dual-stack.md Show resolved Hide resolved
content/en/docs/concepts/services-networking/dual-stack.md Outdated
Comment on lines 84 to 86
When using an external cloud provider, you can pass a dual-stack `--node-ip` value to
kubelet if you enable the `CloudDualStackNodeIPs` feature gate in both kubelet and the
external cloud provider. This is only supported for cloud providers that support dual
stack clusters.
kubelet. This is only supported for cloud providers that support dual stack clusters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we support for --node-ip, dual-stack, and a bare metal cluster? Let's make that clear.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 5, 2023
danwinship
danwinship commented Sep 5, 2023
View reviewed changes
content/en/docs/concepts/services-networking/dual-stack.md
Comment on lines -70 to -73
If a Pod runs on that node in HostNetwork mode, the Pod reports these IP addresses in its
`.status.podIPs` field.
All `podIPs` in a node match the IP family preference defined by the `.status.addresses`
field for that Node.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up dropping this part because it's not really about kubelet --node-ip (it's true whether the IPs were assigned by --node-ip or by default) and because it's kind of obvious anyway... It was interesting to note these things at one point in the past when we had just fixed that behavior, but at this point, this is just how anyone would expect it to work.

content/en/docs/concepts/services-networking/dual-stack.md
* This is required for "bare metal" dual-stack nodes (i.e., nodes with no
`--cloud-provider`). If you are using a cloud provider, you only need to pass an
explicit `--node-ip` if you want to override the node IPs chosen by the cloud provider.
* (The legacy built-in cloud providers do not support dual-stack `--node-ip`.)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about wording here... but it seemed like it made more sense to describe the non-legacy-cloud-provider behavior first and then add a disclaimer about the legacy cloud providers, so we can just remove that sentence once they go away?

sftim
sftim reviewed Sep 5, 2023
View reviewed changes
content/en/docs/concepts/services-networking/dual-stack.md Outdated
Comment on lines 69 to 70
* This is required for "bare metal" dual-stack nodes (i.e., nodes with no
`--cloud-provider`). If you are using a cloud provider, you only need to pass an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

Suggested change
* This is required for "bare metal" dual-stack nodes (i.e., nodes with no
`--cloud-provider`). If you are using a cloud provider, you only need to pass an
* this is required for dual-stack nodes in "bare metal" mode (nodes where you do not specify
`--cloud-provider` as a command line argument to the kubelet). The kubelet publishes the
IP addresses that you specify, as `.status.addresses`.
If you are using a cloud provider integration, you only need to pass an

Does the kubelet ever autodetect dual-stack .status.addresses? If so, we should try to tweak the wording further.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No; if you specify neither --cloud-provider nor --node-ip, then kubelet autodetects a single node IP.

Arhell
Arhell reviewed Sep 6, 2023
View reviewed changes
Copy link
Member

@Arhell Arhell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please re-puch commit for restart netlify

@danwinship
Copy link
Contributor Author

@Arhell done

@tengqm
Copy link
Contributor

tengqm commented Sep 15, 2023

Thanks.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: f3c5abfb138de489a1dfda3eb201aeb23a65b72a

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2023
@k8s-ci-robot k8s-ci-robot requested a review from tengqm September 19, 2023 01:08
@tengqm
Copy link
Contributor

tengqm commented Sep 19, 2023

/lgtm
again.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d64e258543030c3cd0c30500eb00b6269c041802

@tengqm
Copy link
Contributor

tengqm commented Sep 26, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tengqm

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2023
@k8s-ci-robot k8s-ci-robot merged commit 4af01d0 into kubernetes:dev-1.29 Sep 26, 2023
4 checks passed
@danwinship danwinship deleted the kep-3705-beta branch October 16, 2023 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arhell Arhell Arhell left review comments

@sftim sftim sftim left review comments

@kbhawkey kbhawkey kbhawkey left review comments

@khenidak khenidak Awaiting requested review from khenidak

@tengqm tengqm Awaiting requested review from tengqm

Assignees

@tengqm tengqm

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/network Categorizes an issue or PR as relevant to SIG Network. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
1.29
Development

Successfully merging this pull request may close these issues.
7 participants
@danwinship @Shubham82 @tengqm @k8s-ci-robot @kbhawkey @sftim @Arhell