c8d: implement missing image delete logic#45427
Conversation
ad3a94d to
0b78470
Compare
0b78470 to
b8113bb
Compare
d775d72 to
d8f4e5b
Compare
|
Failures are unrelated: see: #45415 |
d8f4e5b to
9d68cc3
Compare
3b3b1af to
c8c76b1
Compare
c8c76b1 to
79a74fb
Compare
79a74fb to
b482a09
Compare
20cbe62 to
6c29ecb
Compare
| // are divided into two categories grouped by their severity: | ||
| // | ||
| // Hard Conflict: | ||
| // - a pull or build using the image. |
There was a problem hiding this comment.
Mostly unrelated, but I did not find any check for this in the old implementation either 😅 We should probably remove it, unless I missed it somewhere (not even sure how we would check for this, or how a pull would be using the image – for build, I guess with the classic builder this would fall under the "check if a running container is using it"?).
cc @thaJeztah
6c29ecb to
5200291
Compare
corhere
left a comment
There was a problem hiding this comment.
It's time to dig into the nuances of error handling
03945c2 to
0678c9d
Compare
corhere
left a comment
There was a problem hiding this comment.
LGTM! I'm not too thrilled about the number of ImageService().List calls (containerd does a full table scan for every list call, even with filters) but we can always iterate on the implementation later if it is found to be a performance issue in practice.
|
I agree re: performance! I can think of a few changes we can make to reduce the no. of I've addressed the last nits for now, and I have another branch I'll open a PR for soon with some of those refactors (although that still needs a good amount of work for readability – lmk if you have any good ideas!). |
0678c9d to
95c3f29
Compare
Ports over all the previous image delete logic, such as:
- Introduce `prune` and `force` flags
- Introduce the concept of hard and soft image delete conflics, which represent:
- image referenced in multiple tags (soft conflict)
- image being used by a stopped container (soft conflict)
- image being used by a running container (hard conflict)
- Implement delete logic such as:
- if deleting by reference, and there are other references to the same image, just
delete the passed reference
- if deleting by reference, and there is only 1 reference and the image is being used
by a running container, throw an error if !force, or delete the reference and create
a dangling reference otherwise
- if deleting by imageID, and force is true, remove all tags (otherwise soft conflict)
- if imageID, check if stopped container is using the image (soft conflict), and
delete anyway if force
- if imageID was passed in, check if running container is using the image (hard conflict)
- if `prune` is true, and the image being deleted has dangling parents, remove them
This commit also implements logic to get image parents in c8d by comparing shared layers.
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
95c3f29 to
cad9713
Compare
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM; left some thoughts/nits, but I think they can be looked at in a follow-up
| return false, err | ||
| } | ||
| for _, ref := range refs { | ||
| if !isDanglingImage(ref) && ref.Name != img.Name { |
There was a problem hiding this comment.
Micro-nit/optimizatino (no blocker);
| if !isDanglingImage(ref) && ref.Name != img.Name { | |
| if ref.Name != img.Name && !isDanglingImage(ref) { |
There was a problem hiding this comment.
isDanglingImage() is a straightforward string compare, so either order should have about the performance, no?
There was a problem hiding this comment.
Yeah, it'd be very minimal 😂 isDanglingImage() does slightly more but definitely no issue
| func (i *ImageService) isSingleReference(ctx context.Context, img images.Image) (bool, error) { | ||
| refs, err := i.client.ImageService().List(ctx, "target.digest=="+img.Target.Digest.String()) | ||
| if err != nil { | ||
| return false, err |
There was a problem hiding this comment.
Not a blocker, because this is something we need to look at in a wider scope; this would return a containerd error; we may have to translate those to one of our errdefs definitions.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| i.LogImageEvent(imgID.String(), imgID.String(), "untag") |
There was a problem hiding this comment.
Should one of these be the image name? https://github.com/moby/moby/blob/3f7fde76c2e4e7c234e85c9a8634e2d7639f4f6d/daemon/containerd/image_events.go#L10C1-L11
| i.LogImageEvent(imgID.String(), imgID.String(), "untag") | |
| i.LogImageEvent(imgID.String(), img.Name, "untag") |
(or the familiar string from below)?
edit: ❓ looks like the non-containerd implementation does the same, so no need to change (but we should look "why")
There was a problem hiding this comment.
Yeah! I wondered the same (but did this over a while so I forgot to call it out when opening the PR) but yes, I left it like this since this was the non-containerd implementation as well. I'll leave it for a follow up to change both/discuss any potential impacts of doing so.
| return nil, err | ||
| } | ||
| i.LogImageEvent(imgID.String(), imgID.String(), "untag") | ||
| records := []types.ImageDeleteResponseItem{{Untagged: reference.FamiliarString(reference.TagNameOnly(parsedRef))}} |
There was a problem hiding this comment.
Remind me; I was looking into some of these conversions; we have some very silly code-paths where we convert "friendly" name to "fully-qualified" and back (and normalise again).
We could consider defining refString once, as most "happy" code-paths need it (only in case of an error we discard it);
refString := reference.FamiliarString(reference.TagNameOnly(parsedRef))| using := func(c *container.Container) bool { | ||
| return c.ImageID == imgID | ||
| } | ||
| ctr := i.containers.First(using) |
There was a problem hiding this comment.
Could inline the function here, which makes it clearer it's only used once.
| using := func(c *container.Container) bool { | |
| return c.ImageID == imgID | |
| } | |
| ctr := i.containers.First(using) | |
| ctr := i.containers.First( func(c *container.Container) bool { | |
| return c.ImageID == imgID | |
| }) |
| type imageDeleteConflict struct { | ||
| hard bool | ||
| used bool | ||
| reference string |
There was a problem hiding this comment.
We should look at synchronising this code with the old implementation (I see that one uses imgID here instead of a string).
There was a problem hiding this comment.
I initially had it like that, and then changed it so we could reuse it here:
https://github.com/moby/moby/pull/45427/files#diff-5d5a46c403a4d9666b17b889c12655b3c1539ab462cc38f3fd2e43cf99749b43R95
re @corhere's comment: #45427 (comment)
Would probably be good to make the same change in the old implementation
| return fmt.Sprintf("conflict: unable to delete %s (%s) - %s", idc.reference, forceMsg, idc.message) | ||
| } | ||
|
|
||
| func (imageDeleteConflict) Conflict() {} |
There was a problem hiding this comment.
Should this be a pointer as well? I noticed my IDE is complaining about mixed pointer and non-pointer receivers;
| func (imageDeleteConflict) Conflict() {} | |
| func (*imageDeleteConflict) Conflict() {} |
There was a problem hiding this comment.
It doesn't really matter in practice especially since this method is never called. It would technically make a difference for whether or not (*imageDeleteConflict)(nil).Conflict() nil-deref panics, but that's about it.
Why is your IDE complaining about mixing pointer and non-pointer receivers? Mixing is a perfectly reasonable thing to do to distinguish methods which mutate the receiver from "const" methods.
There was a problem hiding this comment.
Yeah, that's why I removed it initially. I don't use an IDE (just gopls and golang-ci-langserver) but maybe it complains not because there are different methods with mixed pointer/non-pointer receivers but because there are other implementators of that interface with pointer/non-pointer receivers.
| // Implements the error interface. | ||
| type imageDeleteConflict struct { | ||
| hard bool | ||
| used bool |
There was a problem hiding this comment.
Looks like used is not used anywhere in the containerd implementation.
- Related
--no-prune) option for image delete #43849- What I did
Ports over all the previous image delete logic, such as:
pruneandforceflagsdelete the passed reference
by a running container, throw an error if !force, or delete the reference and create
a dangling reference otherwise
delete anyway if force
pruneis true, and the image being deleted has dangling parents, remove themAlso implements logic to get image parents in c8d by comparing shared layers, like what @vvoland did in #45265, so that we can prune dangling parents on image delete.
- How I did it
With neovim! 👀
- How to verify it
Pull and delete a ton of different images, tag them and use them and try to delete them 😅
Compare the results, and see if they make sense.
Some examples from me:
(complains if it's the only reference and a container is using it, allows deleting if there is another reference, forcing leaves dangling image)
(complains if image referenced in multiple repos, removes all and deletes with force)
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)