libpod: correctly pass env so alternative locales work #19635
Conversation
openshift-ci
bot
added
release-note
approved
|
thanks for taking it over! |
|
Changes look OK, but the test results aren't very happy. |
Error messages should always be logged to stderr not stdout. I found this while working on: containers/podman#19635 Signed-off-by: Paul Holzinger <[email protected]>
Error messages should always be logged to stderr not stdout. I found this while working on: containers/podman#19635 Signed-off-by: Paul Holzinger <[email protected]>
|
I feel a great disturbance in the Force, as if @Luap99 were crying out in despair, bitterly regretting having taken over this PR... |
He finally freed me! 🤸 I am very grateful |
|
Well the bug fix is valid and was already working but once again I thought I should test it right. |
in addition to containers@b6167ce we also need to pass LANG. Do so, and add a test to verify Signed-off-by: Peter Hunt <[email protected]>
Signed-off-by: Peter Hunt <[email protected]>
| @@ -162,10 +162,7 @@ func (r *ConmonOCIRuntime) moveConmonToCgroupAndSignal(ctr *Container, cmd *exec | |||
| } | |||
|
|
|||
| /* We set the cgroup, now the child can start creating children */ | |||
| if err := writeConmonPipeData(startFd); err != nil { | |||
There was a problem hiding this comment.
I think there's a linter to catch such cases. Not sure whether it's enabled though.
There was a problem hiding this comment.
clearly not otherwise it should have flagged this one
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, vrothberg 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. I'll apply an optimistic lgtm/hold in the hopes that CI is satisfied. |
openshift-ci
bot
added
the
do-not-merge/hold
A podman test depends on that locale so we need to make it is installed in the image, see containers/podman#19635. Signed-off-by: Paul Holzinger <[email protected]>
Make sure the en_US.UTF-8 LANG is installed and can be used by podman tests, see containers/podman#19635. Signed-off-by: Paul Holzinger <[email protected]>
We need to actually check the output not just exit codes. While doing this it was clear that the first test was not checking what it should be so I had to remove the quotes from the arg. Also this check did not work with remote testing at all, we must set the env then restart the server as the env for conmon must be set on the server obviously. Also we can only match the conmon error messages on the local client. Lastly this test requires the journald driver but we cannot use the in container tests so skip it there. Signed-off-by: Paul Holzinger <[email protected]>
Make sure the en_US.UTF-8 locale is available so that we can use it in tests, namely "podman logs with non ASCII log tag succeeds with env". It is already there in fedora (except container image but we cannot use journald there anyway) so only do this for debian. I think it makes most sense to move this into the image build process in the future to only do it once at build time. Signed-off-by: Paul Holzinger <[email protected]>
When conmon is started it blocks and waits for us to signal it to start via pipe. This works but when conmon exits before it waits for the start message it causes podman to fail with `write child: broken pipe`. This error is meaningless to podman users. The real error is that conmon failed so we should not return early if we fail to send the start message to conmon. Instead ignore the EPIPE error case as it is safe to assume to the conmon died and for other errors we make sure to kill conmon so that the following wait() call does not hang forever. This also fixes problems with having conmon zombie processes leaked as wait() was never called. Signed-off-by: Paul Holzinger <[email protected]>
|
Ok, now it should work. I tried for far to long to get this working in the in container tests until I realized that this requires the journald driver and we have no systemd running there so it will never work. |
I don't regret it, the funny answer why I took this over: German speakers will understand this, for those who don't: The missing letter is "ß" for "scheiß" which means fuck in this context. As a german native I can tell you the amount of software that still does not get it right today is to big so I decided podman shouldn't be on that list. |
|
/lgtm |
|
/hold cancel My last name is Santiago Muñoz; the space is part of it, so is the enye (n-tilde). I can count on one finger the number of databases I'm in that get it right. I am thankful for ISO-8859-1 and UTF-8, and welcome all efforts to improve i18n handling. Thank you @Luap99. |
openshift-ci
bot
removed
the
do-not-merge/hold
Make sure the en_US.UTF-8 LANG is installed and can be used by podman tests, see containers/podman#19635. Signed-off-by: Paul Holzinger <[email protected]>
Make sure the en_US.UTF-8 LANG is installed and can be used by podman tests, see containers/podman#19635. Signed-off-by: Paul Holzinger <[email protected]>
github-actions
bot
added
the
locked - please file new issue/PR
see commits
Does this PR introduce a user-facing change?
Fixes containers/conmon#272