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 <hiroaki.kawai@gmail.com>

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

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
+	}
+}

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)
+		})
+	}
+}

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)

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

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'