Write process context on node start to simplify test orchestration #1729

marun · 2023-07-18T18:09:28Z

Why this should be merged

The new local network fixture proposed in #1700 requires the following process context for each node to manage a local network a) without a daemon and b) with dynamically assigned ports:

PID: used to check if the node process is running and to enable stopping the node
URI: used to connect to the node's API (e.g. for health checks)
staking address: used by other nodes to bootstrap themselves

Initially these details were discovered by the fixture (e.g. addresses from logs and pid from process start), but it seemed more maintainable to have the node write the necessary data directly. Having the node write the process context directly also supports the start/stop and debug of test network nodes without explicit use of the test fixture.

How this works

On node start, process context (PID, URI, and staking address) is collected into the NodeProcessContext type, marshaled to JSON, and written to [base-data-dir]/process.json. On shutdown the file will be removed.
Consumers can read the process context file and unmarshal the resulting JSON to the NodeProcessContext type.

How this was tested

The new test fixture in e2e: Add local network fixture #1700 was refactored to depend on the new process context file and e2e tests were successfully executed against a fixture-managed network.

node/node.go

tests/e2e/e2e.go

config/flags.go

abi87

The only objection I have is to provide a standard location for the node information somwhere in the data dir. I could imagine some downstreams could use the process information we store and providing a single, standard location may simplify adoption

marun · 2023-07-19T15:52:16Z

e2e failure is unrelated to this PR, and will be fixed by #1734

marun · 2023-07-19T17:44:10Z

Rebased to pick up test fixes merged to dev.

marun · 2023-07-20T18:12:15Z

Waiting for #1737 to merge to fix the upgrade timeout failure.

abi87

lgtm

config/flags.go

node/node.go

api/server/server.go

marun · 2023-07-24T20:21:06Z

Updated to always write runtime state to a default path, PTAL.

api/server/mock_server.go

node/node.go

node/config.go

config/keys.go

api/server/server.go

node/node.go

pretty stale

dhrubabasu · 2023-07-24T22:32:53Z

config/flags.go

@@ -369,6 +370,8 @@ func addNodeFlags(fs *pflag.FlagSet) {
 	fs.Float64(TracingSampleRateKey, 0.1, "The fraction of traces to sample. If >= 1, always sample. If <= 0, never sample")
 	fs.StringToString(TracingHeadersKey, map[string]string{}, "The headers to provide the trace indexer")
 	// TODO add flag to take in headers to send from exporter
+
+	fs.String(RuntimeStateFileKey, defaultRuntimeStatePath, "The path to write runtime state to (including PID, API URI, and staking address).")


nit: can we put a comment for this flag like the other groups of flags?

Given that it's a single flag with no foreseeable additional flags to group with, what comment would you suggest beyond the flag description?

Or is there an existing group of flags that this flag might fit with?

By default state will not be written, and providing --runtime-state-path will ensure runtime state (API URI, bootstrap address, and pid) are written to the provided path on startup and removed from the provided path on shutdown. The details in the written file support orchestrating nodes for testing: - pid: check if process is running, stop the process - uri: connect to the node's API - bootstrap address: used by other nodes to bootstrap themselves

As per review comments, this includes using a default path with GetExpandedArg.

As per review commentary, it is preferable to bind the listener on construction to enable determining the URI to access the API endpoint without the overhead of synchronization.

marun requested a review from StephenButtolph as a code owner July 18, 2023 18:09

marun force-pushed the write-runtime-state branch 3 times, most recently from eb15b3b to 178c9fc Compare July 18, 2023 19:30

marun requested review from abi87 and gyuho as code owners July 18, 2023 19:30

marun force-pushed the write-runtime-state branch 2 times, most recently from 2792c36 to 395815f Compare July 18, 2023 20:05

hexfusion reviewed Jul 18, 2023

View reviewed changes

node/node.go Show resolved Hide resolved

marun force-pushed the write-runtime-state branch from 395815f to 1e6a308 Compare July 19, 2023 06:21

marun requested review from danlaine, darioush and dboehm-avalabs as code owners July 19, 2023 06:21

marun force-pushed the write-runtime-state branch 2 times, most recently from e0ebb82 to 8bc6244 Compare July 19, 2023 06:24

abi87 reviewed Jul 19, 2023

View reviewed changes

node/node.go Outdated Show resolved Hide resolved

abi87 reviewed Jul 19, 2023

View reviewed changes

tests/e2e/e2e.go Outdated Show resolved Hide resolved

abi87 reviewed Jul 19, 2023

View reviewed changes

config/flags.go Outdated Show resolved Hide resolved

abi87 suggested changes Jul 19, 2023

View reviewed changes

abi87 assigned marun Jul 19, 2023

abi87 added testing This primarily focuses on testing cleanup Code quality improvement labels Jul 19, 2023

marun force-pushed the write-runtime-state branch from 8bc6244 to a067cbb Compare July 19, 2023 17:43

marun force-pushed the write-runtime-state branch from 5579f4b to 7b4d38b Compare July 20, 2023 17:17

marun changed the title ~~Add --runtime-state-path parameter to support test orchestration~~ Add --write-runtime-state-enabled parameter to support test orchestration Jul 20, 2023

marun force-pushed the write-runtime-state branch 2 times, most recently from a6c8a69 to 105c4ce Compare July 20, 2023 17:26

marun requested a review from abi87 July 20, 2023 22:48

marun force-pushed the write-runtime-state branch 2 times, most recently from ac90e91 to a4d415a Compare July 21, 2023 06:36

marun mentioned this pull request Jul 21, 2023

e2e: Add local network fixture #1700

Merged

1 task

abi87 previously approved these changes Jul 21, 2023

View reviewed changes

StephenButtolph reviewed Jul 22, 2023

View reviewed changes

config/flags.go Outdated Show resolved Hide resolved

node/node.go Outdated Show resolved Hide resolved

StephenButtolph added this to the v1.10.6 milestone Jul 22, 2023

marun force-pushed the write-runtime-state branch 3 times, most recently from 1e8e74d to e0abe51 Compare July 24, 2023 20:01

marun commented Jul 24, 2023

View reviewed changes

api/server/server.go Outdated Show resolved Hide resolved

marun changed the title ~~Add --write-runtime-state-enabled parameter to support test orchestration~~ Write runtime state on node start to simplify test orchestration Jul 24, 2023

StephenButtolph reviewed Jul 24, 2023

View reviewed changes

StephenButtolph approved these changes Jul 24, 2023

View reviewed changes

dhrubabasu reviewed Jul 24, 2023

View reviewed changes

dhrubabasu approved these changes Jul 24, 2023

View reviewed changes

maru-ava and others added 6 commits July 24, 2023 17:09

fixup: Ensure --runtime-state-path arg handling follows best practice

5c1f112

As per review comments, this includes using a default path with GetExpandedArg.

fixup: Write runtime state without concurrency

144ff6a

As per review commentary, it is preferable to bind the listener on construction to enable determining the URI to access the API endpoint without the overhead of synchronization.

Suggest HTTPs API cleanup

4e371b7

fixup: Respond to review feedback.

1f26248

fixup: Rename 'runtime state' to 'process context'

396095e

marun force-pushed the write-runtime-state branch from fbff19d to 396095e Compare July 25, 2023 00:09

marun changed the title ~~Write runtime state on node start to simplify test orchestration~~ Write process context on node start to simplify test orchestration Jul 25, 2023

StephenButtolph approved these changes Jul 25, 2023

View reviewed changes

dhrubabasu approved these changes Jul 25, 2023

View reviewed changes

StephenButtolph merged commit 2b8dc5f into dev Jul 25, 2023

StephenButtolph deleted the write-runtime-state branch July 25, 2023 03:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write process context on node start to simplify test orchestration #1729

Write process context on node start to simplify test orchestration #1729

marun commented Jul 18, 2023 •

edited

Loading

abi87 left a comment

marun commented Jul 19, 2023

marun commented Jul 19, 2023

marun commented Jul 20, 2023

abi87 left a comment

marun commented Jul 24, 2023

dhrubabasu Jul 24, 2023

marun Jul 24, 2023

marun Jul 24, 2023

Write process context on node start to simplify test orchestration #1729

Write process context on node start to simplify test orchestration #1729

Conversation

marun commented Jul 18, 2023 • edited Loading

Why this should be merged

How this works

How this was tested

abi87 left a comment

Choose a reason for hiding this comment

marun commented Jul 19, 2023

marun commented Jul 19, 2023

marun commented Jul 20, 2023

abi87 left a comment

Choose a reason for hiding this comment

marun commented Jul 24, 2023

dhrubabasu Jul 24, 2023

Choose a reason for hiding this comment

marun Jul 24, 2023

Choose a reason for hiding this comment

marun Jul 24, 2023

Choose a reason for hiding this comment

marun commented Jul 18, 2023 •

edited

Loading