runsc: Move --metric-server-allow-unknown-root flag to a subcommand flag.

Prior to this change, the `--metric-server-allow-unknown-root` flag was set
at the `runsc` top level, which made sense for the case where this needed to
be set in e.g. Docker runtime flags with auto-spawn. Since this is no longer
the supported mode, having this flag at the top level no longer makes sense.

This change moves this flag to be a subcommand flag of `runsc metric-server`
instead, and renames it to `--allow-unknown-root` (since it's obvious it's
about the metric server now).

This change is backwards-incompatible to anyone who was specifying this flag,
but as this feature is very new and this flag was never documented, hopefully
this isn't much of a disruption.

Same deal for `--metric-exporter-prefix`, though this one is used in the
`runsc export-metrics` subcommand as well, so duplicated it in both places.

PiperOrigin-RevId: 514580178

g3doc/user_guide/observability.md

@@ -157,8 +157,8 @@ metric-server`. Simply point Prometheus at this address.
 
 If desired, you can change the
 [exporter name](https://prometheus.io/docs/instrumenting/writing_exporters/)
-(prefix applied to all metric names) using the `--metric-exporter-prefix` flag.
-It defaults to `runsc_`.
+(prefix applied to all metric names) using the `--exporter-prefix` flag. It
+defaults to `runsc_`.
 
 The metric server also supports listening on a
 [Unix Domain Socket](https://en.wikipedia.org/wiki/Unix_domain_socket). This can
@@ -277,8 +277,8 @@ own metrics as well. All metrics have documentation and type annotations in the
 
 *   `process_start_time_seconds`: Unix timestamp representing the time at which
     the metric server started. This specific metric name is used by Prometheus,
-    and as such its name is not affected by the `--metric-exporter-prefix` flag.
-    This metric is process-wide and has no labels.
+    and as such its name is not affected by the `--exporter-prefix` flag. This
+    metric is process-wide and has no labels.
 *   `num_sandboxes_total`: A process-wide metric representing the total number
     of sandboxes that the metric server knows about.
 *   `num_sandboxes_running`: A process-wide metric representing the number of

runsc/cmd/metric_export.go

@@ -29,6 +29,7 @@ import (
 
 // MetricExport implements subcommands.Command for the "metric-export" command.
 type MetricExport struct {
+	exporterPrefix string
 }
 
 // Name implements subcommands.Command.Name.
@@ -43,11 +44,13 @@ func (*MetricExport) Synopsis() string {
 
 // Usage implements subcommands.Command.Usage.
 func (*MetricExport) Usage() string {
-	return `export-metrics <container id> - prints sandbox metric data in Prometheus metric format`
+	return `export-metrics [-exporter-prefix=<runsc_>] <container id> - prints sandbox metric data in Prometheus metric format
+`
 }
 
 // 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")
 }
 
 // Execute implements subcommands.Command.Execute.
@@ -78,7 +81,7 @@ func (m *MetricExport) Execute(ctx context.Context, f *flag.FlagSet, args ...any
 		CommentHeader: fmt.Sprintf("Command-line export for sandbox %s", cont.Sandbox.ID),
 	}, map[*prometheus.Snapshot]prometheus.SnapshotExportOptions{
 		snapshot: {
-			ExporterPrefix: conf.MetricExporterPrefix,
+			ExporterPrefix: m.exporterPrefix,
 			ExtraLabels:    prometheusLabels,
 		},
 	})

runsc/cmd/metric_server.go

@@ -293,13 +293,16 @@ func (*MetricServer) Synopsis() string {
 
 // Usage implements subcommands.Command.Usage.
 func (*MetricServer) Usage() string {
-	return `-root=<root dir> -metric-server=<addr> [-metric-exporter-prefix=<prefix_>] metric-server`
+	return `-root=<root dir> -metric-server=<addr> metric-server [-exporter-prefix=<runsc_>]
+`
 }
 
 // SetFlags implements subcommands.Command.SetFlags.
 func (m *MetricServer) SetFlags(f *flag.FlagSet) {
+	f.StringVar(&m.exporterPrefix, "exporter-prefix", "runsc_", "Prefix for all metric names, following Prometheus exporter convention")
 	f.StringVar(&m.pidFile, "pid-file", "", "If set, write the metric server's own PID to this file after binding to the --metric-server address. The parent directory of this file must already exist.")
 	f.BoolVar(&m.exposeProfileEndpoints, "allow-profiling", false, "If true, expose /runsc-metrics/profile-cpu and /runsc-metrics/profile-heap to get profiling data about the metric server")
+	f.BoolVar(&m.allowUnknownRoot, "allow-unknown-root", false, "if set, the metric server will keep running regardless of the existence of --root or the metric server's ability to access it.")
 }
 
 // sufficientlyEqualStats returns whether the given FileInfo's are sufficiently
@@ -901,7 +904,7 @@ func (m *MetricServer) Execute(ctx context.Context, f *flag.FlagSet, args ...any
 		return util.Errorf("Metric server address contains '%%ID%%': %v. This should have been replaced by the parent process.", conf.MetricServer)
 	}
 	if _, err := container.ListSandboxes(conf.RootDir); err != nil {
-		if !conf.MetricServerAllowUnknownRoot {
+		if !m.allowUnknownRoot {
 			return util.Errorf("Invalid root directory %q: tried to list sandboxes within it and got: %v", conf.RootDir, err)
 		}
 		log.Warningf("Invalid root directory %q: tried to list sandboxes within it and got: %v. Continuing anyway, as the server is configured to tolerate this.", conf.RootDir, err)
@@ -909,15 +912,13 @@ func (m *MetricServer) Execute(ctx context.Context, f *flag.FlagSet, args ...any
 	// container.ListSandboxes uses a glob pattern, which doesn't error out on
 	// permission errors. Double-check by actually listing the directory.
 	if _, err := ioutil.ReadDir(conf.RootDir); err != nil {
-		if !conf.MetricServerAllowUnknownRoot {
+		if !m.allowUnknownRoot {
 			return util.Errorf("Invalid root directory %q: tried to list all entries within it and got: %v", conf.RootDir, err)
 		}
 		log.Warningf("Invalid root directory %q: tried to list all entries within it and got: %v. Continuing anyway, as the server is configured to tolerate this.", conf.RootDir, err)
 	}
 	m.startTime = time.Now()
 	m.rootDir = conf.RootDir
-	m.allowUnknownRoot = conf.MetricServerAllowUnknownRoot
-	m.exporterPrefix = conf.MetricExporterPrefix
 	if strings.Contains(conf.MetricServer, "%RUNTIME_ROOT%") {
 		newAddr := strings.ReplaceAll(conf.MetricServer, "%RUNTIME_ROOT%", m.rootDir)
 		log.Infof("Metric server address replaced %RUNTIME_ROOT%: %q -> %q", conf.MetricServer, newAddr)

runsc/config/config.go

@@ -154,17 +154,6 @@ type Config struct {
 	// The value of this flag must also match across the two command lines.
 	MetricServer string `flag:"metric-server"`
 
-	// MetricServerAllowUnknownRoot, if set, makes the metric server tolerate a non-existent or bad
-	// --root directory, and will remain running regardless of its validity.
-	// This is useful if the existence of the --root directory depends on the state of the machine,
-	// e.g. it is only created after the first pod that uses runsc has been created.
-	MetricServerAllowUnknownRoot bool `flag:"metric-server-allow-unknown-root"`
-
-	// MetricExporterPrefix is added as prefix to all metric names.
-	// It is used to follow Prometheus's exporter convention, whereby all metric names should be
-	// prefixed by a name meaningfully identifying the software exporting the metric.
-	MetricExporterPrefix string `flag:"metric-exporter-prefix"`
-
 	// Strace indicates that strace should be enabled.
 	Strace bool `flag:"strace"`
 

runsc/config/flags.go

@@ -55,8 +55,6 @@ func RegisterFlags(flagSet *flag.FlagSet) {
 
 	// Metrics flags.
 	flagSet.String("metric-server", "", "if set, export metrics on this address. This may either be 1) 'addr:port' to export metrics on a specific network interface address, 2) ':port' for exporting metrics on all interfaces, or 3) an absolute path to a Unix Domain Socket. The substring '%ID%' will be replaced by the container ID, and '%RUNTIME_ROOT%' by the root. This flag must be specified in both `runsc metric-server` and `runsc create`, and their values must match.")
-	flagSet.Bool("metric-server-allow-unknown-root", false, "if set, the metric server will keep running regardless of the existence of --root or the metric server's ability to access it.")
-	flagSet.String("metric-exporter-prefix", "runsc_", "prefix for all metric names, following Prometheus exporter convention")
 
 	// Debugging flags: strace related
 	flagSet.Bool("strace", false, "enable strace.")

runsc/container/metric_server_test.go

@@ -43,13 +43,14 @@ const (
 
 // metricsTest is returned by setupMetrics.
 type metricsTest struct {
-	testCtx   context.Context
-	rootDir   string
-	bundleDir string
-	sleepSpec *specs.Spec
-	sleepConf *config.Config
-	udsPath   string
-	client    *metricclient.MetricClient
+	testCtx         context.Context
+	rootDir         string
+	bundleDir       string
+	sleepSpec       *specs.Spec
+	sleepConf       *config.Config
+	udsPath         string
+	client          *metricclient.MetricClient
+	serverExtraArgs []string
 }
 
 // setupMetrics sets up a container configuration with metrics enabled, and returns it all.
@@ -67,7 +68,7 @@ func setupMetrics(t *testing.T, forceTempUDS bool) (*metricsTest, func()) {
 
 	spec, conf := sleepSpecConf(t)
 	conf.MetricServer = "%RUNTIME_ROOT%/metrics.sock"
-	conf.MetricExporterPrefix = "testmetric_"
+	serverExtraArgs := []string{"--exporter-prefix=testmetric_"}
 	rootDir, bundleDir, cleanup, err := testutil.SetupContainer(spec, conf)
 	if err != nil {
 		t.Fatalf("error setting up container: %v", err)
@@ -90,19 +91,20 @@ func setupMetrics(t *testing.T, forceTempUDS bool) (*metricsTest, func()) {
 	cu.Add(func() { os.Remove(udsPath) })
 
 	metricClient := metricclient.NewMetricClient(udsPath, rootDir)
-	if err := metricClient.SpawnServer(testCtx, conf); err != nil {
+	if err := metricClient.SpawnServer(testCtx, conf, serverExtraArgs...); err != nil {
 		t.Fatalf("Cannot start metric server: %v", err)
 	}
 	cu.Add(func() { metricClient.ShutdownServer(cleanupCtx) })
 
 	return &metricsTest{
-		testCtx:   testCtx,
-		rootDir:   rootDir,
-		bundleDir: bundleDir,
-		sleepSpec: spec,
-		sleepConf: conf,
-		udsPath:   udsPath,
-		client:    metricClient,
+		testCtx:         testCtx,
+		rootDir:         rootDir,
+		bundleDir:       bundleDir,
+		sleepSpec:       spec,
+		sleepConf:       conf,
+		udsPath:         udsPath,
+		client:          metricClient,
+		serverExtraArgs: serverExtraArgs,
 	}, cu.Clean
 }
 
@@ -345,7 +347,7 @@ func TestContainerMetricsRobustAgainstRestarts(t *testing.T) {
 	}
 
 	// Start the metric server.
-	if err := te.client.SpawnServer(te.testCtx, te.sleepConf); err != nil {
+	if err := te.client.SpawnServer(te.testCtx, te.sleepConf, te.serverExtraArgs...); err != nil {
 		t.Fatalf("Cannot re-spawn server: %v", err)
 	}
 
@@ -534,7 +536,7 @@ func TestMetricServerChecksRootDirectoryAccess(t *testing.T) {
 	}
 	shorterCtx, shorterCtxCancel := context.WithTimeout(te.testCtx, time.Second)
 	defer shorterCtxCancel()
-	if err := te.client.SpawnServer(shorterCtx, te.sleepConf); err == nil {
+	if err := te.client.SpawnServer(shorterCtx, te.sleepConf, te.serverExtraArgs...); err == nil {
 		t.Error("Metric server was successfully able to be spawned despite not having access to the root directory")
 	}
 }
@@ -548,14 +550,12 @@ func TestMetricServerToleratesNoRootDirectory(t *testing.T) {
 	if err := os.RemoveAll(te.sleepConf.RootDir); err != nil {
 		t.Fatalf("cannot remove root directory %q: %v", te.sleepConf.RootDir, err)
 	}
-	te.sleepConf.MetricServerAllowUnknownRoot = false
 	shortCtx, shortCtxCancel := context.WithTimeout(te.testCtx, time.Second)
 	defer shortCtxCancel()
-	if err := te.client.SpawnServer(shortCtx, te.sleepConf); err == nil {
+	if err := te.client.SpawnServer(shortCtx, te.sleepConf, append([]string{"--allow-unknown-root=false"}, te.serverExtraArgs...)...); err == nil {
 		t.Fatalf("Metric server was successfully able to be spawned despite a non-existent root directory")
 	}
-	te.sleepConf.MetricServerAllowUnknownRoot = true
-	if err := te.client.SpawnServer(te.testCtx, te.sleepConf); err != nil {
+	if err := te.client.SpawnServer(te.testCtx, te.sleepConf, append([]string{"--allow-unknown-root=true"}, te.serverExtraArgs...)...); err != nil {
 		t.Errorf("Metric server was not able to be spawned despite being configured to tolerate a non-existent root directory: %v", err)
 	}
 }

test/metricclient/metricclient.go

@@ -167,7 +167,7 @@ func (c *MetricClient) HealthCheck(ctx context.Context) error {
 // Callers should call ShutdownServer to stop the server.
 // A running server must be stopped before a new one can be successfully started.
 // baseConf is used for passing other flags to the server, e.g. debug log directory.
-func (c *MetricClient) SpawnServer(ctx context.Context, baseConf *config.Config) error {
+func (c *MetricClient) SpawnServer(ctx context.Context, baseConf *config.Config, extraArgs ...string) error {
 	c.mu.Lock()
 	defer c.mu.Unlock()
 	if c.server != nil {
@@ -209,6 +209,7 @@ func (c *MetricClient) SpawnServer(ctx context.Context, baseConf *config.Config)
 	// shown as `exe`.
 	c.server.Args[0] = "runsc-metrics"
 	c.server.Args = append(c.server.Args, "metric-server")
+	c.server.Args = append(c.server.Args, extraArgs...)
 	if err := c.server.Start(); err != nil {
 		return fmt.Errorf("cannot start metrics server: %w", err)
 	}