-
Notifications
You must be signed in to change notification settings - Fork 758
[tmpnet] Refactor runtime configuration in preparation for kube #3867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6a78622
to
ca512a2
Compare
4983079
to
b824686
Compare
if err := n.EnsureNodeConfig(node); err != nil { | ||
return err | ||
} | ||
if err := node.Write(); err != nil { | ||
return err | ||
} | ||
|
||
// Check the VM binaries after EnsureNodeConfig to ensure node.RuntimeConfig is non-nil | ||
if err := checkVMBinaries(log, n.Subnets, node.RuntimeConfig.AvalancheGoPath, pluginDir); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this check to the node
@@ -298,17 +284,6 @@ func (n *Network) Create(rootDir string) error { | |||
} | |||
n.Dir = canonicalDir | |||
|
|||
// Ensure the existence of the plugin directory or nodes won't be able to start. | |||
pluginDir, err := n.GetPluginDir() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this to node start
b824686
to
ff2f225
Compare
@@ -220,15 +218,6 @@ func (n *Network) EnsureDefaultConfig(log logging.Logger, avalancheGoPath string | |||
n.DefaultFlags = FlagsMap{} | |||
} | |||
|
|||
// Only configure the plugin dir with a non-empty value to ensure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plugin dir is now only ever set in node flags rather than as a network default flag
77b1a04
to
7db5b52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
tests/fixture/tmpnet/network.go:1039
- The PluginDir field is used directly in forming the VM binary path. Consider adding a validation to ensure PluginDir is not empty (or providing a default) to prevent constructing an invalid path.
vmPath := filepath.Join(config.PluginDir, chain.VMID.String())
tests/antithesis/compose.go:76
- The removal of avalancheGoPath and pluginDir parameters in initBootstrapDB and the subsequent call to BootstrapNewNetwork could lead to misconfiguration in environments that expect these values. Consider ensuring that default values or validations are in place to maintain correct behavior.
if err := initBootstrapDB(network, bootstrapVolumePath); err != nil {
case processRuntime: | ||
processRuntimeConfig, err := v.processRuntimeVars.getProcessRuntimeConfig() | ||
if err != nil { | ||
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit it might be useful when supporting more runtimes (kube) to mention what runtime we tried to get, in case it's misconfigured:
return nil, err | |
return nil, fmt.Errorf("for runtime %s: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
IsEphemeral bool `json:",omitempty"` | ||
Flags FlagsMap `json:",omitempty"` | ||
RuntimeConfig *NodeRuntimeConfig `json:",omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these omitempty tags necessary or an extra addition to the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's secondary cleanup related to how runtime configuration is serialized.
type NodeRuntimeConfig struct { | ||
Process *ProcessRuntimeConfig | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the aim is to then have a Process and Kube field right?
Could/Should we instead have an interface here, with the process implementation and a future kube implementation? 🤔
Maybe I'm missing the point completely though, in which case, my apologies 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a struct, it has fields, and there are no fields in common between process and kube to suggest an interface that would be common between them. There is an an interface for the runtime for which there are methods common to both types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for this. Preaproving with just a couple nits/questions
@@ -518,7 +479,8 @@ func (n *Network) StartNode(ctx context.Context, log logging.Logger, node *Node) | |||
|
|||
// Restart a single node. | |||
func (n *Network) RestartNode(ctx context.Context, log logging.Logger, node *Node) error { | |||
if node.RuntimeConfig.ReuseDynamicPorts { | |||
runtimeConfig := node.getRuntimeConfig() | |||
if runtimeConfig.Process != nil && runtimeConfig.Process.ReuseDynamicPorts { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q/ couldn't ReuseDynamicPorts be attempted on kube? I wonder if this setting can be separated from the kind of runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Reuse' implies the use of dynamic ports in the first place, but dynamic ports are not compatible with kube due to the kubelet needing a fixed port to perform readiness checks against. Also not needed because a kube pod gets its own IP so there is no potential for clashing with other nodes deployed as pods.
@@ -37,8 +37,9 @@ const ( | |||
var ( | |||
AvalancheGoPluginDirEnvName = config.EnvVarName(config.EnvPrefix, config.PluginDirKey) | |||
|
|||
errNodeAlreadyRunning = errors.New("failed to start node: node is already running") | |||
errNotRunning = errors.New("node is not running") | |||
errNodeAlreadyRunning = errors.New("failed to start node: node is already running") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may it be desirable for tmpnet as a library to export errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to prefer YAGI, and I'm not getting any requests to export these errors. The driver for exporting is more likely to be implementing e2e testing of tmpnet itself, which is a near-term goal to ensure behavior that isn't implicitly tested as part of avalanchego's e2e suite.
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
Adding the kube runtime for tmpnet requires being able to configure it via cli args. This PR refactors the existing runtime configuration to better support the introduction of the kube runtime configuration.
How this works
How this was tested
CI, manual use of tmpnetctl
Need to be documented in RELEASES.md?
N/A
TODO