-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
executor: Don't repull image if pinned by digest #28265
Conversation
@aaronlehmann I'm not sure this is the right component to handle this. The format of the image ref should be opaque to the the controller. Shouldn't the engine make this determination? |
named, err := reference.ParseNamed(r.adapter.container.spec().Image) | ||
if err == nil { | ||
if _, ok := named.(reference.Canonical); ok { | ||
_, err := r.adapter.backend.LookupImage(r.adapter.container.spec().Image) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment, but this should at least be checked in the adapter.
@stevvooe: Do you mean that the pull implementation should short-circuit if a pull by digest was requested and the image with that digest already exists? I'm not completely sure I agree with that, as it means we would skip the flow that normally happens when you run |
I see why this optimization could be problematic in the push/pull code. Let's just move this into the adapter for now. |
If the image reference in the spec uses a digest, and an image with that digest already exists locally, avoid an unnecessary repull. Signed-off-by: Aaron Lehmann <[email protected]>
695f4a3
to
f69e5c1
Compare
@stevvooe: Moved to the adapter. This ended up being cleaner than the original approach. PTAL |
LGTM |
1 similar comment
LGTM |
LGTM |
If the image reference in the spec uses a digest, and an image with that
digest already exists locally, avoid an unnecessary repull.
cc @nishanttotla @stevvooe @aluzzardi @tonistiigi