Skip to content

engine: update buildkit to commit w/ more cache ref logs#5223

Merged
sipsma merged 2 commits intodagger:mainfrom
sipsma:bumpbk
May 31, 2023
Merged

engine: update buildkit to commit w/ more cache ref logs#5223
sipsma merged 2 commits intodagger:mainfrom
sipsma:bumpbk

Conversation

@sipsma
Copy link
Copy Markdown
Contributor

@sipsma sipsma commented May 30, 2023

I sent out a PR to buildkit last week for a few more logs that may be useful for debugging on top of my previous update: moby/buildkit#3905

@sipsma sipsma requested a review from vito May 30, 2023 21:38
@sipsma
Copy link
Copy Markdown
Contributor Author

sipsma commented May 30, 2023

Think there is a non-flake failure in TestContainerExport, probably some backwards incompatible change in buildkit from a separate commit got picked up with the upgrade here, looking into it

@sipsma
Copy link
Copy Markdown
Contributor Author

sipsma commented May 30, 2023

Unfortunately the backwards incompatible change that got picked up incidentally is not incredibly simple to fix:

  1. Previously, containers exported as tarballs included both oci and docker compatible manifests (as verified in our tests here)
  2. Now, exported images will only have oci manifests by default, which seems to be a result of this change in containerd

This means that unless we change something, this update will also result in backwards incompatibility.

I have a guess at an approach that might work with changes to our code only:

  1. If a multiplatform image is being exported, use ExporterOCI. manifest.json doesn't support multi-platform images anyways
  2. If a single platform image is being exported, use ExporterDocker. Based on some buildkit tests, it looks like even those will still have oci indexes, so they should be oci compatible too?

Have to pivot to something else atm, will try this later tonight.

sipsma added 2 commits May 30, 2023 19:32
Signed-off-by: Erik Sipsma <erik@dagger.io>
Signed-off-by: Erik Sipsma <erik@dagger.io>
@sipsma
Copy link
Copy Markdown
Contributor Author

sipsma commented May 31, 2023

I think my update as described here worked, at least it seems that all the existing tests pass, except for one slight modification which was only needed because a bug in the previous implementation was fixed (estargz compression tried to use a nil Attrs map).

cc @vito when you have time let me know if you think this update makes sense too. I think we should add optional args to our publish/export apis that allow users to force use of oci or docker types, but I really don't want to expand the scope of this PR, which was supposed to just be a go.mod version bump, out into new API features 😅 So this just attempts to retain the previous behavior to the extent possible.

@vito
Copy link
Copy Markdown
Contributor

vito commented May 31, 2023

@sipsma Having gone over the linked discussion (thanks for finding it!), I think your change makes the most sense.

I think it could make sense to add a format: OCI | Docker API, but I would prefer to wait until someone really needs it. It would be nice to end up with pure-OCI in the long run, which we might be getting closer to (moby/moby#25779 was closed 2 weeks ago!).

@sipsma
Copy link
Copy Markdown
Contributor Author

sipsma commented May 31, 2023

I think it could make sense to add a format: OCI | Docker API, but I would prefer to wait until someone really needs it. It would be nice to end up with pure-OCI in the long run, which we might be getting closer to (moby/moby#25779 was closed 2 weeks ago!).

Oh good find, I totally agree about not rushing into that. We'll have to wait for the new docker release to make it's way out in the world's install base, but it would be very nice if that happens before anyone needs support for something besides OCI.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Moniture repository directory

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