Add better support for Docker top syntax #3006
Conversation
We should support ',' separated lists of descriptors and ignore any ps options handed to us. This will make it easier to convert from Docker to Podman. Signed-off-by: Daniel J Walsh <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
approved
| markFlagHiddenForRemoteClient("latest", flags) | ||
| } | ||
|
|
||
| func genDescriptors(args []string) []string { |
There was a problem hiding this comment.
Uh. Do we really want to do this? Some of those options do things...
Might be safer to add -e and -o as NOOPs.
There was a problem hiding this comment.
Docker supports a bunch that we do not. I think we should just ignore for now, and then let @vrothberg work on a better solution in the long run.
There was a problem hiding this comment.
Basically Docker allows you to pass any option that the ps command supports.
There was a problem hiding this comment.
-o might actually be a good idea to implement as its own flag - it specifies that format specifiers will follow in comma-separated format, so different than our default.
Potential problem: I don't know if Cobra allows mixing flags with arguments and flags without for single-character flags, so -eo might not work together.
-e is a NO-OP with our current implementation, we always do what it specifies.
There was a problem hiding this comment.
Huh. Cobra does allow it.
podman run --rm -tiv /tmp:/tmp:rw fedora bash works
There was a problem hiding this comment.
Yes, I know, we have different options in podman top, I am just trying to make sure we can handle basic docker top commands but still support our own versions.
There was a problem hiding this comment.
We are more lax about the syntax and not tied to the way the ps command handles it.
There was a problem hiding this comment.
We can pretty easily implement -o as a flag that supports comma-separated format specifiers, though
There was a problem hiding this comment.
I am not sure,but I think Docker might require the ps command inside of the container. We do not since @vrothberg wrote a native go implementation psgo.
There was a problem hiding this comment.
We don't have to, this patch allows that syntax.
vrothberg
left a comment
vrothberg
left a comment
There was a problem hiding this comment.
I am not a big fan of trying to emulate the ps flags other than the descriptors. That was the trade-off we made (and agreed on) when using the c/psgo library.
Considering https://bugzilla.redhat.com/show_bug.cgi?id=1702787 the sacrifice of using psgo is incompatibility with ps flags (the descriptors are 100% compatible). Now, just ignoring the flags will make the BZ work again but others will still fail (see ps -e vs ps -ef) which makes me believe this PR is a symptomatic fix for one specific bug, so I don't feel comfortable merging it. I'd rather prefer to continue be incompatible than giving a false impression of compatibility.
My question is: are we suddenly considering the CLI-incompatibility to be a bug? If users want to use ps they can still podman exec $CTR ps ..., which is what docker is doing behind the docker-top.
If we consider it a bug now, I'd still object the PR as it's a symptomatic fix and may cause follow-up issues: "ps -eo a,b,c" works but "ps -ef" doesn't return what I expect. I'd prefer extending c/psgo so that other users can benefit as well. Warning: looking at man ps, I really don't think we should go this path as we will end up implementing the entire ps CLI which looks like a scary rabbit hole and does not seem worth investing time in. I suggest we first attempt to parse the user-input and fall-back to execing ps in the container.
My preference is asking users to use podman/docker exec ... ps ... but I understand that doesn't count as a drop-in-replacement.
(background for the record)
The reason why we implemented c/psgo is because ps does not allow for parsing its output as entries are separated with whitespaces only breaking all kinds of heuristics to turn the ps output into a tabular format. Then we added all kinds of improvements on-top as psgo knows about namespaces (and hence about a ~container). This will not help users expecting podman-top and docker-top to be identical, but podman-top could return everything as a json to make it even simpler to parse.
|
@vrothberg how about we just add support for the "," separated list of descriptors as well as the " " separated? |
Sure, this could be done in |
|
@rhatdan, we can also cheat and make |
|
No I don't want this leaking into psgo. |
The only change to psgo would be to return a typed error to know if it couldn't parse the input. Everything else must be handled by users of psgo. |
|
Ok that works for me. |
|
🙊 ... we already have that https://github.com/containers/psgo/blob/master/psgo.go#L108. if errors.Cause(err) == psgo.ErrUnkownDescriptor {
container.Exec("ps", args...)
}Will still change the typo in psgo s/ErrUnkownDescriptor/ErrUnkNownDescriptor/ |
|
☔ The latest upstream changes (presumably #3014) made this pull request unmergeable. Please resolve the merge conflicts. |
openshift-ci-robot
added
the
needs-rebase
|
@rhatdan: PR needs rebase. DetailsInstructions 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. |
|
Closed since we will use @vrothberg PR |
github-actions
bot
added
the
locked - please file new issue/PR
5 participants
We should support ',' separated lists of descriptors and ignore any ps options
handed to us. This will make it easier to convert from Docker to Podman.
Signed-off-by: Daniel J Walsh [email protected]