From d56f52c8a7cda3cfbb8d68b132d79e62400678a2 Mon Sep 17 00:00:00 2001 From: Hiroaki KAWAI Date: Thu, 27 Aug 2026 11:31:45 +0900 Subject: [PATCH] api: reflect container stats errors in HTTP status codes Existing KillContainer handling uses HTTP status code 409 when the request cannot be performed because of the current container state. HTTP status code 404 is also used when the target container does not exist. In contrast, errors from the stats handler are not reflected in the HTTP status code. The HTTP status code is always 200, and the errors are recorded only as generic errors in the server log. To maintain compatibility with both streaming enabled and disabled, this change treats obtaining at least one complete unit of response content as the response contract. It then keeps the response content consistent with the HTTP status code. Signed-off-by: Hiroaki KAWAI --- libpod/stats_linux.go | 3 + pkg/api/handlers/compat/containers_stats.go | 68 +++++++++++++------ .../handlers/compat/containers_stats_test.go | 36 ++++++++++ pkg/api/server/register_containers.go | 2 + test/apiv2/19-stats.at | 8 +++ winmake.ps1 | 3 +- 6 files changed, 100 insertions(+), 20 deletions(-) create mode 100644 pkg/api/handlers/compat/containers_stats_test.go diff --git a/libpod/stats_linux.go b/libpod/stats_linux.go index c65d2583e1..502ed70e68 100644 --- a/libpod/stats_linux.go +++ b/libpod/stats_linux.go @@ -132,6 +132,9 @@ func getOnlineCPUs(container *Container) (int, error) { } var cpuSet unix.CPUSet if err := unix.SchedGetaffinity(ctrPID, &cpuSet); err != nil { + if errors.Is(err, unix.ESRCH) { + return -1, fmt.Errorf("container %s exited while obtaining online cpus: %w", container.Name(), errors.Join(err, define.ErrCtrStopped)) + } return -1, fmt.Errorf("failed to obtain Container %s online cpus: %w", container.Name(), err) } return cpuSet.Count(), nil diff --git a/pkg/api/handlers/compat/containers_stats.go b/pkg/api/handlers/compat/containers_stats.go index 95e158b75b..1d02dece63 100644 --- a/pkg/api/handlers/compat/containers_stats.go +++ b/pkg/api/handlers/compat/containers_stats.go @@ -3,7 +3,9 @@ package compat import ( + "bytes" "encoding/json" + "errors" "fmt" "net/http" "time" @@ -45,20 +47,17 @@ func StatsContainer(w http.ResponseWriter, r *http.Request) { stats, err := ctnr.GetContainerStats(nil) if err != nil { - utils.InternalServerError(w, fmt.Errorf("failed to obtain Container %s stats: %w", name, err)) + err = fmt.Errorf("failed to obtain Container %s stats: %w", name, err) + utils.Error(w, statsErrorStatus(err), err) return } - - coder := json.NewEncoder(w) - // Write header and content type. - w.WriteHeader(http.StatusOK) - w.Header().Set("Content-Type", "application/json") - if flusher, ok := w.(http.Flusher); ok { - flusher.Flush() + onlineCPUs, err := libpod.GetOnlineCPUs(ctnr) + if err != nil { + utils.Error(w, statsErrorStatus(err), err) + return } + wroteContent := false - // Set up JSON encoder for streaming. - coder.SetEscapeHTML(true) var preRead time.Time var preCPUStats CPUStats if query.Stream { @@ -66,12 +65,6 @@ func StatsContainer(w http.ResponseWriter, r *http.Request) { preCPUStats = getPreCPUStats(stats) } - onlineCPUs, err := libpod.GetOnlineCPUs(ctnr) - if err != nil { - utils.InternalServerError(w, err) - return - } - streamLabel: // A label to flatten the scope select { case <-r.Context().Done(): @@ -80,11 +73,20 @@ streamLabel: // A label to flatten the scope default: stats, err = ctnr.GetContainerStats(stats) if err != nil { - logrus.Errorf("Unable to get container stats: %v", err) + if wroteContent { + logrus.Errorf("Unable to get container stats: %v", err) + } else { + utils.Error(w, statsErrorStatus(err), err) + } return } s, err := statsContainerJSON(ctnr, stats, preCPUStats, onlineCPUs) if err != nil { + if wroteContent { + logrus.Errorf("Unable to build container stats response: %v", err) + } else { + utils.Error(w, statsErrorStatus(err), err) + } return } s.Stats.PreRead = preRead @@ -96,10 +98,25 @@ streamLabel: // A label to flatten the scope jsonOut = DockerStatsJSON(s) } - if err := coder.Encode(jsonOut); err != nil { - logrus.Errorf("Unable to encode stats: %v", err) + var chunk bytes.Buffer + if err := json.NewEncoder(&chunk).Encode(jsonOut); err != nil { + if wroteContent { + logrus.Errorf("Unable to encode stats: %v", err) + } else { + utils.InternalServerError(w, err) + } return } + + // Do not commit a successful response until the complete sample has + // been collected and encoded. For a stream, each write is one complete + // sample, so an error can only truncate the stream between samples. + w.Header().Set("Content-Type", "application/json") + if _, err := w.Write(chunk.Bytes()); err != nil { + logrus.Errorf("Unable to write stats: %v", err) + return + } + wroteContent = true if flusher, ok := w.(http.Flusher); ok { flusher.Flush() } @@ -112,11 +129,24 @@ streamLabel: // A label to flatten the scope bits, err := json.Marshal(s.CPUStats) if err != nil { logrus.Errorf("Unable to marshal cpu stats: %q", err) + return } if err := json.Unmarshal(bits, &preCPUStats); err != nil { logrus.Errorf("Unable to unmarshal previous stats: %q", err) + return } time.Sleep(defaultStatsPeriod) goto streamLabel } } + +func statsErrorStatus(err error) int { + switch { + case errors.Is(err, define.ErrNoSuchCtr), errors.Is(err, define.ErrCtrRemoved): + return http.StatusNotFound + case errors.Is(err, define.ErrCtrStopped), errors.Is(err, define.ErrCtrStateInvalid), errors.Is(err, define.ErrNoCgroups): + return http.StatusConflict + default: + return http.StatusInternalServerError + } +} diff --git a/pkg/api/handlers/compat/containers_stats_test.go b/pkg/api/handlers/compat/containers_stats_test.go new file mode 100644 index 0000000000..b294566370 --- /dev/null +++ b/pkg/api/handlers/compat/containers_stats_test.go @@ -0,0 +1,36 @@ +//go:build !remote && (linux || freebsd) + +package compat + +import ( + "errors" + "fmt" + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "go.podman.io/podman/v6/libpod/define" +) + +func TestStatsErrorStatus(t *testing.T) { + testCases := []struct { + name string + err error + want int + }{ + {name: "not found", err: define.ErrNoSuchCtr, want: http.StatusNotFound}, + {name: "removed", err: define.ErrCtrRemoved, want: http.StatusNotFound}, + {name: "stopped", err: define.ErrCtrStopped, want: http.StatusConflict}, + {name: "invalid state", err: define.ErrCtrStateInvalid, want: http.StatusConflict}, + {name: "no cgroups", err: define.ErrNoCgroups, want: http.StatusConflict}, + {name: "wrapped stopped", err: fmt.Errorf("collecting stats: %w", define.ErrCtrStopped), want: http.StatusConflict}, + {name: "internal", err: errors.New("permission denied"), want: http.StatusInternalServerError}, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + got := statsErrorStatus(testCase.err) + assert.Equal(t, testCase.want, got) + }) + } +} diff --git a/pkg/api/server/register_containers.go b/pkg/api/server/register_containers.go index 5cd368efce..cdf3f609fb 100644 --- a/pkg/api/server/register_containers.go +++ b/pkg/api/server/register_containers.go @@ -417,6 +417,8 @@ func (s *APIServer) registerContainersHandlers(r *mux.Router) error { // type: object // 404: // $ref: "#/responses/containerNotFound" + // 409: + // $ref: "#/responses/conflictError" // 500: // $ref: "#/responses/internalError" r.HandleFunc(VersionedPath("/containers/{name}/stats"), s.StreamBufferedAPIHandler(compat.StatsContainer)).Methods(http.MethodGet) diff --git a/test/apiv2/19-stats.at b/test/apiv2/19-stats.at index 4431ab8fbb..43a9a2e8a0 100644 --- a/test/apiv2/19-stats.at +++ b/test/apiv2/19-stats.at @@ -33,3 +33,11 @@ podman network rm testnet2 podman run -dt --name testctr3 --memory-reservation=10m $IMAGE top &>/dev/null t GET libpod/containers/testctr3/stats?stream=false 200 podman rm -f testctr3 + +# A stopped container cannot provide CPU stats. The handler must report the +# error before committing a successful response. +podman run --name stopped-stats $IMAGE true +t GET libpod/containers/stopped-stats/stats?stream=false 409 \ + .cause="container is stopped" \ + .response=409 +podman rm stopped-stats diff --git a/winmake.ps1 b/winmake.ps1 index c8b56950af..be0ae13c93 100644 --- a/winmake.ps1 +++ b/winmake.ps1 @@ -66,7 +66,8 @@ function Make-Clean { function Local-Unit { Build-Ginkgo - $skippackages = 'hack,internal\domain\infra\abi,internal\domain\infra\tunnel,libpod\lock\shm,pkg\api\handlers\libpod,pkg\api\handlers\utils,pkg\bindings,' + # The compat stats handler depends on the local libpod stats implementation, which is not built on Windows. + $skippackages = 'hack,internal\domain\infra\abi,internal\domain\infra\tunnel,libpod\lock\shm,pkg\api\handlers\compat,pkg\api\handlers\libpod,pkg\api\handlers\utils,pkg\bindings,' $skippackages += 'pkg\domain\infra\abi,pkg\emulation,pkg\machine\apple,pkg\machine\applehv,pkg\machine\e2e,pkg\machine\libkrun,' $skippackages += 'pkg\machine\proxyenv,pkg\machine\qemu,pkg\specgen\generate,pkg\systemd,test\e2e,test\utils,cmd\rootlessport,' $skippackages += 'pkg\pidhandle'