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

Remove references to userspace proxy #38284

Merged
merged 1 commit into from Dec 8, 2022

Conversation

danwinship
Copy link
Contributor

While belatedly looking at #36675 I saw that we still talk about the userspace proxy in a bunch of places, but it's gone in 1.26 (kubernetes/kubernetes#112133).

I didn't remove static/images/docs/services-userspace-overview.{svg,png} because it's referenced by the translations of service.md too.

/cc @sftim

@k8s-ci-robot k8s-ci-robot added this to the 1.26 milestone Dec 5, 2022
@netlify
Copy link

netlify bot commented Dec 5, 2022

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

Name Link
🔨 Latest commit 37ee1e3
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/638f4d6fe284310008c35d18

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Dec 5, 2022
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.

IMO, this change should land after we merge in main

The proxy modes are now documented in https://kubernetes.io/docs/reference/networking/virtual-ips/

@sftim
Copy link
Contributor

sftim commented Dec 5, 2022

Avoid touching autogenerated files such as content/en/docs/reference/command-line-tools-reference/kube-proxy.md. Those changes will be overwritten during the release process.

/hold
OK to unhold this (unless other grounds have appeared) once that change is omitted.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 5, 2022
@sftim
Copy link
Contributor

sftim commented Dec 5, 2022

You might also like to work on #38244 @danwinship

@danwinship
Copy link
Contributor Author

Avoid touching autogenerated files such as content/en/docs/reference/command-line-tools-reference/kube-proxy.md. Those changes will be overwritten during the release process.

ah, well, I just copied the text from kube-proxy --help so it would hopefully have been overwritten with exactly the same thing, but if it's going to happen automatically anyway, then yay
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 5, 2022
@danwinship
Copy link
Contributor Author

IMO, this change should land after we merge in main

I'm not sure what that means relative to what ends up in the official 1.26 documentation, but note that as of right now, dev-1.26 contains references to something (userspace proxy mode) that doesn't exist in 1.26.

@sftim
Copy link
Contributor

sftim commented Dec 5, 2022

I'm worried that our choices might come down to:

  • ship with stale docs about kube-proxy, and fix them after the release
  • create a merge conflict that we don't really have time / resources to fix before the release

It'd be nice to find a third option here.

@sftim
Copy link
Contributor

sftim commented Dec 5, 2022

#38242 is about to land. If you rebase on upstream's dev-1.26 then we should be able to get this in the release.

If not, it's not a huge problem because this repo is continously deployed: update the PR to target main, go through code review, and we can get an update out, pronto.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2022
Comment on lines 1390 to 1391
kube-proxy supports two proxy modes—iptables and IPVS—which
operate slightly differently.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is also a kernelspace mode - see #38244

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR necessarily conflicts with that one (that PR modifies the text immediately before the ### User space proxy mode heading which this PR removes), so one of us will have to rebase

Copy link
Contributor

@sftim sftim Dec 6, 2022

Choose a reason for hiding this comment

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

What matters most to me is not provoking a merge conflict for dev-1.26 going into main.
I can add the kernelspace one into the dev-1.26 docs and then, after the v1.26, propose a backport.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2022
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

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 8, 2022
Copy link
Contributor

@krol3 krol3 left a comment

Choose a reason for hiding this comment

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

/lgtm

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

LGTM label has been added.

Git tree hash: 9e78ef58b6beb7118706b18601dad274c37592a1

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krol3, sftim

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

@sftim
Copy link
Contributor

sftim commented Dec 8, 2022

Manually triggering a merge by GitHub as repository is frozen for 1.26 release.

@sftim sftim merged commit a0fe485 into kubernetes:dev-1.26 Dec 8, 2022
@danwinship danwinship deleted the no-userspace branch December 12, 2022 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants