containerd: download containerd from upstream instead of using distro specific packages #7970

cristicalin · 2021-09-14T17:34:39Z

What type of PR is this?

/kind feature

What this PR does / why we need it:
This PR changes the way we deploy the containerd container runtime to download it directly from github or a local repository. This enables uniformity between supported distributions and eliminates version discrepancies between distros. Additionally this allows air-gaped deployments without the need for a docker repo mirror.

Which issue(s) this PR fixes:

Fixes #7941

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[Containerd] Download containerd from upstream instead of using distro specific packages

k8s-ci-robot · 2021-09-14T17:34:46Z

Hi @cristicalin. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

champtar · 2021-09-14T18:09:26Z

Thanks for doing this !
I'll try to review this week

muzi502 · 2021-09-17T02:11:28Z

roles/container-engine/containerd-common/tasks/main.yml

+
+- name: install container-selinux
+  package:
+    name: container-selinux


Is it still necessary to install libseccomp/libseccomp2 for yum/apt 🤔

container-selinux is not libsecomp, it is a bunch of selinux modules built for generic container hosts and is needed in case selinux is enabled.

Actually I think you were right, about ensuring libseccomp is present, but this is separate from the need to have container-selinux present and is not required by container-selinux so I added it to containerd-common to ensure it gets installed properly.

muzi502 · 2021-09-17T02:21:54Z

roles/reset/tasks/main.yml

+    - containerd-shim
+    - containerd-shim-runc-v1
+    - containerd-shim-runc-v2
+    - ctr


Missed {{ containerd_bin_dir }}/runc

This may actually need to be changed to make molecule tests happy about the idempotency of the download/unarchive tasks.

Latest changes properly puts runc under it's own location.

cristicalin · 2021-09-17T15:39:27Z

/cc @floryut , this necessarily reverts work you did on #7972 since we no longer rely on distro specific versions, to keep compatibility with the old code I can also add the old hashes for the previous containerd versions but that would go against our policy to keep just one major version (i.e. 1.4.9 and 1.5.5 as of this writing).

champtar

Thanks again for doing that !
Can you confirm we are using the same path as before ? I think this is important for selinux/apparmor to work out of the box.
Have you checked the containerd.service match what was in the deb/rpm ?
Also this removes support for arm64 for now, need to mention that in the release notes

champtar · 2021-09-17T15:43:32Z

roles/container-engine/containerd-common/tasks/main.yml

+- name: containerd-common | install container-selinux
+  package:
+    name: container-selinux
+    state: latest


Use present

This has moved around in the latest change but uses present instead of latest

champtar · 2021-09-17T15:45:02Z

roles/container-engine/containerd-common/tasks/main.yml

+    name: container-selinux
+    state: latest
+  when:
+    - preinstall_selinux_state != 'disabled'


I think we should always install it (this is what was done), and we could even move it to the OS prepare steps

I moved this part to bootstrap-os as suggested.

champtar · 2021-09-17T15:45:57Z

roles/container-engine/containerd-common/tasks/main.yml

+
+- name: containerd-common | install libseccomp
+  package:
+    name: "{{ seccomp_package }}"


Same thing, should we just move that to OS prepare steps ?

Same as above, the logic was moves to bootstrap-os.

champtar · 2021-09-17T15:47:35Z

roles/container-engine/containerd-common/tasks/main.yml

+    name: "{{ containerd_package }}"
+    state: absent
+  when:
+    - not is_ostree


We should also remove the repos

Moved this part back to the containerd role since containerd-common is also referenced by the docker role. That change also cleans up the repos.

roles/container-engine/containerd-common/defaults/main.yml

champtar · 2021-09-17T15:50:34Z

roles/reset/tasks/main.yml

+        state: absent
+      register: services_removed
+      tags:
+        - services


we should also remove the repos here I think

I would do this in a separate PR to inventory all repos we deploy on all supported distributions and add a clean task to the reset role.

cristicalin · 2021-09-17T16:17:19Z

Can you confirm we are using the same path as before ? I think this is important for selinux/apparmor to work out of the box.

Yes, this is why I switched back and forth between using the containerd and cri-containerd-cni release artifacts from upstream. I ended up mapping the binaries in similar places a the deb/rpm.

Have you checked the containerd.service match what was in the deb/rpm ?

I just checked it now, initially I copied the one upstream was bundling in the cri-containerd-cni release artifact but it seems to match what was in the distro packages.

Also this removes support for arm64 for now, need to mention that in the release notes

Fair point, but we would need to ask upstream to build release binaries for arm64. Is noting this in the release notes enough? Do we have the capability to actually trigger CI jobs on arm64 to actually maintain this?

champtar · 2021-09-17T18:16:23Z

Also this removes support for arm64 for now, need to mention that in the release notes

Fair point, but we would need to ask upstream to build release binaries for arm64. Is noting this in the release notes enough? Do we have the capability to actually trigger CI jobs on arm64 to actually maintain this?

way ahead of you :) containerd/containerd#5524

cristicalin · 2021-09-25T07:09:46Z

Rebased to fix merge conflict.

floryut · 2021-10-06T13:35:32Z

/cc @champtar @oomichi do you have any inputs?

champtar

2 more small comments

champtar · 2021-10-06T13:43:54Z

roles/bootstrap-os/tasks/bootstrap-redhat.yml

+    name: "{{ item }}"
+    state: present
+  become: true
+  with_items:
+    - container-selinux
+    - libseccomp


you already edited required_pkgs so this should not be needed ?

Hmm, good catch, I'll remove this piece.

This part was removed.

champtar · 2021-10-06T13:51:51Z

roles/download/defaults/main.yml

+    1.5.5: 0
+  amd64:
+    1.4.3: 0  # to make debian9 jobs happy
+    1.4.6: 0  # to make ubuntu16 jobs happy


debian9 and ubuntu16 could switch to 1.5.5 now no ?

If deploying with container_engine=containerd this would not be needed, but when deploying with container_engine=docker the older versions of docker supported on those platforms bring in dependencies for older containerd and would break the CI runs due to the way the download role does variable interpolation.

We could also add latest instead of the specific versions and set containerd_version=latest in the debian9 and ubuntu16 jobs.

I just pushed a change with using latest instead of specific versions, at least this keeps the code a bit cleaner.
Let's see if the CI gods agree.

… specific packages split runc download to separate role make bootstrap-os role deploy container-selinux and seccomp libraries clean up package manager provided containerd move variables to docker role that are no longer common with containerd

* replace ubuntu18 with ubuntu20 * add centos8 and debian11 to molecule tests * run kubernetes/preinstall role to ensure relevancy of test including dependency packages

oomichi · 2021-10-20T15:47:00Z

/lgtm

… specific packages (kubernetes-sigs#7970) * Containerd: download containerd from upstream instead of using distro specific packages split runc download to separate role make bootstrap-os role deploy container-selinux and seccomp libraries clean up package manager provided containerd move variables to docker role that are no longer common with containerd * Containerd: make molecule testing more relevant * replace ubuntu18 with ubuntu20 * add centos8 and debian11 to molecule tests * run kubernetes/preinstall role to ensure relevancy of test including dependency packages * CI: adjust test scenarios for downloaded containerd

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 14, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 14, 2021

k8s-ci-robot requested review from bozzo and EppO September 14, 2021 17:34

k8s-ci-robot added 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 Sep 14, 2021

muzi502 reviewed Sep 17, 2021

View reviewed changes

cristicalin force-pushed the containerd_download branch 2 times, most recently from a61be2f to 0fe587b Compare September 17, 2021 09:19

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 17, 2021

cristicalin force-pushed the containerd_download branch 5 times, most recently from d0c5b34 to e5bd220 Compare September 17, 2021 15:35

champtar suggested changes Sep 17, 2021

View reviewed changes

cristicalin force-pushed the containerd_download branch 8 times, most recently from bbe4d17 to bb3f164 Compare September 19, 2021 14:15

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2021

cristicalin force-pushed the containerd_download branch from 03d0ba1 to 7b0f408 Compare October 1, 2021 16:59

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2021

champtar reviewed Oct 6, 2021

View reviewed changes

cristicalin force-pushed the containerd_download branch 3 times, most recently from b6354f8 to a26d78e Compare October 6, 2021 17:57

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2021

cristicalin force-pushed the containerd_download branch from a26d78e to d124df4 Compare October 9, 2021 07:18

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2021

This was referenced Oct 10, 2021

Kata-Containers: Fix kata-containers runtime #8068

Merged

REQUEST: New membership for cristicalin kubernetes/org#3043

Closed

cristicalin added 3 commits October 20, 2021 08:04

Containerd: make molecule testing more relevant

c48e090

* replace ubuntu18 with ubuntu20 * add centos8 and debian11 to molecule tests * run kubernetes/preinstall role to ensure relevancy of test including dependency packages

CI: adjust test scenarios for downloaded containerd

6edda60

cristicalin force-pushed the containerd_download branch from d124df4 to 6edda60 Compare October 20, 2021 08:04

k8s-ci-robot assigned oomichi Oct 20, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2021

k8s-ci-robot merged commit ea8e2fc into kubernetes-sigs:master Oct 20, 2021

floryut added kind/network Network section in the release note and removed kind/feature Categorizes issue or PR as related to a new feature. labels Oct 21, 2021

mafn mentioned this pull request Oct 21, 2021

containerd download broken with Fedora CoreOS #8106

Closed

oomichi mentioned this pull request Dec 7, 2021

How to override installation of a software package. #7968

Closed

floryut mentioned this pull request Dec 21, 2021

Release Proposal v2.18 #8325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

containerd: download containerd from upstream instead of using distro specific packages #7970

containerd: download containerd from upstream instead of using distro specific packages #7970

cristicalin commented Sep 14, 2021 •

edited by floryut

Loading

k8s-ci-robot commented Sep 14, 2021

champtar commented Sep 14, 2021

muzi502 Sep 17, 2021

cristicalin Sep 17, 2021

cristicalin Sep 17, 2021

muzi502 Sep 17, 2021

cristicalin Sep 17, 2021

cristicalin Sep 17, 2021

cristicalin commented Sep 17, 2021

champtar left a comment

champtar Sep 17, 2021

cristicalin Sep 18, 2021

champtar Sep 17, 2021

cristicalin Sep 18, 2021

champtar Sep 17, 2021

cristicalin Sep 18, 2021

champtar Sep 17, 2021

cristicalin Sep 18, 2021

champtar Sep 17, 2021

cristicalin Sep 18, 2021

cristicalin commented Sep 17, 2021

champtar commented Sep 17, 2021

cristicalin commented Sep 25, 2021

floryut commented Oct 6, 2021

champtar left a comment

champtar Oct 6, 2021

cristicalin Oct 6, 2021

cristicalin Oct 6, 2021

champtar Oct 6, 2021

cristicalin Oct 6, 2021

cristicalin Oct 6, 2021

oomichi commented Oct 20, 2021

containerd: download containerd from upstream instead of using distro specific packages #7970

containerd: download containerd from upstream instead of using distro specific packages #7970

Conversation

cristicalin commented Sep 14, 2021 • edited by floryut Loading

k8s-ci-robot commented Sep 14, 2021

champtar commented Sep 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristicalin commented Sep 17, 2021

champtar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristicalin commented Sep 17, 2021

champtar commented Sep 17, 2021

cristicalin commented Sep 25, 2021

floryut commented Oct 6, 2021

champtar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oomichi commented Oct 20, 2021

cristicalin commented Sep 14, 2021 •

edited by floryut

Loading