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

Removed kubenet reference from docs #31667

Merged
merged 1 commit into from Feb 22, 2022

Conversation

mk46
Copy link
Member

@mk46 mk46 commented Feb 9, 2022

Removing reference of kubenet from docs.
/kind cleanup
/language en
Fixes #31659

@k8s-ci-robot k8s-ci-robot added this to the 1.24 milestone Feb 9, 2022
@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. language/en Issues or PRs related to English language cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 9, 2022
@netlify
Copy link

netlify bot commented Feb 9, 2022

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

🔨 Explore the source changes: 7fc8963

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6214e58847e1cc00081e04fb

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Feb 9, 2022
@mk46 mk46 mentioned this pull request Feb 9, 2022
@tengqm
Copy link
Contributor

tengqm commented Feb 9, 2022

/lgtm

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

LGTM label has been added.

Git tree hash: 6f56d5f23f27117f93a6be918c8b3a4413bf1104

sftim
sftim previously requested changes Feb 10, 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 @mk46

I have some inline feedback - I hope it's useful.

Also one piece to suggest a change you haven't made. For the dockershim removal, we should also edit:

The kubelet has a single default network plugin, and a default network common to the entire cluster. It probes for plugins when it starts up, remembers what it finds, and executes the selected plugin at appropriate times in the pod lifecycle (this is only true for Docker, as CRI manages its own CNI plugins). There are two Kubelet command line parameters to keep in mind when using plugins:

to avoid mentioning Docker [Engine].

@sftim
Copy link
Contributor

sftim commented Feb 11, 2022

@tengqm in light of my review, what do you recommend:

  • further changes before merge
  • get this in, and a follow-up PR

?

@tengqm
Copy link
Contributor

tengqm commented Feb 11, 2022

@sftim I'd vote for one more revision before kicking this in.

@tengqm
Copy link
Contributor

tengqm commented Feb 22, 2022

@mk46 Would you like give this PR another stab?

@mk46
Copy link
Member Author

mk46 commented Feb 22, 2022

Hi @tengqm, I would like to remove the below pages in favor of https://github.com/kubernetes/kubernetes/blob/release-1.23/CHANGELOG/CHANGELOG-1.23.md#ipv4ipv6-dual-stack-networking-graduates-to-ga

  • content/en/docs/tasks/network/validate-dual-stack.md
  • content/en/docs/concepts/services-networking/dual-stack.md
    and also content/en/docs/concepts/extend-kubernetes/compute-storage-net/network-plugins.md in favor of removal of --network-plugin in Clean up dockershim flags in the kubelet kubernetes#106907

Are these changes make sense?

@tengqm
Copy link
Contributor

tengqm commented Feb 22, 2022

Hi @tengqm, I would like to remove the below pages in favor of https://github.com/kubernetes/kubernetes/blob/release-1.23/CHANGELOG/CHANGELOG-1.23.md#ipv4ipv6-dual-stack-networking-graduates-to-ga

  • content/en/docs/tasks/network/validate-dual-stack.md
  • content/en/docs/concepts/services-networking/dual-stack.md
    and also content/en/docs/concepts/extend-kubernetes/compute-storage-net/network-plugins.md in favor of removal of --network-plugin in Clean up dockershim flags in the kubelet kubernetes#106907

Are these changes make sense?

Maybe you can help file an issue about the problem to address. My understanding of this PR is that it is about removal of kubenet.

@sftim
Copy link
Contributor

sftim commented Feb 22, 2022

@mk46, we still need the following pages:

I'm not so sure about https://kubernetes.io/docs/concepts/services-networking/dual-stack/ but we should not simply remove it. We might possibly want to reshape it to discuss the three network family options (single stack IPv6, single stack IPv4, dual stack) on an equal footing. Alongside such a change we could add a page about validating single-stack networking, and maybe an overview page about validating that your cluster is healthy.

For v1.24 we need a smaller change that merely omits mention of kubenet. The changes you've proposed are about right; my previous feedback still applies though.

@tengqm
Copy link
Contributor

tengqm commented Feb 22, 2022

With respect to changes to dual-stack contents, I agree to @sftim's suggestions.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 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.

A bit more feedback


<!-- body -->

## Installation

The kubelet has a single default network plugin, and a default network common to the entire cluster. It probes for plugins when it starts up, remembers what it finds, and executes the selected plugin at appropriate times in the pod lifecycle (this is only true for Docker, as CRI manages its own CNI plugins). There are two Kubelet command line parameters to keep in mind when using plugins:
The kubelet has a single default network plugin, and a default network common to the entire cluster.The CRI manages its own CNI plugins. There are two Kubelet command line parameters to keep in mind when using plugins:
Copy link
Contributor

Choose a reason for hiding this comment

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

This revised paragraph might need technical review. I don't think there is a default network plugin for the kubelet now that dockershim is gone.

Copy link
Contributor

@sftim sftim Feb 22, 2022

Choose a reason for hiding this comment

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

Also see discussion in #31667 (comment)

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 think this might be barely OK to merge, provided that we go back and do more fixups.

@sftim
Copy link
Contributor

sftim commented Feb 22, 2022

/lgtm
for SIG Docs

@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 47595f1fc05bc9081ded0016841e441e582b0de0

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

/lgtm

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

LGTM label has been added.

Git tree hash: cd4c869865feff49b74988be6463a803aa1aea06


<!-- body -->

## Installation

The kubelet has a single default network plugin, and a default network common to the entire cluster. It probes for plugins when it starts up, remembers what it finds, and executes the selected plugin at appropriate times in the pod lifecycle (this is only true for Docker, as CRI manages its own CNI plugins). There are two Kubelet command line parameters to keep in mind when using plugins:
The kubelet has a single default network plugin, and a default network common to the entire cluster.The CRI manages its own CNI plugins. There are two Kubelet command line parameters to keep in mind when using plugins:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The kubelet has a single default network plugin, and a default network common to the entire cluster.The CRI manages its own CNI plugins. There are two Kubelet command line parameters to keep in mind when using plugins:
The kubelet has a single default network plugin, and a default network common to the entire cluster. The CRI manages its own CNI plugins. There are two Kubelet command line parameters to keep in mind when using plugins:

nit: missing space, can be fixed in a follow-up PR

Copy link
Contributor

Choose a reason for hiding this comment

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

The kubelet has a single default network plugin

Is this true for v1.24 and later?

@sftim sftim dismissed their stale review February 22, 2022 15:29

Review was stale

@sftim
Copy link
Contributor

sftim commented Feb 22, 2022

I'm happy to see this merged, I do hope we also get the nits tidied up.
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

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

sftim commented Feb 22, 2022

@mk46 would you be willing to open a new PR to further tweak that page based on the feedback in this PR?

@sftim sftim moved this from PRs in Flight (Needs Review) to Done in Docs - Dockershim Removal Feb 22, 2022
@k8s-ci-robot k8s-ci-robot merged commit e021e42 into kubernetes:dev-1.24 Feb 22, 2022
Comment on lines +33 to +34
* `--pod-cidr`: The CIDR to use for pod IP addresses, only used in standalone mode.
In cluster mode, this is obtained from the master. For IPv6, the maximum number of IP's allocated is 65536. For example `--pod-cidr=10.180.0.0/24`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `--pod-cidr`: The CIDR to use for pod IP addresses, only used in standalone mode.
In cluster mode, this is obtained from the master. For IPv6, the maximum number of IP's allocated is 65536. For example `--pod-cidr=10.180.0.0/24`.
* `--pod-cidr`: The CIDR to use for pod IP addresses, only used in standalone mode.
In cluster mode, this is obtained from the master. For IPv6, the maximum number of IP's allocated is 65536. For example `--pod-cidr=10.180.0.0/24`.

This parameter is deprecated see reference: https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet/
Maybe we should remove it in a follow-up PR

mk46 pushed a commit to mk46/website that referenced this pull request Feb 22, 2022
@mk46
Copy link
Member Author

mk46 commented Feb 22, 2022

@mk46 would you be willing to open a new PR to further tweak that page based on the feedback in this PR?

Done #31849

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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Development

Successfully merging this pull request may close these issues.

None yet

6 participants