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

kubeadm: adapt docs for 1.24 and dockershim removal #31309

Conversation

neolit123
Copy link
Member

@neolit123 neolit123 commented Jan 12, 2022

Touch the following files:

  • Implementation details: remove docker specifics, which is changing
    in 1.24
  • Create cluster: small language cleanup, remove note about 1.24
  • Install kubeadm: Include two up-to-date tables for Linux / Windows
    with known endpoints. Include cri-dockerd.
  • Kubelet integration: (side cleanup) use "container runtime" instead of
    "CRI runtime" (which is incorrect). Mention that only updating
    "--container-runtime-endpoint=.." is required if the user wishes
    to override the CR on a certain host. Dockershim->CR-foo migration
    guides would make the "--container-runtime=remote" flag explicit
    and we want to remove it at some point.
  • Troubleshooting kubeadm: Remove some instances of Docker troubleshooting
    that imply docker as default CR, or talk about old Docker versions.
  • Adding Windows nodes: move the containerd tab before the Docker
    tab, as containerd is now the default. Remove note about being explicit
    about --cri-socket. Add note that crictl is required for both
    Docker and containerd. Add note that cri-dockerd is required if
    the user wants to use Docker EE on Windows.

xref kubernetes/kubeadm#2626

@neolit123
Copy link
Member Author

/sig cluster-lifecycle
/hold
until kubernetes/kubernetes#107317 merges

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language labels Jan 12, 2022
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Jan 12, 2022
@neolit123 neolit123 changed the base branch from main to dev-1.24 January 12, 2022 16:39
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 12, 2022
@neolit123 neolit123 force-pushed the 1.24-update-kubeadm-docs-for-dockershim branch from 83c9408 to 06e6d2c Compare January 12, 2022 16:41
@netlify
Copy link

netlify bot commented Jan 12, 2022

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

🔨 Explore the source changes: 3184c22

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

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 12, 2022
@neolit123
Copy link
Member Author

/sig windows
for content/en/docs/tasks/administer-cluster/kubeadm/adding-windows-nodes.md

@netlify
Copy link

netlify bot commented Jan 12, 2022

✔️ Deploy Preview for kubernetes-io-main-staging ready!

🔨 Explore the source changes: 83c9408

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/61df04191c0650000769094a

😎 Browse the preview: https://deploy-preview-31309--kubernetes-io-main-staging.netlify.app

@neolit123 neolit123 force-pushed the 1.24-update-kubeadm-docs-for-dockershim branch from 06e6d2c to 65e25c8 Compare January 17, 2022 11:25
@neolit123
Copy link
Member Author

/cc @SataQiu @pacoxu

@neolit123 neolit123 force-pushed the 1.24-update-kubeadm-docs-for-dockershim branch from 65e25c8 to 2323b19 Compare January 17, 2022 11:45
@neolit123 neolit123 force-pushed the 1.24-update-kubeadm-docs-for-dockershim branch from 62317d8 to f125a24 Compare January 20, 2022 14:31
@neolit123
Copy link
Member Author

neolit123 commented Jan 20, 2022

pushed updates:

  • use "Docker Engine" as the container-runtime name (instead of "docker" or "Docker")
  • add note that explains briefly what cri-dockerd is and that it is a new project

@neolit123 neolit123 force-pushed the 1.24-update-kubeadm-docs-for-dockershim branch from f125a24 to 01cee23 Compare January 20, 2022 14:56
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.

LGTM

A couple of concerns about changes not linked to the dockershim removal. Just in case we need to revert this change, I'd prefer to have those changes in their own commit. Ideally: a different PR, that targets the appropriate branch (possible main).

@neolit123 neolit123 force-pushed the 1.24-update-kubeadm-docs-for-dockershim branch 3 times, most recently from 4be8f85 to 078051e Compare January 20, 2022 20:38
Docker Engine does not implement the [CRI](/docs/concepts/architecture/cri/)
which is a requirement for a container runtime to work with Kubernetes.
For that reason, an additional service [cri-dockerd](https://github.com/Mirantis/cri-dockerd)
has to be installed. cri-dockerd is a new project based on the legacy built-in
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: avoid calling things “new” (it creates a follow-up task to reword for the next release).
Maybe “If you're using Kubernetes v1.24 or newer, ”

Copy link
Member Author

Choose a reason for hiding this comment

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

reworded to:

{{< note >}}
Docker Engine does not implement the [CRI](/docs/concepts/architecture/cri/)
which is a requirement for a container runtime to work with Kubernetes.
For that reason, an additional service [cri-dockerd](https://github.com/Mirantis/cri-dockerd)
has to be installed. cri-dockerd is a project based on the legacy built-in
Docker Engine support that was [removed](/dockershim) from the kubelet in version 1.24.
{{< /note >}}

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.

LGTM

I'm not confident about signing off the Windows-specific changes; the other parts look good.

Touch the following files:
- Implementation details: remove docker specifics, which is changing
in 1.24
- Create cluster: small language cleanup, remove note about 1.24
- Install kubeadm: Include two up-to-date tables for Linux / Windows
with known endpoints. Include cri-dockerd.
- Kubelet integration: (side cleanup) use "container runtime" instead of
"CRI runtime" (which is incorrect). Mention that only updating
"--container-runtime-endpoint=.." is required if the user wishes
to override the CR on a certain host. Dockershim->CR-foo migration
guides would make the "--container-runtime=remote" flag explicit
and we want to remove it at some point.
- Troubleshooting kubeadm: Remove some instances of Docker troubleshooting
that imply docker as default CR, or talk about old Docker versions.
Be more generic about container runtimes.
- Adding Windows nodes: move the containerd tab before the Docker
tab, as containerd is now the default. Remove note about being explicit
about --cri-socket. Add note that crictl is required for both
Docker and containerd. Add note that cri-dockerd is required if
the user wants to use Docker EE on Windows.
@neolit123 neolit123 force-pushed the 1.24-update-kubeadm-docs-for-dockershim branch from 078051e to 3184c22 Compare January 25, 2022 16:36
@neolit123
Copy link
Member Author

@sftim updated.

/cc @marosset
PTAL for /lgtm on the kubeadm / Windows changes.
followups can be done in #31395

@marosset
Copy link
Contributor

/lgtm
For Windows changes.

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

LGTM label has been added.

Git tree hash: 36c334d65f901c54dda03eaee5703cf2b0baf9b8

```

Install Docker
Instructions to do so are available at [Install Docker Engine - Enterprise on Windows Servers](https://docs.microsoft.com/en-us/virtualization/windowscontainers/quick-start/set-up-environment?tabs=Windows-Server#install-docker).
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, this links to a page stating that

Customers who want to install a container runtime on Windows server are encouraged to transition to either containerd, Moby, or the Mirantis Container Runtime.

Not sure what we should cover. Maybe we should advise readers to install one of those 3 things and then continue? We can cite those as Microsoft's suggested runtimes for Windows servers.
I think cri-dockerd will work with Moby (Docker Engine without the trademarks) but I'm not sure - I don't actually use Docker very much.

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'm not familiar with the Mirantis product line.

follow ups related to Windows nodes can be tracked in:
#31395

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I think we need a separate issue. #31395 has a specific focus.

@marosset - would you be willing to open a new issue to track any concerns you have about the Windows aspects of setup? (if you don't have any concerns, no need to act)

@sftim
Copy link
Contributor

sftim commented Jan 25, 2022

Let's merge this. It makes the pages much better, even if there's room for more improvement.
/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 Jan 25, 2022
@k8s-ci-robot k8s-ci-robot merged commit 5e35828 into kubernetes:dev-1.24 Jan 25, 2022
@k8s-ci-robot k8s-ci-robot added this to the 1.24 milestone Jan 25, 2022
@jihoon-seo
Copy link
Member

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/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/windows Categorizes an issue or PR as relevant to SIG Windows. 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.

7 participants