confidential-containers / operator

Operator to deploy confidential containers runtime
Apache License 2.0
109 stars 58 forks source link

Release: attempt to fix docker manifest commands #367

Closed portersrc closed 5 months ago

portersrc commented 5 months ago

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

ldoktor commented 5 months ago

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 commented 5 months ago

@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 ef55c2ecbf38f98777cae4946c64d50d95ec3ebb 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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

wainersm commented 5 months ago

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