From 87b3c88c322cb51ccadc4af087e8b6a4c25f0a6f Mon Sep 17 00:00:00 2001 From: Etienne Perot Date: Fri, 27 Jan 2023 14:21:32 -0800 Subject: [PATCH] `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 | 8 +++ runsc/cmd/metric_export.go | 7 ++- runsc/cmd/metric_server.go | 72 ++++++++++++++++++++---- runsc/container/BUILD | 1 + runsc/container/metric_server_test.go | 80 +++++++++++++++++++++++++++ runsc/sandbox/sandbox.go | 20 ------- test/metricclient/BUILD | 2 +- test/metricclient/metricclient.go | 10 ++-- 8 files changed, 161 insertions(+), 39 deletions(-) diff --git a/pkg/prometheus/prometheus.go b/pkg/prometheus/prometheus.go index 7ff46a28d..c5831bc31 100644 --- a/pkg/prometheus/prometheus.go +++ b/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 diff --git a/runsc/cmd/metric_export.go b/runsc/cmd/metric_export.go index ec0ea73a4..260afec23 100644 --- a/runsc/cmd/metric_export.go +++ b/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 { diff --git a/runsc/cmd/metric_server.go b/runsc/cmd/metric_server.go index 938e2ee5d..ac2239b25 100644 --- a/runsc/cmd/metric_server.go +++ b/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%") { diff --git a/runsc/container/BUILD b/runsc/container/BUILD index 5ddfb0dfe..d445ed7d3 100644 --- a/runsc/container/BUILD +++ b/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", diff --git a/runsc/container/metric_server_test.go b/runsc/container/metric_server_test.go index d1d3fe839..bf911834f 100644 --- a/runsc/container/metric_server_test.go +++ b/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, diff --git a/runsc/sandbox/sandbox.go b/runsc/sandbox/sandbox.go index 43bef6efc..41aa5156e 100644 --- a/runsc/sandbox/sandbox.go +++ b/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() diff --git a/test/metricclient/BUILD b/test/metricclient/BUILD index c46aa5d7c..448c4d228 100644 --- a/test/metricclient/BUILD +++ b/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", diff --git a/test/metricclient/metricclient.go b/test/metricclient/metricclient.go index c702ad164..e5aeb232c 100644 --- a/test/metricclient/metricclient.go +++ b/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()