Make 'podman rm' exit with 125 if it had a bogus & a running container #2632
Conversation
|
Hi @debarshiray. Thanks for your PR. I'm waiting for a containers or openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Code looks good, tests need a bit of tweaking. |
|
Hm. So the logic here is exit code 0 if all containers exist, 1 if no containers exist, 125 if we have a mix of exist/notexist? SGTM, but I'd like a signoff from @rhatdan since he originally requested the exit 1 change |
| @@ -195,5 +195,10 @@ func rmCmd(c *cliconfig.RmValues) error { | |||
| exitCode = 1 | |||
| } | |||
| } | |||
|
|
|||
| if failureCnt > 0 { | |||
There was a problem hiding this comment.
This is the default, why are you setting it?
If an err is returned and failureCnt == 0 then it is 1,
else if err is returned exitCode=125
There was a problem hiding this comment.
Ok so this fixed the issue where you have a non existing container and one of the other containers causes an error.
Could you add a test for this situation.
There was a problem hiding this comment.
This is a continuation of commit 85db895 which was meant for #2539.
Getting the list of containers and deleting them are two different steps which can throw different kinds of errors. I guess failureCnt is meant to keep track of errors that aren't libpod.ErrNoSuchCtr, right? For the case of a bogus and a running container, the first step will throw libpod.ErrNoSuchCtr, and we set exitCode to 1 and failureCnt stays 0. Then later when we hit another error due to the state of the container, failureCnt is incremented but exitCode doesn't get reset.
I have now added this text to the commit message also.
There was a problem hiding this comment.
Yes, the branch has a test. :)
That's what's documented in |
|
Thanks for the reviews! |
|
You've some test failures, but they don't seem related to your code changes to me. @mheon are these some of the flakes you're chasing elsewhere? |
|
I've seen the rootless failures elsewhere, but not sure what's going on with them yet. |
|
/lgtm |
Getting a list of containers, and then deleting them are two separate fallible steps that can run into different sets of errors. eg., in the case of a bogus missing container and a container that's running or paused, the first step will only trigger libpod.ErrNoSuchCtr. At this point it might appear that the exit code ought to be 1. However, when attempting the deletion, it will fail once more due to the status of the running or paused container. Since libpod.ErrNoSuchCtr is no longer the only error encountered, the exit code should be reset to 125. This problem is currently masked for rootless usage due to commit 35432ec ("rootless: fix rm when uid in the container != 0"). Fixes: 85db895 ("rm: set exit code to 1 if a specified ...") e41279b ("Change exit code to 1 on podman rm ...") Signed-off-by: Debarshi Ray <rishi@fedoraproject.org>
|
/lgtm |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: debarshiray, giuseppe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
Getting a list of containers, and then deleting them are two separate
fallible steps that can run into different sets of errors. eg., in the
case of a bogus missing container and a container that's running or
paused, the first step will only trigger libpod.ErrNoSuchCtr. At this
point it might appear that the exit code ought to be 1. However, when
attempting the deletion, it will fail once more due to the status of
the running or paused container. Since libpod.ErrNoSuchCtr is no longer
the only error encountered, the exit code should be reset to 125.
This problem is currently masked for rootless usage due to commit
35432ec ("rootless: fix rm when uid in the container != 0").
Fixes: 85db895 ("rm: set exit code to 1 if a specified ...")
e41279b ("Change exit code to 1 on podman rm ...")
Signed-off-by: Debarshi Ray rishi@fedoraproject.org