`tmpnet`: Separate node into orchestration, config and process #2460

marun · 2023-12-10T21:10:40Z

Previously tmpnet/node.go contained methods to orchestrate nodes, manage their processes, and read and write their configuration. This PR moves reading/writing of configuration to node_config.go and process management to node_process.go. The separation is intended to improve maintainability.
A new NodeRuntime interface is added to support future pod-based implementation.

tests/fixture/tmpnet/README.md

danlaine · 2023-12-14T14:54:17Z

tests/fixture/tmpnet/defaults.go

+	RootDirEnvName    = "TMPNET_ROOT_DIR"
+
+	DefaultNetworkTimeout  = 2 * time.Minute
+	DefaultNodeInitTimeout = 10 * time.Second


This is unused

danlaine · 2023-12-14T14:54:34Z

tests/fixture/tmpnet/defaults.go

+
+	DefaultNetworkTimeout  = 2 * time.Minute
+	DefaultNodeInitTimeout = 10 * time.Second
+	DefaultNodeStopTimeout = 5 * time.Second


This is unused

danlaine · 2023-12-14T14:57:42Z

tests/fixture/tmpnet/flags.go

+}
+
+// Return a deep copy of the flags map.
+func (f FlagsMap) Copy() FlagsMap {


I think we can use maps.Copy https://pkg.go.dev/golang.org/x/exp/maps#Copy

Unused - deleted.

danlaine · 2023-12-14T15:08:13Z

tests/fixture/tmpnet/node_process.go

+	switch t := err.(type) {
+	case *net.OpError:
+		if t.Op == "read" {
+			// Connection refused - potentially recoverable
+			return false, nil
+		}
+	case syscall.Errno:
+		if t == syscall.ECONNREFUSED {
+			// Connection refused - potentially recoverable
+			return false, nil
+		}
+	}
+	// Assume all other errors are not recoverable
+	return false, fmt.Errorf("failed to query node health: %w", err)


In general in avalanchego we don't check whether network errors are potentially temporary/recoverable, and just assume they're not. I'd be ok doing the same here

I'm doing my best here to avoid failing the start of a network due to one or more nodes being in an intermediate network state before becoming healthy. Maybe it makes sense in the context of a running network to be more strict, but I think this fixture needs to be more forgiving (i.e. to avoid flakes in CI).

danlaine · 2023-12-14T15:12:06Z

tests/fixture/tmpnet/node_process.go

+// Retrieve the node process if it is running. As part of determining
+// process liveness, the node's process context will be refreshed if
+// live or cleared if not running.
+func (p *NodeProcess) getProcess() (*os.Process, error) {


When we do exec.Command to start the binary, we have a reference to the Process. I think we can use that instead, right?

It might make sense in some cases to maintain a reference to the process - e.g. when the network is started by the test suite - but this fixture also supports temporary networks whose nodes were started by the tmpnetctl command. This has the potential to speed up test development since the cost of network setup only needs to be incurred once for many invocations of the test suite or a subset thereof. I made good use of this capability during migration of the kurtosis tests.

This method also enables shutdown of an existing network via the tmpnetctl command.

And while it would be possible to hold a reference to the process in the case where the network was started in-process, I don't think the complexity would be justified by the minimal overhead imposed by on-demand process lookup in the context of test execution.

abi87 · 2023-12-18T08:57:40Z

tests/fixture/tmpnet/README.md

@@ -31,6 +31,8 @@ the following non-test files:
 | genesis.go        |             | Creates test genesis                           |
 | network.go        | Network     | Orchestrates and configures temporary networks |
 | node.go           | Node        | Orchestrates and configures nodes              |
+| node_config.go    | Node        | Reads and writes node configuration            |
+| node_process.go   | NodeProcess | Orchestrates node processes                    |


line 107: DefaultRuntime: should be renamed to NodeRuntimeConfig I believe

abi87 · 2023-12-18T09:00:45Z

tests/fixture/tmpnet/node_process.go

+	// Avoid attempting to start an already running node.
+	proc, err := p.getProcess()
+	if err != nil {
+		return fmt.Errorf("failed to start node process: %w", err)


nit: this error message seems a bit off. It's not really starting a process here, just checking if it's running already right?

abi87 · 2023-12-18T09:01:51Z

tests/fixture/tmpnet/node_process.go

+func (p *NodeProcess) InitiateStop() error {
+	proc, err := p.getProcess()
+	if err != nil {
+		return fmt.Errorf("failed to retrieve process to stop: %w", err)


this is a better error message. Consider adapting it for p.Start as well

abi87

found minor nits but LGTM overall

This refactor is intended to improve maintainability by separating node into coherent constituent parts and minimizing the exported API.

marun self-assigned this Dec 10, 2023

marun force-pushed the tmpnet-move-local branch from f9aada9 to 3e42f16 Compare December 10, 2023 21:14

marun force-pushed the tmpnet-refactor branch 9 times, most recently from 7761c0c to 96f8d79 Compare December 10, 2023 23:31

marun added the testing This primarily focuses on testing label Dec 11, 2023

marun force-pushed the tmpnet-refactor branch from 96f8d79 to 05a7ca2 Compare December 11, 2023 01:53

marun changed the base branch from tmpnet-move-local to tmpnet-refactor-config December 11, 2023 01:54

marun force-pushed the tmpnet-refactor-config branch from e446b6a to 0a2ab0a Compare December 11, 2023 03:21

marun force-pushed the tmpnet-refactor branch from 05a7ca2 to 29548f2 Compare December 11, 2023 03:22

marun changed the title ~~tmpnet: Refactor for maintainability~~ tmpnet: Separate node into orchestration, config and process Dec 11, 2023

marun force-pushed the tmpnet-refactor branch 3 times, most recently from 7d3474d to 282f77d Compare December 11, 2023 05:01

marun mentioned this pull request Dec 11, 2023

tmpnet: Separate network into orchestration and configuration #2464

Merged

marun changed the title ~~tmpnet: Separate node into orchestration, config and process~~ tmpnet: Separate node into orchestration, config and process Dec 11, 2023

marun force-pushed the tmpnet-refactor-config branch from 0a2ab0a to 2597b74 Compare December 11, 2023 19:01

marun force-pushed the tmpnet-refactor branch from 282f77d to 6b22969 Compare December 11, 2023 19:02

marun marked this pull request as ready for review December 13, 2023 20:36

marun requested review from abi87 and gyuho as code owners December 13, 2023 20:36

marun force-pushed the tmpnet-refactor-config branch from 5c35f62 to dab364e Compare December 14, 2023 00:33

danlaine reviewed Dec 14, 2023

View reviewed changes

marun force-pushed the tmpnet-refactor-config branch from dab364e to e338c6e Compare December 15, 2023 02:50

marun force-pushed the tmpnet-refactor branch from 6b22969 to d4e2f67 Compare December 15, 2023 02:52

marun force-pushed the tmpnet-refactor-config branch from e338c6e to fe08729 Compare December 15, 2023 04:38

marun requested a review from StephenButtolph as a code owner December 15, 2023 04:38

marun force-pushed the tmpnet-refactor-config branch from fe08729 to 205f6e8 Compare December 15, 2023 04:42

marun force-pushed the tmpnet-refactor branch from c6f2252 to 969b064 Compare December 15, 2023 04:47

Base automatically changed from tmpnet-refactor-config to dev December 15, 2023 17:52

marun force-pushed the tmpnet-refactor branch from 969b064 to 5b9bf79 Compare December 15, 2023 18:18

marun linked an issue Dec 18, 2023 that may be closed by this pull request

Add subnet support to the tmpnet fixture #2416

Closed

6 tasks

marun force-pushed the tmpnet-refactor branch from 5b9bf79 to 7c8a744 Compare December 18, 2023 05:14

abi87 reviewed Dec 18, 2023

View reviewed changes

abi87 approved these changes Dec 18, 2023

View reviewed changes

marun added 4 commits December 19, 2023 11:50

tmpnet: Separate node into orchestration, config and process

5e3a877

This refactor is intended to improve maintainability by separating node into coherent constituent parts and minimizing the exported API.

fixup: Add mention of TMPNET_ROOT_DIR in README

973f074

fixup: Remove unused methods and constants

93e6833

Fixup: respond to review feedback

ef41e2e

marun force-pushed the tmpnet-refactor branch from 6beb808 to ef41e2e Compare December 19, 2023 19:50

danlaine approved these changes Dec 19, 2023

View reviewed changes

Merge branch 'dev' into tmpnet-refactor

d820846

StephenButtolph added this to the v1.10.18 milestone Dec 19, 2023

StephenButtolph added this pull request to the merge queue Dec 19, 2023

Merged via the queue into dev with commit fa21d78 Dec 19, 2023
17 checks passed

StephenButtolph deleted the tmpnet-refactor branch December 19, 2023 22:14

marun added a commit that referenced this pull request Dec 20, 2023

tmpnet: Separate node into orchestration, config and process (#2460)

b18830d

marun added a commit that referenced this pull request Dec 21, 2023

tmpnet: Separate node into orchestration, config and process (#2460)

b787dec

marun added a commit that referenced this pull request Dec 22, 2023

tmpnet: Separate node into orchestration, config and process (#2460)

362172c

marun added a commit that referenced this pull request Dec 22, 2023

tmpnet: Separate node into orchestration, config and process (#2460)

3cf7a3a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`tmpnet`: Separate node into orchestration, config and process #2460

`tmpnet`: Separate node into orchestration, config and process #2460

marun commented Dec 10, 2023 •

edited

Loading

danlaine Dec 14, 2023

marun Dec 15, 2023

danlaine Dec 14, 2023

marun Dec 15, 2023

danlaine Dec 14, 2023

marun Dec 15, 2023

danlaine Dec 14, 2023

marun Dec 15, 2023 •

edited

Loading

danlaine Dec 14, 2023

marun Dec 15, 2023

abi87 Dec 18, 2023

marun Dec 18, 2023

abi87 Dec 18, 2023

marun Dec 18, 2023

abi87 Dec 18, 2023

abi87 left a comment

tmpnet: Separate node into orchestration, config and process #2460

tmpnet: Separate node into orchestration, config and process #2460

Conversation

marun commented Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abi87 left a comment

Choose a reason for hiding this comment

`tmpnet`: Separate node into orchestration, config and process #2460

`tmpnet`: Separate node into orchestration, config and process #2460

marun commented Dec 10, 2023 •

edited

Loading

marun Dec 15, 2023 •

edited

Loading