Remove image via storage if a buildah container is associated #522

TomSweeneyRedHat · 2018-03-20T19:09:28Z

Signed-off-by: TomSweeneyRedHat tsweeney@redhat.com

When a removal of an image returns ErrImageUsedByContainer, rather than raising an error immediately we get the image from the containers/storage and see if we can remove containers from there for that image (generally those containers are put into play by Buildah) and then try again to remove the image from Podman.

mheon · 2018-03-20T19:23:41Z

cmd/podman/rmi.go

We already have a storage.Store in libpod, don't need this

mheon · 2018-03-20T19:23:52Z

cmd/podman/rmi.go

We have the image already, can't we pass this in?

mheon · 2018-03-20T19:24:45Z

cmd/podman/rmi.go

We don't need this logic, the standard logic in our image library for removing images should work fine

mheon · 2018-03-20T19:28:17Z

I think we really just need RunningContainers and RemoveContainers here. We can run RunningContainers unconditionally before we try and remove the image but after we remove Podman containers. If it returns that containers are present and --force is present, we use the RemoveContainers code to remove all non-Podman containers associated with the image. If it returns that containers are present and force is not set, we warn that non-Podman containers are using the image, and you should use --force if you want it gone. Then we continue with our removal logic. How does this sound?

baude · 2018-03-20T19:56:51Z

I agree with the proposed approach though part of me would prefer it be an import of buildah code ... nevertheless, I think that approach is spot on.

TomSweeneyRedHat · 2018-03-20T21:43:56Z

@baude @mheon thanks for the looksee and suggestions. I'll fine tune.

TomSweeneyRedHat · 2018-03-24T20:08:36Z

cmd/podman/rmi.go

I was waffling about keeping these following three functions in here or moving them into libpod/runtime_img.go

I'd go ahead and move them. I am trying to keep the store entirely inside libpod, so directly opening a store and deleting outside of libpod is really undesirable. If we can make these part of runtime.RemoveImage, instead of the code here calling it, I think that'd be fine, and we wouldn't have to leak the store outside of libpod.

mheon · 2018-03-26T15:01:08Z

libpod/runtime_img.go

I don't think we need this. We can just get storageContainers for the image and return an error if containers exist or remove them if --force is set. We don't need to do it in an error handling block, that makes things a lot harder to follow.

I'm not sure exactly what you're suggesting here. After line 129 are you saying I should just try getting the storage containers and then error if the image has containers and no force, other wise remove? When we get to this point we've already tried removing the image and that failed due to it being a storage image and a container was associated. I might be able to move the storage image handling that I've below into image.Remove() if that would be preferable. I'll play with that and hit me up on IRC in the morn if I'm off base.

Sorry, should have been more clear - I mean before 129, we can do the check if the container is mounted before the actual remove happens, thus removing the need to check errors.Cause() and perform complicated handling. This might make our fastpath a bit slower (we do a check unconditionally instead of conditionally), but will improve code flow and readability. We can always go back and make things faster later if need be.

This chunk has been reworked and greatly reduced. I still have the error check, but I think it makes more sense now. Cand/will adjust after the next review if you think appropriate.

mheon · 2018-03-26T15:01:27Z

libpod/image/image.go

We should not need this. I really do not want to return a Store anywhere the user can get one.

If you or @baude have a suggestion around it, I'm happy to do it. I tried using image.imageruntime.store in the call in runtime_img.go below but got:

github.com/projectatomic/libpod/libpod _output/src/github.com/projectatomic/libpod/libpod/runtime_img.go:135:65: image.imageruntime undefined (cannot refer to unexported field or method imageruntime)

Thought exposing the function would be better. However if I move that code to handle the storage containers into image.go as talked about below, that might solve that problem.

The image runtime's store is guaranteed to be the same as the normal runtime's store - you should just be able to runtime.store instead

Agreed, i went to great pains to avoid this.

ack, I was over complicating it, I can just do r.store in runtime_img.go and it gets me what I need. Thought I'd to pull it from each image and that wasn't resolving.

mheon · 2018-03-26T15:02:01Z

libpod/runtime_img.go

No printing in libpod - return this as an error

will do....

mheon · 2018-03-26T15:04:08Z

cmd/podman/rmi.go

Why is this necessary? Can we not find the image by its ID?

I did this for the case when a image was tagged. If I do:

podman pull alpine podman tag alpine tg podman images REPOSITORY TAG IMAGE ID CREATED SIZE tg latest 3fd9065eaf02 2 months ago 4.41MB docker.io/library/alpine latest 3fd9065eaf02 2 months ago 4.41MB

Both "images" had the same ID, the only way I could differentiate them was to go by their repo.

baude · 2018-03-27T13:13:21Z

cmd/podman/rmi.go

@TomSweeneyRedHat im confused by this. I'll catch you on IRC for follow-up

@baude and I worked through this before scrum. The issue I was running into was deleting container from an image created by Buildah when the image had a tag added to it. The code as I had it here worked around that, however @baude pointed to a tweak of a if statement down lower that let me remove this code and still get the containers removed. This has all been removed.

baude · 2018-03-27T13:19:04Z

libpod/runtime_img.go

I would prefer to see this and subsequent functions take a *image.Image ... then you have the store and storage.Image already. I say that assuming you at one point already had an *image.Image

reworking. I've passed image.Image to the rmStorageContainers and just the id to one other function rather than the whole image.

baude · 2018-03-27T13:20:10Z

libpod/runtime_img.go

Ill hit you up on irc for clarification on this func as well.

We only talked briefly about this, but I've removed this as you indicated elsewhere I can get the info here from the runtime (r).

TomSweeneyRedHat

New Patch up. I want to test a bit more and just spotted a couple added blank lines. Will test and clean. If you want to review now, great, if not I'll be getting another patch up later today or tomorrow morn.

mheon · 2018-03-27T19:28:23Z

cmd/podman/rmi.go

Shouldn't just call out buildah here - could also be CRI-O.

Ideally, should mention containers/storage specifically, could be a program using c/storage we didn't write

Couple of things here. First, if the err is no ErrImageUserByContainer, isn't the error lost? And second, should your error message also contain the contents of err?

I've changed the error message, bumped it a bit from what @mheon suggested. I don't think the error is lost here, we save it as we do currently in lastError. This only prints out extra verbiage when we have a container tied to the image. The error that prints I didn't think added a lot here, and the error message is currently pretty long so I didn't include it. If you've a strong opinion on adding it I can.

mheon · 2018-03-27T19:30:00Z

libpod/runtime_img.go

Are we retrying on the assumption that the containers using the image were removed? If so, can we get a comment to that effect?

Also, instead of setting error, it's probably better to just return here and below in the else

is there any chance that err might not be ErrImageUsedByContainer ?

I've added a comment.

I think we want to capture the err from this second remove and return it if there's an error. But if you really think it's better, I can just let the original err be returned. As far as returning the errStorage error I was on the fence about returning that or not, but thought it would give a better clue if there was an error. I'll keep them both for now, holler in he morn if you'd like me to remove them.

I think there's a very small chance that the err would be something other then ErrImageUsedByContainer. But if it is, we'll just get the same error from the second image remove and will continue on down the error path. But for the vast majority, it should be the image in use error.

baude · 2018-03-27T19:47:35Z

libpod/runtime_img.go

good call, done.

baude · 2018-03-27T19:47:55Z

libpod/runtime_img.go

if err := remove.... ; err != nil

yep, found a second one too, thx.

mheon · 2018-03-27T22:35:48Z

@TomSweeneyRedHat can you rebase on latest master? The test failures should be fixed

TomSweeneyRedHat · 2018-03-28T23:09:33Z

#564 should hopefully fix up the test errors here.

rhatdan · 2018-03-29T01:40:41Z

@TomSweeneyRedHat Rebase and try.

rhatdan · 2018-03-30T09:20:04Z

cmd/podman/rmi.go

nit: Not crazy about mentioning other tools here since we would love to see other tools start using containers/storage and then this message can become dated.

I hear you, but my main concern that if a customer got an error that there's a "container associated with containers/storage" would they know enough to check Buildah/CRI-O? I've tried to open it up a bit by adding etc.

rhatdan · 2018-03-30T09:22:46Z

libpod/runtime_img.go

Remove random whitespace

Done, don't know how I managed that one.

rhatdan · 2018-03-30T09:23:41Z

A couple of nits, but LGTM

TomSweeneyRedHat

New patch pushed.

TomSweeneyRedHat · 2018-03-30T15:39:52Z

cmd/podman/rmi.go

I hear you, but my main concern that if a customer got an error that there's a "container associated with containers/storage" would they know enough to check Buildah/CRI-O? I've tried to open it up a bit by adding etc.

TomSweeneyRedHat · 2018-03-30T15:40:59Z

libpod/runtime_img.go

Done, don't know how I managed that one.

rhatdan · 2018-04-04T20:58:52Z

@mheon @baude @umohnani8 PTAL
LGTM

mheon · 2018-04-04T21:08:07Z

libpod/runtime_img.go

Shouldn't we throw ErrImageUsedByContainer here if len(ctrIDs) > 0 && !force?

I was relying on the second image.Remove(force) to provide that error at line 132. However it probably does make more sense to throw the error here and not invoke that image.Remove() again as we know it will fail in this instance. Good thought.

mheon · 2018-04-04T21:08:27Z

libpod/runtime_img.go

Extra whitespace

Fixed, found another rogue one and zapped that too.

umohnani8 · 2018-04-05T14:39:06Z

LGTM

Signed-off-by: TomSweeneyRedHat <tsweeney@redhat.com>

mheon · 2018-04-05T15:44:09Z

Changes LGTM
@rh-atomic-bot r+

rh-atomic-bot · 2018-04-05T15:44:09Z

📌 Commit 21b7c40 has been approved by mheon

rh-atomic-bot · 2018-04-05T15:44:14Z

⌛ Testing commit 21b7c40 with merge 54bd0ae...

TomSweeneyRedHat · 2018-04-05T15:44:46Z

@baude @rhatdan @mheon I've just posted a new patch. I fixed up @mheon's comments and while testing noted an error message in rmi.go that would be better served with the image.ID() being printed.

TomSweeneyRedHat · 2018-04-05T15:49:38Z

Quick test output:

# buildah from alpine
Getting image source signatures
Copying blob sha256:ff3a5c916c92643ff77519ffa742d3ec61b7f591b6b7504599d95a4a41134e28
 1.97 MiB / 1.97 MiB [=====================================================] 21s
Copying config sha256:3fd9065eaf02feaf94d68376da52541925650b81698c53c6824d92ff63f98353
 1.48 KiB / 1.48 KiB [======================================================] 0s
Writing manifest to image destination
Storing signatures
alpine-working-container

# podman pull busybox
Trying to pull docker.io/busybox:latest...Getting image source signatures
Copying blob sha256:f70adabe43c0cccffbae8785406d490e26855b8748fc982d14bc2b20c778b929
 706.22 KB / 706.22 KB [====================================================] 0s
Copying config sha256:8ac48589692a53a9b8c2d1ceaa6b402665aa7fe667ba51ccc03002300856d8c7
 1.46 KB / 1.46 KB [========================================================] 0s
Writing manifest to image destination
Storing signatures
8ac48589692a53a9b8c2d1ceaa6b402665aa7fe667ba51ccc03002300856d8c7

# podman tag alpine tg

# podman ps -a

# podman images
REPOSITORY                  TAG      IMAGE ID       CREATED        SIZE
tg                          latest   3fd9065eaf02   2 months ago   4.41MB
docker.io/library/alpine    latest   3fd9065eaf02   2 months ago   4.41MB
docker.io/library/busybox   latest   8ac48589692a   5 hours ago    1.36MB

# podman rmi -a
8ac48589692a53a9b8c2d1ceaa6b402665aa7fe667ba51ccc03002300856d8c7
unable to delete 3fd9065eaf02feaf94d68376da52541925650b81698c53c6824d92ff63f98353 (must force) - image is referred to in multiple tags

# podman images
REPOSITORY                 TAG      IMAGE ID       CREATED        SIZE
docker.io/library/alpine   latest   3fd9065eaf02   2 months ago   4.41MB
tg                         latest   3fd9065eaf02   2 months ago   4.41MB

# podman rmi tg
Untagged: tg:latest

# podman rmi -a
A container associated with containers/storage, i.e. via Buildah, CRI-O, etc., may be associated with this image: 3fd9065eaf02
image is in use by a container

# podman rmi -a -f
3fd9065eaf02feaf94d68376da52541925650b81698c53c6824d92ff63f98353

rh-atomic-bot · 2018-04-05T16:04:11Z

☀️ Test successful - status-papr
Approved by: mheon
Pushing 54bd0ae to master...

TomSweeneyRedHat · 2018-04-07T19:01:03Z

Addresses #174

mheon reviewed Mar 20, 2018

View reviewed changes

cmd/podman/rmi.go Outdated

Copy link

Member

mheon Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a storage.Store in libpod, don't need this

mheon reviewed Mar 20, 2018

View reviewed changes

cmd/podman/rmi.go Outdated

Copy link

Member

mheon Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the image already, can't we pass this in?

mheon reviewed Mar 20, 2018

View reviewed changes

cmd/podman/rmi.go Outdated

Copy link

Member

mheon Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this logic, the standard logic in our image library for removing images should work fine

TomSweeneyRedHat mentioned this pull request Mar 24, 2018

Remove image via Buildah if a buildah container is associated #519

Closed

TomSweeneyRedHat changed the title ~~WIP - Remove image via storage if a buildah container is associated~~ Remove image via storage if a buildah container is associated Mar 24, 2018

TomSweeneyRedHat commented Mar 24, 2018

View reviewed changes

mheon reviewed Mar 26, 2018

View reviewed changes

baude reviewed Mar 27, 2018

View reviewed changes

TomSweeneyRedHat commented Mar 27, 2018

View reviewed changes

mheon reviewed Mar 27, 2018

View reviewed changes

baude reviewed Mar 27, 2018

View reviewed changes

rhatdan reviewed Mar 30, 2018

View reviewed changes

TomSweeneyRedHat commented Mar 30, 2018

View reviewed changes

mheon reviewed Apr 4, 2018

View reviewed changes

Remove image via storage if a buildah container is associated

21b7c40

Signed-off-by: TomSweeneyRedHat <tsweeney@redhat.com>

rh-atomic-bot closed this in 54bd0ae Apr 5, 2018

TomSweeneyRedHat deleted the dev/tsweeney/storagermi branch April 7, 2018 18:56

TomSweeneyRedHat mentioned this pull request Apr 7, 2018

Can't remove image created by Buildah #174

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 27, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023

Remove image via storage if a buildah container is associated #522

Remove image via storage if a buildah container is associated #522

Uh oh!

Conversation

TomSweeneyRedHat commented Mar 20, 2018 • 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!

mheon commented Mar 20, 2018

Uh oh!

baude commented Mar 20, 2018

Uh oh!

TomSweeneyRedHat commented Mar 20, 2018

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!

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!

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!

TomSweeneyRedHat left a comment

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!

TomSweeneyRedHat commented Mar 20, 2018 •

edited

Loading