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

cmd/geth: fix import / export issues related to DB unavailability #21414

Merged
merged 6 commits into from
Aug 6, 2020

Conversation

renaynay
Copy link
Contributor

@renaynay renaynay commented Aug 5, 2020

Fixes #21412

@renaynay renaynay requested a review from fjl August 5, 2020 09:29
@holiman
Copy link
Contributor

holiman commented Aug 5, 2020

Can confirm it fixes the bug that was reported (haven't looked at the code yet)

@holiman
Copy link
Contributor

holiman commented Aug 5, 2020

Some more locations to apply the same fix for:

  • build/bin/geth dump 0
  • echo "lalala" > deleteme; build/bin/geth import-preimages deleteme ; rm deleteme
  • build/bin/geth export-preimages deleteme
  • build/bin/geth copydb /home/user/.ethereum/geth/chaindata /tmp/foo

@holiman
Copy link
Contributor

holiman commented Aug 5, 2020

Looking at the code and fix, it's not quite clear to me why this fails -- since makeFullNode does actually call makeConfigNode under the hood. Perhaps it would be good to document these methods a bit, or at least add a description of which to use when (and when not to).
What was the issue really?

@renaynay
Copy link
Contributor Author

renaynay commented Aug 5, 2020

Yes I noticed that too, made a separate PR for those here: #21415

The explanation for this issue is that, when makeFullNode is called, an eth service is created as well, which attempts to open db on the node while the chaincmd (whether it's import, dump, export, so forth) also try to open the node's db simultaneously. I mentioned this in the comment of the above linked PR.

@renaynay
Copy link
Contributor Author

renaynay commented Aug 5, 2020

Documenting this in the methods is a good idea.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@fjl fjl changed the title cmd: fixes import / export issues related to DB unavailability cmd/geth: fixes import / export issues related to DB unavailability Aug 5, 2020
@fjl fjl changed the title cmd/geth: fixes import / export issues related to DB unavailability cmd/geth: fix import / export issues related to DB unavailability Aug 5, 2020
@@ -278,7 +278,8 @@ func importChain(ctx *cli.Context) error {
utils.SetupMetrics(ctx)
// Start system runtime metrics collection
go metrics.CollectProcessMetrics(3 * time.Second)
stack, _ := makeFullNode(ctx)
// Make the bare-minimum node to avoid unnecessary service start-up
Copy link
Contributor

@fjl fjl Aug 5, 2020

Choose a reason for hiding this comment

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

Would be better to put a documentation comment on the actual function definition instead of documenting the behavior of makeConfigNode at individual call sites.

// makeConfigNode loads geth configuration and creates a blank node instance.
// makeFullNode loads geth configuration and creates the Ethereum backend.

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

SGTM

@karalabe karalabe merged commit 4a04127 into ethereum:master Aug 6, 2020
@karalabe karalabe added this to the 1.9.19 milestone Aug 6, 2020
enriquefynn pushed a commit to enriquefynn/go-ethereum that referenced this pull request Mar 10, 2021
…hereum#21414)

* should fix import / export issues related to DB unavailability

* document reason for makeConfigNode

* fix comment

* comment consistency

* remove comments

* lint
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.

geth import: Fatal: Could not open database: resource temporarily unavailable
4 participants