reqs: handle the absence of disable_snapshot_annotations #284

wainersm · 2023-11-09T18:27:45Z

The disable_snapshot_annotations should be set to false in the containerd's configuration.toml in order to make the remote snapshotter work. The reqs-deploy.sh currently handles only the case where the property is already present on the configuration file, i.e., it will switch from true to false only. However, on some nodes like in AWS EKS and Azure AKS, disable_snapshot_annotations simply doesn't exist so the current sed takes no effect (and the current containerd's behavior is to disable annotations).

This changed the handler so that on the absence of disable_snapshot_annotations, it is appended to the [plugins."io.containerd.grpc.v1.cri".containerd] section.

Fix for issue 2 in kata-containers/kata-containers#8407

wainersm · 2023-11-09T18:33:45Z

I'm marking this as do-not-merge because I still need to handle the uninstall case, i.e., we need to somehow mark that the line was added then remove it.

I was thinking in literally put a comment on the line just above the disable_snapshot_annotations line. For example:

# disable_snapshot_annotations - GENERATED BY COCO OPERATOR
disable_snapshot_annotations = false

Then on uninstall it will know the line should be removed. Yes, ugly hack... any other ideas?

wainersm · 2023-11-09T18:38:09Z

@fitzthum @fidencio @stevenhorsman it would be nice to have your review and hear ideas for the uninstall problem.

stevenhorsman · 2023-11-09T18:45:04Z

Yes, ugly hack... any other ideas?

I agree that's it's not great, but I can't think of anything better and I've definitely written worse hacks!

wainersm · 2023-11-09T21:18:38Z

With the generated comment, it will append the property like this:

[plugins."io.containerd.grpc.v1.cri".containerd]
# disable_snapshot_annotations - DO NOT EDIT. GENERATED BY CoCo OPERATOR
disable_snapshot_annotations = false
  default_runtime_name = "runc"
  discard_unpacked_layers = true

On uninstall, delete both lines resulting in

[plugins."io.containerd.grpc.v1.cri".containerd]
  default_runtime_name = "runc"
  discard_unpacked_layers = true

Here is the diff:

diff --git a/install/pre-install-payload/scripts/reqs-deploy.sh b/install/pre-install-payload/scripts/reqs
-deploy.sh
index a76345a..5185f13 100755
--- a/install/pre-install-payload/scripts/reqs-deploy.sh
+++ b/install/pre-install-payload/scripts/reqs-deploy.sh
@@ -12,6 +12,8 @@ INSTALL_NYDUS_SNAPSHOTTER=${INSTALL_NYDUS_SNAPSHOTTER:-true}
 containerd_config="/etc/containerd/config.toml"
 artifacts_dir="/opt/confidential-containers-pre-install-artifacts"
 
+readonly snapshot_annotations_marker="disable_snapshot_annotations - DO NOT EDIT. GENERATED BY CoCo OPERA
TOR"
+
 die() {
        msg="$*"
        echo "ERROR: $msg" >&2
@@ -192,7 +194,7 @@ EOF
        else
                # In case the property does not exist, let's append it to the
                # [plugins."io.containerd.grpc.v1.cri".containerd] section.
-               sed -i '/\[plugins\..*\.containerd\]/adisable_snapshot_annotations = false' \
+               sed -i '/\[plugins\..*\.containerd\]/a'"# ${snapshot_annotations_marker}"'\ndisable_snapsh
ot_annotations = false' \
                        "${containerd_config}"
        fi
 
@@ -211,7 +213,14 @@ function remove_nydus_snapshotter_from_containerd() {
        sed -i -e "s|\"${containerd_imports_path}/nydus-snapshotter.toml\"||g" ${containerd_config}
        sed -i -e "s|, ]|]|g" ${containerd_config}
 
-       sed -i -e "s|disable_snapshot_annotations = false|disable_snapshot_annotations = true|" ${containe
rd_config}
+       if grep -q "${snapshot_annotations_marker}" "${containerd_config}"; then
+               sed -i '/'"${snapshot_annotations_marker}"'/d' \
+                       "${containerd_config}"
+               sed -i '/disable_snapshot_annotations = false/d' \
+                       "${containerd_config}"
+       else
+               sed -i -e "s|disable_snapshot_annotations = false|disable_snapshot_annotations = true|" ${
containerd_config}
+       fi
 }
 
 label_node() {

wainersm · 2023-11-09T21:29:47Z

@mkulke you can test this PR (including the logic to uninstall nydus) with quay.io/wainersm/reqs-payload:latest

That image was built as:

$ cd install/pre-install-payload
$ make registry=quay.io/wainersm/reqs-payload reqs-image

portersrc · 2023-11-09T21:54:12Z

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

+	else
+		# In case the property does not exist, let's append it to the
+		# [plugins."io.containerd.grpc.v1.cri".containerd] section.
+		sed -i '/\[plugins\..*\.containerd\]/adisable_snapshot_annotations = false' \


is this a typo? adisable_snapshot_annotations vs. disable_snapshot_annotations?

@portersrc the next char after '/' can be a command to sed. In this case, '/a' tells to Append the following string (i.e disable_snapshot_annotations) ;)

Thanks for the careful review!

fwiw: if we want to keep the indent this should work w/ nested capture groups:

sed -e 's|$\(\s*$\[plugins.\"io.containerd.grpc.v1.cri\".containerd\]\)|\1\n\2 ohai=true|' /etc/containerd/config.toml | grep -C1 ohai [plugins."io.containerd.grpc.v1.cri".containerd] ohai=true default_runtime_name = "runc"

mkulke · 2023-11-10T11:02:30Z

@mkulke you can test this PR (including the logic to uninstall nydus) with quay.io/wainersm/reqs-payload:latest

That image was built as:
$ cd install/pre-install-payload
$ make registry=quay.io/wainersm/reqs-payload reqs-image

Thanks. I built reqs-payload images from your branch and used referenced them in the kustomize config for ccruntime/peer-pods. I can confirm it works, the line is being added to /etc/containerd/config.toml

mkulke · 2023-11-10T11:07:35Z

regarding the uninstall part, I think that's the best we can do. Reverting a configuration change is a brittle endeavour, we don't own the config, so a user/tool could do any sort of changes between a coco install and uninstall, which could mess with our undo logic.

wainersm · 2023-11-10T12:32:55Z

@mkulke you can test this PR (including the logic to uninstall nydus) with quay.io/wainersm/reqs-payload:latest
That image was built as:
$ cd install/pre-install-payload
$ make registry=quay.io/wainersm/reqs-payload reqs-image
Thanks. I built reqs-payload images from your branch and used referenced them in the kustomize config for ccruntime/peer-pods. I can confirm it works, the line is being added to /etc/containerd/config.toml

Great! Let me incorporate the uninstall logic on this PR then run the CI.

wainersm · 2023-11-10T12:34:32Z

regarding the uninstall part, I think that's the best we can do. Reverting a configuration change is a brittle endeavour, we don't own the config, so a user/tool could do any sort of changes between a coco install and uninstall, which could mess with our undo logic.

Exactly. Let's see if for long term we find a solution that doesn't need to touch containerd's files.

wainersm · 2023-11-10T12:40:43Z

Let the fun begin:

/test

The `disable_snapshot_annotations` should be set to `false` in the containerd's configuration.toml in order to make the remote snapshotter work. The reqs-deploy.sh currently handles only the case where the property is already present on the configuration file, i.e., it will switch from `true` to `false` only. However, on some nodes like in AWS EKS and Azure AKS, `disable_snapshot_annotations` simply doesn't exist so the current `sed` takes no effect (and the current containerd's behavior is to disable annotations). This changed the handler so that on the absence of `disable_snapshot_annotations`, it is appended to the `[plugins."io.containerd.grpc.v1.cri".containerd]` section. It will leave a comment just below the added line so that the uninstall handler know the line should be removed. Signed-off-by: Magnus Kulke <magnuskulke@microsoft.com> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

fidencio · 2023-11-10T14:20:57Z

/test

fitzthum · 2023-11-10T14:25:49Z

@wainersm, maybe you want to add a commit to bump the operator version to this PR to save us a step?

sure!

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

wainersm · 2023-11-10T14:38:12Z

@fitzthum added the commit to bump the release version

wainersm · 2023-11-10T14:44:16Z

let the fun begin (2):

/test

fitzthum · 2023-11-10T15:11:19Z

I guess we will need one more PR after this to update the reqs payload.

fitzthum

LGTM. Maybe the version bump should actually have gone in the next PR, but not a big deal.

fidencio

lgtm, thanks @wainersm and @mkulke for the back and forth and exhaustive tests on this!

wainersm added the do-not-merge label Nov 9, 2023

portersrc reviewed Nov 9, 2023

View reviewed changes

portersrc mentioned this pull request Nov 9, 2023

config: update operator to use release tags for enclave-cc and kata #283

Merged

stevenhorsman mentioned this pull request Nov 10, 2023

[Release] Check list for v0.8.0 confidential-containers/confidential-containers#145

Closed

23 tasks

wainersm force-pushed the fix_disable_snapshot_annotations branch from f28280a to 6b53e78 Compare November 10, 2023 12:40

wainersm removed the do-not-merge label Nov 10, 2023

mkulke mentioned this pull request Nov 10, 2023

CC | Issues about pulling image with vanilla containerd and remote-snapshotter kata-containers/kata-containers#8407

Closed

wainersm force-pushed the fix_disable_snapshot_annotations branch from a669c41 to 603a555 Compare November 10, 2023 14:19

release: bump operator version to v0.8.0

30135ec

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

fitzthum approved these changes Nov 10, 2023

View reviewed changes

fidencio approved these changes Nov 10, 2023

View reviewed changes

stevenhorsman merged commit e92cb67 into confidential-containers:main Nov 10, 2023
13 checks passed

portersrc mentioned this pull request Nov 10, 2023

config: set reqs-payload for v0.8.0 release #286

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reqs: handle the absence of disable_snapshot_annotations #284

reqs: handle the absence of disable_snapshot_annotations #284

wainersm commented Nov 9, 2023 •

edited

Loading

wainersm commented Nov 9, 2023 •

edited

Loading

wainersm commented Nov 9, 2023

stevenhorsman commented Nov 9, 2023

wainersm commented Nov 9, 2023

wainersm commented Nov 9, 2023

portersrc Nov 9, 2023

wainersm Nov 9, 2023

mkulke Nov 10, 2023

mkulke commented Nov 10, 2023

mkulke commented Nov 10, 2023

wainersm commented Nov 10, 2023

wainersm commented Nov 10, 2023

wainersm commented Nov 10, 2023

fidencio commented Nov 10, 2023

fitzthum commented Nov 10, 2023 •

edited by wainersm

Loading

wainersm commented Nov 10, 2023

wainersm commented Nov 10, 2023

fitzthum commented Nov 10, 2023

fitzthum left a comment

fidencio left a comment

reqs: handle the absence of disable_snapshot_annotations #284

reqs: handle the absence of disable_snapshot_annotations #284

Conversation

wainersm commented Nov 9, 2023 • edited Loading

wainersm commented Nov 9, 2023 • edited Loading

wainersm commented Nov 9, 2023

stevenhorsman commented Nov 9, 2023

wainersm commented Nov 9, 2023

wainersm commented Nov 9, 2023

portersrc Nov 9, 2023

Choose a reason for hiding this comment

wainersm Nov 9, 2023

Choose a reason for hiding this comment

mkulke Nov 10, 2023

Choose a reason for hiding this comment

mkulke commented Nov 10, 2023

mkulke commented Nov 10, 2023

wainersm commented Nov 10, 2023

wainersm commented Nov 10, 2023

wainersm commented Nov 10, 2023

fidencio commented Nov 10, 2023

fitzthum commented Nov 10, 2023 • edited by wainersm Loading

wainersm commented Nov 10, 2023

wainersm commented Nov 10, 2023

fitzthum commented Nov 10, 2023

fitzthum left a comment

Choose a reason for hiding this comment

fidencio left a comment

Choose a reason for hiding this comment

wainersm commented Nov 9, 2023 •

edited

Loading

wainersm commented Nov 9, 2023 •

edited

Loading

fitzthum commented Nov 10, 2023 •

edited by wainersm

Loading