From bac57409fee7a62cb4fb2cdb762c2eff02ad9686 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 27 Apr 2023 12:07:47 +0200 Subject: [PATCH 1/5] test: fix typo Signed-off-by: Giuseppe Scrivano --- test/system/251-system-service.bats | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/system/251-system-service.bats b/test/system/251-system-service.bats index 7a772e2ef4..4ad2cd0311 100644 --- a/test/system/251-system-service.bats +++ b/test/system/251-system-service.bats @@ -15,7 +15,7 @@ function teardown() { basic_teardown } -@test "podman systerm service returns error" { +@test "podman system service returns error" { skip_if_remote "podman system service unavailable over remote" run_podman 125 system service localhost:9292 is "$output" "Error: API Service endpoint scheme \"localhost\" is not supported. Try tcp://localhost:9292 or unix:/localhost:9292" From 891bc117e465babd60ac6ad280cad718e9e9c6e5 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 27 Apr 2023 12:08:14 +0200 Subject: [PATCH 2/5] podman: simplify code with a switch simplify the readerFromArg to avoid the same boilerplate code. Signed-off-by: Giuseppe Scrivano --- cmd/podman/kube/play.go | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/cmd/podman/kube/play.go b/cmd/podman/kube/play.go index 93e8ed4d39..663c89de21 100644 --- a/cmd/podman/kube/play.go +++ b/cmd/podman/kube/play.go @@ -346,34 +346,26 @@ func playKube(cmd *cobra.Command, args []string) error { } func readerFromArg(fileName string) (*bytes.Reader, error) { - errURL := parse.ValidURL(fileName) - if fileName == "-" { // Read from stdin - data, err := io.ReadAll(os.Stdin) - if err != nil { - return nil, err - } - return bytes.NewReader(data), nil - } - if errURL == nil { + var reader io.Reader + switch { + case fileName == "-": // Read from stdin + reader = os.Stdin + case parse.ValidURL(fileName) == nil: response, err := http.Get(fileName) if err != nil { return nil, err } defer response.Body.Close() - - data, err := io.ReadAll(response.Body) + reader = response.Body + default: + f, err := os.Open(fileName) if err != nil { return nil, err } - return bytes.NewReader(data), nil + defer f.Close() + reader = f } - f, err := os.Open(fileName) - if err != nil { - return nil, err - } - defer f.Close() - - data, err := io.ReadAll(f) + data, err := io.ReadAll(reader) if err != nil { return nil, err } From 2932208c2a68535d11d72db7ea63a3cf68bba813 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 27 Apr 2023 12:46:43 +0200 Subject: [PATCH 3/5] test: do not wait 10 seconds before killing myyaml the "run_podman rm -a -f" cleanup would take a long time since myyaml doesn't exit immediately. Signed-off-by: Giuseppe Scrivano --- test/system/700-play.bats | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/system/700-play.bats b/test/system/700-play.bats index 9e5f3bd99f..3c7a1eaeee 100644 --- a/test/system/700-play.bats +++ b/test/system/700-play.bats @@ -480,8 +480,7 @@ _EOF is "$output" ".*Error: inspecting object: no such object: \"test_pod-test\"" run_podman pod rm -a -f - run_podman rm -a -f - run_podman rm -f -t0 myyaml + run_podman rm -a -f -t0 } @test "podman play with init container" { From 5592dc12f981c8f4aca53ca6fd2ddb53712c7759 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 27 Apr 2023 16:22:23 +0200 Subject: [PATCH 4/5] libpod: report unmount idmapped rootfs errors Signed-off-by: Giuseppe Scrivano --- libpod/container_internal.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 3cc3c735a2..e0bf77677a 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -1866,8 +1866,12 @@ func (c *Container) cleanupStorage() error { } } if c.config.RootfsMapping != nil { - if err := unix.Unmount(c.config.Rootfs, 0); err != nil { - logrus.Errorf("Unmounting idmapped rootfs for container %s after mount error: %v", c.ID(), err) + if err := unix.Unmount(c.config.Rootfs, 0); err != nil && err != unix.EINVAL { + if cleanupErr != nil { + logrus.Errorf("Unmounting idmapped rootfs for container %s after mount error: %v", c.ID(), err) + } else { + cleanupErr = fmt.Errorf("unmounting idmapped rootfs for container %s after mount error: %w", c.ID(), err) + } } } From 70870895b7129b3808ac1e51c3b71b81050fc019 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 27 Apr 2023 22:10:43 +0200 Subject: [PATCH 5/5] libpod: improve errors management in cleanupStorage fix some issues with the handling of errors, we print an error only when there is already one set to be returned. Also the first error is not printed, since it is reported back to the caller of the function. Improve some messages with more context that can be helpful when things go wrong. Signed-off-by: Giuseppe Scrivano --- libpod/container_internal.go | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/libpod/container_internal.go b/libpod/container_internal.go index e0bf77677a..9be8b9397e 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -1848,9 +1848,10 @@ func (c *Container) cleanupStorage() error { if c.valid { if err := c.save(); err != nil { if cleanupErr != nil { - logrus.Errorf("Unmounting container %s: %v", c.ID(), cleanupErr) + logrus.Errorf("Unmounting container %s: %v", c.ID(), err) + } else { + cleanupErr = err } - cleanupErr = err } } } @@ -1861,8 +1862,9 @@ func (c *Container) cleanupStorage() error { if err := overlay.Unmount(overlayBasePath); err != nil { if cleanupErr != nil { logrus.Errorf("Failed to clean up overlay mounts for %s: %v", c.ID(), err) + } else { + cleanupErr = fmt.Errorf("failed to clean up overlay mounts for %s: %w", c.ID(), err) } - cleanupErr = err } } if c.config.RootfsMapping != nil { @@ -1878,16 +1880,20 @@ func (c *Container) cleanupStorage() error { for _, containerMount := range c.config.Mounts { if err := c.unmountSHM(containerMount); err != nil { if cleanupErr != nil { - logrus.Errorf("Unmounting container %s: %v", c.ID(), cleanupErr) + logrus.Errorf("Unmounting container %s: %v", c.ID(), err) + } else { + cleanupErr = fmt.Errorf("unmounting container %s: %w", c.ID(), err) } - cleanupErr = err } } if err := c.cleanupOverlayMounts(); err != nil { // If the container can't remove content report the error - logrus.Errorf("Failed to clean up overlay mounts for %s: %v", c.ID(), err) - cleanupErr = err + if cleanupErr != nil { + logrus.Errorf("Failed to clean up overlay mounts for %s: %v", c.ID(), err) + } else { + cleanupErr = fmt.Errorf("failed to clean up overlay mounts for %s: %w", c.ID(), err) + } } if c.config.Rootfs != "" { @@ -1905,8 +1911,9 @@ func (c *Container) cleanupStorage() error { } else { if cleanupErr != nil { logrus.Errorf("Cleaning up container %s storage: %v", c.ID(), cleanupErr) + } else { + cleanupErr = fmt.Errorf("cleaning up container %s storage: %w", c.ID(), cleanupErr) } - cleanupErr = err } } @@ -1915,9 +1922,10 @@ func (c *Container) cleanupStorage() error { vol, err := c.runtime.state.Volume(v.Name) if err != nil { if cleanupErr != nil { - logrus.Errorf("Unmounting container %s: %v", c.ID(), cleanupErr) + logrus.Errorf("Retrieving named volume %s for container %s: %v", v.Name, c.ID(), err) + } else { + cleanupErr = fmt.Errorf("retrieving named volume %s for container %s: %w", v.Name, c.ID(), err) } - cleanupErr = fmt.Errorf("retrieving named volume %s for container %s: %w", v.Name, c.ID(), err) // We need to try and unmount every volume, so continue // if they fail. @@ -1928,9 +1936,10 @@ func (c *Container) cleanupStorage() error { vol.lock.Lock() if err := vol.unmount(false); err != nil { if cleanupErr != nil { - logrus.Errorf("Unmounting container %s: %v", c.ID(), cleanupErr) + logrus.Errorf("Unmounting volume %s for container %s: %v", vol.Name(), c.ID(), err) + } else { + cleanupErr = fmt.Errorf("unmounting volume %s for container %s: %w", vol.Name(), c.ID(), err) } - cleanupErr = fmt.Errorf("unmounting volume %s for container %s: %w", vol.Name(), c.ID(), err) } vol.lock.Unlock() }