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

Revise “Container runtimes” page in light of dockershim removal #30882

Merged

Conversation

PranshuSrivastava
Copy link
Contributor

This updates the container-runtimes [page](https://kubernetes.io/docs/setup/production-environment/container-runtimes/ of the website to include information about the Dockershim deprecation and so that the users can take an informed decision. This is in accordance with #28449

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 12, 2021
@PranshuSrivastava
Copy link
Contributor Author

/cc @sftim

@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Dec 12, 2021
@PranshuSrivastava
Copy link
Contributor Author

@sftim I had to keep the links this way because they were not being rendered properly if I broke them in between.

@netlify
Copy link

netlify bot commented Dec 12, 2021

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

🔨 Explore the source changes: ed73666

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

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

@k8s-triage-robot
Copy link

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 12, 2021
@bradtopol
Copy link
Contributor

Hi @mikebrow Can you serve as the technical reviewer for this PR? Just add a /lgtm if it is technically correct and I'll take care of the rest. Thanks.
/cc @mikebrow
/assign

Copy link
Member

@mikebrow mikebrow 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 13, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 49e6f17f10e2dd176c7911f29803128cf504e333

@sftim
Copy link
Contributor

sftim commented Dec 13, 2021

/label tide/merge-method-squash

I'd prefer to omit the whitespace changes, but it's going to be easier I think to merge this and then do fixups.

@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 Dec 13, 2021
@PranshuSrivastava
Copy link
Contributor Author

/label tide/merge-method-squash

I'd prefer to omit the whitespace changes, but it's going to be easier I think to merge this and then do fixups.

@sftim those whitespace changes are because of my text editor.

@chrisnegus
Copy link
Contributor

/assign @celestehorgan

@@ -22,6 +22,17 @@ Kubernetes, on Linux:
- [CRI-O](#cri-o)
- [Docker](#docker)

{{< note >}}
Docker container runtime (Dockershim) has been [deprecated](
Copy link
Member

Choose a reason for hiding this comment

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

This implies that Docker container runtime is deprecated and that dockershim and Docker container runtime are the same.
I suggest more clarity that dockershim is deprecated

Copy link
Contributor

Choose a reason for hiding this comment

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

@PranshuSrivastava would you be willing to revise the language here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sftim @reylejano sorry I missed this review. I have made the change, PTAL.

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

New changes are detected. LGTM label has been removed.

@@ -22,6 +22,17 @@ Kubernetes, on Linux:
- [CRI-O](#cri-o)
- [Docker](#docker)

{{< note >}}
Dockershim, the module that allowed Kubernetes to interact with Docker has been [deprecated](
Copy link
Member

Choose a reason for hiding this comment

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

suggest "Dockershim, the portion of code in Kubernetes that provided direct integration with Docker in prior releases, has been removed from Kubernetes version 1.24. This removal was announced as a deprecation in Kubernetes v1.20." Then continue with "For more information, .." below

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than “has been”, I recommend “was”. The reason is that “has been” suggests it's a recent removal and we prefer the text of documentation to be more timeless (and easier to maintain).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2022
Copy link
Member

@mikebrow mikebrow 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 removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2022
installing the `containerd.io` package can be found at
[Install Docker Engine](https://docs.docker.com/engine/install/#server).
1. Install the `containerd.io` package from the [official containerd website](
https://containerd.io/downloads/).Instructions for setting up the Docker
Copy link

@afbjorklund afbjorklund Feb 13, 2022

Choose a reason for hiding this comment

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

You don't need to set up the Docker package repository, if installing from containerd.io.

So either you install the "containerd.io" package from Docker, or you install the "containerd"
package from your distribution (apt install containerd) or download it directly from containerd.io

If you want to keep both options (docker.com and containerd.io), they need to be clearly separated.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

we should remove the docker repository from the containerd steps and leave the user two options:

Copy link

@afbjorklund afbjorklund Mar 1, 2022

Choose a reason for hiding this comment

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

Would it be possible to supply deb packages for containerd and runc, the same way as for "cri-tools" and "kubernetes-cni" ?

From apt.kubernetes.io, that is.

They would still need to be configured, but at least wouldn't have to juggle tarballs (and version and arch)

Or are the system ones OK ?

Copy link
Member

@neolit123 neolit123 Mar 1, 2022

Choose a reason for hiding this comment

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

The package management in k8s needs to be community owned. Currently Google still holds the keys. But also some reorganization of the package repositories might be required for better version flexibility. These efforts are WIP.

The containerd maintainers can open a discussion about that with the k8s SIG Release. In the meantime we should document the best existing options, IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to supply deb packages for containerd and runc, the same way as for "cri-tools" and "kubernetes-cni" ?

That's a decision / discussion to happen outside of tech docs. In k/website, we document existing behavior most of all and rarely look to document or decide on future changes. It's fine to plan to document the next release before it comes out, of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

@PranshuSrivastava I recommend following the approach outlined in #30882 (comment)

@afbjorklund, I'm keen to keep the focus of this PR on just the set of changes we'd make to this page so that the v1.24 docs make sense after the dockershim component is removed. Once this merges, we can consider follow-up PRs that make further improvements.

As the proverb doesn't quite say, the perfect is the enemy of the merged.

Copy link

@afbjorklund afbjorklund Mar 1, 2022

Choose a reason for hiding this comment

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

I think changing from docker.com to containerd.io would be enough.

It also saves the extra step of having to wipe the containerd config (that disables the cri plugin which is required)

containerd config default | sudo tee /etc/containerd/config.toml

This is done to remove the disabled_plugins = ["cri"] line, that would otherwise interfere with Kubernetes.

When installing containerd from upstream, then this Docker-specific configuration file is not included anymore...

Copy link
Contributor

@bart0sh bart0sh Mar 2, 2022

Choose a reason for hiding this comment

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

Can we keep containerd installation instructions as they are as they're not actually related to the purpose of this PR? We can update them in a separate PR I believe.

Copy link
Contributor

Choose a reason for hiding this comment

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

After we remove the dockershim, the current instructions are too likely to confuse people. I think fixing that does align with the intent of this PR.

@reylejano
Copy link
Member

/remove-language es id it pt ru uk zh

@k8s-ci-robot k8s-ci-robot removed language/es Issues or PRs related to Spanish language language/id Issues or PRs related to Indonesian language language/it Issues or PRs related to Italian language language/pt Issues or PRs related to Portuguese language language/ru Issues or PRs related to Russian language language/uk Issues or PRs related to Ukrainian language language/zh Issues or PRs related to Chinese language labels Feb 14, 2022
/blog/2020/12/08/kubernetes-1-20-release-announcement/#dockershim-deprecation)
You can check out this [documentation](
/docs/tasks/administer-cluster/migrating-from-dockershim/check-if-dockershim-deprecation-affects-you/)
to understand how this deprecation might affect you. For migrating from
Copy link
Contributor

@bart0sh bart0sh Feb 14, 2022

Choose a reason for hiding this comment

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

"to migrate from dockershim" part looks redundant/tautological to me:

For migrating from dockershim ... to migrate from dockershim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done @bart0sh

Copy link
Contributor

Choose a reason for hiding this comment

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

@PranshuSrivastava thanks, looks better now.

@nate-double-u
Copy link
Contributor

/milestone 1.24

@k8s-ci-robot k8s-ci-robot added this to the 1.24 milestone Feb 21, 2022
@PranshuSrivastava
Copy link
Contributor Author

@vincepri PTAL

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.

It looks like this needs some more changes - I've added inline feedback.

Comment on lines +33 to +42
Dockershim, the portion of code in Kubernetes that provided direct
integration with Docker in prior releases, was removed from Kubernetes
version 1.24. This removal was announced as a [deprecation in Kubernetes v 1.20](
/blog/2020/12/08/kubernetes-1-20-release-announcement/#dockershim-deprecation)
You can check out this [documentation](
/docs/tasks/administer-cluster/migrating-from-dockershim/check-if-dockershim-deprecation-affects-you/)
to understand how this deprecation might affect you. To migrate from
dockershim you can follow [this migration guide](
/docs/tasks/administer-cluster/migrating-from-dockershim/)
to migrate from dockershim.
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
Dockershim, the portion of code in Kubernetes that provided direct
integration with Docker in prior releases, was removed from Kubernetes
version 1.24. This removal was announced as a [deprecation in Kubernetes v 1.20](
/blog/2020/12/08/kubernetes-1-20-release-announcement/#dockershim-deprecation)
You can check out this [documentation](
/docs/tasks/administer-cluster/migrating-from-dockershim/check-if-dockershim-deprecation-affects-you/)
to understand how this deprecation might affect you. To migrate from
dockershim you can follow [this migration guide](
/docs/tasks/administer-cluster/migrating-from-dockershim/)
to migrate from dockershim.
Kubernetes releases before v1.24 included a direct integration with Docker Engine,
using a component named _dockershim_. That special direct integration is no longer
part of Kubernetes (this removal was
[announced](/blog/2020/12/08/kubernetes-1-20-release-announcement/#dockershim-deprecation)
as part of the v1.20 release).
You can read
[Check whether Dockershim deprecation affects you](/docs/tasks/administer-cluster/migrating-from-dockershim/check-if-dockershim-deprecation-affects-you/) to understand how this removal might
affect you. To learn about migrating from using dockershim, see
[Migrating from dockershim](/docs/tasks/administer-cluster/migrating-from-dockershim/).

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

Comment on lines -431 to 448
The following CRI adaptors are designed to work with Docker Engine:
{{< note >}}
`overlay2` is the preferred storage driver for systems running Linux kernel version 4.0 or higher,
or RHEL or CentOS using version 3.10.0-514 and above.
{{< /note >}}

- [`cri-dockerd`](https://github.com/Mirantis/cri-dockerd) from Mirantis
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
The following CRI adaptors are designed to work with Docker Engine:
{{< note >}}
`overlay2` is the preferred storage driver for systems running Linux kernel version 4.0 or higher,
or RHEL or CentOS using version 3.10.0-514 and above.
{{< /note >}}
- [`cri-dockerd`](https://github.com/Mirantis/cri-dockerd) from Mirantis
You can also find third-party adapters that let you use Docker Engine with Kubernetes
through the supported {{< glossary_tooltip term_id="cri" text="Container Runtime Interface">}}
(CRI).
The following CRI adapters are designed to integrate Docker Engine with CRI clients (such
as Kubernetes v{{< skew currentVersion >}}):
- [`cri-dockerd`](https://github.com/Mirantis/cri-dockerd) from Mirantis
{{< note >}}
If you use Docker Engine on a node that is running Linux kernel version 4.0 or higher, or that
runs RHEL or CentOS and uses the Linux kernel version 3.10.0-514 and above,
the Kubernetes project suggests using the `overlay2` storage driver.
{{< /note >}}

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

@sftim
Copy link
Contributor

sftim commented Mar 2, 2022

/retitle Revise “Container runtimes” page in light of dockershim removal

@k8s-ci-robot k8s-ci-robot changed the title Updated Container-Runtime.md Revise “Container runtimes” page in light of dockershim removal Mar 2, 2022
@bradtopol
Copy link
Contributor

Assigning @sftim as the reviewer since he is the one doing the in depth review here.
/unassign
/assign @sftim

@sftim
Copy link
Contributor

sftim commented Mar 24, 2022

@PranshuSrivastava would you have time to revise this further?

Another option that we have is to take your existing work and build upon it, merging a different PR that is based on what's here.

@reylejano
Copy link
Member

@PranshuSrivastava , are you able to continue to review the comments and make revisions to progress this PR?

If not, I agree with @sftim 's comment

@PranshuSrivastava
Copy link
Contributor Author

@reylejano @sftim Yes I have time to work further on this PR. I have addressed all the comments so far and will continue to work on the reviews provided in the following days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language 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/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.