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'