Migrate reference details out Service concept #36675

sftim · 2022-09-07T21:19:29Z

Split out from #30817

Previews:

Migrate away:

details of virtual IP mechanism for Services
detailed information about protocols for Services (UDP, TCP, SCTP)

Edit: this PR helps us to document the removal of userspace kube-proxy, by giving a new better home for the kube-proxy docs.

/sig network

sftim · 2022-09-07T21:19:41Z

Helps with #14770

sftim · 2022-09-07T21:20:06Z

/label refactor

netlify · 2022-09-07T21:31:55Z

✅ Pull request preview available for checking

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`b581bd4`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/637fb9a541cafb0007e15d55
😎 Deploy Preview	https://deploy-preview-36675--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

tengqm · 2022-09-08T00:38:03Z

content/en/docs/concepts/services-networking/service.md

-[supported protocol](#protocol-support).
+The default protocol for Services is
+[TCP](/docs/reference/networking/service-protocols/#protocol-tcp); you can also
+use any other [supported protocol](/docs/reference/networking/service-protocols/).


nit: protocols?

For this PR, I was aiming to retain the original text. Subsequent PRs could tidy that up.

If some of my other PRs merge, I can shrink #30817 until it reaches a reviewable size. That PR includes more fixes than just the reorganisation that this PR covers.

content/en/docs/reference/networking/_index.md

content/en/docs/reference/networking/service-protocols.md

tengqm · 2022-09-08T00:46:16Z

content/en/docs/reference/networking/service-protocols.md

+NAT for multihomed SCTP associations requires special logic in the corresponding kernel modules.
+
+{{< note >}}
+The kube-proxy does not support the management of SCTP associations when it is in userspace mode.


You may want to make 'kube-proxy' a link or a glossary.

For this PR, I was aiming to retain the original text. Subsequent PRs could tidy that up.

If some of my other PRs merge, I can shrink #30817 until it reaches a reviewable size. That PR includes more fixes than just the reorganisation that this PR covers.

tengqm · 2022-09-08T00:46:57Z

content/en/docs/reference/networking/service-protocols.md

+
+### `TCP` {#protocol-tcp}
+
+You can use TCP for any kind of Service, and it's the default network protocol.


Suggested change

You can use TCP for any kind of Service, and it's the default network protocol.

You can use TCP for any kind of Service, and it's the default service protocol.

For this PR, I was aiming to retain the original text. Subsequent PRs could tidy that up.

If some of my other PRs merge, I can shrink #30817 until it reaches a reviewable size. That PR includes more fixes than just the reorganisation that this PR covers.

content/en/docs/reference/networking/virtual-ips.md

tengqm · 2022-09-08T08:58:08Z

content/en/docs/reference/networking/virtual-ips.md

+IP address, for example 10.0.0.1.  Assuming the Service port is 1234, the
+Service is observed by all of the kube-proxy instances in the cluster.
+When a proxy sees a new Service, it opens a new random port, establishes an
+iptables redirect from the virtual IP address to this new port, and starts accepting


Suggested change

iptables redirect from the virtual IP address to this new port, and starts accepting

iptables rule that redirects from the virtual IP address to this new port, and starts accepting

For this PR, I was aiming to retain the original text. Subsequent PRs could tidy that up.

If some of my other PRs merge, I can shrink #30817 until it reaches a reviewable size. That PR includes more fixes than just the reorganisation that this PR covers.

content/en/docs/reference/networking/virtual-ips.md

divya-mohan0209

Just a single non-blocking nit from my side. Since this is a refactor PR, I'd like another pair of eyes to review it too! But thanks for the effort, Tim :)

divya-mohan0209 · 2022-11-25T02:58:13Z

content/en/docs/concepts/services-networking/service.md

@@ -666,6 +490,12 @@ Kubernetes `ServiceTypes` allow you to specify what kind of Service you want.
  to use the `ExternalName` type.
  {{< /note >}}

+The `type` field was designed as nested functionality - each level adds to the


non-blocking nit

Suggested change

The `type` field was designed as nested functionality - each level adds to the

The `type` field was designed as a nested functionality - each level adds to the

I've kept that wording from the original AFAIK. We could do a tiny later PR to add that word if we want to.

divya-mohan0209 · 2022-11-26T02:19:14Z

As long as we intend to fix the nits over a separate PR, I'm LGTMing this change from my side.
/lgtm

k8s-ci-robot · 2022-11-26T02:19:20Z

LGTM label has been added.

Git tree hash: 9b01179d2d260c80de39dd0df40e689b10fedfb9

sftim · 2022-11-29T00:54:26Z

It's not easy to find where the text has moved to.

Locally, you can use a tool like vimdiff. Concatenate the new reference pages and the old Service concept page into one file and use that as the right hand side. Use the old Service page Markdown as the left hand side.

You'll get better results if you set vimdiff to use the Patience diff algorithm. It's not perfect and other tools exist. opendiff on macOS was good, back when I used to use that.

natalisucks · 2022-11-29T19:04:06Z

@sftim Is there an issue already for the revisions as suggested by Qiming? I ask so as not to lose the spirit of wanting to keep current text as is for this refactor, while still committing to improving it after release deadlines are met.

sftim · 2022-11-29T19:12:06Z

We changed our review guidelines since I started on this and we now suggest that errors that the PR didn't introduce are OK to leave.

I am willing to file issues, although if I do that now and we merge, those will be issues against the wrong page.

natalisucks · 2022-11-29T19:14:37Z

Thanks, Tim 🤝
/approve

k8s-ci-robot · 2022-11-29T19:14:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natalisucks

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:

~~OWNERS~~ [natalisucks]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tengqm · 2022-11-30T03:39:55Z

THIS IS REALLY A BAD EXAMPLE TO THE WHOLE COMMUNITY.

IT IS A MIXTURE OF REFACTOR AND REWRITING. IT IS TOO HUGE TO REVIEW. IT CONTAINS MANY DEFECTS THAT WERE IDENTIFIED BUT NOT FIXED. THE AUTHOR PROMISED TO FIX THEM IN FOLLOW-UP PRS BUT NO ISSUES ARE FILED, IN OTHER WORDS, THE BALLS NOW FELL TO THE GROUND.

A GOOD PRACTICE SHOULD BE KEPT COMMUNITY WIDE, ABIDE BY ALL MEMBERS. THIS ONE IS AN EXCEPTION. IT INTRODUCED A NEW PROTOCOL: IF ONE'S PR IS NOT THOROUGHLY REVIEWED AND THE AUTHOR WANTS IT TO BE MERGED, WITH SUFFICIENT PRIVILEGE, THE AUTHOR CAN CHANGE THE REVIEW GUIDELINES.

I AM FEELING VERY UPSET BY THIS.

natalisucks · 2022-11-30T12:05:15Z

@tengqm Hi Qiming, I see that you've raised this concern in the #sig-docs-maintainers channel, so I'd like us to continue having the discussion there – I have linked the thread for visibility so that other folks involved in this PR, and the wider community, can follow along. I agree overall with your point about setting good examples for the community, which is why I'd love us to continue discussions in a respectful way – pushing your point in all caps seems counterintuitive to this.

I would also like to unpack your feeling of the author having privilege, and try and get us to a point where you, as a fellow tech lead, would feel equally empowered amongst your co-chair and tech lead peers. In following my own advice, I'll post comments in the above-linked thread so that we can discuss this more and get us to a point where every contributor and lead feels empowered to contribute changes, but more importantly, to bring up dissent and healthy disagreements for the betterment of pushing good practice.

tengqm · 2022-11-30T12:39:47Z

@natalisucks This PR is huge and very difficult to review. There were quite some issues raised during the past reviews and the author refused to fix. One of the arguments is that this is a refactor PR. No. If you have reviewed the change, it is NOT a refactor, it is introducing a lot of new texts that are misleading or inaccurate. Another argument is that while introducing new text, this PR is not fixing any existing text. Why is that? We don't have such a criteria for approving PRs. The solution, well, was to change the rule. Is that an acceptable practice that shows respects to reviewers?

Even when we merged the PR, it still had 6 commits, more than 800 lines of text changed. Every single change to the Service topic needs some discussions. For example, is application protocol tied to load-balancer type of Services? Is the removal of some texts well justified? Are we introducing many dangling dead links to the Service concept? Since this change involves many new texts, don't we need some tech reviews from the relevant SIGs, especially sig-network?

I'd not be so dismayed if the PR is a simple refactoring. A huge change to one of the core concept needs more eyes. To make sure we are really improving the docs, we could have split this PR into 10+ smaller ones. We are not supposed to merge this prematurely.

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 7, 2022

k8s-ci-robot requested review from daminisatya and divya-mohan0209 September 7, 2022 21:19

k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Sep 7, 2022

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 7, 2022

k8s-ci-robot added the refactor Indicates a PR with large refactoring changes e.g. removes files or moves content label Sep 7, 2022

tengqm reviewed Sep 9, 2022

View reviewed changes

sftim force-pushed the 20221208_refactor_service_docs branch 4 times, most recently from e10f4b5 to 3f4822e Compare September 15, 2022 11:45

sftim marked this pull request as draft October 1, 2022 20:25

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 1, 2022

sftim force-pushed the 20221208_refactor_service_docs branch 2 times, most recently from 6f19b1a to f202b42 Compare October 11, 2022 09:54

sftim force-pushed the 20221208_refactor_service_docs branch from f202b42 to f0ce483 Compare October 22, 2022 00:21

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2022

sftim force-pushed the 20221208_refactor_service_docs branch from f0ce483 to 07a52d0 Compare October 22, 2022 00:22

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2022

sftim force-pushed the 20221208_refactor_service_docs branch from 07a52d0 to 1bc7b2b Compare October 22, 2022 00:25

sftim mentioned this pull request Oct 22, 2022

Revise concept & reference docs for Services, Ingress & networking #30817

Closed

sftim marked this pull request as ready for review October 22, 2022 00:31

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 22, 2022

k8s-ci-robot requested a review from reylejano October 22, 2022 00:31

sftim force-pushed the 20221208_refactor_service_docs branch from f7a2c34 to 28f170b Compare October 22, 2022 00:36

sftim force-pushed the 20221208_refactor_service_docs branch from cf98610 to b581bd4 Compare November 24, 2022 18:36

divya-mohan0209 reviewed Nov 25, 2022

View reviewed changes

k8s-ci-robot assigned divya-mohan0209 Nov 26, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2022

krol3 mentioned this pull request Nov 28, 2022

Minimizing iptables-restore input size kubernetes/enhancements#3453

Closed

15 tasks

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 29, 2022

k8s-ci-robot merged commit 8121620 into kubernetes:main Nov 29, 2022

sftim deleted the 20221208_refactor_service_docs branch November 29, 2022 19:18

danwinship mentioned this pull request Dec 5, 2022

Remove references to userspace proxy #38284

Merged

sftim mentioned this pull request Dec 6, 2022

[zh-cn] sync /networking/virtual-ips.md #38295

Closed

This was referenced Dec 14, 2022

[ko] Outdated Korean contents in the dev-1.26-ko.1 #38458

Closed

[ko] Update outdated files in dev-1.26-ko.1 (M126, M164, R1-R3) #38485

Merged

jihoon-seo mentioned this pull request Feb 2, 2023

[ko] Update outdated files in dev-1.26-ko.1 (M67) #39219

Merged

Shubham82 mentioned this pull request May 25, 2023

Incorrect link for Service Proxies #41317

Closed

Shubham82 mentioned this pull request Jul 5, 2023

Incorrect link for Service Proxy #41891

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate reference details out Service concept #36675

Migrate reference details out Service concept #36675

sftim commented Sep 7, 2022 •

edited

Loading

sftim commented Sep 7, 2022

sftim commented Sep 7, 2022

netlify bot commented Sep 7, 2022 •

edited

Loading

tengqm Sep 8, 2022

sftim Sep 15, 2022

tengqm Sep 8, 2022

sftim Sep 15, 2022

tengqm Sep 8, 2022

sftim Sep 15, 2022

tengqm Sep 8, 2022

sftim Sep 15, 2022

divya-mohan0209 left a comment •

edited

Loading

divya-mohan0209 Nov 25, 2022

sftim Nov 25, 2022

divya-mohan0209 commented Nov 26, 2022

k8s-ci-robot commented Nov 26, 2022

sftim commented Nov 29, 2022 •

edited

Loading

natalisucks commented Nov 29, 2022

sftim commented Nov 29, 2022

natalisucks commented Nov 29, 2022

k8s-ci-robot commented Nov 29, 2022

tengqm commented Nov 30, 2022

natalisucks commented Nov 30, 2022

tengqm commented Nov 30, 2022


		### `TCP` {#protocol-tcp}

		You can use TCP for any kind of Service, and it's the default network protocol.

	iptables redirect from the virtual IP address to this new port, and starts accepting
	iptables rule that redirects from the virtual IP address to this new port, and starts accepting

	The `type` field was designed as nested functionality - each level adds to the
	The `type` field was designed as a nested functionality - each level adds to the

Migrate reference details out Service concept #36675

Migrate reference details out Service concept #36675

Conversation

sftim commented Sep 7, 2022 • edited Loading

sftim commented Sep 7, 2022

sftim commented Sep 7, 2022

netlify bot commented Sep 7, 2022 • edited Loading

✅ Pull request preview available for checking

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divya-mohan0209 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divya-mohan0209 commented Nov 26, 2022

k8s-ci-robot commented Nov 26, 2022

sftim commented Nov 29, 2022 • edited Loading

natalisucks commented Nov 29, 2022

sftim commented Nov 29, 2022

natalisucks commented Nov 29, 2022

k8s-ci-robot commented Nov 29, 2022

tengqm commented Nov 30, 2022

natalisucks commented Nov 30, 2022

tengqm commented Nov 30, 2022

sftim commented Sep 7, 2022 •

edited

Loading

netlify bot commented Sep 7, 2022 •

edited

Loading

divya-mohan0209 left a comment •

edited

Loading

sftim commented Nov 29, 2022 •

edited

Loading