storage.storageImageDestination.Commit(): leverage image options #2067
Conversation
nalind
force-pushed
the
create-image-options
branch
from
3c47e19 to
6186aa7
Compare
There was a problem hiding this comment.
Thanks, this is a great improvement, both in correctness and efficiency.
Compare containers/storage#595 — from a quick check this seems to be a major improvement in that problem space, but maybe we should order the “big data” items in a specific order (config, individual manifest, top-level manifest; what about signatures?!) to avoid the remaining races.
| } | ||
| }() | ||
|
|
||
| // Add the reference's name on the image. We don't need to worry about avoiding duplicate | ||
| // values because AddNames() will deduplicate the list that we pass to it. | ||
| if name := s.imageRef.DockerReference(); name != nil { | ||
| if err := s.imageRef.transport.store.AddNames(img.ID, []string{name.String()}); err != nil { |
There was a problem hiding this comment.
This can also be done using a parameter to CreateImage if the image does not exist yet; that would avoid another trip through the fairly costly locking implementation.
… and then we would really need the commitSucceeded handling only in the “image already exists” case, and that would be an opportunity revisit whether we really want to delete the image, or maybe we can just delete that code.
There was a problem hiding this comment.
Some care has to be taken with that parameter - if any of the names passed at create-time are already in use, we get an ErrDuplicateName error back, and I think we want to avoid that.
There was a problem hiding this comment.
Oops, I didn’t notice that CreateImage returns ErrDuplicateName.
It might make sense to let callers opt in into overriding names with a new field in storage.ImageOptions, but that would not be quite an one-line change, so let’s not wait on that.
|
Can we do test-vendor PRs in Buildah and Podman before merging? |
nalind
force-pushed
the
create-image-options
branch
from
6186aa7 to
f268b8e
Compare
When committing an image, pass big data items and the image metadata string in as part of the set of options we pass to the storage library when creating the image. Fall back to setting them individually, as we would have originally, only if we end up updating an image that was already in local storage. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
nalind
force-pushed
the
create-image-options
branch
from
f268b8e to
4f1bcfb
Compare
|
LGTM once the Buildah/Podman tests run. |
containers/image#2067 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
When committing an image, pass big data items and the image metadata string in as part of the set of options we pass to the storage library when creating the image.
Fall back to setting them individually, as we would have originally, only if we end up updating an image that was already in local storage.