From 2b2c4453f1baeeac4639dc1baf85fb841dde8e00 Mon Sep 17 00:00:00 2001 From: Matt Heon Date: Mon, 10 Jul 2023 15:59:47 -0400 Subject: [PATCH] Fix container errors not being sent via pod removal API When I reworked pod removal to provide more detailed errors (including per-container errors, not just a single multierror with all errors squashed), I made it part of the struct returned by the REST API and assumed that would be enough to get errors through to clients. Unfortunately, in case of an overarching error removing the pod (as any error with any container would cause), we don't send the response struct that would include the container errors - we just send a standardized REST error. We could work around this with custom, potentially backwards incompatible error handling for the REST pod delete endpoint, or we could just do what was done before, and package up all the errors in a multierror to send to the other side. Of those options, the multierror seems far simpler. Fixes #19159 Signed-off-by: Matt Heon --- pkg/api/handlers/libpod/pods.go | 14 ++++++++++++++ test/apiv2/40-pods.at | 19 +++++++++++++++++-- test/system/200-pod.bats | 10 ++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/pkg/api/handlers/libpod/pods.go b/pkg/api/handlers/libpod/pods.go index c13af63136..b01bf8f8df 100644 --- a/pkg/api/handlers/libpod/pods.go +++ b/pkg/api/handlers/libpod/pods.go @@ -20,6 +20,7 @@ import ( "github.com/containers/podman/v4/pkg/specgenutil" "github.com/containers/podman/v4/pkg/util" "github.com/gorilla/schema" + "github.com/hashicorp/go-multierror" "github.com/sirupsen/logrus" ) @@ -248,6 +249,19 @@ func PodDelete(w http.ResponseWriter, r *http.Request) { } ctrs, err := runtime.RemovePod(r.Context(), pod, true, query.Force, query.Timeout) if err != nil { + if len(ctrs) > 0 { + // We have container errors to send as well. + // Since we're just writing an error, and we don't want + // special error-handling for just this endpoint: use a + // multierror to package up all container errors. + var allCtrErrors error + for _, ctrErr := range ctrs { + allCtrErrors = multierror.Append(allCtrErrors, ctrErr) + } + + err = fmt.Errorf("%w. %s", err, allCtrErrors.Error()) + } + utils.Error(w, http.StatusInternalServerError, err) return } diff --git a/test/apiv2/40-pods.at b/test/apiv2/40-pods.at index 0e0f1cb18c..71af150772 100644 --- a/test/apiv2/40-pods.at +++ b/test/apiv2/40-pods.at @@ -3,6 +3,9 @@ # test pod-related endpoints # +# Need to make 1 container +podman pull $IMAGE &>/dev/null + t GET "libpod/pods/json (clean slate at start)" 200 '[]' t POST libpod/pods/create name=foo 201 .Id~[0-9a-f]\\{64\\} @@ -28,11 +31,17 @@ t POST "libpod/pods/create (dup pod)" name=foo 409 \ #t POST libpod/pods/create a=b 400 .cause='bad parameter' # FIXME: unimplemented +# Some tests require a single running container. +# Don't bother saving CID, it will be removed as part of the pod. +t POST libpod/containers/create?name=testctr Image=${IMAGE} Pod=foo Entrypoint='["top"]' 201 \ + .Id~[0-9a-f]\\{64\\} +cid=$(jq -r '.Id' <<<"$output") + t POST libpod/pods/foo/start 200 \ .Errs=null \ .Id=$pod_id -t POST libpod/pods/foo/start 304 \ +t POST libpod/pods/foo/start 304 t POST libpod/pods/fakename/start 404 \ .cause="no such pod" \ @@ -52,11 +61,17 @@ t POST "libpod/pods/fakename/unpause" 404\ .cause="no such pod" \ .message="no pod with name or ID fakename found: no such pod" +t DELETE libpod/pods/foo?force=false 500 \ + .cause="removing pod containers" \ + .message~".*cannot remove container .* as it is running.*" t POST libpod/pods/foo/stop 200 \ .Errs=null \ .Id=$pod_id +# Remove container as it is no longer required +t DELETE libpod/containers/$cid 200 .[0].Id=$cid + t POST "libpod/pods/foo/stop (pod is already stopped)" 304 t POST "libpod/pods/fakename/stop" 404\ .cause="no such pod" \ @@ -85,7 +100,7 @@ t POST "libpod/pods/bar/stop?t=invalid" 400 \ .cause="schema: error converting value for \"t\"" \ .message~"failed to parse parameters for" -podman run -d --pod bar busybox sleep 999 +podman run -d --pod bar busybox top t POST libpod/pods/bar/stop?t=1 200 \ .Errs=null \ diff --git a/test/system/200-pod.bats b/test/system/200-pod.bats index 442517b461..f159ed6ee0 100644 --- a/test/system/200-pod.bats +++ b/test/system/200-pod.bats @@ -55,6 +55,16 @@ function teardown() { is "$output" ".*0 \+1 \+0 \+[0-9. ?s]\+/pause" "there is a /pause container" fi + # Cannot remove pod while containers are still running. Error messages + # differ slightly between local and remote; these are the common elements. + run_podman 125 pod rm $podid + assert "${lines[0]}" =~ "Error: not all containers could be removed from pod $podid: removing pod containers.*" \ + "pod rm while busy: error message line 1 of 3" + assert "${lines[1]}" =~ "cannot remove container .* as it is running - running or paused containers cannot be removed without force: container state improper" \ + "pod rm while busy: error message line 2 of 3" + assert "${lines[2]}" =~ "cannot remove container .* as it is running - running or paused containers cannot be removed without force: container state improper" \ + "pod rm while busy: error message line 3 of 3" + # Clean up run_podman --noout pod rm -f -t 0 $podid is "$output" "" "output should be empty"