Release: attempt to fix docker manifest commands #367

portersrc · 2024-04-29T16:21:16Z

This effectively reverts PR 366 and drops the guarding quotes that were added around extra_docker_manifest_flags in 351.

fitzthum

The failures of this reqs job coincide with the chances you are mostly reverting here so I think we are on the right track.

ldoktor · 2024-04-30T07:40:19Z

install/pre-install-payload/payload.sh

-	purge_previous_manifests "${registry}:${tag}"
-	purge_previous_manifests "${registry}:latest"
+	purge_previous_manifests ${registry}:${tag}
+	purge_previous_manifests ${registry}:latest


dropping those doesn't seem like a good idea

ldoktor · 2024-04-30T07:40:57Z

install/pre-install-payload/payload.sh

-		"${registry}:${tag}" \
-		"${registry}:x86_64-${tag}" \
-		"${registry}:s390x-${tag}"
+	docker manifest create ${extra_docker_manifest_flags} \


here it is a good idea to drop the ${extra_docker_manifest_flags} quotes but keep the other ones

ldoktor · 2024-04-30T07:42:21Z

install/pre-install-payload/payload.sh

+	docker manifest create ${extra_docker_manifest_flags} \
+		${registry}:latest \
+		--amend ${registry}:x86_64-${tag} \
+		--amend ${registry}:s390x-${tag}


As for the amend, I read it more carefully now and there are the purge_previous_manifests commands to remove previously existing ones. So the --amend should not be there (or we should remove the purge_previous_manifest if you're sure the --amend is supported on our builders).

I'm not sure, and I don't understand the docs.

ldoktor

@portersrc I don't think the mass removal of quotes is necessary. Please remove only the extra_docker_manifest_flags quotes and it should be good to go.

As for the --amend, would you please either remove the purge commands (and functions) to utilize it, or just leave it without the --amend (to utilize the custom cleanup)

ldoktor · 2024-04-30T07:53:42Z

Note I tested it locally with:

diff --git a/install/pre-install-payload/payload.sh b/install/pre-install-payload/payload.sh
index 20c6057..345b2af 100755
--- a/install/pre-install-payload/payload.sh
+++ b/install/pre-install-payload/payload.sh
@@ -81,18 +81,18 @@ function build_payload() {
        purge_previous_manifests "${registry}:${tag}"
        purge_previous_manifests "${registry}:latest"
 
-       docker manifest create "${extra_docker_manifest_flags}" \
+       docker manifest create ${extra_docker_manifest_flags} \
                "${registry}:${tag}" \
-               --amend "${registry}:x86_64-${tag}" \
-               --amend "${registry}:s390x-${tag}"
+               "${registry}:x86_64-${tag}" \
+               "${registry}:s390x-${tag}"
 
-       docker manifest create "${extra_docker_manifest_flags}" \
+       docker manifest create ${extra_docker_manifest_flags} \
                "${registry}:latest" \
-               --amend "${registry}:x86_64-${tag}" \
-               --amend "${registry}:s390x-${tag}"
+               "${registry}:x86_64-${tag}" \
+               "${registry}:s390x-${tag}"
 
-       docker manifest push "${extra_docker_manifest_flags}" "${registry}:${tag}"
-       docker manifest push "${extra_docker_manifest_flags}" "${registry}:latest"
+       docker manifest push ${extra_docker_manifest_flags} "${registry}:${tag}"
+       docker manifest push ${extra_docker_manifest_flags} "${registry}:latest"
 
        popd
 }

By running extra_docker_manifest_flags="--insecure" registry=localhost:5000/confidential-containers/reqs-payload make while running the registry by docker run --publish 5000:5000 --name registry docker.io/library/registry:2.8.1 and it worked like a charm ;-)

wainersm · 2024-04-30T10:12:00Z

@portersrc I don't think the mass removal of quotes is necessary. Please remove only the extra_docker_manifest_flags quotes and it should be good to go.

I think above is the only solution needed.

The problem was introduced in commit ef55c2e when extra_docker_manifest_flags started being quoted. We didn't catch on CI because we are passing extra_docker_manifest_flags=--insecure so the manifest create command expands to docker manifest create --insecure foo bar, whereas on merge extra_docker_manifest_flags is empty which results on something like docker manifest create "" foo bar (the first argument becomes the empty string, so the error error parsing name for manifest list : invalid reference format).

As for the --amend, would you please either remove the purge commands (and functions) to utilize it, or just leave it without the --amend (to utilize the custom cleanup)

I recommend to not remove --amend as it has worked fine with that parameter. A removal might introduce more bug that we didn't anticipate.

portersrc · 2024-04-30T10:25:43Z

and it worked like a charm ;-)

Okay, that's interesting. So the --amend could have been dropped. (Maybe it's an alternative way to get the same thing done.)

portersrc · 2024-04-30T10:26:36Z

I recommend to not remove --amend as it has worked fine with that parameter. A removal might introduce more bug that we didn't anticipate.

In general I feel more comfortable leaving them (since it was working before).

portersrc · 2024-04-30T10:28:12Z

@wainersm @ldoktor Thanks for checking this!

Compared with the original version of payload.sh (i.e. before I started messing with it for v0.9.0 release), I have (1) kept --amend but (2) removed quotes around extra_docker_manifest_flags. Let me know if that's OK.

This effectively reverts PR 366 and drops the guarding quotes around extra_docker_manifest_flags that were added to these commands in 351. The CI is still throwing an error to the effect of: "error parsing name for manifest list : invalid reference format". This seems to stem from an empty string in the command b/c of an empty extra_docker_manifest_flag variable. Hence, we are reverting quotes that were added around it in PR 351. Signed-off-by: Chris Porter <[email protected]>

wainersm · 2024-04-30T10:37:22Z

Hi @portersrc !

@wainersm @ldoktor Thanks for checking this!

Compared with the original version of payload.sh (i.e. before I started messing with it for v0.9.0 release), I have (1) kept --amend but (2) removed quotes around extra_docker_manifest_flags. Let me know if that's OK.

Cool! Tested like below and it worked out:

$ export registry=quay.io/wainersm/reqs-payload
$ make reqs-image
coco_containerd_version=1.6.8.2 \
official_containerd_version=1.7.7 \
vfio_gpu_containerd_version=1.7.0.0 \
nydus_snapshotter_version=v0.13.11 \
bash -x payload.sh
+ set -o errexit
+ set -o pipefail
+ set -o nounset
+++ readlink -f payload.sh
++ dirname /home/wmoschet/src/github.com/confidential-containers/operator/install/pre-install-payload/payload.sh
+ script_dir=/home/wmoschet/src/github.com/confidential-containers/operator/install/pre-install-payload
+ coco_containerd_repo=https://github.com/confidential-containers/containerd
+ coco_containerd_version=1.6.8.2
+ official_containerd_repo=https://github.com/containerd/containerd
+ official_containerd_version=1.7.7
+ vfio_gpu_containerd_repo=https://github.com/confidential-containers/containerd
+ vfio_gpu_containerd_version=1.7.0.0
+ nydus_snapshotter_repo=https://github.com/containerd/nydus-snapshotter
+ nydus_snapshotter_version=v0.13.11
++ mktemp -d -t containerd-XXXXXXXXXX
+ containerd_dir=/tmp/containerd-5YOTyyqT1O/containerd
+ extra_docker_manifest_flags=
+ registry=quay.io/wainersm/reqs-payload
+ supported_arches=("linux/amd64" "linux/s390x")
+ main
+ build_payload
+ pushd /home/wmoschet/src/github.com/confidential-containers/operator/install/pre-install-payload
~/src/github.com/confidential-containers/operator/install/pre-install-payload ~/src/github.com/confidential-containers/operator/install/pre-install-payload
+ local tag
++ git rev-parse HEAD
+ tag=d0665c09a7258e8bc54b201690551fdeff3c34f7
+ for arch in "${supported_arches[@]}"
+ setup_env_for_arch linux/amd64
+ case "$1" in
+ kernel_arch=x86_64
+ golang_arch=amd64
+ echo 'Building containerd payload image for linux/amd64'
Building containerd payload image for linux/amd64
+ docker buildx build --build-arg ARCH=amd64 --build-arg COCO_CONTAINERD_VERSION=1.6.8.2 --build-arg COCO_CONTAINERD_REPO=https://github.com/confidential-containers/containerd --build-arg OFFICIAL_CONTAINERD_VERSION=1.7.7 --build-arg OFFICIAL_CONTAINERD_REPO=https://github.com/containerd/containerd --build-arg VFIO_GPU_CONTAINERD_VERSION=1.7.0.0 --build-arg VFIO_GPU_CONTAINERD_REPO=https://github.com/confidential-containers/containerd --build-arg NYDUS_SNAPSHOTTER_VERSION=v0.13.11 --build-arg NYDUS_SNAPSHOTTER_REPO=https://github.com/containerd/nydus-snapshotter -t quay.io/wainersm/reqs-payload:x86_64-d0665c09a7258e8bc54b201690551fdeff3c34f7 --platform=linux/amd64 --load .

SNIP

+ purge_previous_manifests quay.io/wainersm/reqs-payload:d0665c09a7258e8bc54b201690551fdeff3c34f7
+ local manifest
+ local sanitised_manifest
+ manifest=quay.io/wainersm/reqs-payload:d0665c09a7258e8bc54b201690551fdeff3c34f7
++ echo quay.io/wainersm/reqs-payload:d0665c09a7258e8bc54b201690551fdeff3c34f7
++ sed 's|/|_|g'
++ sed 's|:|-|g'
+ sanitised_manifest=quay.io_wainersm_reqs-payload-d0665c09a7258e8bc54b201690551fdeff3c34f7
+ rm -rf /home/wmoschet/.docker/manifests/quay.io_wainersm_reqs-payload-d0665c09a7258e8bc54b201690551fdeff3c34f7
+ purge_previous_manifests quay.io/wainersm/reqs-payload:latest
+ local manifest
+ local sanitised_manifest
+ manifest=quay.io/wainersm/reqs-payload:latest
++ echo quay.io/wainersm/reqs-payload:latest
++ sed 's|/|_|g'
++ sed 's|:|-|g'
+ sanitised_manifest=quay.io_wainersm_reqs-payload-latest
+ rm -rf /home/wmoschet/.docker/manifests/quay.io_wainersm_reqs-payload-latest
+ docker manifest create quay.io/wainersm/reqs-payload:d0665c09a7258e8bc54b201690551fdeff3c34f7 --amend quay.io/wainersm/reqs-payload:x86_64-d0665c09a7258e8bc54b201690551fdeff3c34f7 --amend quay.io/wainersm/reqs-payload:s390x-d0665c09a7258e8bc54b201690551fdeff3c34f7
Created manifest list quay.io/wainersm/reqs-payload:d0665c09a7258e8bc54b201690551fdeff3c34f7
+ docker manifest create quay.io/wainersm/reqs-payload:latest --amend quay.io/wainersm/reqs-payload:x86_64-d0665c09a7258e8bc54b201690551fdeff3c34f7 --amend quay.io/wainersm/reqs-payload:s390x-d0665c09a7258e8bc54b201690551fdeff3c34f7
Created manifest list quay.io/wainersm/reqs-payload:latest
+ docker manifest push quay.io/wainersm/reqs-payload:d0665c09a7258e8bc54b201690551fdeff3c34f7
sha256:d19de01c4672f03729546646c013226e769ee4b5fa07f12c0b2fde2bfd860cac
+ docker manifest push quay.io/wainersm/reqs-payload:latest
sha256:d19de01c4672f03729546646c013226e769ee4b5fa07f12c0b2fde2bfd860cac
+ popd
~/src/github.com/confidential-containers/operator/install/pre-install-payload

wainersm

LGTM. Thanks for this fix as well @portersrc !

ldoktor

Thank you, looks good now. The nit-picky version of me would argue about using git revert to restore the --amend but let's get this in as is.

portersrc force-pushed the docker-manifest-create-fix2 branch 2 times, most recently from 030ddf5 to fdbf22b Compare April 29, 2024 18:25

fitzthum added the ok-to-test label Apr 29, 2024

fitzthum approved these changes Apr 29, 2024

View reviewed changes

ldoktor reviewed Apr 30, 2024

View reviewed changes

ldoktor requested changes Apr 30, 2024

View reviewed changes

portersrc force-pushed the docker-manifest-create-fix2 branch from fdbf22b to d0665c0 Compare April 30, 2024 10:23

portersrc changed the title ~~Release: attempt to fix docker manifest create command~~ Release: attempt to fix docker manifest commands Apr 30, 2024

portersrc force-pushed the docker-manifest-create-fix2 branch from d0665c0 to 13285e0 Compare April 30, 2024 10:33

wainersm approved these changes Apr 30, 2024

View reviewed changes

ldoktor approved these changes Apr 30, 2024

View reviewed changes

ldoktor merged commit e36bf9d into confidential-containers:main Apr 30, 2024
10 checks passed

portersrc deleted the docker-manifest-create-fix2 branch May 24, 2024 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release: attempt to fix docker manifest commands #367

Release: attempt to fix docker manifest commands #367

portersrc commented Apr 29, 2024 •

edited

Loading

fitzthum left a comment

ldoktor Apr 30, 2024

ldoktor Apr 30, 2024

ldoktor Apr 30, 2024

portersrc Apr 30, 2024

ldoktor left a comment

ldoktor commented Apr 30, 2024 •

edited

Loading

wainersm commented Apr 30, 2024

portersrc commented Apr 30, 2024 •

edited

Loading

portersrc commented Apr 30, 2024

portersrc commented Apr 30, 2024

wainersm commented Apr 30, 2024

wainersm left a comment

ldoktor left a comment

Release: attempt to fix docker manifest commands #367

Release: attempt to fix docker manifest commands #367

Conversation

portersrc commented Apr 29, 2024 • edited Loading

fitzthum left a comment

Choose a reason for hiding this comment

ldoktor Apr 30, 2024

Choose a reason for hiding this comment

ldoktor Apr 30, 2024

Choose a reason for hiding this comment

ldoktor Apr 30, 2024

Choose a reason for hiding this comment

portersrc Apr 30, 2024

Choose a reason for hiding this comment

ldoktor left a comment

Choose a reason for hiding this comment

ldoktor commented Apr 30, 2024 • edited Loading

wainersm commented Apr 30, 2024

portersrc commented Apr 30, 2024 • edited Loading

portersrc commented Apr 30, 2024

portersrc commented Apr 30, 2024

wainersm commented Apr 30, 2024

wainersm left a comment

Choose a reason for hiding this comment

ldoktor left a comment

Choose a reason for hiding this comment

portersrc commented Apr 29, 2024 •

edited

Loading

ldoktor commented Apr 30, 2024 •

edited

Loading

portersrc commented Apr 30, 2024 •

edited

Loading