Skip to content
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

go/oasis-node/cmd: unsafe-reset check datadir validity #4702

Merged
merged 1 commit into from
Apr 27, 2022

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented Apr 26, 2022

No description provided.

@@ -67,6 +68,21 @@ func doUnsafeReset(cmd *cobra.Command, args []string) {
return
}

// Do a state dir sanity check.
glob := filepath.Join(dataDir, tendermintCommon.StateDir)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could maybe check for some other files/folders as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe identity.NodeKeyPubFilename as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@ptrus ptrus force-pushed the ptrus/feature/unsafe-reset-datadir branch 3 times, most recently from 2c83399 to 4b1d03e Compare April 26, 2022 15:30
@codecov
Copy link

codecov bot commented Apr 26, 2022

Codecov Report

Merging #4702 (4b1d03e) into master (930ea29) will decrease coverage by 0.31%.
The diff coverage is 30.76%.

❗ Current head 4b1d03e differs from pull request most recent head 3c16987. Consider uploading reports for the commit 3c16987 to get more accurate results

@@            Coverage Diff             @@
##           master    #4702      +/-   ##
==========================================
- Coverage   67.18%   66.86%   -0.32%     
==========================================
  Files         436      436              
  Lines       48977    48990      +13     
==========================================
- Hits        32906    32758     -148     
- Misses      12014    12188     +174     
+ Partials     4057     4044      -13     
Impacted Files Coverage Δ
go/oasis-node/cmd/node/unsafe_reset.go 56.71% <25.00%> (-6.92%) ⬇️
go/oasis-node/cmd/node/node.go 56.01% <100.00%> (+1.83%) ⬆️
go/ias/http/http.go 20.33% <0.00%> (-44.07%) ⬇️
go/runtime/host/sandbox/sandbox.go 59.85% <0.00%> (-16.20%) ⬇️
go/keymanager/api/policy_sgx.go 33.33% <0.00%> (-13.34%) ⬇️
...onsensus/tendermint/apps/beacon/state/state_vrf.go 73.33% <0.00%> (-13.34%) ⬇️
go/oasis-node/cmd/ias/auth.go 71.87% <0.00%> (-6.25%) ⬇️
go/common/grpc/auth/auth.go 94.44% <0.00%> (-5.56%) ⬇️
go/beacon/api/grpc.go 28.00% <0.00%> (-5.15%) ⬇️
go/worker/beacon/tx_retry.go 90.47% <0.00%> (-4.77%) ⬇️
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaab525...3c16987. Read the comment docs.

return
}
if len(matches) == 0 && !cmdFlags.Force() {
fmt.Printf("Expected path: '%s' not found. Not removing any files. Use --force to run the command regardless.\n", glob)
Copy link
Member

@kostko kostko Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just say something like:

Suggested change
fmt.Printf("Expected path: '%s' not found. Not removing any files. Use --force to run the command regardless.\n", glob)
fmt.Printf("%s does not look like an oasis-node data directory, not removing any files. Use --force to run the command regardless.\n", dataDir)

// Completely reset state for one of the compute nodes so we can test initial sync.
sc.Logger.Info("completely resetting state for one of the compute nodes")
cli := cli.New(childEnv, sc.Net, sc.Logger)
if err = cli.UnsafeReset(sc.Net.ComputeWorkers()[1].DataDir(), false, false); err != nil {
if err = cli.UnsafeReset(sc.Net.ComputeWorkers()[1].DataDir(), false, false, true); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is force needed here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because unsafe-reset is already run for all nodes in the DumpRestoreNetwork, so the tendermint directory will already be cleared on this node that we run unsafe-reset (without perserving any runtime dirs) again.

@ptrus ptrus force-pushed the ptrus/feature/unsafe-reset-datadir branch from 4b1d03e to 3c16987 Compare April 27, 2022 07:04
@ptrus ptrus enabled auto-merge April 27, 2022 07:11
@ptrus ptrus merged commit 1e4123a into master Apr 27, 2022
@ptrus ptrus deleted the ptrus/feature/unsafe-reset-datadir branch April 27, 2022 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants