From 3c2f1972c2210ac415c263ad8df26abd1f1cb23d Mon Sep 17 00:00:00 2001 From: Etienne Perot Date: Fri, 31 Mar 2023 18:08:32 -0700 Subject: [PATCH] `runsc metric-server`: Add query parameter to only export a subset of metrics. This adds an optional `GET` parameter `runsc-sandbox-metrics-filter` which can be used to filter the set of sandbox metrics requested from each sandbox. In cases where the metric client isn't interested in learning everything about the sandboxes, this saves on RPC cost and latency. This also has the benefit of reducing memory usage of the `metric-server` further, as it does not need to hold onto data that it will not export. As the sandbox process is untrusted, the metrics returned are double-checked on the `metric-server` side to ensure that the filter was respected. PiperOrigin-RevId: 521055953 --- g3doc/user_guide/observability.md | 6 ++ pkg/metric/metric.go | 15 ++- pkg/metric/metric_test.go | 2 +- pkg/sentry/control/metrics.go | 73 ++++++++++++- runsc/cmd/metric_export.go | 15 ++- runsc/cmd/metric_server.go | 36 ++++++- runsc/container/metric_server_test.go | 141 +++++++++++++++++++++++--- runsc/sandbox/sandbox.go | 11 +- test/metricclient/metricclient.go | 37 ++++--- 9 files changed, 293 insertions(+), 43 deletions(-) diff --git a/g3doc/user_guide/observability.md b/g3doc/user_guide/observability.md index ad7e2122f..6fbd83870 100644 --- a/g3doc/user_guide/observability.md +++ b/g3doc/user_guide/observability.md @@ -160,6 +160,12 @@ If desired, you can change the (prefix applied to all metric names) using the `--exporter-prefix` flag. It defaults to `runsc_`. +The sandbox metrics exported may be filtered by using the optional `GET` +parameter `runsc-sandbox-metrics-filter`, e.g. +`/metrics?runsc-sandbox-metrics-filter=fs_.*`. Metric names must fully match +this regular expression. Note that this filtering is performed before prepending +`--exporter-prefix` to metric names. + The metric server also supports listening on a [Unix Domain Socket](https://en.wikipedia.org/wiki/Unix_domain_socket). This can be convenient to avoid reserving port numbers on the machine's network diff --git a/pkg/metric/metric.go b/pkg/metric/metric.go index d416c728f..c0ba64f3d 100644 --- a/pkg/metric/metric.go +++ b/pkg/metric/metric.go @@ -1125,9 +1125,16 @@ func EmitMetricUpdate() { } } +// SnapshotOptions controls how snapshots are exported in GetSnapshot. +type SnapshotOptions struct { + // Filter, if set, should return true for metrics that should be written to + // the snapshot. If unset, all metrics are written to the snapshot. + Filter func(*prometheus.Metric) bool +} + // GetSnapshot returns a Prometheus snapshot of the metric data. // Returns ErrNotYetInitialized if metrics have not yet been initialized. -func GetSnapshot() (*prometheus.Snapshot, error) { +func GetSnapshot(options SnapshotOptions) (*prometheus.Snapshot, error) { if !initialized.Load() { return nil, ErrNotYetInitialized } @@ -1135,6 +1142,9 @@ func GetSnapshot() (*prometheus.Snapshot, error) { snapshot := prometheus.NewSnapshot() for k, v := range values.uint64Metrics { m := allMetrics.uint64Metrics[k] + if options.Filter != nil && !options.Filter(m.prometheusMetric) { + continue + } switch t := v.(type) { case uint64: if m.metadata.GetCumulative() && t == 0 { @@ -1157,6 +1167,9 @@ func GetSnapshot() (*prometheus.Snapshot, error) { } for k, dists := range values.distributionTotalSamples { m := allMetrics.distributionMetrics[k] + if options.Filter != nil && !options.Filter(m.prometheusMetric) { + continue + } distributionSamples := values.distributionMetrics[k] numFiniteBuckets := m.exponentialBucketer.NumFiniteBuckets() sampleSums := values.distributionSampleSum[k] diff --git a/pkg/metric/metric_test.go b/pkg/metric/metric_test.go index 7202f100b..df7b4e126 100644 --- a/pkg/metric/metric_test.go +++ b/pkg/metric/metric_test.go @@ -42,7 +42,7 @@ const ( // However, it does not verify that the data that was parsed actually matches the metric data. func verifyPrometheusParsing(t *testing.T) { t.Helper() - snapshot, err := GetSnapshot() + snapshot, err := GetSnapshot(SnapshotOptions{}) if err != nil { t.Errorf("failed to get Prometheus snapshot: %v", err) return diff --git a/pkg/sentry/control/metrics.go b/pkg/sentry/control/metrics.go index b73303e1b..c113858b3 100644 --- a/pkg/sentry/control/metrics.go +++ b/pkg/sentry/control/metrics.go @@ -15,9 +15,13 @@ package control import ( + "fmt" + "regexp" + "gvisor.dev/gvisor/pkg/metric" pb "gvisor.dev/gvisor/pkg/metric/metric_go_proto" "gvisor.dev/gvisor/pkg/prometheus" + "gvisor.dev/gvisor/pkg/sync" ) // Metrics includes metrics-related RPC stubs. @@ -46,7 +50,64 @@ func (u *Metrics) GetRegisteredMetrics(_ *GetRegisteredMetricsOpts, out *Metrics } // MetricsExportOpts contains metric exporting options. -type MetricsExportOpts struct{} +type MetricsExportOpts struct { + // If set, this is a regular expression that is used to filter the set of + // exported metrics. + OnlyMetrics string `json:"only_metrics"` +} + +var ( + // lastOnlyMetricsMu protects the variables below. + lastOnlyMetricsMu sync.Mutex + + // lastOnlyMetricsStr is the last value of the "only_metrics" parameter passed to + // MetricsExport. It is used to avoid re-compiling the regular expression on every + // request in the common case where a single metric scraper is scraping the sandbox + // metrics using the same filter in each request. + lastOnlyMetricsStr string + + // lastOnlyMetrics is the compiled version of lastOnlyMetricsStr. + lastOnlyMetrics *regexp.Regexp +) + +// filterFunc returns a filter function to filter relevant Prometheus metric names. +func (m *MetricsExportOpts) filterFunc() (func(*prometheus.Metric) bool, error) { + if m.OnlyMetrics == "" { + return nil, nil + } + lastOnlyMetricsMu.Lock() + defer lastOnlyMetricsMu.Unlock() + onlyMetricsReg := lastOnlyMetrics + if m.OnlyMetrics != lastOnlyMetricsStr { + reg, err := regexp.Compile(m.OnlyMetrics) + if err != nil { + return nil, fmt.Errorf("cannot compile regexp %q: %v", m.OnlyMetrics, err) + } + lastOnlyMetricsStr = m.OnlyMetrics + lastOnlyMetrics = reg + onlyMetricsReg = reg + } + return func(m *prometheus.Metric) bool { + return onlyMetricsReg.MatchString(m.Name) + }, nil +} + +// Verify verifies that the given exported data is compliant with the export +// options. This should be run client-side to double-check results. +func (m *MetricsExportOpts) Verify(data *MetricsExportData) error { + filterFunc, err := m.filterFunc() + if err != nil { + return err + } + if filterFunc != nil && data.Snapshot != nil { + for _, data := range data.Snapshot.Data { + if !filterFunc(data.Metric) { + return fmt.Errorf("metric %v violated the filter set in export options", data.Metric) + } + } + } + return nil +} // MetricsExportData contains data for all metrics being exported. type MetricsExportData struct { @@ -54,8 +115,14 @@ type MetricsExportData struct { } // Export export metrics data into MetricsExportData. -func (u *Metrics) Export(_ *MetricsExportOpts, out *MetricsExportData) error { - snapshot, err := metric.GetSnapshot() +func (u *Metrics) Export(opts *MetricsExportOpts, out *MetricsExportData) error { + filterFunc, err := opts.filterFunc() + if err != nil { + return err + } + snapshot, err := metric.GetSnapshot(metric.SnapshotOptions{ + Filter: filterFunc, + }) if err != nil { return err } diff --git a/runsc/cmd/metric_export.go b/runsc/cmd/metric_export.go index 9d7a9a342..b45f705aa 100644 --- a/runsc/cmd/metric_export.go +++ b/runsc/cmd/metric_export.go @@ -21,6 +21,7 @@ import ( "github.com/google/subcommands" "gvisor.dev/gvisor/pkg/prometheus" + "gvisor.dev/gvisor/pkg/sentry/control" "gvisor.dev/gvisor/runsc/cmd/util" "gvisor.dev/gvisor/runsc/config" "gvisor.dev/gvisor/runsc/container" @@ -29,7 +30,8 @@ import ( // MetricExport implements subcommands.Command for the "metric-export" command. type MetricExport struct { - exporterPrefix string + exporterPrefix string + sandboxMetricsFilter string } // Name implements subcommands.Command.Name. @@ -51,6 +53,7 @@ func (*MetricExport) Usage() string { // SetFlags implements subcommands.Command.SetFlags. func (m *MetricExport) SetFlags(f *flag.FlagSet) { f.StringVar(&m.exporterPrefix, "exporter-prefix", "runsc_", "Prefix for all metric names, following Prometheus exporter convention") + f.StringVar(&m.sandboxMetricsFilter, "sandbox-metrics-filter", "", "If set, filter exported metrics using the specified regular expression. This filtering is applied before adding --exporter-prefix.") } // Execute implements subcommands.Command.Execute. @@ -73,12 +76,18 @@ func (m *MetricExport) Execute(ctx context.Context, f *flag.FlagSet, args ...any util.Fatalf("Cannot compute Prometheus labels of sandbox: %v", err) } - snapshot, err := cont.Sandbox.ExportMetrics() + snapshot, err := cont.Sandbox.ExportMetrics(control.MetricsExportOpts{ + OnlyMetrics: m.sandboxMetricsFilter, + }) if err != nil { util.Fatalf("ExportMetrics failed: %v", err) } + commentHeader := fmt.Sprintf("Command-line export for sandbox %s", cont.Sandbox.ID) + if m.sandboxMetricsFilter != "" { + commentHeader = fmt.Sprintf("%s (filtered using regular expression: %q)", commentHeader, m.sandboxMetricsFilter) + } written, err := prometheus.Write(os.Stdout, prometheus.ExportOptions{ - CommentHeader: fmt.Sprintf("Command-line export for sandbox %s", cont.Sandbox.ID), + CommentHeader: commentHeader, }, map[*prometheus.Snapshot]prometheus.SnapshotExportOptions{ snapshot: { ExporterPrefix: m.exporterPrefix, diff --git a/runsc/cmd/metric_server.go b/runsc/cmd/metric_server.go index 8a254b855..08673a69c 100644 --- a/runsc/cmd/metric_server.go +++ b/runsc/cmd/metric_server.go @@ -27,6 +27,7 @@ import ( "net/http" "os" "os/signal" + "regexp" "runtime" "runtime/debug" "runtime/pprof" @@ -39,6 +40,7 @@ import ( "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/prometheus" + "gvisor.dev/gvisor/pkg/sentry/control" "gvisor.dev/gvisor/pkg/state" "gvisor.dev/gvisor/pkg/sync" "gvisor.dev/gvisor/runsc/cmd/util" @@ -214,7 +216,7 @@ func (s *servedSandbox) cleanup() { } // queryMetrics queries the sandbox for metrics data. -func queryMetrics(ctx context.Context, sand *sandbox.Sandbox, verifier *prometheus.Verifier) (*prometheus.Snapshot, error) { +func (m *MetricServer) queryMetrics(ctx context.Context, sand *sandbox.Sandbox, verifier *prometheus.Verifier, metricsFilter string) (*prometheus.Snapshot, error) { ch := make(chan struct { snapshot *prometheus.Snapshot err error @@ -222,7 +224,9 @@ func queryMetrics(ctx context.Context, sand *sandbox.Sandbox, verifier *promethe canceled := make(chan struct{}, 1) defer close(canceled) go func() { - snapshot, err := sand.ExportMetrics() + snapshot, err := sand.ExportMetrics(control.MetricsExportOpts{ + OnlyMetrics: metricsFilter, + }) select { case <-canceled: case ch <- struct { @@ -281,6 +285,13 @@ type MetricServer struct { // info, we can assume that the last background scan already looked at it. lastStateFileStat map[container.FullID]os.FileInfo + // lastValidMetricFilter stores the last value of the "runsc-sandbox-metrics-filter" parameter for + // /metrics requests. + // It represents the last-known compilable regular expression that was passed to /metrics. + // It is used to avoid re-verifying this parameter in the common case where a single scraper + // is consistently passing in the same value for this parameter in each successive request. + lastValidMetricFilter string + // numSandboxes counts the number of sandboxes that have ever been registered on this server. // Used to distinguish between the case where this metrics serve has sat there doing nothing // because no sandbox ever registered against it (which is unexpected), vs the case where it has @@ -559,7 +570,20 @@ var ServerMetrics = []prometheus.Metric{ func (m *MetricServer) serveMetrics(w http.ResponseWriter, req *http.Request) httpResult { ctx, ctxCancel := context.WithTimeout(req.Context(), metricsExportTimeout) defer ctxCancel() + + metricsFilter := req.URL.Query().Get("runsc-sandbox-metrics-filter") + m.mu.Lock() + + if metricsFilter != "" && metricsFilter != m.lastValidMetricFilter { + _, err := regexp.Compile(metricsFilter) + if err != nil { + m.mu.Unlock() + return httpResult{http.StatusBadRequest, errors.New("provided metric filter is not a valid regular expression")} + } + m.lastValidMetricFilter = metricsFilter + } + m.refreshSandboxesLocked() numGoroutines := exportParallelGoroutines @@ -660,7 +684,7 @@ func (m *MetricServer) serveMetrics(w http.ResponseWriter, req *http.Request) ht sandboxErr := loadErr if loadErr == nil { queryCtx, queryCtxCancel := context.WithTimeout(ctx, perSandboxTime) - snapshot, sandboxErr = queryMetrics(queryCtx, sand, verifier) + snapshot, sandboxErr = m.queryMetrics(queryCtx, sand, verifier, metricsFilter) queryCtxCancel() isRunning = sand.IsRunning() } @@ -730,8 +754,12 @@ func (m *MetricServer) serveMetrics(w http.ResponseWriter, req *http.Request) ht // Write out all data. lastMetricsWrittenSize := int(m.lastMetricsWrittenSize.Load()) metricsWritten := make(map[string]bool, lastMetricsWrittenSize) + commentHeader := fmt.Sprintf("Data for runsc metric server exporting data for sandboxes in root directory %s", m.rootDir) + if metricsFilter != "" { + commentHeader = fmt.Sprintf("%s (filtered using regular expression: %q)", commentHeader, metricsFilter) + } written, err := prometheus.Write(w, prometheus.ExportOptions{ - CommentHeader: fmt.Sprintf("Data for runsc metric server exporting data for sandboxes in root directory %s", m.rootDir), + CommentHeader: commentHeader, MetricsWritten: metricsWritten, }, snapshotsToOptions) if err != nil { diff --git a/runsc/container/metric_server_test.go b/runsc/container/metric_server_test.go index 01f7dc92a..2da9b2a02 100644 --- a/runsc/container/metric_server_test.go +++ b/runsc/container/metric_server_test.go @@ -124,7 +124,7 @@ func TestContainerMetrics(t *testing.T) { te, cleanup := setupMetrics(t /* forceTempUDS= */, false) defer cleanup() - if _, err := te.client.GetMetrics(te.testCtx); err != nil { + if _, err := te.client.GetMetrics(te.testCtx, nil); err != nil { t.Fatal("GetMetrics failed prior to container start") } if te.sleepSpec.Annotations == nil { @@ -149,7 +149,7 @@ func TestContainerMetrics(t *testing.T) { if udsStat.Mode()&os.ModeSocket == 0 { t.Errorf("Stat(%s): Got mode %x, expected socket (mode %x)", te.udsPath, udsStat.Mode(), os.ModeSocket) } - initialData, err := te.client.GetMetrics(te.testCtx) + initialData, err := te.client.GetMetrics(te.testCtx, nil) if err != nil { t.Errorf("Cannot get metrics after creating container: %v", err) } @@ -169,7 +169,7 @@ func TestContainerMetrics(t *testing.T) { if err := cont.Start(te.sleepConf); err != nil { t.Fatalf("Cannot start container: %v", err) } - postStartData, err := te.client.GetMetrics(te.testCtx) + postStartData, err := te.client.GetMetrics(te.testCtx, nil) if err != nil { t.Fatalf("Cannot get metrics after starting container: %v", err) } @@ -188,7 +188,7 @@ func TestContainerMetrics(t *testing.T) { if err != nil { t.Fatalf("Exec failed: %v; output: %v", err, shOutput) } - postExecData, err := te.client.GetMetrics(te.testCtx) + postExecData, err := te.client.GetMetrics(te.testCtx, nil) if err != nil { t.Fatalf("Cannot get metrics after a bunch of open() calls: %v", err) } @@ -224,7 +224,7 @@ func TestContainerMetricsIterationID(t *testing.T) { t.Fatalf("error creating container 1: %v", err) } defer cont1.Destroy() - data1, err := te.client.GetMetrics(te.testCtx) + data1, err := te.client.GetMetrics(te.testCtx, nil) if err != nil { t.Errorf("Cannot get metrics after creating container 1: %v", err) } @@ -248,7 +248,7 @@ func TestContainerMetricsIterationID(t *testing.T) { t.Fatalf("error creating container 2: %v", err) } defer cont2.Destroy() - data2, err := te.client.GetMetrics(te.testCtx) + data2, err := te.client.GetMetrics(te.testCtx, nil) if err != nil { t.Errorf("Cannot get metrics after creating container 2: %v", err) } @@ -294,7 +294,7 @@ func TestContainerMetricsRobustAgainstRestarts(t *testing.T) { if err != nil { t.Fatalf("Exec failed: %v; output: %v", err, shOutput) } - preRestartData, err := te.client.GetMetrics(te.testCtx) + preRestartData, err := te.client.GetMetrics(te.testCtx, nil) if err != nil { t.Fatalf("Cannot get metrics after a bunch of open() calls: %v", err) } @@ -321,7 +321,7 @@ func TestContainerMetricsRobustAgainstRestarts(t *testing.T) { if err := te.client.ShutdownServer(te.testCtx); err != nil { t.Fatalf("Cannot shutdown server: %v", err) } - if rawData, err := te.client.GetMetrics(te.testCtx); err == nil { + if rawData, err := te.client.GetMetrics(te.testCtx, nil); err == nil { t.Fatalf("Unexpectedly was able to get metric data despite shutting down server:\n\n%s\n\n", rawData) } @@ -345,13 +345,13 @@ func TestContainerMetricsRobustAgainstRestarts(t *testing.T) { t.Fatalf("error creating second container: %v", err) } defer cont2.Destroy() - if rawData, err := te.client.GetMetrics(te.testCtx); err == nil { + if rawData, err := te.client.GetMetrics(te.testCtx, nil); err == nil { t.Fatalf("Unexpectedly was able to get metric data after creating second container:\n\n%s\n\n", rawData) } if err := cont2.Start(te.sleepConf); err != nil { t.Fatalf("Cannot start second container: %v", err) } - if rawData, err := te.client.GetMetrics(te.testCtx); err == nil { + if rawData, err := te.client.GetMetrics(te.testCtx, nil); err == nil { t.Fatalf("Unexpectedly was able to get metric data after starting second container:\n\n%s\n\n", rawData) } @@ -378,7 +378,7 @@ func TestContainerMetricsRobustAgainstRestarts(t *testing.T) { // Verify that the metric server was restarted and that we can indeed get all the data we expect // from all the containers this test has started. - postRestartData, err := te.client.GetMetrics(te.testCtx) + postRestartData, err := te.client.GetMetrics(te.testCtx, nil) if err != nil { t.Fatalf("Cannot get metrics after restarting server: %v", err) } @@ -470,7 +470,7 @@ func TestContainerMetricsMultiple(t *testing.T) { defer noMetricsCont.Destroy() // Verify that the metrics server says what we expect. - gotData, err := te.client.GetMetrics(te.testCtx) + gotData, err := te.client.GetMetrics(te.testCtx, nil) if err != nil { t.Fatalf("Cannot get metrics after starting containers: %v", err) } @@ -500,7 +500,7 @@ func TestContainerMetricsMultiple(t *testing.T) { } // Verify that now we only have half the containers. - gotData, err = te.client.GetMetrics(te.testCtx) + gotData, err = te.client.GetMetrics(te.testCtx, nil) if err != nil { t.Fatalf("Cannot get metrics after stopping half the containers: %v", err) } @@ -525,6 +525,117 @@ func TestContainerMetricsMultiple(t *testing.T) { } } +// TestContainerMetricsFilter verifies the ability to filter metrics in /metrics requests. +func TestContainerMetricsFilter(t *testing.T) { + te, cleanup := setupMetrics(t, false /* forceTempUDS */) + defer cleanup() + + args := Args{ + ID: testutil.RandomContainerID(), + Spec: te.sleepSpec, + BundleDir: te.bundleDir, + } + cont, err := New(te.sleepConf, args) + if err != nil { + t.Fatalf("error creating container: %v", err) + } + defer cont.Destroy() + if err := cont.Start(te.sleepConf); err != nil { + t.Fatalf("Cannot start container: %v", err) + } + + // First pass: Unfiltered data. + unfilteredData, err := te.client.GetMetrics(te.testCtx, nil) + if err != nil { + t.Fatalf("Cannot get metrics: %v", err) + } + _, _, err = unfilteredData.GetPrometheusContainerInteger(metricclient.WantMetric{ + Metric: "testmetric_fs_opens", + Sandbox: args.ID, + }) + if err != nil { + t.Errorf("Cannot get testmetric_fs_opens: %v", err) + } + _, err = unfilteredData.GetSandboxMetadataMetric(metricclient.WantMetric{ + Metric: "testmetric_meta_sandbox_metadata", + Sandbox: args.ID, + }) + if err != nil { + t.Errorf("Cannot get sandbox metadata: %v", err) + } + + // Second pass: Filter such that fs_opens does not match. + filteredData, err := te.client.GetMetrics(te.testCtx, map[string]string{ + "runsc-sandbox-metrics-filter": "^$", // Matches nothing. + }) + if err != nil { + t.Fatalf("Cannot get metrics: %v", err) + } + _, _, err = filteredData.GetPrometheusContainerInteger(metricclient.WantMetric{ + Metric: "testmetric_fs_opens", + Sandbox: args.ID, + }) + if err == nil { + t.Errorf("Was unexpectedly able to get fs_opens data from filtered data:\n\n%v\n\n", filteredData) + } + _, err = filteredData.GetSandboxMetadataMetric(metricclient.WantMetric{ + Metric: "testmetric_meta_sandbox_metadata", + Sandbox: args.ID, + }) + if err != nil { + t.Errorf("Cannot get sandbox metadata from filtered data: %v", err) + } + + // Third pass: Filter such that fs_opens does match. + filteredData2, err := te.client.GetMetrics(te.testCtx, map[string]string{ + "runsc-sandbox-metrics-filter": "^fs_.*$", + }) + if err != nil { + t.Fatalf("Cannot get metrics: %v", err) + } + _, _, err = filteredData2.GetPrometheusContainerInteger(metricclient.WantMetric{ + Metric: "testmetric_fs_opens", + Sandbox: args.ID, + }) + if err != nil { + t.Errorf("Cannot get testmetric_fs_opens from filtered data: %v", err) + } + _, err = filteredData2.GetSandboxMetadataMetric(metricclient.WantMetric{ + Metric: "testmetric_meta_sandbox_metadata", + Sandbox: args.ID, + }) + if err != nil { + t.Errorf("Cannot get sandbox metadata from filtered data: %v", err) + } + + // Fourth pass: Filter such that fs_opens does not match, then request with no filtering, + // to ensure that the filter regex caching is correctly applied. + _, err = te.client.GetMetrics(te.testCtx, map[string]string{ + "runsc-sandbox-metrics-filter": "^$", + }) + if err != nil { + t.Fatalf("Cannot get metrics: %v", err) + } + unfilteredData2, err := te.client.GetMetrics(te.testCtx, nil) + if err != nil { + t.Fatalf("Cannot get metrics: %v", err) + } + _, _, err = unfilteredData2.GetPrometheusContainerInteger(metricclient.WantMetric{ + Metric: "testmetric_fs_opens", + Sandbox: args.ID, + }) + if err != nil { + t.Errorf("Cannot get testmetric_fs_opens from unfiltered data: %v", err) + } + _, err = unfilteredData2.GetSandboxMetadataMetric(metricclient.WantMetric{ + Metric: "testmetric_meta_sandbox_metadata", + Sandbox: args.ID, + }) + if err != nil { + t.Errorf("Cannot get sandbox metadata from unfiltered data: %v", err) + } +} + func TestMetricServerChecksRootDirectoryAccess(t *testing.T) { te, cleanup := setupMetrics(t /* forceTempUDS= */, false) defer cleanup() @@ -614,7 +725,7 @@ func TestMetricServerDoesNotExportZeroValueCounters(t *testing.T) { if err := unimpl2.Start(unimpl2Conf); err != nil { t.Fatalf("Cannot start second container: %v", err) } - metricData, err := te.client.GetMetrics(te.testCtx) + metricData, err := te.client.GetMetrics(te.testCtx, nil) if err != nil { t.Fatalf("Cannot get metrics: %v", err) } @@ -662,7 +773,7 @@ func TestMetricServerDoesNotExportZeroValueCounters(t *testing.T) { } select { case <-time.After(20 * time.Millisecond): - newMetricData, err := te.client.GetMetrics(te.testCtx) + newMetricData, err := te.client.GetMetrics(te.testCtx, nil) if err != nil { t.Fatalf("Cannot get metrics: %v", err) } diff --git a/runsc/sandbox/sandbox.go b/runsc/sandbox/sandbox.go index a3d1490f6..e01f76869 100644 --- a/runsc/sandbox/sandbox.go +++ b/runsc/sandbox/sandbox.go @@ -1212,10 +1212,15 @@ func (s *Sandbox) GetRegisteredMetrics() (*metricpb.MetricRegistration, error) { } // ExportMetrics returns a snapshot of metric values from the sandbox in Prometheus format. -func (s *Sandbox) ExportMetrics() (*prometheus.Snapshot, error) { +func (s *Sandbox) ExportMetrics(opts control.MetricsExportOpts) (*prometheus.Snapshot, error) { log.Debugf("Metrics export sandbox %q", s.ID) - data := &control.MetricsExportData{} - if err := s.call(boot.MetricsExport, &control.MetricsExportOpts{}, data); err != nil { + var data control.MetricsExportData + if err := s.call(boot.MetricsExport, &opts, &data); err != nil { + return nil, err + } + // Since we do not trust the output of the sandbox as-is, double-check that the options were + // respected. + if err := opts.Verify(&data); err != nil { return nil, err } return data.Snapshot, nil diff --git a/test/metricclient/metricclient.go b/test/metricclient/metricclient.go index 92b7bed1c..423124d7e 100644 --- a/test/metricclient/metricclient.go +++ b/test/metricclient/metricclient.go @@ -97,27 +97,38 @@ func (c *MetricClient) Close() { // req performs an HTTP request against the metrics server. // It returns an http.Response, and a function to close out the request that should be called when // the response is no longer necessary. -func (c *MetricClient) req(ctx context.Context, timeout time.Duration, endpoint string, params map[string]string) (*http.Response, func(), error) { +func (c *MetricClient) req(ctx context.Context, timeout time.Duration, method, endpoint string, params map[string]string) (*http.Response, func(), error) { cancelFunc := context.CancelFunc(func() {}) if timeout != 0 { ctx, cancelFunc = context.WithTimeout(ctx, timeout) } - method := http.MethodGet var bodyBytes io.Reader - if params != nil { - method = http.MethodPost - values := url.Values{} - for k, v := range params { - values.Set(k, v) + var getSuffix string + if len(params) != 0 { + switch method { + case http.MethodGet: + getParams := url.Values{} + for k, v := range params { + getParams.Add(k, v) + } + getSuffix = fmt.Sprintf("?%s", getParams.Encode()) + case http.MethodPost: + values := url.Values{} + for k, v := range params { + values.Set(k, v) + } + bodyBytes = strings.NewReader(values.Encode()) + default: + cancelFunc() + return nil, nil, fmt.Errorf("unsupported method: %v", method) } - bodyBytes = strings.NewReader(values.Encode()) } - req, err := http.NewRequestWithContext(ctx, method, fmt.Sprintf("http://runsc-metrics%s", endpoint), bodyBytes) + req, err := http.NewRequestWithContext(ctx, method, fmt.Sprintf("http://runsc-metrics%s%s", endpoint, getSuffix), bodyBytes) if err != nil { cancelFunc() return nil, nil, fmt.Errorf("cannot create request object: %v", err) } - if params != nil { + if method == http.MethodPost { req.Header.Set("Content-Type", "application/x-www-form-urlencoded") } resp, err := c.client.Do(req) @@ -144,7 +155,7 @@ func (c *MetricClient) HealthCheck(ctx context.Context) error { // - The server is running, and the /runsc-metrics/healthcheck request succeeds. // - The server is running, but it is shutting down. The metrics server will fail the // /runsc-metrics/healthcheck request in this case. - resp, closeReq, err := c.req(ctx, 5*time.Second, "/runsc-metrics/healthcheck", map[string]string{ + resp, closeReq, err := c.req(ctx, 5*time.Second, http.MethodPost, "/runsc-metrics/healthcheck", map[string]string{ "root": c.rootDir, }) if err != nil { @@ -263,8 +274,8 @@ func (c *MetricClient) ShutdownServer(ctx context.Context) error { type MetricData string // GetMetrics returns the raw Prometheus-formatted metric data from the metric server. -func (c *MetricClient) GetMetrics(ctx context.Context) (MetricData, error) { - resp, closeReq, err := c.req(ctx, 10*time.Second, "/metrics", nil) +func (c *MetricClient) GetMetrics(ctx context.Context, urlParams map[string]string) (MetricData, error) { + resp, closeReq, err := c.req(ctx, 10*time.Second, http.MethodGet, "/metrics", urlParams) if err != nil { return "", fmt.Errorf("cannot get /metrics: %v", err) }