c8d/build: Log image tag event when image was built with Buildkit #48078
c8d/build: Log image tag event when image was built with Buildkit #48078thaJeztah merged 3 commits intomoby:masterfrom
image tag event when image was built with Buildkit #48078Conversation
vvoland
added
area/builder
vvoland
changed the title
image tag event when image was built with Buildkit image tagged event when image was built with Buildkit
vvoland
changed the title
image tagged event when image was built with Buildkit image tag event when image was built with Buildkit
| for _, name := range strings.Split(out[string(exptypes.OptKeyName)], ",") { | ||
| ref, err := reference.ParseNormalizedNamed(name) |
There was a problem hiding this comment.
I'm wondering if we're trying too hard here; my thinking is that ExporterInstance.Export would probably fail if any of the OptKeyName values was invalid. If it didn't fail then we _should _ assume that all of them were accepted.
From that perspective, we could just split the string, and call i.callbacks.Named(ctx, namedTagged, desc)
There was a problem hiding this comment.
Yeah it's unexpected that the parsing will fail at this point (as highlighted in comment). The intent here was to provide a reference.Named type instead of raw string to match what the value actually represents.
| type ImageExportedByBuildkit = func(ctx context.Context, id string, desc ocispec.Descriptor) | ||
| type ( | ||
| ImageExportedByBuildkit = func(ctx context.Context, id string, desc ocispec.Descriptor) | ||
| ImageNamedByBuildkit = func(ctx context.Context, ref reference.NamedTagged, desc ocispec.Descriptor) | ||
| ) | ||
|
|
||
| type BuildkitCallbacks struct { | ||
| Exported ImageExportedByBuildkit | ||
| Named ImageNamedByBuildkit | ||
| } |
There was a problem hiding this comment.
What if we would change the signature of the callbacks to take event.Action as argument? In that case the callback could decide whether it needs to do anything.
We could either change the calllbacks to []Callback or (perhaps less code-churn) continue to accept a single callback (which could either have a switch event.Action, or be a wrapper for multiple callbacks)
There was a problem hiding this comment.
I don't like that because I consider the event.Action to be an implementation detail here.
These callbacks are meant to provide the integration point between the image service/daemon and the builder, that was naturally present with the legacy builder and is lost when using Buildkit.
This can potentially be useful for other things, not only emitting events, so IMO it doesn't make sense for the builder code to be concerned with daemon events at all.
| // ImageNamedByBuildkit is a callback that is called when an image is tagged by buildkit. | ||
| // Note: It is only called if the buildkit didn't call the image service itself to perform the tagging. | ||
| // Currently this only happens when the containerd image store is used. | ||
| func (daemon *Daemon) ImageNamedByBuildkit(ctx context.Context, ref reference.NamedTagged, desc ocispec.Descriptor) { | ||
| id := desc.Digest.String() | ||
| name := reference.FamiliarString(ref) | ||
| daemon.imageService.LogImageEvent(id, name, events.ActionTag) | ||
| } |
There was a problem hiding this comment.
See my previous comment; if we had event-type as argument; all of these could effectively be a single method that issues image events (I would change the ref argument to be a string and keep it generic (nameOrID, or could still be just ref as both are references as well).
This is only a callback that notifies about event so there is no way to react to the error. Signed-off-by: Paweł Gronowski <[email protected]>
When image is built with buildkit with containerd integration the image service has no way of knowing that the image was tagged because buildkit creates the image directly in containerd image store. Add a callback that is called by the exporter wrapper. Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
vvoland
force-pushed
the
c8d-buildkit-tag-event
branch
from
1db51bc to
53bc396
Compare
3 participants
image tagevent #47978- What I did
Fixed
image tagevent not being emitted when building images with buildkit and containerd integration.- How I did it
See commits.
- How to verify it
TestBuildEmitsEvents
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)