fixup: Strictly separate start and stop

maru-ava · maru-ava · commit 2cc6e085d478 · 2025-05-12T21:15:46.000+01:00
diff --git a/.github/actions/run-monitored-tmpnet-cmd/action.yml b/.github/actions/run-monitored-tmpnet-cmd/action.yml
@@ -67,9 +67,10 @@ runs:
       # --impure ensures the env vars are accessible to the command
       run: ${{ inputs.run_env }} ${{ github.action_path }}/nix-develop.sh --impure --command bash -x ${{ inputs.run }}
       env:
-        TMPNET_START_METRIC_COLLECTOR: ${{ inputs.prometheus_username != '' }}
-        TMPNET_START_LOG_COLLECTOR: ${{ inputs.loki_username != '' }}
-        TMPNET_CHECK_MONITORING: ${{ inputs.prometheus_username != '' }}
+        TMPNET_START_METRICS_COLLECTOR: ${{ inputs.prometheus_username != '' }}
+        TMPNET_START_LOGS_COLLECTOR: ${{ inputs.loki_username != '' }}
+        TMPNET_CHECK_METRICS_COLLECTED: ${{ inputs.prometheus_username != '' }}
+        TMPNET_CHECK_LOGS_COLLECTED: ${{ inputs.loki_username != '' }}
         LOKI_USERNAME: ${{ inputs.loki_username }}
         LOKI_PASSWORD: ${{ inputs.loki_password }}
         PROMETHEUS_USERNAME: ${{ inputs.prometheus_username }}
diff --git a/tests/e2e/README.md b/tests/e2e/README.md
@@ -113,9 +113,10 @@ E2E_SKIP_BOOTSTRAP_CHECKS=1 ./bin/ginkgo -v ./tests/e2e ...
 It is possible to enable collection of logs and metrics from the
 temporary networks used for e2e testing by:
 
- - Supplying `--start-collectors` as an argument to the test suite
+ - Supplying `--start-metrics-collector` and `--start-logs-collector`
+   as arguments to the test suite
  - Starting collectors in advance of a test run with `tmpnetctl
-   start-collectors`
+   start-metrics-collector` and ` tmpnetctl start-logs-collector`
 
 Both methods require:
 
@@ -126,11 +127,11 @@ Both methods require:
    - `LOKI_PASSWORD`
  - The availability in the path of binaries for promtail and prometheus
    - Starting a development shell with `nix develop` is one way to
-     ensure this and requires the [installation of
-     nix](https://github.com/DeterminateSystems/nix-installer?tab=readme-ov-file#install-nix).
+     ensure this and requires the installation of nix
+     (e.g. `./scripts/run_task.sh install-nix`).
 
 Once started, the collectors will continue to run in the background
-until stopped by `tmpnetctl stop-collectors`.
+until stopped by `tmpnetctl stop-metrics-collector` and `tmpnetctl stop-logs-collector`.
 
 The results of collection will be viewable at
 https://grafana-poc.avax-dev.network.
diff --git a/tests/fixture/e2e/env.go b/tests/fixture/e2e/env.go
@@ -90,22 +90,36 @@ func NewTestEnvironment(tc tests.TestContext, flagVars *FlagVars, desiredNetwork
 
 	// Consider monitoring flags for any command but stop
 	if networkCmd != StopNetworkCmd {
-		if flagVars.StartMetricCollector() {
+		if flagVars.StartMetricsCollector() {
 			require.NoError(tmpnet.StartPrometheus(tc.DefaultContext(), tc.Log()))
 		}
-		if flagVars.StartLogCollector() {
+		if flagVars.StartLogsCollector() {
 			require.NoError(tmpnet.StartPromtail(tc.DefaultContext(), tc.Log()))
 		}
-		if flagVars.CheckMonitoring() {
-			// Register cleanup before network start to ensure it runs after the network is stopped (LIFO)
+
+		// Register cleanups before network start to ensure they run after the network is stopped (LIFO)
+
+		if flagVars.CheckMetricsCollected() {
+			tc.DeferCleanup(func() {
+				if network == nil {
+					tc.Log().Warn("unable to check that metrics were collected from an uninitialized network")
+					return
+				}
+				ctx, cancel := context.WithTimeout(context.Background(), DefaultTimeout)
+				defer cancel()
+				require.NoError(tmpnet.CheckMetricsExist(ctx, tc.Log(), network.UUID))
+			})
+		}
+
+		if flagVars.CheckLogsCollected() {
 			tc.DeferCleanup(func() {
 				if network == nil {
-					tc.Log().Warn("unable to check that logs and metrics were collected from an uninitialized network")
+					tc.Log().Warn("unable to check that logs were collected from an uninitialized network")
 					return
 				}
 				ctx, cancel := context.WithTimeout(context.Background(), DefaultTimeout)
 				defer cancel()
-				require.NoError(tmpnet.CheckMonitoring(ctx, tc.Log(), network.UUID))
+				require.NoError(tmpnet.CheckLogsExist(ctx, tc.Log(), network.UUID))
 			})
 		}
 	}
@@ -181,7 +195,7 @@ func NewTestEnvironment(tc tests.TestContext, flagVars *FlagVars, desiredNetwork
 	}
 
 	// Once one or more nodes are running it should be safe to wait for promtail to report readiness
-	if flagVars.StartLogCollector() {
+	if flagVars.StartLogsCollector() {
 		require.NoError(tmpnet.WaitForPromtailReadiness(tc.DefaultContext(), tc.Log()))
 	}
 
diff --git a/tests/fixture/e2e/flags.go b/tests/fixture/e2e/flags.go
@@ -31,11 +31,11 @@ type FlagVars struct {
 	startNetworkVars *flags.StartNetworkVars
 
 	// The collectors configured by these flags run as local processes
-	startCollectors      bool
-	startMetricCollector bool
-	startLogCollector    bool
+	startMetricsCollector bool
+	startLogsCollector    bool
 
-	checkMonitoring bool
+	checkMetricsCollected bool
+	checkLogsCollected    bool
 
 	networkDir     string
 	reuseNetwork   bool
@@ -87,16 +87,20 @@ func (v *FlagVars) NodeRuntimeConfig() (*tmpnet.NodeRuntimeConfig, error) {
 	return v.startNetworkVars.GetNodeRuntimeConfig()
 }
 
-func (v *FlagVars) StartMetricCollector() bool {
-	return v.startCollectors || v.startMetricCollector
+func (v *FlagVars) StartMetricsCollector() bool {
+	return v.startMetricsCollector
 }
 
-func (v *FlagVars) StartLogCollector() bool {
-	return v.startCollectors || v.startLogCollector
+func (v *FlagVars) StartLogsCollector() bool {
+	return v.startLogsCollector
 }
 
-func (v *FlagVars) CheckMonitoring() bool {
-	return v.checkMonitoring
+func (v *FlagVars) CheckMetricsCollected() bool {
+	return v.checkMetricsCollected
+}
+
+func (v *FlagVars) CheckLogsCollected() bool {
+	return v.checkLogsCollected
 }
 
 func (v *FlagVars) NetworkDir() string {
@@ -110,7 +114,7 @@ func (v *FlagVars) NetworkDir() string {
 }
 
 func (v *FlagVars) NetworkShutdownDelay() time.Duration {
-	if v.StartLogCollector() || v.StartMetricCollector() {
+	if v.StartMetricsCollector() {
 		// Only return a non-zero value if we want to ensure the collectors have
 		// a chance to collect the metrics at the end of the test.
 		return tmpnet.NetworkShutdownDelay
@@ -138,11 +142,11 @@ func RegisterFlagsWithDefaultOwner(defaultOwner string) *FlagVars {
 
 	vars.startNetworkVars = flags.NewStartNetworkFlagVars(defaultOwner)
 
-	SetAllMonitoringFlags(
-		&vars.startCollectors,
-		&vars.startMetricCollector,
-		&vars.startLogCollector,
-		&vars.checkMonitoring,
+	SetMonitoringFlags(
+		&vars.startMetricsCollector,
+		&vars.startLogsCollector,
+		&vars.checkMetricsCollected,
+		&vars.checkLogsCollected,
 	)
 
 	flag.StringVar(
@@ -183,44 +187,29 @@ func RegisterFlagsWithDefaultOwner(defaultOwner string) *FlagVars {
 	return &vars
 }
 
-// Enable reuse by the upgrade job which doesn't need fine-grained control over collector start since it never runs in kube.
-func SetSimpleMonitoringFlags(startCollectors *bool, checkMonitoring *bool) {
-	SetAllMonitoringFlags(startCollectors, nil, nil, checkMonitoring)
-}
-
-func SetAllMonitoringFlags(startCollectors *bool, startMetricCollector *bool, startLogCollector *bool, checkMonitoring *bool) {
-	// Ensure the upgrade job is configured to start both collectors if they are individually configured
-	defaultStartMetricCollector := cast.ToBool(tmpnet.GetEnvWithDefault("TMPNET_START_METRIC_COLLECTOR", "false"))
-	defaultStartLogCollector := cast.ToBool(tmpnet.GetEnvWithDefault("TMPNET_START_LOG_COLLECTOR", "false"))
-	defaultStartCollectors := cast.ToBool(tmpnet.GetEnvWithDefault("TMPNET_START_COLLECTORS", "false")) || (defaultStartMetricCollector && defaultStartLogCollector)
-
+func SetMonitoringFlags(startMetricsCollector, startLogsCollector, checkMetricsCollected, checkLogsCollected *bool) {
 	flag.BoolVar(
-		startCollectors,
-		"start-collectors",
-		defaultStartCollectors,
-		"[optional] whether to start local collectors of logs and metrics from nodes of the temporary network.",
+		startMetricsCollector,
+		"start-metrics-collector",
+		cast.ToBool(tmpnet.GetEnvWithDefault("TMPNET_START_METRICS_COLLECTOR", "false")),
+		"[optional] whether to start a local collector of metrics from nodes of the temporary network.",
+	)
+	flag.BoolVar(
+		startLogsCollector,
+		"start-logs-collector",
+		cast.ToBool(tmpnet.GetEnvWithDefault("TMPNET_START_LOGS_COLLECTOR", "false")),
+		"[optional] whether to start a local collector of logs from nodes of the temporary network.",
+	)
+	flag.BoolVar(
+		checkMetricsCollected,
+		"check-metrics-collected",
+		cast.ToBool(tmpnet.GetEnvWithDefault("TMPNET_CHECK_METRICS_COLLECTED", "false")),
+		"[optional] whether to check that metrics have been collected from nodes of the temporary network.",
 	)
-	// These 2 flags are not used by the upgrade job
-	if startMetricCollector != nil {
-		flag.BoolVar(
-			startMetricCollector,
-			"start-metric-collector",
-			defaultStartMetricCollector,
-			"[optional] whether to start a local collector of metrics from nodes of the temporary network. Can also be enabled by --start-collectors.",
-		)
-	}
-	if startLogCollector != nil {
-		flag.BoolVar(
-			startMetricCollector,
-			"start-log-collector",
-			defaultStartLogCollector,
-			"[optional] whether to start a local collector of metrics from nodes of the temporary network. Can also be enabled by --start-collectors.",
-		)
-	}
 	flag.BoolVar(
-		checkMonitoring,
-		"check-monitoring",
-		cast.ToBool(tmpnet.GetEnvWithDefault("TMPNET_CHECK_MONITORING", "false")),
-		"[optional] whether to check that logs and metrics have been collected from nodes of the temporary network.",
+		checkLogsCollected,
+		"check-logs-collected",
+		cast.ToBool(tmpnet.GetEnvWithDefault("TMPNET_CHECK_LOGS_COLLECTED", "false")),
+		"[optional] whether to check that logs have been collected from nodes of the temporary network.",
 	)
 }
diff --git a/tests/fixture/tmpnet/README.md b/tests/fixture/tmpnet/README.md
@@ -28,10 +28,9 @@ orchestrate the same temporary networks without the use of an rpc daemon.
     - [Process details](#process-details)
 - [Monitoring](#monitoring)
   - [Example usage](#example-usage)
-  - [Starting collectors](#starting-collectors)
-  - [Stopping collectors](#stopping-collectors)
-  - [Metrics collection](#metrics-collection)
-  - [Log collection](#log-collection)
+  - [Running collectors](#running-collectors)
+  - [Metric collection configuration](#metric-collection-configuration)
+  - [Log collection configuration](#log-collection-configuration)
   - [Labels](#labels)
   - [CI Collection](#ci-collection)
   - [Viewing](#viewing)
@@ -332,49 +331,50 @@ PROMETHEUS_USERNAME=<username> \
 PROMETHEUS_PASSWORD=<password> \
 LOKI_USERNAME=<username> \
 LOKI_PASSWORD=<password> \
-./bin/tmpnetctl start-collectors
+./bin/tmpnetctl start-metrics-collector
+./bin/tmpnetctl start-logs-collector
 
 # Network start emits link to grafana displaying collected logs and metrics
 ./bin/tmpnetctl start-network
 
 # When done with the network, stop the collectors
-./bin/tmpnetctl stop-collectors
+./bin/tmpnetctl stop-metrics-collector
+./bin/tmpnetctl stop-logs-collector
 ```
 
-### Starting collectors
+### Running collectors
 [Top](#table-of-contents)
 
-Collectors for logs and metrics can be started by `tmpnetctl
-start-collectors`:
-
- - Requires that the following env vars be set
-   - `PROMETHEUS_USERNAME`
-   - `PROMETHEUS_PASSWORD`
-   - `LOKI_USERNAME`
-   - `LOKI_PASSWORD`
- - Requires that binaries for promtail and prometheus be available in the path
-   - Starting a development shell with `nix develop` is one way to
-     ensure this and requires the [installation of
-     nix](https://github.com/DeterminateSystems/nix-installer?tab=readme-ov-file#install-nix).
- - Starts prometheus in agent mode configured to scrape metrics from
-   configured nodes and forward them to
-   https://prometheus-poc.avax-dev.network.
- - Starts promtail configured to collect logs from configured nodes
-   and forward them to https://loki-poc.avax-dev.network.
-
-### Stopping collectors
-
-Collectors for logs and metrics can be stopped by `tmpnetctl
-stop-collectors`:
-
-### Metrics collection
+ - `tmpnetctl start-metrics-collector` starts `prometheus` in agent mode
+   configured to scrape metrics from configured nodes and forward
+   them to https://prometheus-poc.avax-dev.network.
+   - Requires:
+     - Credentials supplied as env vars:
+       - `PROMETHEUS_USERNAME`
+       - `PROMETHEUS_PASSWORD`
+     - A `prometheus` binary available in the path
+   - Once started, `prometheus` can be stopped by `tmpnetctl stop-metrics-collector`
+ - `tmpnetctl start-logs-collector` starts `promtail` configured to collect logs
+   from configured nodes and forward them to
+   https://loki-poc.avax-dev.network.
+   - Requires:
+     - Credentials supplied as env vars:
+       - `LOKI_USERNAME`
+       - `LOKI_PASSWORD`
+     - A `promtail` binary available in the path
+   - Once started, `promtail` can be stopped by `tmpnetctl stop-logs-collector`
+ - Starting a development shell with `nix develop` is one way to
+   ensure availability of the necessary binaries and requires the
+   installation of nix (e.g. `./scripts/run_task.sh install-nix`).
+
+### Metric collection configuration
 [Top](#table-of-contents)
 
 When a node is started, configuration enabling collection of metrics
 from the node is written to
 `~/.tmpnet/prometheus/file_sd_configs/[network uuid]-[node id].json`.
 
-### Log collection
+### Log collection configuration
 [Top](#table-of-contents)
 
 Nodes log are stored at `~/.tmpnet/networks/[network id]/[node
diff --git a/tests/fixture/tmpnet/check_monitoring.go b/tests/fixture/tmpnet/check_monitoring.go
@@ -28,16 +28,6 @@ import (
 
 type getCountFunc func() (int, error)
 
-// CheckMonitoring checks if logs and metrics exist for the given network. If no network
-// UUID is provided, an attempt will be made to derive selectors from env vars (GH_*)
-// identifying a github actions run.
-func CheckMonitoring(ctx context.Context, log logging.Logger, networkUUID string) error {
-	return errors.Join(
-		CheckLogsExist(ctx, log, networkUUID),
-		CheckMetricsExist(ctx, log, networkUUID),
-	)
-}
-
 // waitForCount waits until the provided function returns greater than zero.
 func waitForCount(ctx context.Context, log logging.Logger, name string, getCount getCountFunc) error {
 	err := pollUntilContextCancel(
diff --git a/tests/fixture/tmpnet/monitor_processes.go b/tests/fixture/tmpnet/monitor_processes.go
diff --git a/tests/fixture/tmpnet/tmpnetctl/main.go b/tests/fixture/tmpnet/tmpnetctl/main.go
diff --git a/tests/upgrade/upgrade_test.go b/tests/upgrade/upgrade_test.go
