From f08082fbd73bb39ead3b3d4519368d55a157e1de Mon Sep 17 00:00:00 2001 From: Etienne Perot Date: Wed, 8 Feb 2023 10:43:58 -0800 Subject: [PATCH] runsc metric-server: Add flag to tolerate absence of `--root` directory. PiperOrigin-RevId: 508122696 --- runsc/cmd/metric_server.go | 32 ++++++++++----- runsc/config/config.go | 6 +++ runsc/config/flags.go | 1 + runsc/container/metric_server_test.go | 59 ++++++++++++++++++--------- test/metricclient/BUILD | 1 + test/metricclient/metricclient.go | 6 +++ 6 files changed, 75 insertions(+), 30 deletions(-) diff --git a/runsc/cmd/metric_server.go b/runsc/cmd/metric_server.go index 956cdf39e..92121ce18 100644 --- a/runsc/cmd/metric_server.go +++ b/runsc/cmd/metric_server.go @@ -230,11 +230,12 @@ func queryMetrics(ctx context.Context, sand *sandbox.Sandbox, verifier *promethe // MetricServer implements subcommands.Command for the "metric-server" command. type MetricServer struct { - rootDir string - address string - exporterPrefix string - startTime time.Time - srv http.Server + rootDir string + allowUnknownRoot bool + address string + exporterPrefix string + startTime time.Time + srv http.Server // Size of the map of written metrics during the last /metrics export. Initially zero. // Used to efficiently reallocate a map of the right size during the next export. @@ -329,7 +330,9 @@ func (m *MetricServer) refreshSandboxesLocked() { } sandboxIDs, err := container.ListSandboxes(m.rootDir) if err != nil { - log.Warningf("Cannot list containers in root directory %s, it has likely gone away: %v.", m.rootDir, err) + if !m.allowUnknownRoot { + log.Warningf("Cannot list containers in root directory %s, it has likely gone away: %v.", m.rootDir, err) + } return } for sandboxID, sandbox := range m.sandboxes { @@ -752,8 +755,10 @@ func (m *MetricServer) verify(ctx context.Context) { m.mu.Lock() defer m.mu.Unlock() if err != nil { - log.Warningf("Cannot list sandboxes in root directory %s, it has likely gone away: %v. Server shutting down.", m.rootDir, err) - m.shutdownLocked(ctx) + if !m.allowUnknownRoot { + log.Warningf("Cannot list sandboxes in root directory %s, it has likely gone away: %v. Server shutting down.", m.rootDir, err) + m.shutdownLocked(ctx) + } return } m.refreshSandboxesLocked() @@ -797,15 +802,22 @@ 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 { - return util.Errorf("Invalid root directory %q: tried to list sandboxes within it and got: %v", conf.RootDir, err) + if !conf.MetricServerAllowUnknownRoot { + 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) } // 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 { - return util.Errorf("Invalid root directory %q: tried to list all entries within it and got: %v", conf.RootDir, err) + if !conf.MetricServerAllowUnknownRoot { + 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) diff --git a/runsc/config/config.go b/runsc/config/config.go index 96562249c..5d272b5cd 100644 --- a/runsc/config/config.go +++ b/runsc/config/config.go @@ -152,6 +152,12 @@ 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. diff --git a/runsc/config/flags.go b/runsc/config/flags.go index dfa796513..7fb338972 100644 --- a/runsc/config/flags.go +++ b/runsc/config/flags.go @@ -53,6 +53,7 @@ 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 diff --git a/runsc/container/metric_server_test.go b/runsc/container/metric_server_test.go index bf911834f..6a12a91b5 100644 --- a/runsc/container/metric_server_test.go +++ b/runsc/container/metric_server_test.go @@ -54,7 +54,7 @@ type metricsTest struct { // setupMetrics sets up a container configuration with metrics enabled, and returns it all. // Also returns a cleanup function. -func setupMetrics(t *testing.T) (*metricsTest, func()) { +func setupMetrics(t *testing.T, forceTempUDS bool) (*metricsTest, func()) { // Start the child reaper. childReaper := &testutil.Reaper{} childReaper.Start() @@ -73,21 +73,19 @@ func setupMetrics(t *testing.T) (*metricsTest, func()) { t.Fatalf("error setting up container: %v", err) } cu.Add(cleanup) - udsPath := filepath.Join(rootDir, "metrics.sock") - if len(udsPath) >= 100 { - // This is longer than the max UDS path length allowed by Linux. Try somewhere else in /tmp. - tmpDir, err := os.MkdirTemp("/tmp", "metrics-") - if err != nil { - t.Fatalf("Runtime root is %s which means the metrics UDS %s (%d bytes) is longer than the maximum length allowed for a UDS path. The test could also not create a temporary directory in /tmp as a fallback (%v).", rootDir, udsPath, len(udsPath), err) - } - cu.Add(func() { os.RemoveAll(tmpDir) }) - udsPathTmp := filepath.Join(tmpDir, "metrics.sock") - if len(udsPathTmp) >= 100 { - t.Fatalf("Runtime root is %s which means the metrics UDS %s (%d bytes) is longer than the maximum length allowed for a UDS path. The test tried to create a fallback in /tmp but it was too long too (%q is %d characters).", rootDir, udsPath, len(udsPath), udsPathTmp, len(udsPathTmp)) - } - udsPath = udsPathTmp - conf.MetricServer = udsPathTmp + tmpDir, err := os.MkdirTemp("/tmp", "metrics-") + if err != nil { + t.Fatalf("Cannot create temporary directory in /tmp: %v", err) } + cu.Add(func() { os.RemoveAll(tmpDir) }) + udsPath := filepath.Join(rootDir, "metrics.sock") + if forceTempUDS || len(udsPath) >= 100 { + udsPath = filepath.Join(tmpDir, "metrics.sock") + } + if len(udsPath) >= 100 { + t.Fatalf("Cannot come up with a UDS path shorter than the maximum length allowed by Linux (tried to use %q)", udsPath) + } + conf.MetricServer = udsPath // The UDS should be deleted by the metrics server itself, but we clean it up here anyway just in case: cu.Add(func() { os.Remove(udsPath) }) @@ -112,7 +110,7 @@ func setupMetrics(t *testing.T) (*metricsTest, func()) { func TestContainerMetrics(t *testing.T) { targetOpens := 200 - te, cleanup := setupMetrics(t) + te, cleanup := setupMetrics(t /* forceTempUDS= */, false) defer cleanup() if _, err := te.client.GetMetrics(te.testCtx); err != nil { @@ -202,7 +200,7 @@ 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) + te, cleanup := setupMetrics(t /* forceTempUDS= */, false) defer cleanup() args := Args{ @@ -264,7 +262,7 @@ func TestContainerMetricsIterationID(t *testing.T) { // unavailability or restarts. func TestContainerMetricsRobustAgainstRestarts(t *testing.T) { targetOpens := 200 - te, cleanup := setupMetrics(t) + te, cleanup := setupMetrics(t /* forceTempUDS= */, false) defer cleanup() // First, start a container which will kick off the metric server as normal. @@ -414,7 +412,7 @@ func TestContainerMetricsRobustAgainstRestarts(t *testing.T) { func TestContainerMetricsMultiple(t *testing.T) { numConcurrentContainers := 5 - te, cleanup := setupMetrics(t) + te, cleanup := setupMetrics(t /* forceTempUDS= */, false) defer cleanup() var containers []*Container needCleanup := map[*Container]struct{}{} @@ -517,7 +515,7 @@ func TestContainerMetricsMultiple(t *testing.T) { } func TestMetricServerChecksRootDirectoryAccess(t *testing.T) { - te, cleanup := setupMetrics(t) + te, cleanup := setupMetrics(t /* forceTempUDS= */, false) defer cleanup() if err := te.client.ShutdownServer(te.testCtx); err != nil { t.Fatalf("Cannot stop metric server: %v", err) @@ -540,3 +538,24 @@ func TestMetricServerChecksRootDirectoryAccess(t *testing.T) { t.Error("Metric server was successfully able to be spawned despite not having access to the root directory") } } + +func TestMetricServerToleratesNoRootDirectory(t *testing.T) { + te, cleanup := setupMetrics(t /* forceTempUDS= */, true) + defer cleanup() + if err := te.client.ShutdownServer(te.testCtx); err != nil { + t.Fatalf("Cannot stop metric server: %v", err) + } + 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 { + 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 { + t.Errorf("Metric server was not able to be spawned despite being configured to tolerate a non-existent root directory: %v", err) + } +} diff --git a/test/metricclient/BUILD b/test/metricclient/BUILD index 448c4d228..14caf059c 100644 --- a/test/metricclient/BUILD +++ b/test/metricclient/BUILD @@ -12,6 +12,7 @@ go_library( "//runsc:__subpackages__", ], deps = [ + "//pkg/cleanup", "//pkg/prometheus", "//pkg/sync", "//runsc/config", diff --git a/test/metricclient/metricclient.go b/test/metricclient/metricclient.go index e5aeb232c..2a8dfc285 100644 --- a/test/metricclient/metricclient.go +++ b/test/metricclient/metricclient.go @@ -34,6 +34,7 @@ import ( "github.com/cenkalti/backoff" "github.com/prometheus/common/expfmt" "golang.org/x/sys/unix" + "gvisor.dev/gvisor/pkg/cleanup" "gvisor.dev/gvisor/pkg/prometheus" "gvisor.dev/gvisor/pkg/sync" "gvisor.dev/gvisor/runsc/config" @@ -187,6 +188,10 @@ func (c *MetricClient) SpawnServer(ctx context.Context, baseConf *config.Config) overriddenConf.MetricServer = c.addr overriddenConf.RootDir = c.rootDir c.server = exec.Command(specutils.ExePath, overriddenConf.ToFlags()...) + cu := cleanup.Make(func() { + c.server = nil + }) + defer cu.Clean() c.server.SysProcAttr = &unix.SysProcAttr{ // Detach from this session, otherwise cmd will get SIGHUP and SIGCONT // when re-parented. @@ -221,6 +226,7 @@ func (c *MetricClient) SpawnServer(ctx context.Context, baseConf *config.Config) if bindCtx.Err() != nil { return fmt.Errorf("metrics server did not bind to %s in time: %w", c.addr, bindCtx.Err()) } + cu.Release() return nil }