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: Graceful shutdown if disk is full #22103

Merged
merged 10 commits into from
Jan 19, 2021
Merged

cmd/geth: Graceful shutdown if disk is full #22103

merged 10 commits into from
Jan 19, 2021

Conversation

alex347
Copy link
Contributor

@alex347 alex347 commented Jan 3, 2021

Adding warnings of low free disk space left and graceful shutdown when there is not enough space left.

Adding warnings of free disk space left and graceful shutdown when there is not enough space left.
@ligi
Copy link
Member

ligi commented Jan 3, 2021

Is this a replacement for #21884 ?

@alex347
Copy link
Contributor Author

alex347 commented Jan 3, 2021

Is this a replacement for #21884 ?

yes, this PR implements #20965 and takes into account comments from #21884

@ligi
Copy link
Member

ligi commented Jan 3, 2021

Thanks for the work on it - but I think it would be better to do the changes in the old PR then - wondering why you opened a new one. Seems you know how to push to a existing PR.

@alex347
Copy link
Contributor Author

alex347 commented Jan 3, 2021

I don't think I can modify PR if I'm not the author of it? Do you mean pushing my changes to vyrwu:handle-full-disk ?

@ligi
Copy link
Member

ligi commented Jan 3, 2021

Oh sorry - just saw that the other PR is not from you - you did everything good then.

cmd/utils/diskusage.go Outdated Show resolved Hide resolved
cmd/utils/diskusage_windows.go Outdated Show resolved Hide resolved
cmd/utils/cmd.go Outdated Show resolved Hide resolved
cmd/utils/cmd.go Outdated Show resolved Hide resolved
cmd/utils/diskusage.go Outdated Show resolved Hide resolved
cmd/utils/diskusage_windows.go Outdated Show resolved Hide resolved
cmd/utils/diskusage.go Outdated Show resolved Hide resolved
cmd/utils/cmd.go Outdated Show resolved Hide resolved
cmd/utils/cmd.go Outdated Show resolved Hide resolved
alex347 and others added 2 commits January 4, 2021 14:24
code formatting

Co-authored-by: Martin Holst Swende <martin@swende.se>
cmd/utils/cmd.go Outdated Show resolved Hide resolved
cmd/utils/cmd.go Outdated Show resolved Hide resolved
cmd/utils/cmd.go Outdated Show resolved Hide resolved
changing free disk warn level;
check free disk space once in a minute
cmd/utils/cmd.go Outdated
if err := stack.Start(); err != nil {
Fatalf("Error starting protocol stack: %v", err)
}
go func() {
sigc := make(chan os.Signal, 1)
signal.Notify(sigc, syscall.SIGINT, syscall.SIGTERM)
defer signal.Stop(sigc)

syncMode := *GlobalTextMarshaler(ctx, SyncModeFlag.Name).(*downloader.SyncMode)
if chainID.Int64() == 1 && syncMode != downloader.FastSync {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's the light sync mode we want to make an exception for.

Still, I'm not 100% sure this is the best approach, I'll think about it.

@holiman
Copy link
Contributor

holiman commented Jan 5, 2021

Two comments from the triage:

  • We need to add a flag to explicitly set the limit. If it's set to 0, then the mechanism should be disabled.
  • Instead of using chain id / syncmode, we should default to using the limit == dirty-cache size (25% of the cache size).

@fjl fjl removed the status:triage label Jan 5, 2021
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.

This turned out pretty neat in the end, thanks for working on it!
It appears to work fine on linux, would be neat to get this tested on Windows, Darwin and/or FreeBsd too.
And potentially if the datadir is mapped via some other filesystem, like an NFS mount.

@alex347
Copy link
Contributor Author

alex347 commented Jan 6, 2021

would be neat to get this tested on Windows, Darwin and/or FreeBsd too.
And potentially if the datadir is mapped via some other filesystem, like an NFS mount.

I've tested it and it works fine on Windows, Darwin and NFS mount

@ligi
Copy link
Member

ligi commented Jan 6, 2021

Do you mind showing some proof or elaborating?

if you really want to help: test it yourself instead of asking for proof from someone that put in really good work into this feature - also not sure what kind of elaboration is needed here

@holiman holiman merged commit 24c1e30 into ethereum:master Jan 19, 2021
@holiman holiman added this to the 1.10.0 milestone Jan 19, 2021
bulgakovk pushed a commit to bulgakovk/go-ethereum that referenced this pull request Jan 26, 2021
Adding warnings of free disk space left and graceful shutdown when there is not enough space left.
This also adds a flag datadir.minfreedisk which can be used to set the trigger for low disk space, and setting it to zero disables the check. 

Co-authored-by: Martin Holst Swende <martin@swende.se>
Co-authored-by: Felix Lange <fjl@twurst.com>
@gaia
Copy link

gaia commented Mar 3, 2021

How low is low? Is it % or MB? Is it possible to adjust this setting?

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.

6 participants