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.

Sign up for GitHub

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

Jump to bottom

[docker] add support for cri-dockerd as a replacement for dockershim #8623

Merged
merged 1 commit into from
Mar 16, 2022
Merged

[docker] add support for cri-dockerd as a replacement for dockershim #8623

merged 1 commit into from
Mar 16, 2022

Conversation

cristicalin
Copy link
Contributor

@cristicalin cristicalin commented Mar 14, 2022 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:
With the coming release of kubernetes 1.24, the support for dockershim will be removed. This PR adds experimental support for using the Mirantis cri-dockerd as an alternative to the dockershim.

Additionally, this PR adds some docs for the docker runtime which were missing from out public docs.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[docker] add support for cri-dockerd as a replacement for dockershim

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. 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 Mar 14, 2022
@k8s-ci-robot k8s-ci-robot requested review from bozzo and EppO March 14, 2022 20:53
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristicalin

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 Mar 14, 2022
@cristicalin cristicalin force-pushed the cri-docker branch 2 times, most recently from 7def886 to aad3ba2 Compare March 14, 2022 21:06
@cristicalin cristicalin marked this pull request as draft March 14, 2022 21:07
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 14, 2022
@cristicalin cristicalin force-pushed the cri-docker branch 10 times, most recently from cf369bb to d9b889f Compare March 14, 2022 22:44
@cristicalin cristicalin marked this pull request as ready for review March 14, 2022 22:52
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 14, 2022
@k8s-ci-robot k8s-ci-robot requested a review from oomichi March 14, 2022 22:52
@cristicalin cristicalin mentioned this pull request Mar 15, 2022
Closed
@cristicalin cristicalin force-pushed the cri-docker branch 2 times, most recently from 800576d to 2d4c6eb Compare March 15, 2022 12:35
oomichi
oomichi reviewed Mar 15, 2022
View reviewed changes
Copy link
Contributor

@oomichi oomichi left a comment

Choose a reason for hiding this comment

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

Nice work, just some questions.

tests/files/packet_ubuntu20-aio-docker.yml
@@ -15,3 +15,4 @@ enable_nodelocaldns: False
container_manager: docker
etcd_deployment_type: docker
resolvconf_mode: docker_dns
cri_dockerd_enabled: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice testing

.gitignore
@@ -102,7 +102,6 @@ ENV/

# molecule
roles/**/molecule/**/__pycache__/
roles/**/molecule/**/*.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to remove this line?

Copy link
Contributor Author

@cristicalin cristicalin Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I needed to add a cni definition file which has the *.conf extension, 10-mynet.conf and this was prevented by this .gitignore rule. It's not clear to me why this was added in the first place since *.conf files are valid objects in molecule tests and we already have a few for other tests such as the kata-containers and gvisor molecule tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked the pull request #8080 which added the above line.
But I cannot find the reason.
It is fine to remove it for me.

/lgtm

docs/docker.md
Using `cri-dockerd` instead of `dockershim`:

```yaml
cri_dockerd_enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding

container_manager: docker

to understand what are necessary easily?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the docs in the latest push.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2022
@k8s-ci-robot k8s-ci-robot merged commit 394857b into kubernetes-sigs:master Mar 16, 2022
@floryut floryut added kind/container-managers Containers section in the release note and removed kind/feature Categorizes issue or PR as related to a new feature. labels Mar 17, 2022
sakuraiyuta pushed a commit to sakuraiyuta/kubespray that referenced this pull request Apr 16, 2022
@cristicalin @sakuraiyuta
[docker] add support for cri-dockerd as a replacement for dockershim (k…
e9d28a7 
…ubernetes-sigs#8623)
@oomichi oomichi mentioned this pull request May 28, 2022
Closed
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jun 29, 2023
@cristicalin @LuckySB
[docker] add support for cri-dockerd as a replacement for dockershim (k…
625f6ec 
…ubernetes-sigs#8623)
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Oct 23, 2023
@cristicalin @LuckySB
[docker] add support for cri-dockerd as a replacement for dockershim (k…
c40f823 
…ubernetes-sigs#8623)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oomichi oomichi oomichi left review comments

@bozzo bozzo Awaiting requested review from bozzo

@EppO EppO Awaiting requested review from EppO

Assignees

@oomichi oomichi

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/container-managers Containers section in the release note lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cristicalin @k8s-ci-robot @oomichi @floryut