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

Adds cri-dockerd initd service #1766

Merged
merged 7 commits into from Mar 17, 2022
Merged

Adds cri-dockerd initd service #1766

merged 7 commits into from Mar 17, 2022

Conversation

Nino-K
Copy link
Member

@Nino-K Nino-K commented Mar 8, 2022

This change adds the cri-dockerd service to initd. It needs clarification on creating and passing /etc/conf.d/cri-dockerd with the ENGINE type variable.

This PR bumps the Alpine ISO and WSL distro:
Fixes #1788

Signed-off-by: Nino Kodabande <nkodabande@suse.com>
@Nino-K Nino-K requested review from jandubois and mook-as March 8, 2022 23:10
src/assets/scripts/service-k3s.initd Show resolved Hide resolved
src/assets/scripts/cri-dockerd.initd Outdated Show resolved Hide resolved
src/assets/scripts/cri-dockerd.initd Outdated Show resolved Hide resolved
@mook-as mook-as marked this pull request as draft March 9, 2022 19:17
Signed-off-by: Nino Kodabande <nkodabande@suse.com>
Signed-off-by: Nino Kodabande <nkodabande@suse.com>
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

Looks reasonable, just a syntax error.

src/assets/scripts/cri-dockerd.initd Outdated Show resolved Hide resolved
Signed-off-by: Nino Kodabande <nkodabande@suse.com>
@Nino-K Nino-K changed the title WIP - adds cri-dockerd initd service Adds cri-dockerd initd service Mar 10, 2022
@Nino-K Nino-K marked this pull request as ready for review March 10, 2022 21:37
Signed-off-by: Nino Kodabande <nkodabande@suse.com>
mook-as
mook-as previously approved these changes Mar 10, 2022
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Maybe I misunderstand how this works, but it looks incomplete to me. It is my understanding that k3s will no longer support the --docker option, but instead you have to point the --container-runtime-endpoint to cri-dockerd, which in turn will point to docker.

This PR so far seems to still use --docker.

Since there is no official k3s-1.24 release yet (k8s 1.24 is scheduled for an April 19th release), the best way to test this for now would be to unconditionally use cri-dockerd instead of dockerd for all versions of k3s, and then we can test this against older releases.

Signed-off-by: Nino Kodabande <nkodabande@suse.com>
@Nino-K
Copy link
Member Author

Nino-K commented Mar 11, 2022

@jandubois I enabled the cri-docker shim for all versions of k3s. I was under the assumption that we are going to do the final enabling once the --docker is deprecated in k3s. But enabling this for all k3s versions makes a lot of sense from a testing perspective.

Signed-off-by: Nino Kodabande <nkodabande@suse.com>
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

Seems to work on Windows & macOS.

I notice that it's using k8s.gcr.io/pause:3.1 instead of rancher/mirrored-pause:*. That corresponds to cri-dockerd --pod-infra-container-image=…. I'm not sure what the best way to get the desired image out of k3s is, though.

Approving, assume we can fix the pause thing in a separate issue — filed it as #1808

Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

LGTM, and tested with 1.16.14, 1.20.14, 1.23.3, and 1.24.0-alpha and seems to work fine with each.

@jandubois jandubois merged commit 658e123 into main Mar 17, 2022
@jandubois jandubois deleted the cri-dockerd-openrc branch March 17, 2022 02:56
@jandubois jandubois mentioned this pull request Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DNS resolution failures in centos:7 arm64 container on m1 Mac
3 participants