fixup: Cleanup ephemeral node implementation

 - rename 'temporary node' to 'ephemeral node' for clarity

 - ensure ephemeral node data is stored in the network path and not
 removed on teardown to allow archiving in CI to collect all data for
 a test run.

 - Remove `Node.Remove()` method since it is not being used

Author: maru-ava

tests/e2e/e2e.go

@@ -14,7 +14,6 @@ import (
 
 	"github.com/stretchr/testify/require"
 
-	"github.com/ava-labs/avalanchego/config"
 	"github.com/ava-labs/avalanchego/tests"
 	"github.com/ava-labs/avalanchego/tests/fixture"
 	"github.com/ava-labs/avalanchego/tests/fixture/testnet"
@@ -117,23 +116,20 @@ func (te *TestEnvironment) NewWallet(keychain *secp256k1fx.Keychain) primary.Wal
 	return wallet
 }
 
-// Add a temporary node that is only intended to be used by a single test. Its ID and
+// Add an ephemeral node that is only intended to be used by a single test. Its ID and
 // URI are not intended to be returned from the Network instance to minimize
 // accessibility from other tests.
-func AddTemporaryNode(network testnet.Network, flags testnet.FlagsMap) testnet.Node {
+func AddEphemeralNode(network testnet.Network, flags testnet.FlagsMap) testnet.Node {
 	require := require.New(ginkgo.GinkgoT())
 
-	// A temporary location ensures the node won't be accessible from
-	// the Network instance.
-	flags[config.DataDirKey] = ginkgo.GinkgoT().TempDir()
-
-	node, err := network.AddNode(ginkgo.GinkgoWriter, flags)
+	node, err := network.AddEphemeralNode(ginkgo.GinkgoWriter, flags)
 	require.NoError(err)
 
-	// Ensure node is stopped and its configuration removed on teardown
+	// Ensure node is stopped on teardown. It's configuration is not removed to enable
+	// collection in CI to aid in troubleshooting failures.
 	ginkgo.DeferCleanup(func() {
-		tests.Outf("Shutting down temporary node %s\n", node.GetID())
-		require.NoError(node.Remove())
+		tests.Outf("Shutting down ephemeral node %s\n", node.GetID())
+		require.NoError(node.Stop())
 	})
 
 	return node

tests/e2e/faultinjection/duplicate_node_id.go

@@ -5,12 +5,13 @@ package faultinjection
 
 import (
 	"context"
+	"fmt"
 
 	ginkgo "github.com/onsi/ginkgo/v2"
 	"github.com/stretchr/testify/require"
 
 	"github.com/ava-labs/avalanchego/api/info"
-	cfg "github.com/ava-labs/avalanchego/config"
+	"github.com/ava-labs/avalanchego/config"
 	"github.com/ava-labs/avalanchego/ids"
 	"github.com/ava-labs/avalanchego/tests/e2e"
 	"github.com/ava-labs/avalanchego/tests/fixture/testnet"
@@ -25,7 +26,7 @@ var _ = ginkgo.Describe("Duplicate node handling", func() {
 		nodes := network.GetNodes()
 
 		ginkgo.By("creating new node")
-		node1 := e2e.AddTemporaryNode(network, testnet.FlagsMap{})
+		node1 := e2e.AddEphemeralNode(network, testnet.FlagsMap{})
 		e2e.WaitForHealthy(node1)
 
 		ginkgo.By("checking that the new node is connected to its peers")
@@ -34,10 +35,14 @@ var _ = ginkgo.Describe("Duplicate node handling", func() {
 		ginkgo.By("creating a second new node with the same staking keypair as the first new node")
 		node1Flags := node1.GetConfig().Flags
 		node2Flags := testnet.FlagsMap{
-			cfg.StakingTLSKeyContentKey: node1Flags[cfg.StakingTLSKeyContentKey],
-			cfg.StakingCertContentKey:   node1Flags[cfg.StakingCertContentKey],
+			config.StakingTLSKeyContentKey: node1Flags[config.StakingTLSKeyContentKey],
+			config.StakingCertContentKey:   node1Flags[config.StakingCertContentKey],
+			// Construct a unique data dir to ensure the two nodes' data will be stored
+			// separately. Usually the dir name is the node ID but in this one case the nodes have
+			// the same node ID.
+			config.DataDirKey: fmt.Sprintf("%s-second", node1Flags[config.DataDirKey]),
 		}
-		node2 := e2e.AddTemporaryNode(network, node2Flags)
+		node2 := e2e.AddEphemeralNode(network, node2Flags)
 
 		ginkgo.By("checking that the second new node fails to become healthy within timeout")
 		ctx, cancel := context.WithTimeout(context.Background(), e2e.DefaultNodeStartTimeout)

tests/fixture/testnet/interfaces.go

@@ -16,6 +16,7 @@ type Network interface {
 	GetConfig() NetworkConfig
 	GetNodes() []Node
 	AddNode(w io.Writer, flags FlagsMap) (Node, error)
+	AddEphemeralNode(w io.Writer, flags FlagsMap) (Node, error)
 }
 
 // Defines node capabilities supportable regardless of how a network is orchestrated.
@@ -26,5 +27,4 @@ type Node interface {
 	IsHealthy(ctx context.Context) (bool, error)
 	WaitForHealthy(ctx context.Context) error
 	Stop() error
-	Remove() error
 }

tests/fixture/testnet/local/README.md

@@ -152,7 +152,10 @@ HOME
             │       └── config.json                      // C-Chain config for all nodes
             ├── defaults.json                            // Default flags and configuration for network
             ├── genesis.json                             // Genesis for all nodes
-            └── network.env                              // Sets network dir env to simplify use of network
+            ├── network.env                              // Sets network dir env to simplify use of network
+            └── ephemeral                                // Parent directory for ephemeral nodes (e.g. created by tests)
+                └─ NodeID-FdxnAvr4jK9XXAwsYZPgWAHW2QnwSZ // Data dir for an ephemeral node
+                   └── ...
 
 ```
 

tests/fixture/testnet/local/network.go

@@ -24,10 +24,14 @@ import (
 	"github.com/ava-labs/avalanchego/utils/set"
 )
 
-// This interval was chosen to avoid spamming node APIs during
-// startup, as smaller intervals (e.g. 50ms) seemed to noticeably
-// increase the time for a network's nodes to be seen as healthy.
-const networkHealthCheckInterval = 200 * time.Millisecond
+const (
+	// This interval was chosen to avoid spamming node APIs during
+	// startup, as smaller intervals (e.g. 50ms) seemed to noticeably
+	// increase the time for a network's nodes to be seen as healthy.
+	networkHealthCheckInterval = 200 * time.Millisecond
+
+	defaultEphemeralDirName = "ephemeral"
+)
 
 var (
 	errInvalidNodeCount      = errors.New("failed to populate local network config: non-zero node count is only valid for a network without nodes")
@@ -102,16 +106,26 @@ func (ln *LocalNetwork) GetNodes() []testnet.Node {
 	return nodes
 }
 
-// Adds a backend-agnostic node to the network
-func (ln *LocalNetwork) AddNode(w io.Writer, flags testnet.FlagsMap) (testnet.Node, error) {
+// Internal method supporting AddNode and AddEphemeralNode in creating a backend-agnostic node.
+func (ln *LocalNetwork) addNode(w io.Writer, flags testnet.FlagsMap, isEphemeral bool) (testnet.Node, error) {
 	if flags == nil {
 		flags = testnet.FlagsMap{}
 	}
 	return ln.AddLocalNode(w, &LocalNode{
 		NodeConfig: testnet.NodeConfig{
 			Flags: flags,
 		},
-	})
+	}, isEphemeral)
+}
+
+// Adds a backend-agnostic node to the network
+func (ln *LocalNetwork) AddNode(w io.Writer, flags testnet.FlagsMap) (testnet.Node, error) {
+	return ln.addNode(w, flags, false /* isEphemeral */)
+}
+
+// Adds a backend-agnostic ephemeral node to the network
+func (ln *LocalNetwork) AddEphemeralNode(w io.Writer, flags testnet.FlagsMap) (testnet.Node, error) {
+	return ln.addNode(w, flags, true /* isEphemeral */)
 }
 
 // Starts a new network stored under the provided root dir. Required
@@ -278,17 +292,18 @@ func (ln *LocalNetwork) PopulateLocalNetworkConfig(networkID uint32, nodeCount i
 	for _, node := range ln.Nodes {
 		// Ensure the node is configured for use with the network and
 		// knows where to write its configuration.
-		if err := ln.PopulateNodeConfig(node); err != nil {
+		if err := ln.PopulateNodeConfig(node, ln.Dir); err != nil {
 			return err
 		}
 	}
 
 	return nil
 }
 
-// Ensure the provided node has the configuration it needs to
-// start. Requires that the network has valid genesis data.
-func (ln *LocalNetwork) PopulateNodeConfig(node *LocalNode) error {
+// Ensure the provided node has the configuration it needs to start. If the data dir is
+// not set, it will be defaulted to [nodeParentDir]/[node ID]. Requires that the
+// network has valid genesis data.
+func (ln *LocalNetwork) PopulateNodeConfig(node *LocalNode, nodeParentDir string) error {
 	flags := node.Flags
 
 	// Set values common to all nodes
@@ -310,7 +325,7 @@ func (ln *LocalNetwork) PopulateNodeConfig(node *LocalNode) error {
 	dataDir := node.GetDataDir()
 	if len(dataDir) == 0 {
 		// NodeID will have been set by EnsureKeys
-		dataDir = filepath.Join(ln.Dir, node.NodeID.String())
+		dataDir = filepath.Join(nodeParentDir, node.NodeID.String())
 		flags[config.DataDirKey] = dataDir
 	}
 
@@ -645,15 +660,30 @@ func (ln *LocalNetwork) ReadAll() error {
 	return ln.ReadNodes()
 }
 
-func (ln *LocalNetwork) AddLocalNode(w io.Writer, node *LocalNode) (*LocalNode, error) {
+func (ln *LocalNetwork) AddLocalNode(w io.Writer, node *LocalNode, isEphemeral bool) (*LocalNode, error) {
 	// Assume network configuration has been written to disk and is current in memory
 
 	if node == nil {
 		// Set an empty data dir so that PopulateNodeConfig will know
 		// to set the default of `[network dir]/[node id]`.
 		node = NewLocalNode("")
 	}
-	if err := ln.PopulateNodeConfig(node); err != nil {
+
+	// Default to a data dir of [network-dir]/[node-ID]
+	nodeParentDir := ln.Dir
+	if isEphemeral {
+		// For an ephemeral node, default to a data dir of [network-dir]/[ephemeral-dir][node-ID]
+		// to provide a clear separation between nodes that are expected to expose stable API
+		// endpoints and those that will live for only a short time (e.g. a node started by a test
+		// and stopped on test teardown).
+		//
+		// The data for an ephemeral node is still in the file tree rooted at the network dir to
+		// ensure that archiving the network dir in CI will collect all node data used for a test
+		// run. Otherwise the data dir of ephemeral nodes could be stored in a random temp dir.
+		nodeParentDir = filepath.Join(ln.Dir, defaultEphemeralDirName)
+	}
+
+	if err := ln.PopulateNodeConfig(node, nodeParentDir); err != nil {
 		return nil, err
 	}
 

tests/fixture/testnet/local/node.go

@@ -186,15 +186,16 @@ func (n *LocalNode) Start(w io.Writer, defaultExecPath string) error {
 		return err
 	}
 
-	// A temporary node directory won't match its node name.
-	isTemporaryNode := filepath.Base(n.GetDataDir()) != n.NodeID.String()
-	var nodeDescription string
-	if isTemporaryNode {
-		// Qualify the description for a temporary node with its path
-		// since it won't be stored in the network's directory.
-		nodeDescription = fmt.Sprintf("temporary node %q with path: %s", n.NodeID, n.GetDataDir())
-	} else {
-		nodeDescription = fmt.Sprintf("node %q", n.NodeID)
+	// Determine appropriate level of node description detail
+	nodeDescription := fmt.Sprintf("node %q", n.NodeID)
+	isEphemeralNode := filepath.Base(filepath.Dir(n.GetDataDir())) == defaultEphemeralDirName
+	if isEphemeralNode {
+		nodeDescription = "ephemeral " + nodeDescription
+	}
+	nonDefaultNodeDir := filepath.Base(n.GetDataDir()) != n.NodeID.String()
+	if nonDefaultNodeDir {
+		// Only include the data dir if its base is not the default (the node ID)
+		nodeDescription = fmt.Sprintf("%s with path: %s", nodeDescription, n.GetDataDir())
 	}
 
 	go func() {
@@ -367,15 +368,3 @@ func (n *LocalNode) WaitForProcessContext(ctx context.Context) error {
 	}
 	return nil
 }
-
-// Ensure the node is stopped and its configuration removed.
-func (n *LocalNode) Remove() error {
-	// Ensure the node is stopped before removing its configuration
-	if err := n.Stop(); err != nil {
-		return fmt.Errorf("failed to stop node %q: %w", n.NodeID, err)
-	}
-	if err := os.RemoveAll(n.GetDataDir()); err != nil {
-		return fmt.Errorf("failed to remove configuration for node %q: %w", n.NodeID, err)
-	}
-	return nil
-}