Fix rmi bug when checking containers with deleted image
#1998
Conversation
djdongjin
changed the title
rmi err checking containers with deleted image
djdongjin
changed the title
rmi err checking containers with deleted imagermi bug when checking containers with deleted image
|
okay seems docker and nerdctl have different behavior :) If passing image id, then docker prints errors and doesn't delete images with running/paused containers. If passing image names, docker just untag images: # d = docker
$ d ps -a
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
f2cacb0af911 ubuntu "sleep 1h" 4 seconds ago Up 3 seconds musing_hellman
c19d9113d612 alpine "sleep 1h" 28 seconds ago Up 28 seconds (Paused) compassionate_hugle
$ d rmi -f $(d images -q)
Error response from daemon: conflict: unable to delete 58db3edaf2be (cannot be forced) - image is being used by running container f2cacb0af911
Error response from daemon: conflict: unable to delete 042a816809aa (cannot be forced) - image is being used by running container c19d9113d612
$ d images
REPOSITORY TAG IMAGE ID CREATED SIZE
ubuntu latest 58db3edaf2be 2 weeks ago 77.8MB
alpine latest 042a816809aa 4 weeks ago 7.05MB
$ d rmi -f ubuntu
Untagged: ubuntu:latest
Untagged: ubuntu@sha256:9a0bdde4188b896a372804be2384015e90e3f84906b750c1a53539b585fbbe7f
$ d images
REPOSITORY TAG IMAGE ID CREATED SIZE
<none> <none> 58db3edaf2be 2 weeks ago 77.8MB
alpine latest 042a816809aa 4 weeks ago 7.05MBWill fix #1997 first. |
|
|
| base.Cmd("rmi", "-f", testutil.CommonImage).AssertOK() | ||
|
|
||
| base.Cmd("images").AssertNoOut(testutil.CommonImage) | ||
| base.Cmd("images").AssertNoOut(testutil.ImageRepo(testutil.CommonImage)) |
There was a problem hiding this comment.
Is it possible to change this to test without force maybe by building a new image or retagging? I think we should not prune the new image as it leads to flakiness.
There was a problem hiding this comment.
Do you mean we should avoid prune/rmi -f common images (e.g. testutil.CommonImage) and only rmi -f a custom name (either from tag or build)? Both here and other integration tests
There was a problem hiding this comment.
Yep (also other integ tests; but not in this PR).
I think we should start with this PR or when we write new tests should be fine.
There was a problem hiding this comment.
tested locally. using tag makes it incompatible with docker.
If there is a container (assume exited) from an image, docker rmi image will succeed if image is a tag (e.g. foobar, so foobar is untagged) but fail if image is the real image name (e.g., alpine)
djdongjin
force-pushed
the
fix-1988
branch
4 times, most recently
from
08299d8 to
fae82fe
Compare
| var errs []string | ||
| var fatalErr bool | ||
| for _, req := range args { | ||
| n, err := walker.Walk(ctx, req) | ||
| if err != nil { | ||
| fatalErr = true | ||
| } | ||
| if err == nil && n == 0 { | ||
| err = fmt.Errorf("no such image: %s", req) | ||
| } | ||
| if err != nil { | ||
| errs = append(errs, err.Error()) | ||
| } | ||
| } | ||
|
|
||
| if len(errs) > 0 { | ||
| msg := fmt.Sprintf("%d errors:\n%s", len(errs), strings.Join(errs, "\n")) | ||
| if !options.Force || fatalErr { | ||
| return errors.New(msg) | ||
| } | ||
| logrus.Error(msg) |
There was a problem hiding this comment.
This is to exactly match docker's error handling behavior (for each err, if not IsErrNotFound, then it's fatal; if there is any fatal error, return a joined error, else log error and return nil).
If using WalkAll, an edge case will be inconsistent with docker: if all errors are IsErrNotFound (e.g. docker|nerdctl rmi -f afsd afdafaf aff), docker return nil and nerdctl return a joined err. TBH I don't feel it makes much sense to distinguish the specific edge case in docker...
djdongjin
force-pushed
the
fix-1988
branch
2 times, most recently
from
9998e69 to
5bb5ad0
Compare
djdongjin
force-pushed
the
fix-1988
branch
2 times, most recently
from
2b75971 to
dba1473
Compare
Signed-off-by: Jin Dong <[email protected]>
Fix #1988 (
and part of #1997fixed in a separate PR).Previous: when
rmichecksrunningImagesby containers, return directly if there is error.Now: skip this container (since no image returned from it).
Signed-off-by: Jin Dong [email protected]