-
Notifications
You must be signed in to change notification settings - Fork 67
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
Integrate local machine into subnet deploy #2235
Conversation
cmd/blockchaincmd/deploy.go
Outdated
aggregatorExtraPeerEndpoints, err := GetAggregatorExtraPeerEndpoints(network) | ||
|
||
// TODO: replace wait below wiht check for healhty in local machine | ||
time.Sleep(70 * time.Second) |
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.
will have to change this
Signed-off-by: sukantoraymond <[email protected]>
cmd/blockchaincmd/deploy.go
Outdated
} | ||
|
||
if useLocalMachine { | ||
bootstrapEndpoints = []string{"http://127.0.0.1:9650"} |
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.
this might be tricky to assume this endpoint as port can be not 9650 but rather dynamic
cc @felipemadero
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.
maybe we should ask user for the endpoint and not yes/no for local machine.
we can hint that it can be 127.0.0.1:9650 for the local machine
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 see now. After the previous question, a Start should be made and from that, the endpoint
can be obtained from the anr itself, not need to assume anything.
This is not related to the flag useLocalMachine. This relates to the local cluster that exists previous
to the deploy. The best way should be to check the network, which should be a local cluster,
in that case take the endpoint from anr.
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.
Probably for another PR. Make an issue for this and keep current static assignment.
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.
pkg/node/helper.go
Outdated
if exists, err := CheckClusterExists(app, clusterName); err != nil || !exists { | ||
return nil, fmt.Errorf("cluster %q not found", clusterName) | ||
} | ||
clustersConfig, err := app.LoadClustersConfig() |
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: use
clusterConfig, err := app.GetClusterConfig(clusterName)
if err != nil {
return err
}
```
and
`clusterConfig.Nodes` instead
pkg/subnet/helpers.go
Outdated
) | ||
|
||
var errIllegalNameCharacter = errors.New( |
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.
fix format
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.
this is just copying from refactoring part.
cmd/blockchaincmd/deploy.go
Outdated
ux.Logger.PrintToUser("You can use your local machine as a bootstrap validator on the blockchain") | ||
ux.Logger.PrintToUser("This means that you don't have to to set up a remote server on a cloud service (e.g. AWS / GCP) to be a validator on the blockchain.") | ||
|
||
useLocalMachine, err := app.Prompt.CaptureYesNo("Do you want to use your local machine as a bootstrap validator?") |
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.
flag to avoid question for this and just proceed
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.
addressed
cmd/blockchaincmd/deploy.go
Outdated
@@ -464,6 +466,18 @@ func deployBlockchain(cmd *cobra.Command, args []string) error { | |||
return PrintSubnetInfo(blockchainName, true) | |||
} | |||
|
|||
ux.Logger.PrintToUser("You can use your local machine as a bootstrap validator on the blockchain") |
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.
also this to be guarded by a flag
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.
addressed
cmd/blockchaincmd/deploy.go
Outdated
@@ -464,6 +466,18 @@ func deployBlockchain(cmd *cobra.Command, args []string) error { | |||
return PrintSubnetInfo(blockchainName, true) | |||
} | |||
|
|||
ux.Logger.PrintToUser("You can use your local machine as a bootstrap validator on the blockchain") | |||
ux.Logger.PrintToUser("This means that you don't have to to set up a remote server on a cloud service (e.g. AWS / GCP) to be a validator on the blockchain.") |
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.
probably we want to explain more here? like this is temporary bootstrap validator
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.
Its not temporary if user wants to leave it the node indefinitely as validator
cmd/blockchaincmd/deploy.go
Outdated
} | ||
|
||
if useLocalMachine { | ||
bootstrapEndpoints = []string{"http://127.0.0.1:9650"} |
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 see now. After the previous question, a Start should be made and from that, the endpoint
can be obtained from the anr itself, not need to assume anything.
This is not related to the flag useLocalMachine. This relates to the local cluster that exists previous
to the deploy. The best way should be to check the network, which should be a local cluster,
in that case take the endpoint from anr.
cmd/blockchaincmd/deploy.go
Outdated
} | ||
|
||
if useLocalMachine { | ||
bootstrapEndpoints = []string{"http://127.0.0.1:9650"} |
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.
Probably for another PR. Make an issue for this and keep current static assignment.
chainSpec, | ||
) | ||
if err != nil { | ||
clusterName, err := node.GetClusterNameFromList(app) |
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.
The cluster name to be taken from the input network itself IMO
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.
which should be a cluster
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.
models.Networks has ClusterName
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.
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.
better to do it when integrating local node start
cmd/nodecmd/local.go
Outdated
@@ -415,14 +416,17 @@ func localStopNode(_ *cobra.Command, _ []string) error { | |||
func localDestroyNode(_ *cobra.Command, args []string) error { | |||
clusterName := args[0] | |||
|
|||
localStopNode(nil, nil) | |||
if err := localStopNode(nil, nil); err != nil { | |||
return fmt.Errorf("failed to destroy local node: %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.
node can be already stopped. for the moment on ignore the error or verify is the node is already stopped,
we want to continue removing all the directorioes that is the reason the error is ignored
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.
just need to make sure this passes lint. removed 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.
addressed
} | ||
ux.Logger.GreenCheckmarkToUser("%s successfully tracking %s", clusterName, blockchainName) | ||
return nil | ||
return node.TrackSubnetWithLocalMachine(app, clusterName, blockchainName) |
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 am not sure it is the moment to move this to node pgk. we keep a part of logic here and a part in node.
also, maybe node is not the final dest package.
pkg/node/helper.go
Outdated
@@ -0,0 +1,298 @@ | |||
// Copyright (C) 2022, Ava Labs, Inc. All rights reserved. | |||
// See the file LICENSE for licensing terms. | |||
package node |
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.
this is for another 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.
anyway, no problem keeping here
Why this should be merged
Integrates using local machine as bootstrap validator into subnet deploy command.
How this works
// create the local node connected to etna
go run main.go node local start --etna-devnet --avalanchego-path <AVALANCHEGO_PATH>
// create a blockchain conf
go run main.go blockchain create
// create subnet+blockchain+convert on the blokchain
go run main.go blockchain deploy --cluster
How this was tested
./bin/avalanche subnet deploy will have to achieve L1 set up with local machine as sole bootstrap validator in Etna Devnet
How is this documented
NA