containerd integration: Implement ImageDelete for containerd by thaJeztah · Pull Request #43820 · moby/moby

thaJeztah · 2022-07-15T13:07:29Z

~~depends on add support for image inspect with containerd-integration #43818~~
upstreaming Implement ImageDelete for containerd rumpl/moby#11

- What I did

Implemented image deletion for containerd

thaJeztah · 2022-07-21T09:25:05Z

daemon/containerd/image_delete.go

+	// FIXME(thaJeztah): implement ImageDelete "force"
+	// FIXME(thaJeztah): implement ImageDelete "prune"


Let me create tracking issues for these

Created:

containerd integration: implement "prune" (--no-prune) option for image delete #43849

containerd integration: implement "force" option for image delete #43850

thaJeztah · 2022-07-21T18:40:36Z

daemon/containerd/image_delete.go

-	panic("not implemented")
+func (i *ImageService) ImageDelete(ctx context.Context, imageRef string, force, prune bool) ([]types.ImageDeleteResponseItem, error) {
+	// TODO(thaJeztah): implement ImageDelete "force" options; see https://github.com/moby/moby/issues/43850
+	// TODO(thaJeztah): implement ImageDelete "prune" options; see https://github.com/moby/moby/issues/43849


Per discussion in the maintainers meeting; we need to add more tracking issues for; partial IDs etc. (some of that logic is implemented in GetImage() as part of #43818)

thaJeztah · 2022-07-26T11:13:45Z

@corhere @tianon @rumpl PTAL

corhere · 2022-07-28T16:39:23Z

daemon/containerd/image_delete.go

+	// TODO(thaJeztah): implement ImageDelete "force" options; see https://github.com/moby/moby/issues/43850
+	// TODO(thaJeztah): implement ImageDelete "prune" options; see https://github.com/moby/moby/issues/43849
+
+	records := []types.ImageDeleteResponseItem{}


https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices

Hm, yeah, I don't always agree with that rule ~~(sometimes it's really more convenient to initialise it if later on we need to append()~~ (edit: ignore me on that one), and make(...) is a bit cumbersome (or sometimes overdoing it).

That said; it looks like (at least currently) it'll only ever create a single instance, so I don't think we need this variable at all; we can just return []types.ImageDeleteResponseItem{}{d} at the end of the function.

sometimes it's really more convenient to initialise it if later on we need to append()

Hmm? append() works on nil slices, same as non-nil but empty slices. You might be thinking of maps, where assigning to a nil map panics.

Yes, I just edited, indeed had maps in mind 🙈

corhere · 2022-07-28T16:40:30Z

daemon/containerd/image_delete.go

+	ref := reference.TagNameOnly(parsedRef)
+
+	if err := i.client.ImageService().Delete(ctx, ref.String(), images.SynchronousDelete()); err != nil {
+		return []types.ImageDeleteResponseItem{}, err


How come an empty slice is returned here but the error-return for ParseNotmalizedNamed (line 52) returns nil?

Good catch; at a glance, I think can be just nil as well, unless there's some expectation somewhere (nil vs nil type)? Overall the returned value shouldn't be used if there's an error.

Let me have a look at that.

corhere · 2022-07-28T16:44:56Z

daemon/images/image_prune.go

 				for _, ref := range refs {
-					imgDel, err := i.ImageDelete(ref.String(), false, true)
+					imgDel, err := i.ImageDelete(ctx, ref.String(), false, true)
 					if imageDeleteFailed(ref.String(), err) {


Looks like imageDeleteFailed() needs to be updated so it doesn't log a spurious warning when the context is canceled.

corhere · 2022-07-28T20:54:28Z

daemon/images/image_prune.go

 	case err == nil:
 		return false
-	case errdefs.IsConflict(err):
+	case errdefs.IsConflict(err), errors.Is(err, context.Canceled):


Suggested change

case errdefs.IsConflict(err), errors.Is(err, context.Canceled):

case errdefs.IsConflict(err), errors.Is(err, context.Canceled), errors.Is(err, context.DeadlineExceeded):

Annoyingly, you need to enumerate all the possible context errors.

LOL; I actually had the DeadlineExceeded, and started to think if that one would be one to actually log, so removed it; let me add it back 😂

And it's back (it was literally CMD+Z - last edit I made 😂 )

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2022-08-02T12:24:35Z

@AkihiroSuda @tonistiigi PTAL

containerd integration: Implement ImageDelete for containerd Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

thaJeztah added status/2-code-review area/images Image Distribution labels Jul 15, 2022

thaJeztah added this to the v-next milestone Jul 15, 2022

thaJeztah force-pushed the image_delete branch 2 times, most recently from 3473542 to 224b540 Compare July 18, 2022 18:57

thaJeztah marked this pull request as ready for review July 19, 2022 08:22

thaJeztah added the containerd-integration Issues and PRs related to containerd integration label Jul 21, 2022

thaJeztah commented Jul 21, 2022

View reviewed changes

This was referenced Jul 21, 2022

containerd integration: implement "prune" (--no-prune) option for image delete #43849

Closed

containerd integration: implement "force" option for image delete #43850

Closed

thaJeztah force-pushed the image_delete branch from 224b540 to 81ba1b9 Compare July 21, 2022 09:33

thaJeztah commented Jul 21, 2022

View reviewed changes

This was referenced Jul 21, 2022

containerd integration: add support for image delete using image (short)ID #43854

Closed

containerd integration: image delete should send image "untag" events and prometheus counters #43855

Closed

rumpl approved these changes Jul 26, 2022

View reviewed changes

corhere requested changes Jul 28, 2022

View reviewed changes

thaJeztah force-pushed the image_delete branch from 81ba1b9 to dd10417 Compare July 28, 2022 20:51

corhere approved these changes Jul 28, 2022

View reviewed changes

Implement ImageDelete for containerd

26c6544

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the image_delete branch from dd10417 to 26c6544 Compare July 28, 2022 20:56

cpuguy83 approved these changes Aug 3, 2022

View reviewed changes

cpuguy83 merged commit 7e8df0e into moby:master Aug 3, 2022

thaJeztah deleted the image_delete branch August 3, 2022 22:49

crazy-max pushed a commit to crazy-max/moby that referenced this pull request Sep 29, 2022

Merge pull request moby#43820 from thaJeztah/image_delete

c07becb

containerd integration: Implement ImageDelete for containerd Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

rumpl mentioned this pull request Apr 4, 2023

containerd integration: image delete should send prometheus counters #45268

Closed

		// FIXME(thaJeztah): implement ImageDelete "force"
		// FIXME(thaJeztah): implement ImageDelete "prune"

	case errdefs.IsConflict(err), errors.Is(err, context.Canceled):
	case errdefs.IsConflict(err), errors.Is(err, context.Canceled), errors.Is(err, context.DeadlineExceeded):

Conversation

thaJeztah commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jul 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thaJeztah commented Jul 15, 2022 •

edited

Loading

thaJeztah Jul 28, 2022 •

edited

Loading