Ensure libseccomp is installed before starting containerd on CentOS 8 #6922

OwenTuz · 2020-11-17T17:55:30Z

What type of PR is this?

/kind bug

What this PR does / why we need it:
Starting containerd (and therefore docker, which requires containerd) currently fails on Centos 8 due to a missing dependency. This PR copies what we already do for CRI-O to ensure libseccomp is installed.

To test, create vagrant/config.rb containing the line $os = "centos8" and provision, as described in issue #6920

Which issue(s) this PR fixes:
Fixes #6920

Special notes for your reviewer:
Please flag if there's any way I can introduce better tests for this.

I had hoped to make sure it was covered, but it seems molecule only tests against ubuntu (reasonably enough, since it's meant to be fast), and I was unclear on whether CI currently tests against Centos 8.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2020-11-17T17:55:38Z

Welcome @OwenTuz!

It looks like this is your first PR to kubernetes-sigs/kubespray 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubespray has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-11-17T17:55:38Z

Hi @OwenTuz. 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.

OwenTuz · 2020-11-17T17:59:29Z

Re-pushed to fix invalid commit message

floryut · 2020-11-23T08:05:50Z

@OwenTuz now that #6910 is merged, could you please rebase master ? Thank you!

OwenTuz · 2020-11-23T09:45:47Z

@floryut done! Thanks for the heads up

OwenTuz · 2020-11-23T10:33:47Z

Build failure looks like we've hit a rate limit on DockerHub:

fatal: [instance-1 -> None]: FAILED! => {
    "attempts": 4,
    "changed": true,
    "cmd": [
        "/usr/bin/docker",
        "pull",
        "docker.io/library/nginx:1.19"
    ],
    "delta": "0:00:01.132849",
    "end": "2020-11-23 09:58:51.061621",
    "msg": "non-zero return code",
    "rc": 1,
    "start": "2020-11-23 09:58:49.928772",
    "stderr": "Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit",
    "stderr_lines": [
        "Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit"
    ],
    "stdout": "",
    "stdout_lines": []
}

https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/jobs/866416207

karlism · 2020-11-25T07:38:43Z

@OwenTuz, wouldn't it be better idea instead of command module to use yum, dnf or package module with state: latest?

OwenTuz · 2020-11-25T11:08:00Z

@karlism that's totally reasonable, this is almoost entirely a copy/paste job on my part. I can update and push?

OwenTuz · 2020-11-25T17:45:58Z

Simplified this quite a bit as suggested by @karlism. Some notes:

I've removed the version check that was in the previous commands. I hope I'm not missing a good reason for introducing this but I think ~~latest~~ present (EDIT: fixed to avoid upsetting ansible-lint) works better as suggested. I'm also not sad to see the big chain of regexes disappear.
Other packages are installed from the array variable {docker,containerd,cri-o}-packages, and it may be more consistent to add libseccomp to these. I've avoided this for now as I don't think the extra logic required (to add one package, only when using CentOS 8) helps readability.
However I think it's a matter of code style, so I'll flag it up here and am happy to change if it's preferable.

OwenTuz · 2020-11-26T10:45:58Z

Found out why the version check was present! cri-o needs a version of libseccomp > 2.3. Docker and containerd don't seem to care.

I've reverted this to use state=latest and added a noqa comment for the corresponding ansible-lint rule, as I think it's probably the cleanest option here. Just testing and I'll push if all goes well.

- Uses `package` module - Replaces complex version check with 'state: latest'. The version must be > 2.3 when using with cri-o. - Removes unnecessary `not is_ostree` condition as CentOS 8 does not use ostree

LuckySB · 2020-11-28T17:03:16Z

/ok-to-test

LuckySB · 2020-11-28T17:06:25Z

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

+    name: libseccomp
+    state: latest
+  when:
+    - ansible_distribution == "CentOS"


but what about the RedHat or Fedora ?
do they need this package too?

This is also copied and pasted from the CRI-O code: but I can say I haven't had this problem when testing with Fedora. Some other issues blocked me from investigating RHEL8, but I'll see if I can get past those and let you know.

@LuckySB Now that I've managed to get a developer subscription and test: I can confirm that RHEL8 and Fedora do not seem to need this package explicitly installed.

Seems my RHEL issues had been encountered before and fixed but there was a typo, I'll raise another PR :)

chrony depends on libseccomp, that is why, please make it for all RedHat family (EL7/EL8)

oomichi · 2020-11-30T22:50:49Z

/lgtm

champtar · 2020-12-03T14:44:28Z

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

+- name: Ensure latest version of libseccomp installed  # noqa 403
+  package:
+    name: libseccomp
+    state: latest


it should be state: present no ?

champtar · 2020-12-03T15:13:16Z

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

+    name: libseccomp
+    state: latest
+  when:
+    - ansible_distribution == "CentOS"


chrony depends on libseccomp, that is why, please make it for all RedHat family (EL7/EL8)

LuckySB · 2020-12-03T21:39:10Z

/approve

k8s-ci-robot · 2020-12-03T21:39:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LuckySB, OwenTuz

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:

~~OWNERS~~ [LuckySB]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

champtar · 2020-12-03T21:43:57Z

@LuckySB you completely ignored my review ...

OwenTuz · 2020-12-03T22:03:00Z

Sorry for not responding earlier, I was planning on taking a look tomorrow - just saw the emails that this has merged. I'm happy to raise another PR to address these comments if necessary. Re `latest` vs `present` - I used `latest` here because `present` installs the wrong version for CRI-O. Perhaps there's a better way to work around this? I'm probably missing something obvious, sorry, but I'm not entirely clear on the objection to using `latest` here? I've no problem with installing libseccomp for all the Red Hat variants.

…

On Thu, 3 Dec 2020, 21:44 Etienne Champetier, ***@***.***> wrote: @LuckySB <https://github.com/LuckySB> you completely ignored my review ... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6922 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABYSNPAZ2PNPNWF5JTRYUG3STABCXANCNFSM4TY4ZGEA> .

champtar · 2020-12-03T22:37:13Z

@OwenTuz no problem, I just commented today.
Re running cluster.yml on a cluster 1 month after first deployment should not change anything IMO (don't think it's the case right now)
Now looking at the changelog: https://centos.pkgs.org/8/centos-baseos-x86_64/libseccomp-2.4.1-1.el8.x86_64.rpm.html
CentOS 8 has 2.4.1-1 since 2019-05-31, so we could just drop this part for CRIO and be fine

Docker and containerd don't need latest, so lets not put present there

floryut · 2020-12-04T08:36:30Z

Yup that's an approval a bit quick... sorry about that, maybe raise another PR indeed to address the issues/comments raise by @champtar

OwenTuz · 2020-12-04T17:36:21Z

Sorry, things got busy elsewhere: just a note to say I've not forgotten abut this and will raise that PR in the next few days.

…kubernetes-sigs#6922) * Ensure libseccomp is installed before starting containerd on CentOS 8 * Simplify libseccomp install on CentOS 8 - Uses `package` module - Replaces complex version check with 'state: latest'. The version must be > 2.3 when using with cri-o. - Removes unnecessary `not is_ostree` condition as CentOS 8 does not use ostree

k8s-ci-robot requested review from bozzo and oomichi November 17, 2020 17:55

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 17, 2020

OwenTuz force-pushed the issue-6920-libseccomp-missing-on-centos8 branch from a65dff6 to 08bb507 Compare November 17, 2020 17:59

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Nov 17, 2020

OwenTuz force-pushed the issue-6920-libseccomp-missing-on-centos8 branch from 08bb507 to b60df76 Compare November 23, 2020 09:45

Ensure libseccomp is installed before starting containerd on CentOS 8

67b2bbf

OwenTuz force-pushed the issue-6920-libseccomp-missing-on-centos8 branch from b60df76 to d946d93 Compare November 25, 2020 17:34

OwenTuz force-pushed the issue-6920-libseccomp-missing-on-centos8 branch from d946d93 to 587f380 Compare November 25, 2020 17:49

OwenTuz force-pushed the issue-6920-libseccomp-missing-on-centos8 branch from 587f380 to 0307a9d Compare November 26, 2020 11:00

Simplify libseccomp install on CentOS 8

da66e60

- Uses `package` module - Replaces complex version check with 'state: latest'. The version must be > 2.3 when using with cri-o. - Removes unnecessary `not is_ostree` condition as CentOS 8 does not use ostree

OwenTuz force-pushed the issue-6920-libseccomp-missing-on-centos8 branch from 0307a9d to da66e60 Compare November 26, 2020 11:06

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 28, 2020

LuckySB reviewed Nov 28, 2020

View reviewed changes

k8s-ci-robot assigned oomichi Nov 30, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2020

champtar suggested changes Dec 3, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2020

k8s-ci-robot merged commit d315f73 into kubernetes-sigs:master Dec 3, 2020

floryut mentioned this pull request Dec 19, 2020

Release Proposal v2.15 #7069

Closed

champtar mentioned this pull request Dec 21, 2020

Remove libseccomp install tasks #7074

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure libseccomp is installed before starting containerd on CentOS 8 #6922

Ensure libseccomp is installed before starting containerd on CentOS 8 #6922

OwenTuz commented Nov 17, 2020

k8s-ci-robot commented Nov 17, 2020

k8s-ci-robot commented Nov 17, 2020

OwenTuz commented Nov 17, 2020

floryut commented Nov 23, 2020

OwenTuz commented Nov 23, 2020

OwenTuz commented Nov 23, 2020 •

edited

karlism commented Nov 25, 2020

OwenTuz commented Nov 25, 2020

OwenTuz commented Nov 25, 2020 •

edited

OwenTuz commented Nov 26, 2020

LuckySB commented Nov 28, 2020

LuckySB Nov 28, 2020

OwenTuz Nov 30, 2020

OwenTuz Nov 30, 2020

champtar Dec 3, 2020

oomichi commented Nov 30, 2020

champtar Dec 3, 2020

champtar Dec 3, 2020

LuckySB commented Dec 3, 2020

k8s-ci-robot commented Dec 3, 2020

champtar commented Dec 3, 2020

OwenTuz commented Dec 3, 2020 via email

champtar commented Dec 3, 2020

floryut commented Dec 4, 2020

OwenTuz commented Dec 4, 2020

Ensure libseccomp is installed before starting containerd on CentOS 8 #6922

Ensure libseccomp is installed before starting containerd on CentOS 8 #6922

Conversation

OwenTuz commented Nov 17, 2020

k8s-ci-robot commented Nov 17, 2020

k8s-ci-robot commented Nov 17, 2020

OwenTuz commented Nov 17, 2020

floryut commented Nov 23, 2020

OwenTuz commented Nov 23, 2020

OwenTuz commented Nov 23, 2020 • edited

karlism commented Nov 25, 2020

OwenTuz commented Nov 25, 2020

OwenTuz commented Nov 25, 2020 • edited

OwenTuz commented Nov 26, 2020

LuckySB commented Nov 28, 2020

LuckySB Nov 28, 2020

Choose a reason for hiding this comment

OwenTuz Nov 30, 2020

Choose a reason for hiding this comment

OwenTuz Nov 30, 2020

Choose a reason for hiding this comment

champtar Dec 3, 2020

Choose a reason for hiding this comment

oomichi commented Nov 30, 2020

champtar Dec 3, 2020

Choose a reason for hiding this comment

champtar Dec 3, 2020

Choose a reason for hiding this comment

LuckySB commented Dec 3, 2020

k8s-ci-robot commented Dec 3, 2020

champtar commented Dec 3, 2020

OwenTuz commented Dec 3, 2020 via email

champtar commented Dec 3, 2020

floryut commented Dec 4, 2020

OwenTuz commented Dec 4, 2020

OwenTuz commented Nov 23, 2020 •

edited

OwenTuz commented Nov 25, 2020 •

edited