runsc metric-server: Ensure iteration ID is stable across server restarts.

Prior to this CL, the metric server generated its own ID when it discovered
a new sandbox. This means existing sandboxes get new iteration IDs, which
breaks the continuity of counter metrics across process restarts.

This CL uses the container creation time in the sandbox state file, and uses
this (together with the sandbox ID) to generate a unique ID for each
instantiation of a sandbox with a given ID.

Added test to verify this behavior.

PiperOrigin-RevId: 505208685

pkg/prometheus/prometheus.go

@@ -31,6 +31,14 @@ import (
 // timeNow is the time.Now() function. Can be mocked in tests.
 var timeNow = time.Now
 
+// Prometheus label names used to identify each sandbox.
+const (
+	SandboxIDLabel   = "sandbox"
+	PodNameLabel     = "pod"
+	NamespaceLabel   = "namespace"
+	IterationIDLabel = "iteration"
+)
+
 // Type is a Prometheus metric type.
 type Type int
 

runsc/cmd/metric_export.go

@@ -65,6 +65,11 @@ func (m *MetricExport) Execute(ctx context.Context, f *flag.FlagSet, args ...any
 		util.Fatalf("loading container: %v", err)
 	}
 
+	prometheusLabels, err := sandboxPrometheusLabels(cont)
+	if err != nil {
+		util.Fatalf("Cannot compute Prometheus labels of sandbox: %v", err)
+	}
+
 	snapshot, err := cont.Sandbox.ExportMetrics()
 	if err != nil {
 		util.Fatalf("ExportMetrics failed: %v", err)
@@ -74,7 +79,7 @@ func (m *MetricExport) Execute(ctx context.Context, f *flag.FlagSet, args ...any
 	}, map[*prometheus.Snapshot]prometheus.SnapshotExportOptions{
 		snapshot: {
 			ExporterPrefix: conf.MetricExporterPrefix,
-			ExtraLabels:    cont.Sandbox.PrometheusLabels(),
+			ExtraLabels:    prometheusLabels,
 		},
 	})
 	if err != nil {

runsc/cmd/metric_server.go

@@ -16,6 +16,8 @@ package cmd
 
 import (
 	"context"
+	"crypto/sha256"
+	"encoding/binary"
 	"errors"
 	"fmt"
 	"io"
@@ -25,6 +27,7 @@ import (
 	"net/http"
 	"os"
 	"os/signal"
+	"strconv"
 	"strings"
 	"syscall"
 	"time"
@@ -60,11 +63,6 @@ const (
 	exportParallelGoroutines = 8
 )
 
-// Prometheus label names.
-const (
-	iterationIDLabel = "iterationid"
-)
-
 // servedSandbox is a sandbox that we serve metrics from.
 // A single metrics server will export data about multiple sandboxes.
 type servedSandbox struct {
@@ -80,6 +78,11 @@ type servedSandbox struct {
 	// Once set, it is immutable.
 	sandbox *sandbox.Sandbox
 
+	// createdAt stores the time the sandbox was created.
+	// It is loaded from the container state file.
+	// Once set, it is immutable.
+	createdAt time.Time
+
 	// labelsWithMetadata is the union of `extraLabels` and `sandbox.MetricMetadata`.
 	// This is exported as the set of labels for the `sandbox_metadata` metric.
 	// Once set, it is immutable.
@@ -97,6 +100,36 @@ type servedSandbox struct {
 	verifier *prometheus.Verifier
 }
 
+// sandboxPrometheusLabels returns a set of Prometheus labels that identifies the sandbox running
+// the given root container.
+func sandboxPrometheusLabels(rootContainer *container.Container) (map[string]string, error) {
+	s := rootContainer.Sandbox
+	labels := make(map[string]string, 4)
+	labels[prometheus.SandboxIDLabel] = s.ID
+
+	// Compute iteration ID label in a stable manner.
+	// This uses sha256(ID + ":" + creation time).
+	h := sha256.New()
+	if _, err := io.WriteString(h, s.ID); err != nil {
+		return nil, err
+	}
+	if _, err := io.WriteString(h, ":"); err != nil {
+		return nil, err
+	}
+	if _, err := io.WriteString(h, rootContainer.CreatedAt.UTC().String()); err != nil {
+		return nil, err
+	}
+	labels[prometheus.IterationIDLabel] = strconv.FormatUint(binary.BigEndian.Uint64(h.Sum(nil)[:8]), 36)
+
+	if s.PodName != "" {
+		labels[prometheus.PodNameLabel] = s.PodName
+	}
+	if s.Namespace != "" {
+		labels[prometheus.NamespaceLabel] = s.Namespace
+	}
+	return labels, nil
+}
+
 // load loads the sandbox being monitored and initializes its metric verifier.
 // If it returns an error other than container.ErrStateFileLocked, the sandbox is either
 // non-existent, or has not requested instrumentation to be enabled, or does not have
@@ -124,14 +157,24 @@ func (s *servedSandbox) load() (*sandbox.Sandbox, *prometheus.Verifier, error) {
 		}
 		// Update label data as read from the state file.
 		// Do not store empty labels.
-		authoritativeLabels := cont.Sandbox.PrometheusLabels()
-		for _, label := range []string{sandbox.SandboxIDLabel, sandbox.PodNameLabel, sandbox.NamespaceLabel} {
+		authoritativeLabels, err := sandboxPrometheusLabels(cont)
+		if err != nil {
+			return nil, nil, fmt.Errorf("cannot compute Prometheus labels of sandbox: %v", err)
+		}
+		s.extraLabels = make(map[string]string, len(authoritativeLabels))
+		for _, label := range []string{
+			prometheus.SandboxIDLabel,
+			prometheus.IterationIDLabel,
+			prometheus.PodNameLabel,
+			prometheus.NamespaceLabel,
+		} {
 			s.extraLabels[label] = authoritativeLabels[label]
 			if s.extraLabels[label] == "" {
 				delete(s.extraLabels, label)
 			}
 		}
 		s.sandbox = cont.Sandbox
+		s.createdAt = cont.CreatedAt
 	}
 	if s.verifier == nil {
 		registeredMetrics, err := s.sandbox.GetRegisteredMetrics()
@@ -191,7 +234,6 @@ type MetricServer struct {
 	address        string
 	exporterPrefix string
 	startTime      time.Time
-	rand           *rand.Rand
 	srv            http.Server
 
 	// Size of the map of written metrics during the last /metrics export. Initially zero.
@@ -301,9 +343,17 @@ func (m *MetricServer) refreshSandboxesLocked() {
 		if !found {
 			log.Warningf("Sandbox %s no longer exists but did not explicitly unregister. Removing it.", sandboxID)
 			delete(m.sandboxes, sandboxID)
-		} else if _, _, err := sandbox.load(); err != nil && err != container.ErrStateFileLocked {
+			continue
+		}
+		if _, _, err := sandbox.load(); err != nil && err != container.ErrStateFileLocked {
 			log.Warningf("Sandbox %s cannot be loaded, deleting it: %v", sandboxID, err)
 			delete(m.sandboxes, sandboxID)
+			continue
+		}
+		if !sandbox.sandbox.IsRunning() {
+			log.Infof("Sandbox %s is no longer running, deleting it.", sandboxID)
+			delete(m.sandboxes, sandboxID)
+			continue
 		}
 	}
 	newSandboxIDs := make(map[container.FullID]bool, len(sandboxIDs))
@@ -370,8 +420,7 @@ func (m *MetricServer) refreshSandboxesLocked() {
 			rootDir:          m.rootDir,
 			metricServerAddr: m.address,
 			extraLabels: map[string]string{
-				sandbox.SandboxIDLabel: sid.SandboxID,
-				iterationIDLabel:       fmt.Sprintf("%d", m.rand.Uint64()),
+				prometheus.SandboxIDLabel: sid.SandboxID,
 			},
 		}
 		// Best-effort attempt to load the state file instantly.
@@ -750,7 +799,6 @@ func (m *MetricServer) Execute(ctx context.Context, f *flag.FlagSet, args ...any
 		return util.Errorf("Invalid root directory %q: tried to list all entries within it and got: %v", conf.RootDir, err)
 	}
 	m.startTime = time.Now()
-	m.rand = rand.New(rand.NewSource(m.startTime.UnixNano()))
 	m.rootDir = conf.RootDir
 	m.exporterPrefix = conf.MetricExporterPrefix
 	if strings.Contains(conf.MetricServer, "%RUNTIME_ROOT%") {

runsc/container/BUILD

@@ -87,6 +87,7 @@ go_test(
         "//runsc/specutils",
         "//test/metricclient",
         "@com_github_cenkalti_backoff//:go_default_library",
+        "@com_github_google_go_cmp//cmp:go_default_library",
         "@com_github_kr_pty//:go_default_library",
         "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
         "@org_golang_google_protobuf//proto:go_default_library",

runsc/container/metric_server_test.go

@@ -20,9 +20,11 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"gvisor.dev/gvisor/pkg/cleanup"
 	"gvisor.dev/gvisor/pkg/test/testutil"
@@ -197,6 +199,67 @@ func TestContainerMetrics(t *testing.T) {
 	}
 }
 
+// TestContainerMetricsIterationID verifies that two successive containers with the same ID
+// do not have the same iteration ID.
+func TestContainerMetricsIterationID(t *testing.T) {
+	te, cleanup := setupMetrics(t)
+	defer cleanup()
+
+	args := Args{
+		ID:        testutil.RandomContainerID(),
+		Spec:      te.sleepSpec,
+		BundleDir: te.bundleDir,
+	}
+	cont1, err := New(te.sleepConf, args)
+	if err != nil {
+		t.Fatalf("error creating container 1: %v", err)
+	}
+	defer cont1.Destroy()
+	data1, err := te.client.GetMetrics(te.testCtx)
+	if err != nil {
+		t.Errorf("Cannot get metrics after creating container 1: %v", err)
+	}
+	metadata1, err := data1.GetSandboxMetadataMetric(metricclient.WantMetric{
+		Metric:  "testmetric_meta_sandbox_metadata",
+		Sandbox: args.ID,
+	})
+	if err != nil {
+		t.Errorf("Cannot get sandbox 1 metadata: %v", err)
+	}
+	t.Logf("Container 1 metadata: %v", metadata1)
+	iterationID1 := metadata1["iteration"]
+	if iterationID1 == "" {
+		t.Fatalf("Cannot find iteration ID in metadata 1: %v", metadata1)
+	}
+	if err := cont1.Destroy(); err != nil && !strings.Contains(err.Error(), "no child process") {
+		t.Fatalf("Cannot destroy container 1: %v", err)
+	}
+	cont2, err := New(te.sleepConf, args)
+	if err != nil {
+		t.Fatalf("error creating container 2: %v", err)
+	}
+	defer cont2.Destroy()
+	data2, err := te.client.GetMetrics(te.testCtx)
+	if err != nil {
+		t.Errorf("Cannot get metrics after creating container 2: %v", err)
+	}
+	metadata2, err := data2.GetSandboxMetadataMetric(metricclient.WantMetric{
+		Metric:  "testmetric_meta_sandbox_metadata",
+		Sandbox: args.ID,
+	})
+	if err != nil {
+		t.Errorf("Cannot get sandbox 2 metadata: %v", err)
+	}
+	t.Logf("Container 2 metadata: %v", metadata2)
+	iterationID2 := metadata2["iteration"]
+	if iterationID2 == "" {
+		t.Fatalf("Cannot find iteration ID in metadata 2: %v", metadata2)
+	}
+	if iterationID1 == iterationID2 {
+		t.Errorf("Iteration IDs of successive instances with the same ID unexpectedly matched: %v", iterationID1)
+	}
+}
+
 // TestContainerMetricsRobustAgainstRestarts that exporting metrics is robust against metric server
 // unavailability or restarts.
 func TestContainerMetricsRobustAgainstRestarts(t *testing.T) {
@@ -236,6 +299,13 @@ func TestContainerMetricsRobustAgainstRestarts(t *testing.T) {
 	if err != nil {
 		t.Errorf("Cannot get testmetric_fs_opens from following data (err: %v):\n\n%s\n\n", err, preRestartData)
 	}
+	preRestartMetadata, err := preRestartData.GetSandboxMetadataMetric(metricclient.WantMetric{
+		Metric:  "testmetric_meta_sandbox_metadata",
+		Sandbox: args.ID,
+	})
+	if err != nil {
+		t.Errorf("Cannot get sandbox metadata: %v", err)
+	}
 	t.Logf("After exec'ing %d open()s, fs_opens=%d (snapshotted at %v)", targetOpens, preRestartOpens, postExecTimestamp)
 
 	// Now shut down the metric server and verify we can no longer fetch metrics.
@@ -313,6 +383,16 @@ func TestContainerMetricsRobustAgainstRestarts(t *testing.T) {
 	if diff := postRestartOpens - preRestartOpens; diff < int64(targetOpens) {
 		t.Errorf("testmetric_fs_opens for first container did not increase by at least %d after metric server restart: went from %d to %d (diff: %d)", targetOpens, preRestartOpens, postRestartOpens, diff)
 	}
+	postRestartMetadata, err := postRestartData.GetSandboxMetadataMetric(metricclient.WantMetric{
+		Metric:  "testmetric_meta_sandbox_metadata",
+		Sandbox: args.ID,
+	})
+	if err != nil {
+		t.Fatalf("Cannot get post-restart sandbox metadata: %v", err)
+	}
+	if diff := cmp.Diff(preRestartMetadata, postRestartMetadata); diff != "" {
+		t.Errorf("Sandbox metadata changed after restart:\nBefore: %v\nAfter: %v\nDiff: %v", preRestartMetadata, postRestartMetadata, diff)
+	}
 	_, _, err = postRestartData.GetPrometheusContainerInteger(metricclient.WantMetric{
 		Metric:  "testmetric_fs_opens",
 		Sandbox: args2.ID,

runsc/sandbox/sandbox.go

@@ -1183,26 +1183,6 @@ func (s *Sandbox) ExportMetrics() (*prometheus.Snapshot, error) {
 	return data.Snapshot, nil
 }
 
-// Label names used to identify each sandbox.
-const (
-	SandboxIDLabel = "sandbox"
-	PodNameLabel   = "pod"
-	NamespaceLabel = "namespace"
-)
-
-// PrometheusLabels returns a set of Prometheus labels that identifies the sandbox.
-func (s *Sandbox) PrometheusLabels() map[string]string {
-	labels := make(map[string]string, 3)
-	labels[SandboxIDLabel] = s.ID
-	if s.PodName != "" {
-		labels[PodNameLabel] = s.PodName
-	}
-	if s.Namespace != "" {
-		labels[NamespaceLabel] = s.Namespace
-	}
-	return labels
-}
-
 // IsRunning returns true if the sandbox or gofer process is running.
 func (s *Sandbox) IsRunning() bool {
 	pid := s.Pid.load()

test/metricclient/BUILD

@@ -12,9 +12,9 @@ go_library(
         "//runsc:__subpackages__",
     ],
     deps = [
+        "//pkg/prometheus",
         "//pkg/sync",
         "//runsc/config",
-        "//runsc/sandbox",
         "//runsc/specutils",
         "@com_github_cenkalti_backoff//:go_default_library",
         "@com_github_prometheus_common//expfmt",

test/metricclient/metricclient.go

@@ -34,9 +34,9 @@ import (
 	"github.com/cenkalti/backoff"
 	"github.com/prometheus/common/expfmt"
 	"golang.org/x/sys/unix"
+	"gvisor.dev/gvisor/pkg/prometheus"
 	"gvisor.dev/gvisor/pkg/sync"
 	"gvisor.dev/gvisor/runsc/config"
-	"gvisor.dev/gvisor/runsc/sandbox"
 	"gvisor.dev/gvisor/runsc/specutils"
 )
 
@@ -381,9 +381,9 @@ func (m MetricData) GetSandboxMetadataMetric(want WantMetric) (map[string]string
 		}
 		allMatching := true
 		for wantLabel, wantValue := range map[string]string{
-			sandbox.SandboxIDLabel: want.Sandbox,
-			sandbox.NamespaceLabel: want.Namespace,
-			sandbox.PodNameLabel:   want.Pod,
+			prometheus.SandboxIDLabel: want.Sandbox,
+			prometheus.NamespaceLabel: want.Namespace,
+			prometheus.PodNameLabel:   want.Pod,
 		} {
 			if dataLabels[wantLabel] != wantValue {
 				allMatching = false
@@ -403,7 +403,7 @@ func (m MetricData) GetSandboxMetadataMetric(want WantMetric) (map[string]string
 	data := metricData.GetMetric()[foundIndex]
 	metadataLabels := make(map[string]string, len(data.GetLabel()))
 	for _, label := range data.GetLabel() {
-		if label.GetName() == sandbox.SandboxIDLabel || label.GetName() == sandbox.NamespaceLabel || label.GetName() == sandbox.PodNameLabel {
+		if label.GetName() == prometheus.SandboxIDLabel || label.GetName() == prometheus.NamespaceLabel || label.GetName() == prometheus.PodNameLabel {
 			continue
 		}
 		metadataLabels[label.GetName()] = label.GetValue()