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

Docs to change Container runtime #30141

Merged
merged 17 commits into from
Feb 11, 2022
Merged

Docs to change Container runtime #30141

merged 17 commits into from
Feb 11, 2022

Conversation

Debanitrkl
Copy link
Member

This adds the detailed instructions to help change container runtime to containerd.
related to kubernetes/kubernetes#104878
/assign @adisky

@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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 19, 2021
@k8s-ci-robot 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 Oct 19, 2021
@netlify
Copy link

netlify bot commented Oct 19, 2021

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

🔨 Explore the source changes: c38dc24

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

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

@Debanitrkl
Copy link
Member Author

Also related to #25879

@adisky
Copy link
Contributor

adisky commented Oct 20, 2021

@adisky
Copy link
Contributor

adisky commented Oct 20, 2021

you need to add contents in the format used here https://github.com/kubernetes/website/pull/25787/files

@Debanitrkl
Copy link
Member Author

@Debanitrkl I am not able find the new doc added with deployment preview https://deploy-preview-30141--kubernetes-io-main-staging.netlify.app/docs/tasks/administer-cluster/migrating-from-dockershim/

I guess this is because it's still a WIP

@adisky
Copy link
Contributor

adisky commented Oct 20, 2021

@Debanitrkl I am not able find the new doc added with deployment preview https://deploy-preview-30141--kubernetes-io-main-staging.netlify.app/docs/tasks/administer-cluster/migrating-from-dockershim/

I guess this is because it's still a WIP

I think it is because of content format, update the content format

Copy link
Contributor

@adisky adisky left a comment

Choose a reason for hiding this comment

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

Overall if we can add more description on the steps and little better formatting

@adisky
Copy link
Contributor

adisky commented Oct 20, 2021

/cc @MadhavJivrajani

Copy link
Contributor

@MadhavJivrajani MadhavJivrajani left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!
Deployment preview can be found here: https://deploy-preview-30141--kubernetes-io-main-staging.netlify.app/docs/tasks/administer-cluster/migrating-from-dockershim/change-runtime/

I don't have a lot of experience with docs but I have a few suggestions:

  • Since this page is going to be important during 1.24 release and post that, it would really help providing context on dockershim in general; this can be done through linking the other two documents that are under the migrating-from-dockershim section.
  • There needs to be information provided for why the said commands are necessary to be run.
  • This is just a small nit but for the sake of consistency with the other files present, file name of change-runtime over Change-runtime might be better.

/cc @RinkiyaKeDad
for further feedback from a docs perspective.

Comment on lines 107 to 112
- finally if everything goes well remove docker
```
apt purge docker-ce docker-ce-cli
OR
yum remove docker-ce docker-ce-cli
```
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Is this still under Windows Powershell?
  • Not sure if the OR here is the right way to go about it. Maybe a separate code section?

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 didn't try this myself took it from the reference here https://kruyt.org/migrate-docker-containerd-kubernetes/. Also, I guess given the formatting style of this doc a separate code section would be better

Comment on lines 78 to 90
```powershell
Copy-Item -Path ".\bin\" -Destination "$Env:ProgramFiles\containerd" -Recurse -Force
cd $Env:ProgramFiles\containerd\
.\containerd.exe config default | Out-File config.toml -Encoding ascii

# Review the configuration. Depending on setup you may want to adjust:
# - the sandbox_image (Kubernetes pause image)
# - cni bin_dir and conf_dir locations
Get-Content config.toml

# (Optional - but highly recommended) Exclude containerd from Windows Defender Scans
Add-MpPreference -ExclusionProcess "$Env:ProgramFiles\containerd\containerd.exe"
```
Copy link
Contributor

@MadhavJivrajani MadhavJivrajani Oct 21, 2021

Choose a reason for hiding this comment

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

  • I usually prefer each command to typically be its own code section (unless relevant commands can be grouped in order for the text description to explain things more effectively) with a description above it and the comments can be put as text and not as comments.
    • Please use this for reference.
  • # (Optional - but highly recommended) Exclude containerd from Windows Defender Scans: Please describe why this is recommended.

Copy link
Member Author

Choose a reason for hiding this comment

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

This piece of code block has also been taken from the documentation here https://github.com/kubernetes/website/blob/main/content/en/docs/setup/production-environment/container-runtimes.md#container-runtimes, but yes I will add more descriptions to make this more clear and if required separately group them.

Comment on lines 101 to 105
- restart kubelet
`systemctl start kubelet`

- verify pods are running
Run `kubectl get nodes -o wide` and containerd appears as the runtime for the node we just changed.
Copy link
Contributor

@MadhavJivrajani MadhavJivrajani Oct 21, 2021

Choose a reason for hiding this comment

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

- configure kubelet to use containerd
Edit the file `/var/lib/kubelet/kubeadm-flags.env` and add the containerd runtime to the flags. `--container-runtime=remote` and `--container-runtimeendpoint=unix:///run/containerd/containerd.sock"`

This is not rendered on separate lines if that was your original intent. And same for the ones below and above it as well.

@RinkiyaKeDad
Copy link
Member

Thanks for working on this! Will take a look from Docs side once the PR is ready to go and not WIP 😄

@reylejano
Copy link
Member

There is an existing page on how to use containerd
https://kubernetes.io/docs/setup/production-environment/container-runtimes/#containerd

@Debanitrkl
Copy link
Member Author

There is an existing page on how to use containerd https://kubernetes.io/docs/setup/production-environment/container-runtimes/#containerd

@reylejano this docs has steps for users to shift from dockershim to containerd. The link you provided has the steps to run only containerd, it doesn't specify how to stop and drain node and then change the runtime.

@reylejano
Copy link
Member

There is an existing page on how to use containerd https://kubernetes.io/docs/setup/production-environment/container-runtimes/#containerd

@reylejano this docs has steps for users to shift from dockershim to containerd. The link you provided has the steps to run only containerd, it doesn't specify how to stop and drain node and then change the runtime.

@Debanitrkl thank you, looking forward to this PR when it's ready to review

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.

Hi. Here's some early feedback that I hope helps you make progress.

Comment on lines 107 to 112
- finally if everything goes well remove docker
```
apt purge docker-ce docker-ce-cli
OR
yum remove docker-ce docker-ce-cli
```
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use https://kubernetes.io/docs/contribute/style/hugo-shortcodes/#tabs and have a tab for Debian-heritage systems and another for RedHat-heritage systems.

```shell
sudo systemctl restart containerd
```
### For windows powershell
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider splitting this into two task pages, one for Linux nodes and one for Windows nodes.

Comment on lines 48 to 50
Instructions for setting up the Docker repository for your respective Linux distribution and
installing the `containerd.io` package can be found at
[Install Docker Engine](https://docs.docker.com/engine/install/#server).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Instructions for setting up the Docker repository for your respective Linux distribution and
installing the `containerd.io` package can be found at
[Install Docker Engine](https://docs.docker.com/engine/install/#server).
You can find instructions for installing `containerd` at
[Starting Containerd](https://containerd.io/docs/getting-started/#starting-containerd).


Install containerd:

1. Install the `containerd.io` package from the official Docker repositories.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use the Docker repository for this? I would recommend that people fetch it from their
OS distribution or, failing that, from source and build it.

I suspect that telling people to get containerd from Docker is liable to add to the confusion.

Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

added a few comments that feel like blocking in the current draft and we should resolve them.
but thanks for working on this!

@@ -0,0 +1,115 @@
---
title: "Migrating Container runtime from dockershim to containerd"
Copy link
Member

@neolit123 neolit123 Nov 23, 2021

Choose a reason for hiding this comment

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

this seems to be an opinionated suggestion for transition.
what if the user wishes to use cri-o instead of containerd?

in the CR popularity survey we did before docker was first, followed by containerd and then cri-o.
so if containerd wins here by being second, it makes sense, but cri-o might not be very happy about this.

cc @saschagrunert for feedback on CRI-O.

Copy link
Member

@saschagrunert saschagrunert Nov 23, 2021

Choose a reason for hiding this comment

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

Thanks for reaching out! I think the whole guide is opinionated to a particular setup, which is probably fine. Mentioning CRI-O as alternative container runtime to containerd would make us very happy, though. 🙃

Copy link
Member

Choose a reason for hiding this comment

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

ok, i think it's worth denoting with a sentence near the top of the guide that this covers an example scenario for migrating from dockershim to containerd and that alternative container runtime such as cri-o can be picked from the https://kubernetes.io/docs/setup/production-environment/container-runtimes/ page.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a good way to head off issues around https://kubernetes.io/docs/contribute/style/content-guide/#third-party-content

We make an exception for container runtimes - these are necessary out-of-project packages that you must have to make a working Kubernetes cluster - but we don't like to be in the business of picking favorites.

### For linux systems
Use the following commands to install Containerd on your system:

Install and configure prerequisites:
Copy link
Member

@neolit123 neolit123 Nov 23, 2021

Choose a reason for hiding this comment

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

the guide seems to be duplicating steps that already existing the in the "installing a container runtime" page:
https://kubernetes.io/docs/setup/production-environment/container-runtimes/
instead we can cross-link.

the steps can be generalized as:

  • drain node
  • stop kubelet
  • uninstall old CR
  • install new CR (CR of your chose, not picking a preference in this page, cross-link to the main CR page)
  • update kubelet config and kubeadm bits (optional)
  • restart kubelet
  • uncordon node
  • check if the node / pods work

i know that it works and i've seen users already do it, but before we merge such a guide for hotswap SIG node must tell us if this is even supported and if something unpredictable can happen. cgroup (drivers)? proper cleanup after uninstalling the old CR - logs, container state, etc?

cc @SergeyKanzhelev

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do want a “dummies guide to” here, because the Docker deprecation is already worrying quite a few people.

sftim
sftim previously requested changes Nov 23, 2021
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.

BTW please make the filename lowercase (change content/en/docs/tasks/administer-cluster/migrating-from-dockershim/Change-runtime.md to content/en/docs/tasks/administer-cluster/migrating-from-dockershim/change-runtime.md)

or, much better (it leaves room for alternatives):
content/en/docs/tasks/administer-cluster/migrating-from-dockershim/change-runtime-containerd.md

@Debanitrkl
Copy link
Member Author

I have updated with some final changes and also have started a hackmd file here for ease of reviewing.

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.

Hi @Debanitrkl

This PR is going to need some work before we can merge it. I've added suggestions and feedback inline.

if you have questions about what to write, please feel free to ask them.

@sftim sftim dismissed their stale review February 4, 2022 14:32

Previous review was stale

@tengqm
Copy link
Contributor

tengqm commented Feb 9, 2022

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 9, 2022
@tengqm
Copy link
Contributor

tengqm commented Feb 11, 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 11, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b196437c02cd1647d921d13f735357270cf3bd0d

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.

/approve


## Remove Docker Engine

{{% thirdparty-content %}}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not needed as the page already has this disclaimer

@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 11, 2022
@k8s-ci-robot k8s-ci-robot merged commit b03dfa8 into kubernetes:main Feb 11, 2022
@Debanitrkl
Copy link
Member Author

@sftim just a small doubt I haven't added the author and reviewer names and the dates wasn't that required

@shannonxtreme
Copy link
Contributor

shannonxtreme commented Feb 11, 2022 via email

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. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.