daemon: ImageService.LogImageEvent: pass through context#49014
daemon: ImageService.LogImageEvent: pass through context#49014thaJeztah merged 1 commit intomoby:masterfrom
Conversation
thaJeztah
added
status/2-code-review
kind/refactor
| func (i *ImageService) LogImageEvent(ctx context.Context, imageID, refName string, action events.Action) { | ||
| attributes := map[string]string{} | ||
|
|
||
| img, err := i.GetImage(ctx, imageID, backend.GetImageOpts{}) |
There was a problem hiding this comment.
Wonder if this needs some WithoutCancel as it's called on defer in various cases
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
thaJeztah
force-pushed
the
eventlog_context
branch
from
587c969 to
d3ed125
Compare
|
LOL; probably works better if I stage all changes 🙈 😂 |
thaJeztah
force-pushed
the
eventlog_context
branch
from
d3ed125 to
ba8f037
Compare
thaJeztah
force-pushed
the
eventlog_context
branch
from
ba8f037 to
4d6663b
Compare
| if !creatingDangling { | ||
| ctx := context.WithoutCancel(ctx) | ||
| defer i.LogImageEvent(string(newImg.Target.Digest), imageFamiliarName(newImg), events.ActionTag) | ||
|
|
||
| if err := i.images.Delete(ctx, danglingName); err != nil { | ||
| if !cerrdefs.IsNotFound(err) { | ||
| logger.WithError(err).Warn("unexpected error when deleting dangling image") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| i.LogImageEvent(ctx, string(newImg.Target.Digest), imageFamiliarName(newImg), events.ActionTag) |
There was a problem hiding this comment.
Double check if this change is still correct now that the if !creatingDangling { condition was added
There was a problem hiding this comment.
No, it should only be logged if !creatingDangling. Otherwise a tag event will be emitted when a dangling image is created (in this case, only create should be emitted).
There was a problem hiding this comment.
Ah, thx! I'll update. I confused myself for a bit when looking at the diff again after rebasing.
| func (i *ImageService) LogImageEvent(ctx context.Context, imageID, refName string, action events.Action) { | ||
| ctx = context.WithoutCancel(ctx) |
There was a problem hiding this comment.
I moved the WithoutCancel into these functions (as we were discussing above)
| func (i *ImageService) LogImageEvent(ctx context.Context, imageID, refName string, action events.Action) { | ||
| ctx = context.WithoutCancel(ctx) |
thaJeztah
force-pushed
the
eventlog_context
branch
from
4d6663b to
b45d1cb
Compare
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
eventlog_context
branch
from
b45d1cb to
ee1a15a
Compare
2 participants
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)