From ff66f31ddd85deccbbd7c579e62a986c2ce148c2 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Mon, 12 Jun 2023 15:05:46 -0400 Subject: [PATCH 1/5] libpod: correctly pass env so alternative locales work in addition to https://github.com/containers/podman/commit/b6167cedb2ba69a66646a94eb27c49bbbea88e15 we also need to pass LANG. Do so, and add a test to verify Signed-off-by: Peter Hunt --- libpod/oci_conmon_common.go | 3 +++ test/e2e/logs_test.go | 15 ++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/libpod/oci_conmon_common.go b/libpod/oci_conmon_common.go index 32877c5ad2..10842a011b 100644 --- a/libpod/oci_conmon_common.go +++ b/libpod/oci_conmon_common.go @@ -1311,6 +1311,9 @@ func (r *ConmonOCIRuntime) configureConmonEnv(runtimeDir string) []string { if strings.HasPrefix(e, "LC_") { env = append(env, e) } + if strings.HasPrefix(e, "LANG=") { + env = append(env, e) + } } if path, ok := os.LookupEnv("PATH"); ok { env = append(env, fmt.Sprintf("PATH=%s", path)) diff --git a/test/e2e/logs_test.go b/test/e2e/logs_test.go index 953f4c226e..0ae1b79af3 100644 --- a/test/e2e/logs_test.go +++ b/test/e2e/logs_test.go @@ -2,6 +2,7 @@ package integration import ( "fmt" + "os" "os/exec" "time" @@ -584,6 +585,19 @@ var _ = Describe("Podman logs", func() { Expect(logs.ErrorToString()).To(ContainSubstring("this container is using the 'none' log driver, cannot read logs: this container is not logging output")) }) + It("podman logs with non ASCII log tag fails without env", func() { + // Env won't be passed to the podman command by default, so it will be empty + logc := podmanTest.Podman([]string{"run", "--log-opt", "tag=\"äöüß\"", ALPINE, "echo", "podman"}) + logc.WaitWithDefaultTimeout() + Expect(logc).To(Not(Exit(0))) + }) + + It("podman logs with non ASCII log tag succeeds with env", func() { + env := append(os.Environ(), "LANG=en_US.UTF-8", "LC_ALL=") + logc := podmanTest.PodmanAsUser([]string{"run", "--log-opt", "tag=äöüß", ALPINE, "echo", "podman"}, 0, 0, "", env) + logc.WaitWithDefaultTimeout() + Expect(logc).To(Exit(0)) + }) It("podman pod logs with container names", func() { SkipIfRemote("Remote can only process one container at a time") podName := "testPod" @@ -638,5 +652,4 @@ var _ = Describe("Podman logs", func() { g.Expect(output[1]).To(MatchRegexp(`\x1b\[3[0-9a-z ]+\x1b\[0m`)) }).Should(Succeed()) }) - }) From 9b4f1cdb9780d871e5f88468b23af301ac6215bf Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Tue, 8 Aug 2023 10:12:29 -0400 Subject: [PATCH 2/5] cirrus/lib.sh: extend env to passthrough at start for locale work Signed-off-by: Peter Hunt --- contrib/cirrus/lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/cirrus/lib.sh b/contrib/cirrus/lib.sh index 3e88fed73b..70bfb74309 100644 --- a/contrib/cirrus/lib.sh +++ b/contrib/cirrus/lib.sh @@ -96,7 +96,7 @@ EPOCH_TEST_COMMIT="$CIRRUS_BASE_SHA" PASSTHROUGH_ENV_EXACT='CGROUP_MANAGER|DEST_BRANCH|DISTRO_NV|GOCACHE|GOPATH|GOSRC|NETWORK_BACKEND|OCI_RUNTIME|ROOTLESS_USER|SCRIPT_BASE|SKIP_USERNS|EC2_INST_TYPE|PODMAN_DB' # List of envariable patterns which must match AT THE BEGINNING of the name. -PASSTHROUGH_ENV_ATSTART='CI|TEST' +PASSTHROUGH_ENV_ATSTART='CI|LANG|LC_|TEST' # List of envariable patterns which can match ANYWHERE in the name PASSTHROUGH_ENV_ANYWHERE='_NAME|_FQIN' From 8f85aaf07ffbb3e98b7838f70b1a73e78e74023b Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 15 Aug 2023 18:26:10 +0200 Subject: [PATCH 3/5] fixup "podman logs with non ASCII log tag" tests 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 --- libpod/oci_conmon_common.go | 2 +- test/e2e/logs_test.go | 55 ++++++++++++++++++++++++++++++++----- 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/libpod/oci_conmon_common.go b/libpod/oci_conmon_common.go index 10842a011b..6b0d6bc679 100644 --- a/libpod/oci_conmon_common.go +++ b/libpod/oci_conmon_common.go @@ -1266,7 +1266,7 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co /* Wait for initial setup and fork, and reap child */ err = cmd.Wait() if err != nil { - return 0, err + return 0, fmt.Errorf("conmon failed: %w", err) } pid, err := readConmonPipeData(r.name, parentSyncPipe, ociLog) diff --git a/test/e2e/logs_test.go b/test/e2e/logs_test.go index 0ae1b79af3..80bf69ca93 100644 --- a/test/e2e/logs_test.go +++ b/test/e2e/logs_test.go @@ -585,19 +585,38 @@ var _ = Describe("Podman logs", func() { Expect(logs.ErrorToString()).To(ContainSubstring("this container is using the 'none' log driver, cannot read logs: this container is not logging output")) }) - It("podman logs with non ASCII log tag fails without env", func() { - // Env won't be passed to the podman command by default, so it will be empty - logc := podmanTest.Podman([]string{"run", "--log-opt", "tag=\"äöüß\"", ALPINE, "echo", "podman"}) + It("podman logs with non ASCII log tag fails without correct LANG", func() { + SkipIfJournaldUnavailable() + // need to set the LANG to something that does not support german umlaute to trigger the failure case + cleanup := setLangEnv("C") + if IsRemote() { + podmanTest.RestartRemoteService() + } + defer cleanup() + logc := podmanTest.Podman([]string{"run", "--log-driver", "journald", "--log-opt", "tag=äöüß", ALPINE, "echo", "podman"}) logc.WaitWithDefaultTimeout() - Expect(logc).To(Not(Exit(0))) + Expect(logc).To(Exit(126)) + if !IsRemote() { + // Error is only seen on local client + // Why does conmon log this to stdout? This must be fixed after https://github.com/containers/conmon/pull/447. + Expect(logc.OutputToString()).To(Equal("conmon: option parsing failed: Invalid byte sequence in conversion input")) + } + Expect(logc.ErrorToString()).To(ContainSubstring("conmon failed: exit status 1")) }) - It("podman logs with non ASCII log tag succeeds with env", func() { - env := append(os.Environ(), "LANG=en_US.UTF-8", "LC_ALL=") - logc := podmanTest.PodmanAsUser([]string{"run", "--log-opt", "tag=äöüß", ALPINE, "echo", "podman"}, 0, 0, "", env) + It("podman logs with non ASCII log tag succeeds with proper env", func() { + SkipIfJournaldUnavailable() + cleanup := setLangEnv("en_US.UTF-8") + if IsRemote() { + podmanTest.RestartRemoteService() + } + defer cleanup() + logc := podmanTest.Podman([]string{"run", "--log-driver", "journald", "--log-opt", "tag=äöüß", ALPINE, "echo", "podman"}) logc.WaitWithDefaultTimeout() Expect(logc).To(Exit(0)) + Expect(logc.OutputToString()).To(Equal("podman")) }) + It("podman pod logs with container names", func() { SkipIfRemote("Remote can only process one container at a time") podName := "testPod" @@ -653,3 +672,25 @@ var _ = Describe("Podman logs", func() { }).Should(Succeed()) }) }) + +func setLangEnv(lang string) func() { + oldLang, okLang := os.LookupEnv("LANG") + oldLcAll, okLc := os.LookupEnv("LC_ALL") + err := os.Setenv("LANG", lang) + Expect(err).ToNot(HaveOccurred()) + err = os.Setenv("LC_ALL", "") + Expect(err).ToNot(HaveOccurred()) + + return func() { + if okLang { + os.Setenv("LANG", oldLang) + } else { + os.Unsetenv("LANG") + } + if okLc { + os.Setenv("LC_ALL", oldLcAll) + } else { + os.Unsetenv("LC_ALL") + } + } +} From ed1f514d5512d229b4285de8316dfa65cdd5e3df Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 15 Aug 2023 18:30:01 +0200 Subject: [PATCH 4/5] cirrus setup: install en_US.UTF-8 locale 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 --- contrib/cirrus/setup_environment.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/contrib/cirrus/setup_environment.sh b/contrib/cirrus/setup_environment.sh index 80129a8d35..06dd3acf7e 100755 --- a/contrib/cirrus/setup_environment.sh +++ b/contrib/cirrus/setup_environment.sh @@ -150,6 +150,11 @@ case "$OS_RELEASE_ID" in # (Checked on 2023-08-08 and it's still too old: 1.1.5) # FIXME: please remove this once runc >= 1.2 makes it into debian. modprobe tun + + # TODO: move this into image build process + # We need the "en_US.UTF-8" locale for the "podman logs with non ASCII log tag" tests + sed -i '/en_US.UTF-8/s/^#//g' /etc/locale.gen + locale-gen ;; fedora) if ((CONTAINER==0)); then From c726cf81063eb26c6ffbbcfcc661fad3779bfc3a Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 17 Aug 2023 10:40:02 +0200 Subject: [PATCH 5/5] libpod: improve conmon error handling 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 --- libpod/oci_conmon_common.go | 10 +++++++++- libpod/oci_conmon_linux.go | 5 +---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/libpod/oci_conmon_common.go b/libpod/oci_conmon_common.go index 6b0d6bc679..f315d94397 100644 --- a/libpod/oci_conmon_common.go +++ b/libpod/oci_conmon_common.go @@ -1261,8 +1261,16 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co return 0, err } if err := r.moveConmonToCgroupAndSignal(ctr, cmd, parentStartPipe); err != nil { - return 0, err + // The child likely already exited in which case the cmd.Wait() below should return the proper error. + // EPIPE is expected if the child already exited so not worth to log and kill the process. + if !errors.Is(err, syscall.EPIPE) { + logrus.Errorf("Failed to signal conmon to start: %v", err) + if err := cmd.Process.Kill(); err != nil && !errors.Is(err, syscall.ESRCH) { + logrus.Errorf("Failed to kill conmon after error: %v", err) + } + } } + /* Wait for initial setup and fork, and reap child */ err = cmd.Wait() if err != nil { diff --git a/libpod/oci_conmon_linux.go b/libpod/oci_conmon_linux.go index 9819c83bae..a3a552bc64 100644 --- a/libpod/oci_conmon_linux.go +++ b/libpod/oci_conmon_linux.go @@ -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 { - return err - } - return nil + return writeConmonPipeData(startFd) } // GetLimits converts spec resource limits to cgroup consumable limits