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.

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-3836 documentation for 1.30 #45290

Conversation

alexanderConstantinescu
Copy link
Member

Enhancement: kubernetes/enhancements#3836

Original implementation PR: kubernetes/kubernetes#116470

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 22, 2024
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Feb 22, 2024
Copy link

netlify bot commented Feb 22, 2024

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit c94d2eb
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6602b3ead66a0a0008d9d093
😎 Deploy Preview https://deploy-preview-45290--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@chanieljdan
Copy link
Contributor

Hi @alexanderConstantinescu 👋 just a reminder to take a look at Documenting for a release - PR Ready for Review to get your PR ready for review before Tuesday March 12th 2024 18:00 PST.

Thank you!

@alexanderConstantinescu alexanderConstantinescu changed the title WIP: KEP-3836 documentation for 1.30 KEP-3836 documentation for 1.30 Mar 12, 2024
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. language/en Issues or PRs related to English language and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 12, 2024
@alexanderConstantinescu
Copy link
Member Author

alexanderConstantinescu commented Mar 12, 2024

OK, I've added a section under https://kubernetes.io/docs/concepts/services-networking/service/#loadbalancer which details the readiness / liveness logic around Kube-proxy and terminating/deleting nodes. From the enhancement we had agreed that we wanted to cover pitfalls w.r.t configuring health check for services of type: LoadBalancer and provide recommendations.

I am not fully sure about the wording to use when providing recommendations to service proxy's / cloud providers, but we can refine this later on. I wanted to get this up today for the "ready for review" deadline

/cc @thockin

@drewhagen
Copy link
Member

drewhagen commented Mar 15, 2024

/sig network
/milestone 1.30

Hello @alexanderConstantinescu 👋, thanks for getting this up for review! A friendly reminder of timeline, this PR needs doc review complete by Doc Freeze on March 26th 18:00 PT to get this into the release.

@thockin @danwinship @aojea
When one of y'all have a moment, please give a review for technical accuracy.

@chanieljdan Please give this a review also - I think your perspective in this domain would be valuable.
Here is a document on the KEP, including the kep.yaml which has information we'll want to spot-check before release. You can also check a preview of the documentation with Netlify above.

I appreciate everyone's help and so will our users! Thank you greatly.

@k8s-ci-robot k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Mar 15, 2024
@k8s-ci-robot k8s-ci-robot added this to the 1.30 milestone Mar 15, 2024
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.

Thanks.

I wonder whether all of this content would find a better home within
https://kubernetes.io/docs/reference/networking/; the Service concept page can have a two sentence summary and a link to the specifics.

How about doing that?


Also, it's kube-proxy (no capital K); this matches the container images we build.

content/en/docs/concepts/services-networking/service.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/service.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/service.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/service.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/service.md Outdated Show resolved Hide resolved
@sftim
Copy link
Contributor

sftim commented Mar 18, 2024

Does https://kubernetes.io/docs/reference/networking/ports-and-protocols/ need an update for the changes in this KEP?

@chanieljdan
Copy link
Contributor

Hi @alexanderConstantinescu 👋 I'm reaching out from the Docs team. Just checking in as we approach Docs Freeze on Tuesday March 26th 18:00 PDT. This documentation appears to still be under review. To meet the Docs Freeze, this PR must have a technical review as well as lgtm and approve labels applied, without any unaddressed comments or concerns from SIG Docs.

@thockin @danwinship @aojea
Could we get a review for technical accuracy. Thanks!

@sftim
Copy link
Contributor

sftim commented Mar 24, 2024

Any thoughts specifically on #45290 (comment)?

@k8s-ci-robot k8s-ci-robot added the language/zh Issues or PRs related to Chinese language label Mar 25, 2024
@alexanderConstantinescu
Copy link
Member Author

Any thoughts specifically on #45290 (comment)?

No objection from me, I added an additional commit which addresses that: approvers can decide on if we should keep or not

@@ -488,6 +488,63 @@ route to ready node-local endpoints. If the traffic policy is `Local` and there
are no node-local endpoints, the kube-proxy does not forward any traffic for the
relevant Service.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this mostly belongs under the heading ### Traffic to terminating endpoints. (my guess is: yes it does; you could leave in a one sentence hyperlink to the moved text)

and somewhere you ought to add:

{{< feature-state feature_gate_name="KubeProxyDrainingTerminatingNodes" >}}

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if this mostly belongs under the heading ### Traffic to terminating endpoints

Not quite, what I've added deals with "traffic to terminating nodes"

and somewhere you ought to add:

What does that do? The feature is beta in 1.30 so enabled by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

The feature state shortcode tells people they need a particular Kubernetes version to use it - see https://kubernetes.io/docs/contribute/new-content/new-features/#ready-for-review-feature-gates

Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

LGTM on tech review

@thockin
Copy link
Member

thockin commented Mar 25, 2024

/lgtm

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

LGTM label has been added.

Git tree hash: 17d0b83e23b65769bb072dec4c77283beb0fb3b6

@drewhagen
Copy link
Member

Looks like we're just missing approve label on this one. Would prefer to get that before Doc Freeze tomorrow March 26th at 18:00 PDT, to evade the need to file an exception. (new process this release)

@kubernetes/sig-docs-en-owners Are there any outstanding concerns or is this ready to approve?

Copy link
Member

@dipesh-rawat dipesh-rawat left a comment

Choose a reason for hiding this comment

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

@alexanderConstantinescu Perhaps we should consider setting the dev-1.30 branch as the target branch rather than the main branch for this pull request. Since the change is related to the feature targeting beta in 1.30.

Also, the PR is currently lacking the doc change to transition the feature gate KubeProxyDrainingTerminatingNodes from Alpha to Beta. The necessary change should be made in the feature gate description file - kube-proxy-draining-terminating-nodes.md (here).

Please refer to the documentation here for further information.

@alexanderConstantinescu
Copy link
Member Author

@alexanderConstantinescu Perhaps we should consider setting the dev-1.30 branch as the target branch rather than the main branch for this pull request. Since the change is related to the feature targeting beta in 1.30.

Sure, I will open an additional PR and target dev-1.30 then, you can decide on which PR to merge.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 26, 2024
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@alexanderConstantinescu
Copy link
Member Author

Opened #45678 - feel free to close this one if needed.

@dipesh-rawat
Copy link
Member

Opened #45678 - feel free to close this one if needed.

@alexanderConstantinescu If you could kindly update the Enhancements Issue kubernetes/enhancements#3836 with the merged PR number targeting dev-1.30- PR #45678. Ensuring that the release team is properly informed and tracking the correct pull request would be immensely helpful. Thanks!

Copy link
Member

@dipesh-rawat dipesh-rawat left a comment

Choose a reason for hiding this comment

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

Closing this PR as these changes having been merged in dev-1.30 as part of another PR #45678

/close

@k8s-ci-robot
Copy link
Contributor

@dipesh-rawat: Closed this PR.

In response to this:

Closing this PR as these changes having been merged in dev-1.30 as part of another PR #45678

/close

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.

@drewhagen
Copy link
Member

/milestone 1.30

@k8s-ci-robot k8s-ci-robot added this to the 1.30 milestone Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language language/zh Issues or PRs related to Chinese language 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants