Fixes #20859 added unsafe shutdown detector #21133

muhammednagy · 2020-05-27T19:30:40Z

Added unsafe shutdown detector in case the app crashed.

fixes: #20859

muhammednagy · 2020-05-28T10:27:38Z

Not sure why it's failing on ARM or OSX.
I am using OSX myself.

MariusVanDerWijden · 2020-05-29T13:31:43Z

@muhammednagy Don't worry, these tests are known to be flaky and fail sometimes

MariusVanDerWijden · 2020-05-29T13:49:23Z

cmd/geth/main.go

@@ -378,13 +378,25 @@ func startNode(ctx *cli.Context, stack *node.Node) {
 	}
 	ethClient := ethclient.NewClient(rpcClient)

+	var ethService *eth.Ethereum


Hey @muhammednagy this is unfortunately not valid. You can run a light-node without having the eth.Ethereum service started. @ligi correct me if I'm wrong but we mostly care about crashes in full client not in light clients, so I think its valid to move this code into the if ctx.GlobalInt... on line 383.

To repro failure:

> make geth > ./build/bin/geth --syncmode=light --goerli ... Fatal: Failed to retrieve ethereum service: unknown service

yea - it would mainly be interesting for full nodes - so I think we should go with what you proposed. But if there is a way to have it for both this would also be nice

What about using a different database? is it possible?
We could probably store them in a file instead which will work for whatever type of node we are running.

It's definitely possible, but I think we want to have it in the same database. Otherwise we just wind up with a separate set of problems, where we need to care about consistency over many files, and keeping that file in sync with when e.g people do removedb or removes the db manually.

okay i think in that case a file would work best for all regardless of the type of node.
Should i go ahead with having a file?
If not then do you know which db i can use that would work for all kinds of nodes?

Just use the leveldb instance we already have throughout the code for everything.

muhammednagy · 2020-06-02T09:51:45Z

What about if it's not serving LES requests?
I am just running it locally with no flags and db is not getting set.

holiman · 2020-06-02T10:40:11Z

You most definitely have a database. It's in the Ethereum object, instantiated in eth/backend.go, line 139.

muhammednagy · 2020-06-02T15:11:04Z

Hey,
Yes i do.
The example was relying on getting the database if we are running a light sync mode or light chain.
thus it wasn't setting db in normal times.
I've gone about it in a different way.
Tell me what do you think and if you think it's good i can move it to a function in utils or somewhere else to save the duplicate code.

holiman · 2020-06-09T09:00:55Z

We've discussed this today. This PR is a good first step in the right direction, but it is a bit lacking. We would like to be able to show, in the startup log, when the last unclean shutdown happened.

In order to do that, we cannot delete the unclean-shutdown when we boot up next time, but need to have either two different keys, or extend it to be a collection of timestamps when unclean shutdowns were performed.

karalabe · 2020-06-09T09:01:59Z

eth/backend.go

@@ -567,6 +577,9 @@ func (s *Ethereum) Stop() error {
 		s.lesServer.Stop()
 	}

+	if err := s.chainDb.Delete([]byte("unsafe-shutdown")); err != nil {
+		log.Error("err", err)
+	}


You'll need to move this down, right above chaindb.Close(). The biggest offender for an unclean shutdown is the blockchain shutdown mechanism.

karalabe · 2020-06-09T09:05:16Z

eth/backend.go

+	// Create an invalid shutdown in database in case the app crashed
+	if err = chainDb.Put([]byte("unsafe-shutdown"), []byte(time.Now().String())); err != nil {
+		log.Warn("Failed to record possible future unsafe shutdown", "err", err)
+	}


The PR uses a single unsafe-shutdown key to store a single timestamp. That's a good start, but we need a bit more information, otherwise quirky scenarios get lost. Notably, it's important to retain past crashes too, so a user might see if there was a fault recently, not just in the last restart (user's tend to restart their nodes a few times until attempting to investigate an issue, so a restart cannot delete the cause).

A simplistic suggestion would be to use a slice of timestamps instead of a single timestamp as the value of the entry. When Geth starts, you can append the current time, and on shutdown, you can pop off the tail. That way on startup you can actually list all the recent crashes, not just the last one.

karalabe · 2020-06-09T09:05:54Z

eth/backend.go

+	}
+	// Create an invalid shutdown in database in case the app crashed
+	if err = chainDb.Put([]byte("unsafe-shutdown"), []byte(time.Now().String())); err != nil {
+		log.Warn("Failed to record possible future unsafe shutdown", "err", err)


I think it's safe to return nil, err here. Although it's not a critical system, if writing to the database failed something's going to go belly up either way.

karalabe · 2020-06-09T09:06:44Z

eth/backend.go

+	// Check for invalid shutdown
+	invalidShutdown, _ := chainDb.Get([]byte("unsafe-shutdown"))
+	if invalidShutdown != nil {
+		log.Error("unsafe shutdown detected", "time", string(invalidShutdown))


Please capitalize the log message. Also please use a proper time value, not a pre-stringified version.

karalabe · 2020-06-09T09:07:06Z

eth/backend.go

+		log.Error("unsafe shutdown detected", "time", string(invalidShutdown))
+	}
+	// Create an invalid shutdown in database in case the app crashed
+	if err = chainDb.Put([]byte("unsafe-shutdown"), []byte(time.Now().String())); err != nil {


Pls use time.Now().Unix() to store the current timestamp instead of a localized string

karalabe · 2020-06-09T09:07:50Z

eth/backend.go

@@ -231,6 +232,15 @@ func New(ctx *node.ServiceContext, config *Config) (*Ethereum, error) {
 		return nil, err
 	}

+	// Check for invalid shutdown
+	invalidShutdown, _ := chainDb.Get([]byte("unsafe-shutdown"))


Since you're using unsafe-shutdown in quite a few places and in different packages, it might make sense to move this into core/rawdb/schema.go. We have a lot of other metadata keys defined there.

muhammednagy · 2020-06-09T11:08:08Z

Hey @holiman @karalabe
Thank you so much for your comments.
I was actually waiting for your comments.
I will do the changes as requested and let you know here once am done or have any questions.
It's my first issue to solve here.
Thanks for your understanding.

muhammednagy · 2020-06-22T19:29:45Z

working on this.
Should I show only the last error message on startup? or all of them?
@karalabe @holiman

holiman · 2020-07-07T08:34:35Z

Should I show only the last error message on startup? or all of them?

You can't show the error message per se, because you don't have it, but I'm thinking you can show something like Unclean shutdown detected "unclean shutdown"="5d3h ago"

There's no need to list every single one

adamschmideg · 2020-08-18T08:43:48Z

This PR seems to be abandoned. If you feel like taking it from here, please go ahead.

muhammednagy · 2020-08-18T10:08:15Z

Hey
Sorry, I got swamped in other stuff.
I can try to work on it over the weekend.

holiman · 2020-11-23T17:55:53Z

Thanks for getting started on this @muhammednagy! I took your work and extended it with the additional requirements we had, which is implemented in #21893. I'm closing this PR in favour of that one

muhammednagy requested review from holiman, karalabe and rjl493456442 as code owners May 27, 2020 19:30

muhammednagy force-pushed the master branch from e4b22bb to 13c3e24 Compare May 27, 2020 19:44

MariusVanDerWijden reviewed May 29, 2020

View reviewed changes

cmd added unsafe shutdown detection

eb39602

muhammednagy force-pushed the master branch from 13c3e24 to eb39602 Compare June 1, 2020 19:14

This comment has been minimized.

Sign in to view

cmd added unsafe shutdown detection to ethereum and lightchain

b407aa9

muhammednagy requested a review from zsfelfoldi as a code owner June 2, 2020 15:09

adamschmideg added the status:triage label Jun 9, 2020

adamschmideg added pr:change-requested and removed status:triage labels Jun 9, 2020

karalabe reviewed Jun 9, 2020

View reviewed changes

adamschmideg added status:triage and removed status:triage labels Jul 7, 2020

adamschmideg added the status:triage label Aug 3, 2020

holiman added the help wanted label Aug 18, 2020

adamschmideg removed the status:triage label Aug 18, 2020

holiman mentioned this pull request Nov 23, 2020

implement unclean-shutdown marker #21893

Merged

1 task

holiman closed this Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #20859 added unsafe shutdown detector #21133

Fixes #20859 added unsafe shutdown detector #21133

muhammednagy commented May 27, 2020 •

edited by MariusVanDerWijden

Loading

muhammednagy commented May 28, 2020

MariusVanDerWijden commented May 29, 2020

MariusVanDerWijden May 29, 2020

ligi May 29, 2020

muhammednagy May 31, 2020

holiman Jun 1, 2020

muhammednagy Jun 1, 2020

karalabe Jun 1, 2020

This comment has been minimized.

This comment has been minimized.

muhammednagy commented Jun 2, 2020

holiman commented Jun 2, 2020

muhammednagy commented Jun 2, 2020

holiman commented Jun 9, 2020

karalabe Jun 9, 2020

karalabe Jun 9, 2020

karalabe Jun 9, 2020

karalabe Jun 9, 2020

karalabe Jun 9, 2020

karalabe Jun 9, 2020

muhammednagy commented Jun 9, 2020

muhammednagy commented Jun 22, 2020 •

edited

Loading

holiman commented Jul 7, 2020 •

edited

Loading

adamschmideg commented Aug 18, 2020

muhammednagy commented Aug 18, 2020

holiman commented Nov 23, 2020

Fixes #20859 added unsafe shutdown detector #21133

Fixes #20859 added unsafe shutdown detector #21133

Conversation

muhammednagy commented May 27, 2020 • edited by MariusVanDerWijden Loading

muhammednagy commented May 28, 2020

MariusVanDerWijden commented May 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

muhammednagy commented Jun 2, 2020

holiman commented Jun 2, 2020

muhammednagy commented Jun 2, 2020

holiman commented Jun 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhammednagy commented Jun 9, 2020

muhammednagy commented Jun 22, 2020 • edited Loading

holiman commented Jul 7, 2020 • edited Loading

adamschmideg commented Aug 18, 2020

muhammednagy commented Aug 18, 2020

holiman commented Nov 23, 2020

muhammednagy commented May 27, 2020 •

edited by MariusVanDerWijden

Loading

muhammednagy commented Jun 22, 2020 •

edited

Loading

holiman commented Jul 7, 2020 •

edited

Loading