Conversation
Having more logs is always helpful than having none. containers#1052
b8e4d77 to
42a86c1
Compare
|
Build failed. ✔️ unit-test SUCCESS in 7m 20s |
|
recheck |
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
debarshiray
left a comment
There was a problem hiding this comment.
I managed to finally fish this out from my backlog. Sorry about that.
Some changes by Debarshi Ray. containers#957 containers#1052
Some changes by Debarshi Ray. containers#957 containers#1052
Some changes by Debarshi Ray. containers#957 containers#1052
debarshiray
left a comment
There was a problem hiding this comment.
Don't we also need to handle the help functions in the same way?
Some changes by Debarshi Ray. containers#957 containers#1052
The test suite uses its own separate local container/storage store to isolate itself from the default store, so that the tests' interactions with containers and images don't affect anything else. This is done by using the CONTAINERS_STORAGE_CONF environment variable [1] to specify a separate storage.conf(5) file [2]. Therefore, when running the test suite, the CONTAINERS_STORAGE_CONF environment variable must be preserved when forwarding toolbox(1) invocations inside containers to the host. Otherwise, the initial toolbox(1) invocation on the host and the forwarded invocation running on the host won't use the same local container/storage store. This problem only impacts test cases that cover toolbox(1) code paths that invoke podman(1). [1] https://docs.podman.io/en/latest/markdown/podman.1.html [2] https://manpages.debian.org/testing/containers-storage/containers-storage.conf.5.en.html containers#957 containers#1052
When 'toolbox run' is invoked on the host, an exit code of 126 from 'podman exec' means that the specified command couldn't be invoked because it's not an executable. eg., the command was actually a directory. Note that this doesn't mean that the command couldn't be found. That's denoted by exit code 127. Secondly, Toolbx containers always have an executable toolbox(1) binary at /usr/bin/toolbox and it's assumed that /usr/bin is always part of the PATH environment variable. When 'toolbox run toolbox ...' is invoked, the inner toolbox(1) invocation will be forwarded back to the host by the Toolbx container's /usr/bin/toolbox, which is always present as an executable. Hence, if the outer 'podman exec' on the host fails with an exit code of 126, then it doesn't mean that the container didn't have a working toolbox(1) executable, but that some subordinate process started by the container's toolbox(1) failed with that exit code. Therefore, handle this as a special case to avoid showing an extra error message. Otherwise, it leads to: $ toolbox run toolbox run /etc bash: line 1: /etc: Is a directory bash: line 1: exec: /etc: cannot execute: Is a directory Error: failed to invoke command /etc in container fedora-toolbox-40 Error: failed to invoke command toolbox in container fedora-toolbox-40 $ echo "$?" 126 Instead, it will now be: $ toolbox run toolbox run /etc bash: line 1: /etc: Is a directory bash: line 1: exec: /etc: cannot execute: Is a directory Error: failed to invoke command /etc in container fedora-toolbox-40 $ echo "$?" 126 containers#957 containers#1052
When 'toolbox run' is invoked on the host, an exit code of 127 from
'podman exec' means either that the specified command couldn't be found
or that the working directory didn't exist. The only way to tell these
two scenarios apart is to actually look inside the container.
Secondly, Toolbx containers always have an executable toolbox(1) binary
at /usr/bin/toolbox and it's assumed that /usr/bin is always part of the
PATH environment variable.
When 'toolbox run toolbox ...' is invoked, the inner toolbox(1)
invocation will be forwarded back to the host by the Toolbx container's
/usr/bin/toolbox, which is always present as an executable. Hence, if
the outer 'podman exec' on the host fails with an exit code of 127,
then it doesn't mean that the container didn't have a toolbox(1)
executable, but that some subordinate process started by the container's
toolbox(1) failed with that exit code.
Therefore, handle this as a special case to avoid losing the exit code.
Otherwise, it leads to:
$ toolbox run toolbox run non-existent-command
bash: line 1: exec: non-existent-command: not found
Error: command non-existent-command not found in container
fedora-toolbox-40
$ echo "$?"
0
Instead, it will now be:
$ toolbox run toolbox run non-existent-command
bash: line 1: exec: non-existent-command: not found
Error: command non-existent-command not found in container
fedora-toolbox-40
$ echo "$?"
127
containers#957
containers#1052
To answer my own question: First of all, if Secondly, the help functions deal with the Things like This leaves us with the scenario where Ideally, we would have returned a non-zero exit code, but it's probably not a big enough problem and we can set it aside for another day. Ultimately, Toolbx containers are expected to have |
| $PODMAN rm --all --force >/dev/null | ||
| $PODMAN rmi --all --force >/dev/null | ||
| $PODMAN rm --all --force | ||
| $PODMAN rmi --all --force |
There was a problem hiding this comment.
I don't necessarily disagree that more logs can be better, especially in test suites, but I would like to understand if there was a specific problem that this change was addressing.
As far as I understand, it's only the standard output stream that is being silenced, and error messages are still free to make their way to the standard error stream. So, if there was indeed a problem, then it would be good to document it in the Git commit message to inform future decisions.
Anyway, this particular commit isn't the main thrust of this pull request. We can resurrect it later if needed.
|
Closing. My apologies that it took so long to get to it. |
See commit messages for detailed information.
Fixes #957