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

Make "tags" arch-specific again #45

Merged
merged 3 commits into from
May 13, 2024
Merged

Conversation

tianon
Copy link
Member

@tianon tianon commented May 2, 2024

This fixes edge case bugs where tags which share a sourceId but across disparate architectures will be grouped incorrectly (see also #27, which is where tags was moved to be global instead of arch-specific because I had forgotten about and didn't have an explicit test case for the edge cases where it does matter, which this PR also corrects).

This can be seen concretely in https://oci.dag.dev/?image=infosiftr/moby:s390x@sha256:9a423230716c0f934e2fffa46585df1b2babc96ea0a1dd01bbd6ce29c6233aea (where infosiftr/moby:s390x should be just the s390x build, but instead gets all the architecture builds which share that same sourceId).

This is currently split into separate concrete commits to ease review, but I don't feel strongly about keeping them separated.

(this PR is dedicated to @paultag, to whom I was explaining how multi-architecture image storage in the registry works, and reached for this image to illustrate, and thus immediately noticed this bug)

tianon added 3 commits May 2, 2024 12:56
This adds more of the esoteric `busybox` tags with interesting overlaps so that tagging bugs can be more obvious in the future, and a new "canonical" file to help us spot them / debug them.
…e cases

This includes the edge case of arch-particular but not arch-specific tags (similar to busybox, but with the primary difference being that they *share* a sourceId instead of having disparate sourceIds).

In this, I have hand-modified the `all-tags.json` file to what it *should* be so that the tests will fail until it is fixed appropriately (but all other generated files are as-generated).
This fixes edge case bugs where tags which share a sourceId but across disparate architectures will be grouped.

This can be seen concretely in https://oci.dag.dev/?image=infosiftr/moby:s390x@sha256:9a423230716c0f934e2fffa46585df1b2babc96ea0a1dd01bbd6ce29c6233aea (where `infosiftr/moby:s390x` should be _just_ the s390x build, but instead gets _all_ the architecture builds which share that same sourceId).
@tianon tianon requested a review from yosifkit as a code owner May 2, 2024 21:24
@tianon
Copy link
Member Author

tianon commented May 2, 2024

GitCommit: 37f1b58c0dc59e6589a0f55849f73d6ef0988cd8
Directory: infosiftr-moby
Architectures: riscv64
riscv64-File: Dockerfile.unstable
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that it's public, I can link to https://github.com/tianon/bashbrew-tianon/blob/96f417f832852216e0cc276680897c5d63a1fdeb/infosiftr/moby to show where this file comes from (there wasn't really a great reason to keep this private except that I wasn't sure if I wanted to keep doing it, but it's been many years now 😂).

@tianon
Copy link
Member Author

tianon commented May 2, 2024

I updated my personal builds to include this change, and so the actual images on Hub are now fixed (which is why my link above is by-digest to illustrate even after it's fixed).

You can see the effect on the JSON in tianon/bashbrew-tianon-meta@371d468, and the fixed example s390x image at https://oci.dag.dev/?image=infosiftr/moby:s390x@sha256:d80803750379566d830418b38d0069809891993ec8779b354aa6915d3e5e36bf

Copy link
Collaborator

@whalelines whalelines left a comment

Choose a reason for hiding this comment

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

If I understand this correctly, it changes the data structure and therefore would require regenerating the various source-of-truth JSON files. How long does it take to regenerate them?

@tianon
Copy link
Member Author

tianon commented May 2, 2024

The jobs that regenerate them are ~2.5m each when they're load-bearing.

@yosifkit yosifkit merged commit d3c3c3c into docker-library:main May 13, 2024
1 check passed
@yosifkit yosifkit deleted the tags-fix branch May 13, 2024 21:01
tianon added a commit to docker-library/meta that referenced this pull request May 13, 2024
@tianon
Copy link
Member Author

tianon commented May 13, 2024

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

Successfully merging this pull request may close these issues.

3 participants