Use the nydus-snapshotter by default #267

fidencio · 2023-10-20T13:38:54Z

This PR is a rework of #263, but I don't want to lose that public reference of what's been done.

Please, take a look at each commit for more info on what it's doing and why.

This will be used, either by the admin or by our tests, to set the correct debug level we want for the payload we're shipping, including the pre-install one. The default is "false", as usually folks won't need fully verbose logs in production. Signed-off-by: Fabiano Fidêncio <[email protected]>

That was a typo that was there since d1268cf, when the code was moved to this repo. Signed-off-by: ChengyuZhu6 <[email protected]> Signed-off-by: Fabiano Fidêncio <[email protected]>

Signed-off-by: ChengyuZhu6 <[email protected]> Signed-off-by: Fabiano Fidêncio <[email protected]>

17a9248 introduced this, and was calling the wrong method. Signed-off-by: ChengyuZhu6 <[email protected]> Signed-off-by: Fabiano Fidêncio <[email protected]>

fidencio · 2023-10-20T13:39:00Z

/test

install/pre-install-payload/Makefile

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

fidencio · 2023-10-24T09:15:57Z

/test

fidencio · 2023-10-24T09:45:02Z

/test

This will reduce the risk of something breaking on the container / host due to the lack of libraries that needed to be loaded with a dynamically linked binary. Signed-off-by: Fabiano Fidêncio <[email protected]>

As we'll need to use it to properly clean up the nydus snapshots when uninstalling nydus. Signed-off-by: Fabiano Fidêncio <[email protected]>

Nydus snapshotter will be used for replacing the forked containerd, allowing us to pull the image inside the guest, and this commits is adding the ability to build it. Getting it to be used will come later on on this series. Signed-off-by: ChengyuZhu6 <[email protected]> Signed-off-by: Fabiano Fidêncio <[email protected]>

This is a release of nydus-snapshotter that has absolutely **ZERO** code changed, but that adds a tarball for s390x. This has been done in order to work around the issue we have seen building the binaries for s390x as part of our CI, at least till nydus-snapshotter releases a s390x tarball, which is being tracked as part of containerd/nydus-snapshotter#548 ``` The error we're facing is: 22.67 -> unzip /go/pkg/mod/cache/download/github.com/freddierice/go-losetup/@v/v0.0.0-20220711213114-2a14873012db.zip: zip: checksum error 22.69 go: downloading github.com/rs/xid v1.4.0 22.69 go: downloading github.com/containerd/stargz-snapshotter/estargz v0.14.3 22.69 go: downloading golang.org/x/net v0.10.0 22.71 go: downloading github.com/docker/distribution v2.8.2+incompatible 22.77 -> unzip /go/pkg/mod/cache/download/github.com/rs/xid/@v/v1.4.0.zip: zip: checksum error 22.83 go: downloading github.com/moby/locker v1.0.1 24.73 pkg/filesystem/stargz_adaptor.go:19:2: github.com/KarpelesLab/[email protected]: Get "https://proxy.golang.org/github.com/%21karpeles%21lab/reflink/@v/v1.0.1.zip": local error: tls: bad record MAC ``` This is not ideal, but this is "good enough" to unblock this release. Signed-off-by: Fabiano Fidêncio <[email protected]>

fidencio · 2023-10-24T12:00:17Z

/test

fidencio · 2023-10-24T12:48:18Z

/test

fidencio · 2023-10-24T13:10:22Z

/test

fidencio · 2023-10-24T14:01:23Z

/test-kata-qemu-tdx

fidencio · 2023-10-24T14:05:13Z

/test-kata-qemu-tdx

wainersm

@fidencio amazing work as always! Ready to land this IMO.

fidencio · 2023-10-24T15:00:41Z

I will debug the TDX CI for one day before getting it merged, so if we have enough approvals, Tomorrow at noon, UTC, I will get this one merged regardless of the state of the TDX CI.

fitzthum

Tested a bit on my node. I think we might not be uninstalling nydus at all (see comment). Otherwise looks good.

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

fidencio · 2023-10-25T08:04:51Z

/test

fitzthum

LGTM

We might see more SEV/SNP failures if other PRs (or the baseline) have run given that they will leave the nodes in a bad state. Should work if we rerun a few times tho.

fidencio · 2023-10-25T13:23:56Z

We might see more SEV/SNP failures if other PRs (or the baseline) have run given that they will leave the nodes in a bad state. Should work if we rerun a few times tho.

Cool, thanks!

Right now I'm re-working the uninstall as it made the jobs related to uninstall / reinstall break.

fidencio · 2023-10-25T18:55:35Z

/test

fidencio · 2023-10-25T18:56:18Z

/test

The nydus-snapshotter is going to be used in order to replace our dependency on the forked containerd. For now, we're not actually removing the forked containerd dependency, but rather giving the admin the choice to use nydus-snapshotter. Using the forked containerd will be deprecated as part of our **next** release. Signed-off-by: ChengyuZhu6 <[email protected]> Signed-off-by: Fabiano Fidêncio <[email protected]>

This is done for the kata-containers based payloads. Enclave CC will still keep using the forked containerd Signed-off-by: ChengyuZhu6 <[email protected]> Signed-off-by: Fabiano Fidêncio <[email protected]>

Signed-off-by: Fabiano Fidêncio <[email protected]>

The condition to set the snapshotter to be used is defintely wrong, as it'd never ever set the snapshotter variable. Signed-off-by: Fabiano Fidêncio <[email protected]>

…ging` ``` panic: odd number of arguments passed as key-value pairs for logging [recovered] panic: odd number of arguments passed as key-value pairs for logging ... github.com/confidential-containers/operator/controllers.(*CcRuntimeReconciler).monitorCcRuntimeInstallation(0xc0002d4190) /workspace/controllers/ccruntime_controller.go:473 +0x3bf ``` Signed-off-by: Huang Huang <[email protected]>

On operator_tests.bats's teardown let's print the description of all pods in the confidential-containers namespace to help on debug fails. Signed-off-by: Wainer dos Santos Moschetta <[email protected]>

This should be treated in a better way than adding an one minute sleep, as just having the sleep here won't do us any good in the future. What's basically happening, and forcing us to do this, is the fact that the Uninstall and postUninstall daemonsets are being started at exactly the same time, leading to a race condition when changing the containerd configuration. When looking at the kata-containers payload code, we see that the the label is only set after containerd is successfully reconfigured, and looking at this function we see we shouldn't reach this part before the label is set. However, that's not what we're facing ... In order to unblock this PR to get merged, we're good enough. But this needs proper investigation and hopefully we'll see this patch being reverted sooner than later. Signed-off-by: Fabiano Fidêncio <[email protected]>

fidencio · 2023-10-25T19:08:12Z

/test

fidencio · 2023-10-25T19:14:48Z

/test

bpradipt · 2023-10-26T06:12:45Z

controllers/ccruntime_controller.go

@@ -226,6 +226,20 @@ func (r *CcRuntimeReconciler) processCcRuntimeDeleteRequest() (ctrl.Result, erro
 			if r.ccRuntime.Spec.Config.PostUninstall.Image == "" {
 				controllerutil.RemoveFinalizer(r.ccRuntime, RuntimeConfigFinalizer)
 			} else if r.ccRuntime.Spec.Config.PostUninstall.Image != "" {
+				// FXIME: This should be treated in a better way, as just having the sleep


nit: FXIME-> FIXME
This looks reasonable. We'll focus on a proper fix in the next release

I will fix the fixme in the next iteration, just in order to save some trees and unblock peer-pods folks. :-)

bpradipt

/lgtm
Thanks @fidencio

fidencio added 4 commits October 20, 2023 12:38

pre-install: Fix setup_env_for_arch() switch

c94de05

That was a typo that was there since d1268cf, when the code was moved to this repo. Signed-off-by: ChengyuZhu6 <[email protected]> Signed-off-by: Fabiano Fidêncio <[email protected]>

pre-install: Don't fail the build if there's no manifest to be purged

b385546

Signed-off-by: ChengyuZhu6 <[email protected]> Signed-off-by: Fabiano Fidêncio <[email protected]>

pre-install: Fix official containerd installation

36a34f5

17a9248 introduced this, and was calling the wrong method. Signed-off-by: ChengyuZhu6 <[email protected]> Signed-off-by: Fabiano Fidêncio <[email protected]>

fidencio force-pushed the topic/enable-nydus-snapshotter-usage-for-pulling-on-the-guest branch from aeb6b99 to 7f4ccb7 Compare October 20, 2023 15:38

wainersm reviewed Oct 20, 2023

View reviewed changes

install/pre-install-payload/Makefile Outdated Show resolved Hide resolved

wainersm reviewed Oct 20, 2023

View reviewed changes

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

wainersm reviewed Oct 20, 2023

View reviewed changes

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

fidencio force-pushed the topic/enable-nydus-snapshotter-usage-for-pulling-on-the-guest branch from 7f4ccb7 to 3505319 Compare October 24, 2023 09:15

fidencio added 2 commits October 24, 2023 12:00

pre-install: Ensure we use static releases of containerd (when possible)

c909be2

This will reduce the risk of something breaking on the container / host due to the lack of libraries that needed to be loaded with a dynamically linked binary. Signed-off-by: Fabiano Fidêncio <[email protected]>

pre-install: Ensure ctr is part of the image

b85d78c

As we'll need to use it to properly clean up the nydus snapshots when uninstalling nydus. Signed-off-by: Fabiano Fidêncio <[email protected]>

fidencio force-pushed the topic/enable-nydus-snapshotter-usage-for-pulling-on-the-guest branch 3 times, most recently from db7b913 to 8dbfc63 Compare October 24, 2023 10:53

fidencio force-pushed the topic/enable-nydus-snapshotter-usage-for-pulling-on-the-guest branch from 8dbfc63 to 66e697e Compare October 24, 2023 11:36

fidencio force-pushed the topic/enable-nydus-snapshotter-usage-for-pulling-on-the-guest branch from 66e697e to 4f3a56c Compare October 24, 2023 11:42

fidencio mentioned this pull request Oct 24, 2023

Fix panic: odd number of arguments passed as key-value pairs for logging #261

Closed

fidencio force-pushed the topic/enable-nydus-snapshotter-usage-for-pulling-on-the-guest branch from ee51469 to a062ef1 Compare October 24, 2023 12:47

wainersm approved these changes Oct 24, 2023

View reviewed changes

wainersm mentioned this pull request Oct 24, 2023

tests/e2e: describe all pods on teardown #268

Closed

fitzthum reviewed Oct 24, 2023

View reviewed changes

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

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

fidencio force-pushed the topic/enable-nydus-snapshotter-usage-for-pulling-on-the-guest branch from 1663f90 to 796b637 Compare October 25, 2023 07:45

fitzthum approved these changes Oct 25, 2023

View reviewed changes

fidencio force-pushed the topic/enable-nydus-snapshotter-usage-for-pulling-on-the-guest branch from 796b637 to 1adf99d Compare October 25, 2023 18:14

fidencio force-pushed the topic/enable-nydus-snapshotter-usage-for-pulling-on-the-guest branch from 3e5ab7f to a1be903 Compare October 25, 2023 18:56

fidencio and others added 7 commits October 25, 2023 21:07

config: Switch to using the nydus-snapshotter by default

411ca5a

This is done for the kata-containers based payloads. Enclave CC will still keep using the forked containerd Signed-off-by: ChengyuZhu6 <[email protected]> Signed-off-by: Fabiano Fidêncio <[email protected]>

pre-install: Bump containerd to its latest release

1797397

Signed-off-by: Fabiano Fidêncio <[email protected]>

controllers:: Fix snapshotter setup

6bec6fe

The condition to set the snapshotter to be used is defintely wrong, as it'd never ever set the snapshotter variable. Signed-off-by: Fabiano Fidêncio <[email protected]>

tests/e2e: describe all pods on teardown

a9ee6b0

On operator_tests.bats's teardown let's print the description of all pods in the confidential-containers namespace to help on debug fails. Signed-off-by: Wainer dos Santos Moschetta <[email protected]>

fidencio force-pushed the topic/enable-nydus-snapshotter-usage-for-pulling-on-the-guest branch from a1be903 to 619eb49 Compare October 25, 2023 19:07

bpradipt reviewed Oct 26, 2023

View reviewed changes

bpradipt approved these changes Oct 26, 2023

View reviewed changes

fidencio merged commit d454e76 into confidential-containers:main Oct 26, 2023
5 checks passed

This was referenced Oct 26, 2023

do-not-merge: This will be used to test the kata-containers CI related stuff #263

Closed

pre-install: Setup nydus-snapshotter #251

Closed

Take into consideration that /etc/containerd/config.toml may not exists #271

Closed

fffmonkeyking mentioned this pull request Jan 15, 2024

About the containerd of v0.8.0 #315

Open

dcmiddle mentioned this pull request Apr 19, 2024

guides: add document about how to use nydus snapshotter confidential-containers/confidential-containers#157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the nydus-snapshotter by default #267

Use the nydus-snapshotter by default #267

fidencio commented Oct 20, 2023

fidencio commented Oct 20, 2023

fidencio commented Oct 24, 2023

fidencio commented Oct 24, 2023

fidencio commented Oct 24, 2023

fidencio commented Oct 24, 2023

fidencio commented Oct 24, 2023

fidencio commented Oct 24, 2023

fidencio commented Oct 24, 2023

wainersm left a comment

fidencio commented Oct 24, 2023

fitzthum left a comment

fidencio commented Oct 25, 2023

fitzthum left a comment

fidencio commented Oct 25, 2023

fidencio commented Oct 25, 2023

fidencio commented Oct 25, 2023

fidencio commented Oct 25, 2023

fidencio commented Oct 25, 2023

bpradipt Oct 26, 2023

fidencio Oct 26, 2023

bpradipt left a comment

Use the nydus-snapshotter by default #267

Use the nydus-snapshotter by default #267

Conversation

fidencio commented Oct 20, 2023

fidencio commented Oct 20, 2023

fidencio commented Oct 24, 2023

fidencio commented Oct 24, 2023

fidencio commented Oct 24, 2023

fidencio commented Oct 24, 2023

fidencio commented Oct 24, 2023

fidencio commented Oct 24, 2023

fidencio commented Oct 24, 2023

wainersm left a comment

Choose a reason for hiding this comment

fidencio commented Oct 24, 2023

fitzthum left a comment

Choose a reason for hiding this comment

fidencio commented Oct 25, 2023

fitzthum left a comment

Choose a reason for hiding this comment

fidencio commented Oct 25, 2023

fidencio commented Oct 25, 2023

fidencio commented Oct 25, 2023

fidencio commented Oct 25, 2023

fidencio commented Oct 25, 2023

bpradipt Oct 26, 2023

Choose a reason for hiding this comment

fidencio Oct 26, 2023

Choose a reason for hiding this comment

bpradipt left a comment

Choose a reason for hiding this comment