-
Notifications
You must be signed in to change notification settings - Fork 9.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify etcd image release process #15605
Conversation
faf02cc
to
4806d8f
Compare
Only test and publish the single multi arch image. Signed-off-by: James Blair <mail@jamesblair.net>
4806d8f
to
6aab6e7
Compare
@@ -82,13 +82,3 @@ if [ "${GET}" != "${VALUE}" ]; then | |||
fi | |||
|
|||
echo "Succesfully tested etcd local image ${TAG}" | |||
|
|||
for TARGET_ARCH in "amd64" "arm64" "ppc64le" "s390x"; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the testing of ARCH, so we validate that multi-arch image correctly links to subimages for all supported architectures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing - I noticed while I was working on it there is duplication of key variables and functions between test_images.sh
and release.sh
so I have refactored into a new docker_lib.sh
, added a lot more error handling and improved consistency with how errors are reported. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there is a hitch here as well, when in dry run the images don't actually exist in the remote registry. With docker manifest
only working on an actual registry rather than local, refer docker/cli#3350
Reading through there are some potential hacks to work around, or we could spin up a temporary registry as part of ci to test this more naturally, not sure what the appetite for either is though.
@@ -258,13 +258,6 @@ main() { | |||
log_warning "login failed, retrying" | |||
done | |||
|
|||
for TARGET_ARCH in "amd64" "arm64" "ppc64le" "s390x"; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to test if you can push multi-arch image without pushing each arch. Workaround would be to just push images but avoid tagging them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh bad news, yeah docker manifest
will only ever work with the registry images and can't do any magic that would allow us to only push the multi arch by itself.
Unfortunately even docker buildx
appears to work pretty much the same way, just with simpler ux.
So yeah we could push and not tag but actually reviewing kubernetes docs they are still publishing individual arch images so we may as well remain on that pattern also:
https://kubernetes.io/releases/download/
All container images are available for multiple architectures, whereas the container runtime should choose the correct one based on the underlying platform. It is also possible to pull a dedicated architecture by suffixing the container image name, for example registry.k8s.io/kube-apiserver-arm64:v1.26.0. All those derivations are signed in the same way as the multi-architecture manifest lists.
So after this research I don't think we can simplify the image pushing at all? Your call if we just close this or continue with just the refactoring I mentioned above.
Signed-off-by: James Blair <mail@jamesblair.net>
500f60f
to
ce01dd6
Compare
Signed-off-by: James Blair <mail@jamesblair.net>
ce01dd6
to
888b099
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions. |
Only test and publish the single multi arch image to simplify etcd release process and the image landscape for downstream users to consume.
Leaving this in draft as it needs to be discussed when it would be appropriate to make this change and how it would be communicated.
Fixes: #15594