[tmpnet] Ensure all node runtime methods accept a context#3894
[tmpnet] Ensure all node runtime methods accept a context#3894
Conversation
maru-ava
force-pushed
the
tmpnet-ensure-logger
branch
from
e05a502 to
ed26001
Compare
maru-ava
force-pushed
the
tmpnet-ensure-context
branch
from
26072d7 to
5b2c08d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR updates node runtime methods to require a context parameter, ensuring that operations involving local processes can now be cancelled or time out appropriately.
- Modified ProcessRuntime methods (readState, Start, InitiateStop) to accept a context.
- Updated Node and Network configuration and runtime functions to propagate context parameters.
- Adjusted test cases to use context when calling runtime functions.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fixture/tmpnet/process_runtime.go | Updated runtime methods to accept context parameters. |
| tests/fixture/tmpnet/node_config.go | Modified Node.Read to propagate context to readState. |
| tests/fixture/tmpnet/node.go | Updated NodeRuntime interface and methods to include context. |
| tests/fixture/tmpnet/network_test.go | Adjusted tests to pass context to network read functions. |
| tests/fixture/tmpnet/network_config.go | Propagated context through network readNodes and related calls. |
| tests/fixture/tmpnet/network.go | Updated network functions to use context when calling methods. |
| tests/fixture/e2e/env.go | Revised environment setup to pass context to network reads. |
| func (p *ProcessRuntime) getProcess() (*os.Process, error) { | ||
| // Read the process context to ensure freshness. The node may have | ||
| // stopped or been restarted since last read. | ||
| if err := p.readState(); err != nil { | ||
| if err := p.readState(context.Background()); err != nil { | ||
| return nil, fmt.Errorf("failed to read process context: %w", err) |
There was a problem hiding this comment.
Consider passing a meaningful context instead of context.Background() to ensure proper cancellation propagation.
There was a problem hiding this comment.
The context is necessary for the method, but it isn't used.
There was a problem hiding this comment.
context.Background() is generally reserved for the entry point into the program... so I feel like we should either not ignore this comment and expect getProcess to pass in a context, or just use context.TODO().
michaelkaplan13
left a comment
michaelkaplan13
left a comment
There was a problem hiding this comment.
I agree with @joshua-kim on using context.TODO() in that one case (or passing a context to getProcess, but that doesn't seem necessary right now).
LGTM otherwise
maru-ava
force-pushed
the
tmpnet-ensure-logger
branch
from
ed26001 to
aa5189d
Compare
maru-ava
force-pushed
the
tmpnet-ensure-context
branch
from
5b2c08d to
a61bdec
Compare
Previously, runtime methods didn't always require a context to interact with local processes. The kube runtime almost always involves network calls so a context is required.
maru-ava
force-pushed
the
tmpnet-ensure-context
branch
from
a61bdec to
8511fd9
Compare
|
Rebased |
PR Chain: tmpnet+kube
This PR chain enables tmpnet to deploy temporary networks to Kubernetes. Early PRs refactor tmpnet to support the addition in #3615 of a new tmpnet node runtime for kube.
Why this should be merged
Previously, runtime methods didn't always require a context to interact with local processes. The kube runtime always involves network calls so a context is required.
How this was tested
C/I
Need to be documented in RELEASES.md?
N/A
TODO