Give summary when client operation to containers failed

[WeiZhang555]

When we failed to delete one or more containers, besides the error
message for each container, we should give a more useful summary
information e.g. "failed to delete containers: ctrA,ctrB", instead of
"one or more of the container deletions failed", the old summary info is
totally useless.

Signed-off-by: Zhang Wei zhangwei555@huawei.com

Update: this can also apply to other commands including start/pause/resume

[WeiZhang555]

PS: I replaced specs.Linux.Resources with r which will do no harm to anything, it's not related to this "summary" issue. Just think it's small nit and not worth a new PR, so I carry it here.

If anyone feels uncomfortable with this way, I can separate them and file a new PR for the wording replacement. :-)

[hqhq]

You don't have to split each command, please squash your commits.

[WeiZhang555]

squashed

[hqhq]

You could just use == here and all other test cases.

[WeiZhang555]

Fixed

[hqhq]

LGTM, thanks.

[crosbymichael]

Isn't this just redundant information as its already printed to stderr along with the reason why it failed?

[WeiZhang555]

@crosbymichael Considering that a user could delete lots of containers at a time, some would fail and the list could be long, or the error message could be long, it would be better to print a summary including all the failed containers to give user a better look and let him/her know which and how many containers failed.

By the way, this is also compliant to docker API, I'm not saying it's the best, but I quite like it.

[cyphar]

I don't really like this either. Not to mention that this summary output will stop making sense if someone created a container with , in its name. Personally I always thought the Docker error message setup was a bit redundant.

[WeiZhang555]

Sad about that, to be honest, I really like the "summary".

If @cyphar and @crosbymichael are both uncomfortable about this, I can remove them :-( currently we have some summary but provide useless info.

What do you think?

[crosbymichael]

I think it really depends on how you are using it. To me its just redundant information because the failures are already printed to stderr so why print it again.

If you are using this pragmatically there maybe better options for output, such as json output of a summary but we will still have to think about that more also. Overall, I'm not a fan of the current batch operations and think they add alot of noise to the output and its hard to reason about atomic operations when 1 out of 100 fail

[WeiZhang555]

Actually there is another technical reason that we should do this.

We need to return some error if the operation to multiple containers failed (see https://github.com/opencontainers/runc/pull/1156/files#diff-635f3cde55281671a8db5283d1597a71R103), then the cli will print the error and exit.

If we don't return the "summary" information as error, we may have some other substitutions:

1. store error message for each failed container, and return them at last. But this will lead to some delay of printing the error to user
2. don't return error, just call os.Exit(1). I don't quite like this, prefer to use existing cli library.
3. return empty error (fmt.Error("") ), this is the last choice I want to use, because it looks weird that you return an error but error message is empty.

So comparing to previous 3 options, I think the "noise" is much better 😄

[cyphar]

I'm actually starting to lean to what @crosbymichael said. The "batch" operations are quite confusing in some cases to users, especially when it comes to partial failures. I've never been a fan of Docker's docker rm <multiple> <container> <names> interface for the same reason I don't like rm's interface for multiple-file-deletion.

[WeiZhang555]

I've never been a fan of Docker's docker rm interface for the same reason I don't like rm's interface for multiple-file-deletion.

This is only about preference and taste, not about right or wrong, so I think it should be fine. Besides, rm multiple files is quite convenient to me 😄

It's already part of runc, we can keep improving it, I don't think we have to revert this.

[crosbymichael]

Thinking about this more. I would like to propose that we remove all the multi container operations from runc. The feature look innocent when it was first proposed but after multiple PRs expanding this type of operations to other commands and seeing how errors and output are handled it shows that this is not a good design for runc and its not what it was built for, single container operations and nothing more.

It also breaks the runtime spec actions as they all expect to call the actions with one id, not multiple.

Since we are pre-1.0 I would like to remove these and go back to single ID operations. When I look at the code its so much more complex for even simple things. What does everyone think about this change?

[cyphar]

I completely agree with @crosbymichael. I also started feeling buyers remorse soon after the PR was merged, especially considering the "atomic operation" argument that was given above.

[hqhq]

I agree we can keep this behavior in runc simple and consistent, Since we all agree, I can work on a PR to revert that behavior. Close this one.