Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix issue in nerdctl rm. #57

Merged
merged 2 commits into from
Jan 12, 2021
Merged

Fix issue in nerdctl rm. #57

merged 2 commits into from
Jan 12, 2021

Conversation

shishir-a412ed
Copy link
Contributor

Signed-off-by: Shishir Mahajan smahajan@roblox.com

@shishir-a412ed
Fix issue in nerdctl rm.
14c3eaf 
Signed-off-by: Shishir Mahajan <smahajan@roblox.com>
@shishir-a412ed
Copy link
Contributor Author

shishir-a412ed commented Jan 11, 2021 •
edited

Issue:

Currently (on master HEAD), if I have a running, paused and stopped container.
And I try to do docker rm $(docker ps -aq)

alias docker="nerdctl"

root@vagrant:~/go/src/github.com/nerdctl# docker ps -a
CONTAINER ID    IMAGE                             COMMAND        CREATED           STATUS                         PORTS    NAMES
03545c1d0c52    docker.io/library/ubuntu:18.04    "sleep 600"    26 seconds ago    Paused                                  c2
6d459455950b    docker.io/library/ubuntu:18.04    "sleep 600"    21 seconds ago    Stopped (137) 3 seconds ago             c3
86fbc4265da1    docker.io/library/ubuntu:18.04    "sleep 600"    30 seconds ago    Running                                 c1
root@vagrant:~/go/src/github.com/nerdctl# docker rm $(docker ps -aq)
WARN[0000] failed to remove container "03545c1d0c52b9daf2add34bd94b96f3352288243108bf4ebfcec76db1f7d2e6"  error="cannot remove a paused container 03545c1d0c52b9daf2add34bd94b96f3352288243108bf4ebfcec76db1f7d2e6"
WARN[0000] failed to remove container "03545c1d0c52b9daf2add34bd94b96f3352288243108bf4ebfcec76db1f7d2e6"  error="cannot remove a paused container 03545c1d0c52b9daf2add34bd94b96f3352288243108bf4ebfcec76db1f7d2e6"
FATA[0000] cannot remove a paused container 03545c1d0c52b9daf2add34bd94b96f3352288243108bf4ebfcec76db1f7d2e6

It will error out if it is not able to remove one of the containers e.g. In this case, it is not able to remove the Paused container since no force flag was set.

The correct behavior should be to remove the containers it can (Stopped), and print errors on the containers it can't (Paused, Running).

This PR fixes this issue.

root@vagrant:~/go/src/github.com/nerdctl# docker ps -a
CONTAINER ID    IMAGE                             COMMAND        CREATED          STATUS                         PORTS    NAMES
03545c1d0c52    docker.io/library/ubuntu:18.04    "sleep 600"    4 minutes ago    Paused                                  c2
6d459455950b    docker.io/library/ubuntu:18.04    "sleep 600"    4 minutes ago    Stopped (137) 3 minutes ago             c3
86fbc4265da1    docker.io/library/ubuntu:18.04    "sleep 600"    4 minutes ago    Running                                 c1
root@vagrant:~/go/src/github.com/nerdctl# docker rm $(docker ps -aq)
Error response from daemon: You cannot remove a paused container 03545c1d0c52b9daf2add34bd94b96f3352288243108bf4ebfcec76db1f7d2e6. Unpause the container before attempting removal or force remove
6d459455950b
Error response from daemon: You cannot remove a running container 86fbc4265da1af30732ae8821e3d6e5e08e5cfb760485239db30957f7bd98223. Stop the container before attempting removal or force remove

/cc @AkihiroSuda

AkihiroSuda
AkihiroSuda reviewed Jan 12, 2021
View reviewed changes
rm.go Outdated
msg = "Stop"
}

_, err := fmt.Fprintf(clicontext.App.Writer, "Error response from daemon: You cannot remove a %v container %v. %s the container before attempting removal or force remove\n", status.Status, id, msg)
Copy link
Member

Choose a reason for hiding this comment

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

Not really "Error response from daemon". I don't think the error message needs to 100% correspond to Docker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it doesn't need to be 100% matching to docker 🙂
I felt it just makes it easier for someone who is transitioning from docker CLI to nerdctl

Either way, let me know what do you want me to change it to? and I will update the message.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can just remove ""Error response from daemon: " part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

AkihiroSuda
AkihiroSuda reviewed Jan 12, 2021
View reviewed changes
rm.go Outdated
@@ -144,9 +141,18 @@ func removeContainer(ctx context.Context, client *containerd.Client, id string,
}
default:
if !force {
return errors.Errorf("cannot remove a %v container %v", status.Status, id)
var msg string
if status.Status == containerd.Paused || status.Status == containerd.Running {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move these comparison to switch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@shishir-a412ed
Move to switch and update error message.
cf7cc17 
Signed-off-by: Shishir Mahajan <smahajan@roblox.com>
AkihiroSuda
AkihiroSuda approved these changes Jan 12, 2021
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit cb64cfe into containerd:master Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@shishir-a412ed @AkihiroSuda