Skip to content

daemon: ImageService.LogImageEvent: pass through context #49014

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.

Sign up for GitHub

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

Merged
thaJeztah merged 1 commit into from
Jan 8, 2025
Merged

daemon: ImageService.LogImageEvent: pass through context #49014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions daemon/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,12 @@ import (
// When no tag is given, buildkit doesn't call the image service so it has no
// way of knowing the image was created.
func (daemon *Daemon) ImageExportedByBuildkit(ctx context.Context, id string, desc ocispec.Descriptor) {
daemon.imageService.LogImageEvent(id, id, events.ActionCreate)
daemon.imageService.LogImageEvent(ctx, id, id, events.ActionCreate)
}

// 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)
daemon.imageService.LogImageEvent(ctx, desc.Digest.String(), reference.FamiliarString(ref), events.ActionTag)
}
2 changes: 1 addition & 1 deletion daemon/containerd/image_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ func (i *ImageService) createImageOCI(ctx context.Context, imgToCreate imagespec
}

id := image.ID(img.Target.Digest)
i.LogImageEvent(id.String(), id.String(), events.ActionCreate)
i.LogImageEvent(ctx, id.String(), id.String(), events.ActionCreate)

if err := i.unpackImage(ctx, i.StorageDriver(), img, manifestDesc); err != nil {
return "", err
Expand Down
4 changes: 2 additions & 2 deletions daemon/containerd/image_delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ func (i *ImageService) deleteAll(ctx context.Context, imgID image.ID, all []c8di
return records, err
}
}
i.LogImageEvent(imgID.String(), imgID.String(), events.ActionDelete)
i.LogImageEvent(ctx, imgID.String(), imgID.String(), events.ActionDelete)
records = append(records, imagetypes.DeleteResponse{Deleted: imgID.String()})

for _, parent := range parents {
Expand All @@ -248,7 +248,7 @@ func (i *ImageService) deleteAll(ctx context.Context, imgID image.ID, all []c8di
break
}
parentID := parent.Target.Digest.String()
i.LogImageEvent(parentID, parentID, events.ActionDelete)
i.LogImageEvent(ctx, parentID, parentID, events.ActionDelete)
records = append(records, imagetypes.DeleteResponse{Deleted: parentID})
}

Expand Down
4 changes: 2 additions & 2 deletions daemon/containerd/image_events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import (
)

// LogImageEvent generates an event related to an image with only the default attributes.
func (i *ImageService) LogImageEvent(imageID, refName string, action events.Action) {
ctx := context.TODO()
func (i *ImageService) LogImageEvent(ctx context.Context, imageID, refName string, action events.Action) {
ctx = context.WithoutCancel(ctx)
Comment on lines +12 to +13
Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Jan 6, 2025

Choose a reason for hiding this comment

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

Same here

attributes := map[string]string{}

img, err := i.GetImage(ctx, imageID, backend.GetImageOpts{})
Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Dec 2, 2024

Choose a reason for hiding this comment

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

Wonder if this needs some WithoutCancel as it's called on defer in various cases

Copy link
Copy Markdown
Contributor

@vvoland vvoland Dec 2, 2024

Choose a reason for hiding this comment

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

I think it depends on the case. The only usage in defer I see now is: https://github.com/moby/moby/pull/49014/files#diff-911a739572dfba17ed589736bce4dc155519a814c86a4c8d72a6ae556feab63bR67

Which definitely would need a WithoutCancel, but I think it should be done on the caller side.

Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Dec 2, 2024

Choose a reason for hiding this comment

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

Yeah, makes sense; I first peeked at the code below this line, and it should handle cancelled contexts gracefully (in which case it just skips image-labels in the event), so probably still would be ok.

Looking at that one case where we use defer, it looks like it doesn't have to be a defer, so perhaps I'll just move it to the end, and add a WithoutCancel().

Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Dec 2, 2024

Choose a reason for hiding this comment

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

OK; updated; https://github.com/moby/moby/compare/d3ed1250aab9a5feeede58a9c24223bb7002985d..ba8f0378f9dbc5306954de22a6311cd0f09acc22

Copy link
Copy Markdown
Contributor

@vvoland vvoland Dec 2, 2024

Choose a reason for hiding this comment

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

OTOH, given our current usages, it would make sense to make LogImageEvent non-cancelable by default.
Mostly my thinking is: if we're calling this function, then something already happened, so we could be missing an event while the actual effect will not be rolled back.

UNLESS, the calling function would actually be able to roll-back the effects that the event advertises.
However, I don't think it would make much sense to roll back in case the operation gets cancelled just at the time of logging the event.. As it only really happens after everything has already been done.

So, given the above, I think it's also fine to just slap a ctx = context.WithoutCancel(ctx) at the top of LogImageEvent

Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Dec 3, 2024

Choose a reason for hiding this comment

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

Yeah, perhaps that makes sense. My slight concern was if the GetImage( would take a long time; would that be problematic? (just for the extra metadata) But we already do so before this PR, so I guess this would not make a difference here.

Expand Down
4 changes: 2 additions & 2 deletions daemon/containerd/image_exporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ func (i *ImageService) ExportImage(ctx context.Context, names []string, platform
}).Debug("export image without name")
}

i.LogImageEvent(target.Digest.String(), target.Digest.String(), events.ActionSave)
i.LogImageEvent(ctx, target.Digest.String(), target.Digest.String(), events.ActionSave)
return nil
}

Expand Down Expand Up @@ -368,7 +368,7 @@ func (i *ImageService) LoadImage(ctx context.Context, inTar io.ReadCloser, platf
})

fmt.Fprintf(progress, "%s: %s\n", loadedMsg, name)
i.LogImageEvent(img.Target.Digest.String(), img.Target.Digest.String(), events.ActionLoad)
i.LogImageEvent(ctx, img.Target.Digest.String(), img.Target.Digest.String(), events.ActionLoad)

if err != nil {
// The image failed to unpack, but is already imported, log the error but don't fail the whole load.
Expand Down
2 changes: 1 addition & 1 deletion daemon/containerd/image_import.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ func (i *ImageService) ImportImage(ctx context.Context, ref reference.Named, pla
if err != nil {
logger.WithError(err).Debug("failed to unpack image")
} else {
i.LogImageEvent(id.String(), id.String(), events.ActionImport)
i.LogImageEvent(ctx, id.String(), id.String(), events.ActionImport)
}

return id, err
Expand Down
2 changes: 1 addition & 1 deletion daemon/containerd/image_pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ func (i *ImageService) pullTag(ctx context.Context, ref reference.Named, platfor
logger.WithError(err).Warn("unexpected error while removing outdated dangling image reference")
}

i.LogImageEvent(reference.FamiliarString(ref), reference.FamiliarName(ref), events.ActionPull)
i.LogImageEvent(ctx, reference.FamiliarString(ref), reference.FamiliarName(ref), events.ActionPull)
outNewImg = img
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion daemon/containerd/image_push.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ func (i *ImageService) pushRef(ctx context.Context, targetRef reference.Named, p

appendDistributionSourceLabel(ctx, realStore, targetRef, target)

i.LogImageEvent(reference.FamiliarString(targetRef), reference.FamiliarName(targetRef), events.ActionPush)
i.LogImageEvent(ctx, reference.FamiliarString(targetRef), reference.FamiliarName(targetRef), events.ActionPush)

return nil
}
Expand Down
5 changes: 2 additions & 3 deletions daemon/containerd/image_tag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (i *ImageService) createOrReplaceImage(ctx context.Context, newImg c8dimage
// No need to do anything.
if replacedImg.Target.Digest == newImg.Target.Digest {
if !creatingDangling {
i.LogImageEvent(replacedImg.Target.Digest.String(), imageFamiliarName(newImg), events.ActionTag)
i.LogImageEvent(ctx, replacedImg.Target.Digest.String(), imageFamiliarName(newImg), events.ActionTag)
}
return nil
}
Expand All @@ -81,8 +81,7 @@ func (i *ImageService) createOrReplaceImage(ctx context.Context, newImg c8dimage
logger.Info("image created")

if !creatingDangling {
ctx := context.WithoutCancel(ctx)
defer i.LogImageEvent(string(newImg.Target.Digest), imageFamiliarName(newImg), events.ActionTag)
defer i.LogImageEvent(ctx, string(newImg.Target.Digest), imageFamiliarName(newImg), events.ActionTag)

if err := i.images.Delete(ctx, danglingName); err != nil {
if !cerrdefs.IsNotFound(err) {
Expand Down
2 changes: 1 addition & 1 deletion daemon/image_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ type ImageService interface {
PerformWithBaseFS(ctx context.Context, c *container.Container, fn func(string) error) error
LoadImage(ctx context.Context, inTar io.ReadCloser, platform *ocispec.Platform, outStream io.Writer, quiet bool) error
Images(ctx context.Context, opts imagetype.ListOptions) ([]*imagetype.Summary, error)
LogImageEvent(imageID, refName string, action events.Action)
LogImageEvent(ctx context.Context, imageID, refName string, action events.Action)
CountImages(ctx context.Context) int
ImagesPrune(ctx context.Context, pruneFilters filters.Args) (*imagetype.PruneReport, error)
ImportImage(ctx context.Context, ref reference.Named, platform *ocispec.Platform, msg string, layerReader io.Reader, changes []string) (image.ID, error)
Expand Down
2 changes: 1 addition & 1 deletion daemon/images/image_commit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func (i *ImageService) CommitImage(ctx context.Context, c backend.CommitConfig)
return "", err
}

i.LogImageEvent(id.String(), id.String(), events.ActionCreate)
i.LogImageEvent(ctx, id.String(), id.String(), events.ActionCreate)

if err := i.imageStore.SetBuiltLocally(id); err != nil {
return "", err
Expand Down
8 changes: 4 additions & 4 deletions daemon/images/image_delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func (i *ImageService) ImageDelete(ctx context.Context, imageRef string, force,

untaggedRecord := imagetypes.DeleteResponse{Untagged: reference.FamiliarString(parsedRef)}

i.LogImageEvent(imgID.String(), imgID.String(), events.ActionUnTag)
i.LogImageEvent(ctx, imgID.String(), imgID.String(), events.ActionUnTag)
records = append(records, untaggedRecord)

repoRefs = i.referenceStore.References(imgID.Digest())
Expand Down Expand Up @@ -164,7 +164,7 @@ func (i *ImageService) ImageDelete(ctx context.Context, imageRef string, force,
if err != nil {
return nil, err
}
i.LogImageEvent(imgID.String(), imgID.String(), events.ActionUnTag)
i.LogImageEvent(ctx, imgID.String(), imgID.String(), events.ActionUnTag)
records = append(records, imagetypes.DeleteResponse{Untagged: reference.FamiliarString(parsedRef)})
}
}
Expand Down Expand Up @@ -245,7 +245,7 @@ func (i *ImageService) removeAllReferencesToImageID(imgID image.ID, records *[]i
if err != nil {
return err
}
i.LogImageEvent(imgID.String(), imgID.String(), events.ActionUnTag)
i.LogImageEvent(context.TODO(), imgID.String(), imgID.String(), events.ActionUnTag)
*records = append(*records, imagetypes.DeleteResponse{
Untagged: reference.FamiliarString(parsedRef),
})
Expand Down Expand Up @@ -322,7 +322,7 @@ func (i *ImageService) imageDeleteHelper(imgID image.ID, records *[]imagetypes.D
return err
}

i.LogImageEvent(imgID.String(), imgID.String(), events.ActionDelete)
i.LogImageEvent(context.TODO(), imgID.String(), imgID.String(), events.ActionDelete)
*records = append(*records, imagetypes.DeleteResponse{Deleted: imgID.String()})
for _, removedLayer := range removedLayers {
*records = append(*records, imagetypes.DeleteResponse{Deleted: removedLayer.ChainID.String()})
Expand Down
4 changes: 2 additions & 2 deletions daemon/images/image_events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import (
)

// LogImageEvent generates an event related to an image with only the default attributes.
func (i *ImageService) LogImageEvent(imageID, refName string, action events.Action) {
ctx := context.TODO()
func (i *ImageService) LogImageEvent(ctx context.Context, imageID, refName string, action events.Action) {
ctx = context.WithoutCancel(ctx)
Comment on lines +11 to +12
Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Jan 6, 2025

Choose a reason for hiding this comment

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

I moved the WithoutCancel into these functions (as we were discussing above)

attributes := map[string]string{}

img, err := i.GetImage(ctx, imageID, backend.GetImageOpts{})
Expand Down
2 changes: 1 addition & 1 deletion daemon/images/image_import.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,6 @@ func (i *ImageService) ImportImage(ctx context.Context, newRef reference.Named,
}
}

i.LogImageEvent(id.String(), id.String(), events.ActionImport)
i.LogImageEvent(ctx, id.String(), id.String(), events.ActionImport)
return id, nil
}
2 changes: 1 addition & 1 deletion daemon/images/image_tag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,6 @@ func (i *ImageService) TagImage(ctx context.Context, imageID image.ID, newTag re
if err := i.imageStore.SetLastUpdated(imageID); err != nil {
return err
}
i.LogImageEvent(imageID.String(), reference.FamiliarString(newTag), events.ActionTag)
i.LogImageEvent(ctx, imageID.String(), reference.FamiliarString(newTag), events.ActionTag)
return nil
}
2 changes: 1 addition & 1 deletion distribution/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ type Config struct {
// and endpoint lookup.
RegistryService RegistryResolver
// ImageEventLogger notifies events for a given image
ImageEventLogger func(id, name string, action events.Action)
ImageEventLogger func(ctx context.Context, id, name string, action events.Action)
// MetadataStore is the storage backend for distribution-specific
// metadata.
MetadataStore metadata.Store
Expand Down
2 changes: 1 addition & 1 deletion distribution/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func Pull(ctx context.Context, ref reference.Named, config *ImagePullConfig, loc
})

if err == nil {
config.ImageEventLogger(reference.FamiliarString(ref), reference.FamiliarName(repoInfo.Name), events.ActionPull)
config.ImageEventLogger(ctx, reference.FamiliarString(ref), reference.FamiliarName(repoInfo.Name), events.ActionPull)
}

return err
Expand Down
2 changes: 1 addition & 1 deletion distribution/push.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func Push(ctx context.Context, ref reference.Named, config *ImagePushConfig) err
return err
}

config.ImageEventLogger(reference.FamiliarString(ref), reference.FamiliarName(repoInfo.Name), events.ActionPush)
config.ImageEventLogger(ctx, reference.FamiliarString(ref), reference.FamiliarName(repoInfo.Name), events.ActionPush)
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion image/tarexport/load.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ func (l *tarexporter) Load(ctx context.Context, inTar io.ReadCloser, outStream i
}

parentLinks = append(parentLinks, parentLink{imgID, m.Parent})
l.loggerImgEvent.LogImageEvent(imgID.String(), imgID.String(), events.ActionLoad)
l.loggerImgEvent.LogImageEvent(ctx, imgID.String(), imgID.String(), events.ActionLoad)
}

for _, p := range validatedParentLinks(parentLinks) {
Expand Down
2 changes: 1 addition & 1 deletion image/tarexport/save.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ func (s *saveSession) save(ctx context.Context, outStream io.Writer) error {

parentID, _ := s.is.GetParent(id)
parentLinks = append(parentLinks, parentLink{id, parentID})
s.tarexporter.loggerImgEvent.LogImageEvent(id.String(), id.String(), events.ActionSave)
s.tarexporter.loggerImgEvent.LogImageEvent(ctx, id.String(), id.String(), events.ActionSave)
}

for i, p := range validatedParentLinks(parentLinks) {
Expand Down
4 changes: 3 additions & 1 deletion image/tarexport/tarexport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package tarexport // import "github.com/docker/docker/image/tarexport"

import (
"context"

"github.com/containerd/platforms"
"github.com/docker/distribution"
"github.com/docker/docker/api/types/events"
Expand Down Expand Up @@ -37,7 +39,7 @@ type tarexporter struct {
// LogImageEvent defines interface for event generation related to image tar(load and save) operations
type LogImageEvent interface {
// LogImageEvent generates an event related to an image operation
LogImageEvent(imageID, refName string, action events.Action)
LogImageEvent(ctx context.Context, imageID, refName string, action events.Action)
}

// NewTarExporter returns new Exporter for tar packages
Expand Down