Skip to content
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

Fixes: #15266 All docker images of Architecture show amd64 #15270

Merged
merged 1 commit into from Mar 31, 2023

Conversation

zhangguanzhang
Copy link
Contributor

@@ -261,8 +261,15 @@ main() {
for TARGET_ARCH in "amd64" "arm64" "ppc64le" "s390x"; do
log_callout "Pushing container images to quay.io ${RELEASE_VERSION}-${TARGET_ARCH}"
maybe_run docker push "quay.io/coreos/etcd:${RELEASE_VERSION}-${TARGET_ARCH}"
maybe_run docker manifest create "quay.io/coreos/etcd:${RELEASE_VERSION}-${TARGET_ARCH}" "quay.io/coreos/etcd:${RELEASE_VERSION}-${TARGET_ARCH}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please integrate it with existing block for creating manifest list on line 275.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please integrate it with existing block for creating manifest list on line 275.

Done, PTAL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serathius Why do you think the new place is better ?
I actually liked the previous version more:

The loop in 261 is responsible for preparing the arch-specific images and pushing them.

The loop in 275 is preparing a single multi-arch image.

After the modification the code got mixed and in my opinion is more confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that separating image vs manifest operation will make it cleaner. However I'm not sure after seeing it.
Delegating to @ptabor and @justinsb to review.

@geetasg
Copy link

geetasg commented Feb 17, 2023

@serathius @ptabor can you please comment on how did this work alright earlier? thanks

output for 3.5.6
docker image inspect quay.io/coreos/etcd:v3.5.6-arm64 --format '{{.Architecture}}'
arm64

@zhangguanzhang
Copy link
Contributor Author

Is there anyone here who can take a look? 😘

@ahrtr
Copy link
Member

ahrtr commented Feb 21, 2023

It seems that the issue was introduced by #15016, but did not get time to dig into the root cause yet.

I would suggest to add a new workflow test to do some sanity test on the images firstly (See #15139), and then apply this PR so as to confirm that it indeed fixes the issue.

@ahrtr
Copy link
Member

ahrtr commented Feb 21, 2023

The multi-arch manifest looks good.

# docker manifest inspect quay.io/coreos/etcd:v3.5.7
{
   "schemaVersion": 2,
   "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
   "manifests": [
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1781,
         "digest": "sha256:ed53908abf741f2f29e7cb1fc7f53cd41b9ecea1d85308d5f1b51dae07decd2a",
         "platform": {
            "architecture": "amd64",
            "os": "linux"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1574,
         "digest": "sha256:82aa51752864de767380d9d5770f5628d2f68a561f7c98c65ae13f69cd4440af",
         "platform": {
            "architecture": "arm64",
            "os": "linux"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1574,
         "digest": "sha256:ad17d68dbb8fd05e9a3d5da131f8b83796ee9ef9a1a4f43127a7c79ef96fec2a",
         "platform": {
            "architecture": "ppc64le",
            "os": "linux"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1574,
         "digest": "sha256:b287e2b298faea9a05ede6284498b7cf79c9cb3bb89a9840fbbf222cad8f0c8e",
         "platform": {
            "architecture": "s390x",
            "os": "linux"
         }
      }
   ]
}

@ahrtr
Copy link
Member

ahrtr commented Mar 30, 2023

Please add a test case. You can follow #15505

@serathius
Copy link
Member

serathius commented Mar 30, 2023

Instead fixing all images, should we stop publishing tags other than multi-arch? There is no benefits to publishing single-arch image tags, and k8s doesn't do that.

@zhangguanzhang
Copy link
Contributor Author

Please add a test case. You can follow #15505

Done

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me, thanks @zhangguanzhang .

Agree that it might simplify things to just publish the multi arch images in future. I've created #15594 to discuss that further.

@ahrtr
Copy link
Member

ahrtr commented Mar 31, 2023

Please resolve the workflow failures before I take a look.

@zhangguanzhang
Copy link
Contributor Author

zhangguanzhang commented Mar 31, 2023

Using buildkit seems to be a simple solution to this problem 🤦‍♂️, @ahrtr @serathius PATL

Signed-off-by: zhangguanzhang <zhangguanzhang@qq.com>
@ahrtr
Copy link
Member

ahrtr commented Mar 31, 2023

Instead fixing all images, should we stop publishing tags other than multi-arch? There is no benefits to publishing single-arch image tags, and k8s doesn't do that.

Agreed. But let's make the change on main branch only. Thanks @jmhbnz for raising #15594 to track it.

@ahrtr
Copy link
Member

ahrtr commented Mar 31, 2023

It seems that the issue was introduced by #15016, but did not get time to dig into the root cause yet.

The root cause turned out to be that builtkit wasn't enabled. Thanks @zhangguanzhang

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good old docker quirks.

This solution looks much better. Thanks for diving in and finding the root issue!

@serathius serathius merged commit 7c626e0 into etcd-io:main Mar 31, 2023
27 checks passed
@ahrtr
Copy link
Member

ahrtr commented Mar 31, 2023

@zhangguanzhang could you please backport this PR to 3.5 and 3.4? thx

@zhangguanzhang
Copy link
Contributor Author

@zhangguanzhang could you please backport this PR to 3.5 and 3.4? thx

There is no test_image.sh in 3.5,so don't cherry-pick just commit?

@ahrtr
Copy link
Member

ahrtr commented Mar 31, 2023

@zhangguanzhang could you please backport this PR to 3.5 and 3.4? thx

There is no test_image.sh in 3.5,so don't cherry-pick just commit?

Yes, we need to backport #15505 firstly. Could you or @pchan finish it? It's a priority to me, so as to unblock releasing 3.5.8.

@pchan
Copy link
Contributor

pchan commented Apr 1, 2023

Yes, we need to backport #15505 firstly. Could you or @pchan finish it? It's a priority to me, so as to unblock releasing 3.5.8.

I have created the cherry-pick #15608 to backport #15505

@ahrtr
Copy link
Member

ahrtr commented Apr 1, 2023

Thanks @pchan

@ahrtr
Copy link
Member

ahrtr commented Apr 1, 2023

@zhangguanzhang The test_image.sh file was just added into release-3.5, please go ahead to backport this PR to 3.5, thx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

All docker images of Architecture show amd64
7 participants