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

Add session callback before export completion #4139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Aug 11, 2023

This allows for more neatly adding client-side logic that allows affecting the status of the export.

For example, in buildx for the docker driver, we have logic for taking the resulting type=image export and pushing it to a registry: https://github.com/docker/buildx/blob/master/build/build.go#L1149. Additionally, we have follow-ups for multi-node manifest list merging https://github.com/docker/buildx/blob/master/build/build.go#L1035.

Eventually, we'll also want something similar in relation to moby/moby#44369, where we'll want the client to stream content from the BuildKit image exporter's containerd store into the client's containerd store directly.

These kind of client-side exporter follow-ups are reasonably common, but currently, because the result of the build export is only available after a call to Solve, then we can't:

  1. Guarantee any access to internal resources from the exporter - e.g. if we import the results into the containerd content store using unpack, the results will be cleaned up by GC at some point after Solve.
  2. Cause the build to fail after a client-side failure - e.g. if the manifest merge fails for some reason, then each individual build will still succeed. Ideally, the build should fail with a client-side provided error message, since what the user requested wasn't able to be completed correctly.
  3. Modify the exporter result, for example, after the result of a client-side merge. If we are doing a multi-platform build split across multiple builders, then the digests of the resulting image in the exporter may be incorrect (and the build history will record this).

To resolve this, we can add a new session API - this new API will be called immediately after the exporter runs and we know whether it succeeded or failed, but before the underlying resources are cleaned up, and before Solve succeeds. This allows us to affect the final error message and exporter response from Solve, as well as guarantee access to any image layers/etc before they are cleaned up.

A few considered alternatives:

  • Simply allow using leases, or an allocate/free pattern with the image exporter to guarantee the lifetimes - this requires the client to manage these leases, and essentially ties the release of objects to well-behaved clients. This also doesn't handle any errors propagated from the client.
  • Add a new custom exporter that simply calls the client-side callback, without propagating it through each exporter. The issue is that we'd still need to duplicate much of the image exporter to ensure that the layers we'd want to pull are present. This is also much less flexible, and can't be used everywhere.
  • Maybe this might not be required if we could drive exports from the gateway API? I'm a bit unsure on whether this would work at all. This method would definitely allow us to change the error message from the build, may be able to handle the lifetime of underlying resources depending on the exact semantics of the new API, but probably wouldn't allow us to modify the export responses.

Note: this should work with #2760, so we'd likely want to modify this to include an exporter ID if we take this approach.

Naming is hard, export is definitely the wrong name for this new session API, but struggling to come up with anything different 😢

Signed-off-by: Justin Chadwell <me@jedevc.com>
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

I'm a bit confused why this is useful for anything else than image. I thought the intention was that buildkit creates image, sends a digest to the client, and blocks until the client has handled it.

In that case it is more like the push option in the current image exporter, but instead of push=true it would be something like signalsession=sessionid.

Regarding naming, maybe session/resultcallback or something similar.

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.

None yet

2 participants