ccruntime: Set `INSTALL_OFFICIAL_CONTAINERD` to false #274

fidencio · 2023-10-27T09:06:45Z

The majority of the managed kubernetes solutions are already relying on
a new enough (v1.7+) version of containerd.

/cc @wainersm, we'd need to adapt CI for this, and I'd need your help to do so.

stevenhorsman

LGTM. Thanks!

stevenhorsman

I hit an error testing this out locally:

# kubectl logs pod/cc-operator-pre-install-daemon-8xwt9 -n confidential-containers-system
/opt/confidential-containers-pre-install-artifacts/scripts/reqs-deploy.sh: line 208: INSTALL_COCO_CONTAINERD: unbound variable

fidencio · 2023-10-27T15:25:25Z

I hit an error testing this out locally:

# kubectl logs pod/cc-operator-pre-install-daemon-8xwt9 -n confidential-containers-system
/opt/confidential-containers-pre-install-artifacts/scripts/reqs-deploy.sh: line 208: INSTALL_COCO_CONTAINERD: unbound variable

Are you sure you'be been testing the latest version of this PR? Asking as looking at the reqs script I don't easily see it happening, as we set those at the beginning of the file.

stevenhorsman · 2023-10-27T16:02:10Z

I hit an error testing this out locally:
# kubectl logs pod/cc-operator-pre-install-daemon-8xwt9 -n confidential-containers-system
/opt/confidential-containers-pre-install-artifacts/scripts/reqs-deploy.sh: line 208: INSTALL_COCO_CONTAINERD: unbound variable
Are you sure you'be been testing the latest version of this PR? Asking as looking at the reqs script I don't easily see it happening, as we set those at the beginning of the file.

I'm not sure, but given all the changes you've made since then I've re-tested in a 'clean' environment

stevenhorsman

I wasn't able to get this tested properly in the original environment that I found the issue in as I had issues when trying to rebuild and deploy a new operator container image, so I just created a new VM and ran the code that the e2e tests use to do this and it looks like it's working for me now:

# kubectl logs ds/cc-operator-pre-install-daemon -n confidential-containers-system
INSTALL_COCO_CONTAINERD: false
INSTALL_OFFICIAL_CONTAINERD: false
INSTALL_VFIO_GPU_CONTAINERD: false
INSTALL_NYDUS_SNAPSHOTTER: true
Copying nydus-snapshotter artifacts onto host
Created symlink /etc/systemd/system/containerd.service.requires/nydus-snapshotter.service → /etc/systemd/system/nydus-snapshotter.service.
configure nydus snapshotter for containerd
Create /etc/containerd/config.toml.d
Drop-in the nydus configuration
[proxy_plugins]
  [proxy_plugins.nydus]
    type = "snapshot"
    address = "/run/containerd-nydus/containerd-nydus-grpc.sock"
Restarting containerd

fidencio · 2023-10-27T16:22:51Z

@wainersm, I need your help here for changing the tests.

install/pre-install-payload/scripts/reqs-deploy.sh

wainersm · 2023-10-27T20:03:19Z

@wainersm, I need your help here for changing the tests.

ok, looking at it.

wainersm · 2023-10-30T17:32:21Z

@fidencio I'm thinking in tackle CI like this:

Current jobs that are run on Ubuntu 20.04 (or CentOS Stream) got containerd 1.6 (containerd.io package from docker). Those I am testing a change so that the operator's pre-reqs script will install the official containerd 1.7.
Add jobs for kata-qemu and kata-clh on Ubuntu 22.04 which will have containerd 1.7.2, so to test the default installation behavior. I am still trying to test this will work out with current tests/e2e scripts but I've faced networking issues when building the pre-reqs image....

sound as a good plan?

wainersm · 2023-10-30T18:01:17Z

@fidencio I'm thinking in tackle CI like this:

1. Current jobs that are run on Ubuntu 20.04 (or CentOS Stream) got containerd 1.6 (`containerd.io` package from docker). Those I am [testing a change](https://github.com/confidential-containers/operator/pull/276) so that the operator's pre-reqs script will install the official containerd 1.7.

CI pass with that change: http://jenkins.katacontainers.io/job/confidential-containers-operator-main-ubuntu-20.04-x86_64-containerd_kata-qemu-PR/328/console

2. Add jobs for kata-qemu and kata-clh on Ubuntu 22.04 which will have containerd 1.7.2, so to test the default installation behavior. I am still trying to test this will work out with current tests/e2e scripts but I've faced networking issues when building the pre-reqs image....

Here we have a problem... the CI scripts leverage docker to build images and service with a local registry, but docker depends on its containerd.io bundle which install containerd 1.6. So although Ubuntu 22.04 provides the packaged 1.7.2 containerd, docker seems to not like it. I'm looking for a solution...

sound as a good plan?

wainersm · 2023-10-31T17:26:01Z

@fidencio I'm thinking in tackle CI like this:
1. Current jobs that are run on Ubuntu 20.04 (or CentOS Stream) got containerd 1.6 (`containerd.io` package from docker). Those I am [testing a change](https://github.com/confidential-containers/operator/pull/276) so that the operator's pre-reqs script will install the official containerd 1.7.
CI pass with that change: http://jenkins.katacontainers.io/job/confidential-containers-operator-main-ubuntu-20.04-x86_64-containerd_kata-qemu-PR/328/console
2. Add jobs for kata-qemu and kata-clh on Ubuntu 22.04 which will have containerd 1.7.2, so to test the default installation behavior. I am still trying to test this will work out with current tests/e2e scripts but I've faced networking issues when building the pre-reqs image....
Here we have a problem... the CI scripts leverage docker to build images and service with a local registry, but docker depends on its containerd.io bundle which install containerd 1.6. So although Ubuntu 22.04 provides the packaged 1.7.2 containerd, docker seems to not like it. I'm looking for a solution...

sound as a good plan?

Managed to fix the execution on Ubuntu 22.04. @fidencio you will need to pick my two commits from #276

Here is the final coverage:

a) Jobs running on Ubuntu 20.04, containerd 1.6, INSTALL_OFFICIAL_CONTAINERD=true

http://jenkins.katacontainers.io/job/confidential-containers-operator-main-ubuntu-20.04-x86_64-containerd_kata-clh-PR
http://jenkins.katacontainers.io/job/confidential-containers-operator-main-ubuntu-20.04-x86_64-containerd_kata-qemu-PR

If I'm not mistaken SEV/SNP is Ubuntu 20.04 and TDX is CentOS Stream 8 then they will fall on this case.

b) Jobs running on Ubuntu 22.04, containerd 1.7, INSTALL_OFFICIAL_CONTAINERD=false (default behavior)

http://jenkins.katacontainers.io/job/confidential-containers-operator-main-ubuntu-22.04-x86_64-containerd_kata-clh-PR
http://jenkins.katacontainers.io/job/confidential-containers-operator-main-ubuntu-22.04-x86_64-containerd_kata-qemu-PR

stevenhorsman · 2023-11-03T14:25:29Z

/test

Right now we're just ignoring the global env vars passed, which is not the expected behaviour, mainly as we're duplicating a bunch of env vars passed to the preInstall / postUninstall payloads. Let's ensure we actually read the global ones and take those into consideration as well. Signed-off-by: Fabiano Fidêncio <[email protected]>

Otherwise we may end up in situations where users will simply try to add / replace an environment variable via kustomize and will end up with the operator broken as this env var would simply be vanished. :-/ Signed-off-by: Fabiano Fidêncio <[email protected]>

Otherwise we may hit issues in in cases where folks will use kustomize to add / replace one the values, as kustomize just overrides the whole set of env vars. We need to keep those in sync with whatever is set in the configs, but this shouldn't be as much of a burden as leaving it for the user to do so. Signed-off-by: Fabiano Fidêncio <[email protected]>

In theory those could very well be set only for the pre-install / post-uninstall, but that doesn't make much sense as the user experience on setting those would be to having to set those twice. With the user experience in mind, let's just move them to the global env vars and let the user set / kustomize them only once. Signed-off-by: Fabiano Fidêncio <[email protected]>

fidencio · 2023-11-03T16:01:53Z

/test

fidencio · 2023-11-06T09:33:39Z

TDX CI is failing due to:

17:16:13 not ok 4 [cc][agent][kubernetes][containerd] Test can pull an unencrypted image inside the guest

SEV CI is failing due to:

17:06:06 not ok 2 [cc][kubernetes][containerd][sev] Test SEV encrypted container launch failure with INVALID measurement

tests/e2e/operator.sh

fidencio · 2023-11-06T13:35:21Z

/test

install/pre-install-payload/scripts/reqs-deploy.sh

bpradipt · 2023-11-06T14:41:08Z

config/samples/ccruntime/default/kustomization.yaml

+        # containerd v1.7+ running.  If you know for sure that's not
+        # the case, please, set this variable to `true`
+        - name: "INSTALL_OFFICIAL_CONTAINERD"
+          value: "false"


Related to my previous question on setting INSTALL_OFFICIAL_CONTAINERD to false in reqs-deploy.sh, if it makes sense to set the default there, and comment out this patch for users to uncomment and install containerd if needed ?

Users can comment this out to install containerd if needed, IIRC.

Ok. Would it make sense to document these variables ?
It can be done in a separate PR as well

Yes, it would and I will do it in a separate PR before the release.
This should also be mentioned in the release notes,

The majority of the managed kubernetes solutions are already relying on a new enough (v1.7+) version of containerd. Fixes: confidential-containers#272 Signed-off-by: Fabiano Fidêncio <[email protected]>

With the new requirement of containerd 1.7+, the operator.sh was adapted to enable the operator to install containerd in case the system's installed version is less than or equal to 1.6. Signed-off-by: Wainer dos Santos Moschetta <[email protected]>

On Ubuntu 22.04 installs docker from the distro and consequently containerd version 1.7. This ways we cover the default installation case where INSTALL_OFFICIAL_CONTAINERD=false. Signed-off-by: Wainer dos Santos Moschetta <[email protected]>

fidencio · 2023-11-07T10:47:18Z

/test

fidencio · 2023-11-07T11:54:10Z

/test-s390x

bpradipt

/lgtm
Thanks Fabiano

stevenhorsman approved these changes Oct 27, 2023

View reviewed changes

fidencio marked this pull request as draft October 27, 2023 09:11

stevenhorsman requested changes Oct 27, 2023

View reviewed changes

fidencio force-pushed the topic/expose-pre-install-vars-via-kustomize branch 5 times, most recently from 9418a21 to 0573d19 Compare October 27, 2023 12:57

stevenhorsman approved these changes Oct 27, 2023

View reviewed changes

fidencio marked this pull request as ready for review October 27, 2023 16:22

wainersm reviewed Oct 27, 2023

View reviewed changes

install/pre-install-payload/scripts/reqs-deploy.sh Outdated Show resolved Hide resolved

fidencio force-pushed the topic/expose-pre-install-vars-via-kustomize branch from 0573d19 to 34ef4d4 Compare October 30, 2023 10:03

stevenhorsman mentioned this pull request Oct 31, 2023

config: set operator payloads for v0.8.0 release #277

Merged

wainersm mentioned this pull request Oct 31, 2023

job-builder: add CoCo operator jobs for Ubuntu 22.04 kata-containers/ci#552

Open

fidencio added 4 commits November 3, 2023 17:00

fidencio force-pushed the topic/expose-pre-install-vars-via-kustomize branch from 34ef4d4 to c71cba4 Compare November 3, 2023 16:01

fidencio mentioned this pull request Nov 3, 2023

do-not-merge: testing the handler for containerd <= 1.6 #276

Closed

stevenhorsman reviewed Nov 6, 2023

View reviewed changes

tests/e2e/operator.sh Outdated Show resolved Hide resolved

fidencio force-pushed the topic/expose-pre-install-vars-via-kustomize branch from c71cba4 to 22e7e3c Compare November 6, 2023 13:34

bpradipt reviewed Nov 6, 2023

View reviewed changes

install/pre-install-payload/scripts/reqs-deploy.sh Outdated Show resolved Hide resolved

bpradipt reviewed Nov 6, 2023

View reviewed changes

fidencio and others added 3 commits November 6, 2023 18:21

ccruntime: Set INSTALL_OFFICIAL_CONTAINERD to false

475c686

The majority of the managed kubernetes solutions are already relying on a new enough (v1.7+) version of containerd. Fixes: confidential-containers#272 Signed-off-by: Fabiano Fidêncio <[email protected]>

tests/e2e: handle containerd <= 1.6

d3fc6ea

With the new requirement of containerd 1.7+, the operator.sh was adapted to enable the operator to install containerd in case the system's installed version is less than or equal to 1.6. Signed-off-by: Wainer dos Santos Moschetta <[email protected]>

fidencio force-pushed the topic/expose-pre-install-vars-via-kustomize branch from 22e7e3c to 812289b Compare November 6, 2023 17:22

bpradipt approved these changes Nov 7, 2023

View reviewed changes

fidencio merged commit c31205e into confidential-containers:main Nov 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ccruntime: Set `INSTALL_OFFICIAL_CONTAINERD` to false #274

ccruntime: Set `INSTALL_OFFICIAL_CONTAINERD` to false #274

fidencio commented Oct 27, 2023

stevenhorsman left a comment

stevenhorsman left a comment

fidencio commented Oct 27, 2023

stevenhorsman commented Oct 27, 2023

stevenhorsman left a comment

fidencio commented Oct 27, 2023

wainersm commented Oct 27, 2023

wainersm commented Oct 30, 2023

wainersm commented Oct 30, 2023

wainersm commented Oct 31, 2023

stevenhorsman commented Nov 3, 2023

fidencio commented Nov 3, 2023

fidencio commented Nov 6, 2023

fidencio commented Nov 6, 2023

bpradipt Nov 6, 2023

fidencio Nov 6, 2023

bpradipt Nov 7, 2023

fidencio Nov 7, 2023

fidencio commented Nov 7, 2023

fidencio commented Nov 7, 2023

bpradipt left a comment

ccruntime: Set INSTALL_OFFICIAL_CONTAINERD to false #274

ccruntime: Set INSTALL_OFFICIAL_CONTAINERD to false #274

Conversation

fidencio commented Oct 27, 2023

stevenhorsman left a comment

Choose a reason for hiding this comment

stevenhorsman left a comment

Choose a reason for hiding this comment

fidencio commented Oct 27, 2023

stevenhorsman commented Oct 27, 2023

stevenhorsman left a comment

Choose a reason for hiding this comment

fidencio commented Oct 27, 2023

wainersm commented Oct 27, 2023

wainersm commented Oct 30, 2023

wainersm commented Oct 30, 2023

wainersm commented Oct 31, 2023

stevenhorsman commented Nov 3, 2023

fidencio commented Nov 3, 2023

fidencio commented Nov 6, 2023

fidencio commented Nov 6, 2023

bpradipt Nov 6, 2023

Choose a reason for hiding this comment

fidencio Nov 6, 2023

Choose a reason for hiding this comment

bpradipt Nov 7, 2023

Choose a reason for hiding this comment

fidencio Nov 7, 2023

Choose a reason for hiding this comment

fidencio commented Nov 7, 2023

fidencio commented Nov 7, 2023

bpradipt left a comment

Choose a reason for hiding this comment

ccruntime: Set `INSTALL_OFFICIAL_CONTAINERD` to false #274

ccruntime: Set `INSTALL_OFFICIAL_CONTAINERD` to false #274