Sync blockchain before running e2e tests on public testnet #568 #612

canercidam · 2018-02-04T14:46:59Z

Option to sync blockchain and exit in statusd, for synchronizing before e2e tests.

Changes:

-exit-when-synced flag is added to statusd.
NodeManager is extended with Sync.
Sync reuses the code in "testing/testing.go", EnsureNodeSync.
.travis.yml is modified to sync before e2e tests on public network.

Closes #568

adambabik

Thanks for the PR! Overall, it's the way to go but there are a few things that could be improved.

adambabik · 2018-02-04T22:47:27Z

cmd/statusd/main.go

@@ -62,6 +62,8 @@ var (
 	// Push Notification
 	enablePN     = flag.Bool("shh.notify", false, "Node is capable of sending Push Notifications")
 	firebaseAuth = flag.String("shh.firebaseauth", "", "FCM Authorization Key used for sending Push Notifications")
+
+	exitWhenSynced = flag.Bool("exit-when-synced", false, "Exit after blockchain sync")


Maybe we can make it an integer and default to -1? If it's larger than -1 then this option is active and its value tells how long to wait for a synchronization to finish? 0 could mean wait infinitely long until it's synced.

Sure, shall we expect minutes for values larger than 0?

Ah okay, it's already clear in the comment below.

adambabik · 2018-02-04T22:50:29Z

cmd/statusd/main.go

+	// Sync blockchain and stop.
+	if *exitWhenSynced {
+		syncStopErr := backend.NodeManager().SyncAndStopNode(time.Minute * 50)
+		defer handleSyncStopErr(syncStopErr)


I am not really sure why defer was chosen. I would suggest making SyncAndStopNode synchronous and actually, change this method a bit (more about that below).

err := backend.NodeManager().SyncAndStopNode(*exitWhenSynced * time.Minute) if err != nil { log.Fatalf("Failed to sync: %v", err) } return

I just wanted it to play along with the existing flow and make it such as:

Start node.

Launch sync asynchronously.

Wait until node is stopped.

Handle aggregated errors from sync (max 2).

So I used defer there to handle aggregated errors before exiting the program and it makes the least amount of modification to main function thus more readable.

I think I missed the fact that reused code just tracks the progress, sorry about that. :)

adambabik · 2018-02-04T22:53:56Z

geth/node/manager.go

+
+// SyncAndStopNode waits until node synchronization and exits.
+// Reports errors to returned channel.
+func (m *NodeManager) SyncAndStopNode(timeout time.Duration) (errChan <-chan error) {


I don't see any benefits in having a single method that does all this stuff. There is a method to stop a node so I guess only Sync() would be needed. In main.go, it can be called like this:

if err := m.Sync(); err != nil { // exit } if err := m.StopNode(); err != nil { // exit }

Also, it should have a synchronous interface. If the caller wants to call it asynchronously, then it's its trouble.

adambabik · 2018-02-04T22:56:12Z

geth/node/manager.go

+	defer ticker.Stop()
+	for {
+		select {
+		case <-timer.C:


I would use <-time.After(timeout) as it's used just once in contrast to ticker.

canercidam · 2018-02-04T23:20:09Z

@adambabik I think you're right about reducing it to Sync and making it synchronous. It's cleaner and also yields much less trouble. Also makes more sense in the interface as an extra function.

dshulyak · 2018-02-05T06:49:44Z

cmd/statusd/main.go

@@ -62,6 +62,8 @@ var (
 	// Push Notification
 	enablePN     = flag.Bool("shh.notify", false, "Node is capable of sending Push Notifications")
 	firebaseAuth = flag.String("shh.firebaseauth", "", "FCM Authorization Key used for sending Push Notifications")
+
+	exitWhenSynced = flag.Int("exit-when-synced", -1, "Timeout for blockchain sync and exit in minutes, zero is infinite")


name is a bit too long, could it be sync-only?

Sure, changing.

mandrigin

Thanks for your submission!

In general, it looks good! It would be nice to see some small changes, mostly related to documentation.

mandrigin · 2018-02-05T10:14:35Z

cmd/statusd/main.go

@@ -62,6 +62,8 @@ var (
 	// Push Notification
 	enablePN     = flag.Bool("shh.notify", false, "Node is capable of sending Push Notifications")
 	firebaseAuth = flag.String("shh.firebaseauth", "", "FCM Authorization Key used for sending Push Notifications")
+
+	exitWhenSynced = flag.Int("exit-when-synced", -1, "Timeout for blockchain sync and exit in minutes, zero is infinite")


That's a separate mode of an app. Maybe we should add this to the documentation.
"Special mode of the app to sync blockchain and exit when synced. Timeout is N seconds. If N is zero, timeout is infinite"

The flag name was probably specified in the task, but I'd rather call it -sync-and-exit.

> statusd -sync-and-exit

is more readable to my eyes, IMO.

Also, please note that the waiting time is in minutes, that is not obvious without reading code.

Actually, -sync-and-exit looks better to me than -sync-only. Just because -sync-only is rather vague.

About the waiting time: statusd -h should print the usage line which says "in minutes". But I can also add your description to somewhere else as well if you like. Where else do you use for denoting such things in the project?

I'll change "Timeout for blockchain sync and exit in minutes, zero is infinite" to "Timeout in minutes for blockchain sync and exit, zero is infinite" which is more intelligible.

any reasons not to use flag.Duration ?

We could but I think we rely on only minutes here.

mandrigin · 2018-02-05T10:22:25Z

cmd/statusd/main.go

@@ -106,6 +108,21 @@ func main() {
 		go startCollectingStats(interruptCh, backend.NodeManager())
 	}

+	// Sync blockchain and stop.
+	if *exitWhenSynced > -1 {


I'd add a log entry there that tells that the node will just sync blockchain and then exit. I'd also probably extract this to a separate function to keep main() simpler.

if *exitWhenSynced >= 0 { syncAndStopNode(backend) return }

Yes, sync and exit stuff look like a fairly big block of code in main(). I'll do both.

mandrigin · 2018-02-05T10:26:34Z

geth/common/types.go

@@ -53,6 +53,9 @@ type NodeManager interface {
 	// Stopped node cannot be resumed, one starts a new node instead.
 	StopNode() (<-chan struct{}, error)

+	// Sync waits until blockchain is synchronized.
+	Sync(timeout time.Duration) error


It would be nice to mention that 0 means infinite timeout and not "no timeout"?
Maybe also add that it should be > 2 seconds.

I didn't only because it's not an implementation of the function. I'll instead add it to ./geth/node/manager.go if that works for you as well.

Yeah, but that means some terra incognita between a contract (an interface) and an implementation of the interface.
Maybe we could make a custom type to make the contract more explicit?
Currently, there are 2 cases that 1 parameter tries to match, using some implicit logic.
Can we "explicify" this logic using a custom type on top of time.Duration, so we can call

m.Sync(common.SyncTimeout(1 * time.Minute)) m.Sync(common.SyncTimeoutInfinite)

Or just extract all the logic about that to the main.go.

One "go" way to do that would be to actually provide a context inside Sync function, so we can cancel it. Then we can just use select in main.go to provide the timeout functionality.

@adambabik @mandrigin

One "go" way to do that would be to actually provide a context inside Sync function, so we can cancel it.

I think this is by far the best option here in terms of usage and contract definition. One could simply pass in context.Background() or context.WithTimeout(). That way, handling the timeout in Sync would be simplified and we could use the context timing logic in main().

mandrigin · 2018-02-05T10:28:41Z

geth/node/manager.go

+		timeout = time.Hour * 8765
+	}
+
+	ticker := time.NewTicker(1 * time.Second)


Can you please extract 1 * Second as a constant (tickerResolution) and re-use it in the timeout check in Sync.

To be honest, I don't know why it's 1 * time.Second. time.Second should already suit well. 😄

And consider it done.

adambabik · 2018-02-05T13:05:52Z

cmd/statusd/main.go

@@ -62,6 +62,8 @@ var (
 	// Push Notification
 	enablePN     = flag.Bool("shh.notify", false, "Node is capable of sending Push Notifications")
 	firebaseAuth = flag.String("shh.firebaseauth", "", "FCM Authorization Key used for sending Push Notifications")
+
+	syncAndExit = flag.Int("sync-and-exit", -1, "Timeout in minutes for blockchain sync and exit, zero is infinite")


I guess there was even a better suggestion from @mandrigin to replace zero is infinite with zero means no timeout unless sync is finished.

Sorry, changing.

adambabik · 2018-02-05T13:11:59Z

cmd/statusd/main.go

@@ -106,6 +108,12 @@ func main() {
 		go startCollectingStats(interruptCh, backend.NodeManager())
 	}

+	// Sync blockchain and stop.
+	if *syncAndExit >= 0 {
+		syncAndStopNode(backend.NodeManager())


It's better to reduce the number of functions that do os.Exit(). So maybe we could have syncAndStopNode to simply return error if there was any and do log.Fatalf() here in one place.

adambabik · 2018-02-05T13:21:10Z

geth/node/manager.go

+	// We need to have a larger timeout than ticker delta here
+	// unless we use zero which means infinite timeout.
+	if timeout < tickerResolution*2 && timeout != 0 {
+		return errors.New("Sync timeout can only be zero (infinite) or at least two seconds")


Usually, errors start with a lower case letter for easier joining (log.Fatalf("sync error: %v", err)) and as we talked in a different thread, maybe we can rephrase it to: invalid timeout that can be zero (meaning no timeout) or greater than two seconds.

However, I would also get rid of this at least two seconds. If, for instance, the chain is already synced, I should not wait at all. It can be easily achieved by running a check before for loop. It may require running some checks on downloader in a separate method as well.

adambabik · 2018-02-05T13:30:46Z

geth/node/manager.go

+	defer ticker.Stop()
+	for {
+		select {
+		case <-time.After(timeout):


I am sorry, I remember I suggested changing it to this form but in this case, it may be better to do it differently:

var timeoutCh <-chan time.Time if timeout == 0 { timeoutCh = make(chan time.Time) } else { timeoutCh = time.After(timeout) } // below select { case <-timeoutCh: // ... case <-ticker.C: // ...

And in this way you can avoid a hackish one year timeout :)

Thank you, I was actually looking for something like this before but couldn't imagine 😄

mandrigin

Great, thanks! Just a few tiny changes.

mandrigin · 2018-02-05T13:33:17Z

cmd/statusd/main.go

+	log.Println("Node will synchronize and exit")
+	err := nodeManager.Sync((time.Duration)(*syncAndExit) * time.Minute)
+	if err != nil {
+		log.Fatalf("Failed while waiting for sync: %v", err)


Yeah, as @adambabik suggested, use errors.Wrap here and in line 179 and return error from the function.

mandrigin · 2018-02-05T13:40:09Z

geth/common/types.go

@@ -53,6 +53,9 @@ type NodeManager interface {
 	// Stopped node cannot be resumed, one starts a new node instead.
 	StopNode() (<-chan struct{}, error)

+	// Sync waits until blockchain is synchronized.
+	Sync(timeout time.Duration) error


Yeah, but that means some terra incognita between a contract (an interface) and an implementation of the interface.
Maybe we could make a custom type to make the contract more explicit?
Currently, there are 2 cases that 1 parameter tries to match, using some implicit logic.
Can we "explicify" this logic using a custom type on top of time.Duration, so we can call

m.Sync(common.SyncTimeout(1 * time.Minute)) m.Sync(common.SyncTimeoutInfinite)

Or just extract all the logic about that to the main.go.

One "go" way to do that would be to actually provide a context inside Sync function, so we can cancel it. Then we can just use select in main.go to provide the timeout functionality.

canercidam · 2018-02-05T14:06:41Z

@adambabik @mandrigin Would you agree renaming Sync to EnsureSync as it just tracks the progress?

mandrigin

Only 1 tiny thing left and it should be approved, I think :)

mandrigin · 2018-02-05T15:55:41Z

cmd/statusd/main.go

+	if *syncAndExit >= 0 {
+		if err = syncAndStopNode(backend.NodeManager()); err != nil {
+			log.Fatalf("Node synchronization failed: %v", err)
+		}


there should be a return, right? :)

log.Fatalf calls os.Exit(1) but I would use err := as there is no point in overriding previous err value.

yeah, but if there is no error?

@mandrigin Thanks for pointing out. :)

I think it should return if there is no error.

@adambabik

but I would use err := ...

I can reflect this change in the next push.

adambabik

Almost there!

adambabik · 2018-02-05T16:15:56Z

cmd/statusd/main.go

+	if *syncAndExit >= 0 {
+		if err = syncAndStopNode(backend.NodeManager()); err != nil {
+			log.Fatalf("Node synchronization failed: %v", err)
+		}


log.Fatalf calls os.Exit(1) but I would use err := as there is no point in overriding previous err value.

adambabik · 2018-02-05T16:17:45Z

cmd/statusd/main.go

@@ -158,6 +168,28 @@ func startCollectingStats(interruptCh <-chan struct{}, nodeManager common.NodeMa
 	}
 }

+func syncAndStopNode(nodeManager common.NodeManager) (err error) {
+	log.Println("Node will synchronize and exit")
+	v := *syncAndExit


Why v? :P value or timeout should be easier to reason about.

I'd even move it as a param of the function to make it stateless.

v is just short for value. I can make the change if you like but I don't think it would make a big difference. :)

(Basically, I just thought there's no need for a meaningful name for it in that scope.)

I am just not a fan of one character variable names except for method receiver names. I don't see any value in shortening them but I see disadvantages :D

Alright, will change this as well :)

adambabik · 2018-02-05T16:19:23Z

cmd/statusd/main.go

+		err = nodeManager.EnsureSync(ctx)
+		defer cancel()
+	}
+	if err != nil {


I think you can skip this error check. It's ok to properly stop a node even if it didn't manage to sync the chain.

Alright, then I definitely need to use (github.com/pkg/errors).Wrap here to return combined errors. Shall I dep ensure --add that?

isn't errors.Wrap a standard library function?

Looks like it isn't. I also got confused for a moment: https://golang.org/pkg/errors/

@adambabik @mandrigin Would you mind adding this as a dependency?

Yeah, I think it makes sense to add it. It is useful in many places. @adambabik what do you think?

An alternative is moving stopping node to main.go. Wrapping both errors together is also not really correct because the sync error has nothing to do with stop node error.

Maybe something like this:

func ensureSync(nodeManager common.NodeManager, timeout int) error { // it's not possible to pass negative timeout from the condition in main.go if timeout == 0 { return nodeManager.EnsureSync(context.Background()) } ctx, cancel := context.WithTimeout(context.Background(), (time.Duration)(timeout)*time.Minute) defer cancel() return nodeManager.EnsureSync(ctx) } // in main.go if *syncAndExit >= 0 { var exitCode int if err := ensureSync(backend.NodeManager(), *syncAndExit); err != nil { log.Printf("Failed to sync the chain: %v", err) exitCode = 1 } done, err := backend.NodeManager().StopNode() if err != nil { log.Fatalf("Failed to stop node: %v", err) } <-done os.Exit(exitCode) }

Ugh, I am not happy with this neither :(

adambabik · 2018-02-05T16:20:14Z

cmd/statusd/main.go

+		return
+	}
+	var done <-chan struct{}
+	done, err = nodeManager.StopNode()


done, err := nodeManager.StopNode() <-done

should work just fine.

Technically, it should also have a timeout but I think we can skip it.

It returns (err error) so return is compulsory there. And I wanted it to write the error to existing var (return var).

mandrigin

LGTM!

dshulyak · 2018-02-05T21:50:59Z

@canercidam do you see any reasons why EnsureSync can't be reused for tests helper?

canercidam · 2018-02-05T21:58:16Z

@dshulyak Now that NodeManager has an implementation of it, it doesn't seem to make sense to have that helper I guess.

adambabik

Nice solution with returning exitCode from syncAndStopNode!

Minor comments but please make sure <-done is called after err check as it may be nil and it will blow up.

adambabik · 2018-02-06T11:00:36Z

cmd/statusd/main.go

+	}
+	var done <-chan struct{}
+	done, err = nodeManager.StopNode()
+	<-done


<-done should be after checking error if err != nil {.

adambabik · 2018-02-06T11:01:38Z

cmd/statusd/main.go

+		defer cancel()
+	}
+	if err != nil {
+		log.Printf("Sync and stop error: %v\n", err)


Wording: Sync error: %v (no need for a new line when using log).

adambabik · 2018-02-06T11:01:58Z

cmd/statusd/main.go

+	done, err = nodeManager.StopNode()
+	<-done
+	if err != nil {
+		log.Printf("Sync and stop err: %v\n", err)


Wording: Stop node err: %v.

adambabik · 2018-02-06T11:04:31Z

@canercidam @dshulyak if the test helper is not needed we can create a follow-up issue to clean it up and fix tests after this one is merged.

canercidam · 2018-02-06T12:29:41Z

@dshulyak @adambabik I'm happy to submit a PR for it.

adambabik · 2018-02-06T15:26:13Z

cmd/statusd/main.go

+		log.Printf("Stop node err: %v", err)
+		exitCode = 1
+	}
+	<-done


Not sure I was precise enough. If err occurs during StopNode() call, we should return from the if statement. This still can blow up if err occurred as done may be nil.

var done <-chan struct{} done, err = nodeManager.StopNode() if err != nil { log.Printf("Stop node err: %v", err) return 1 } <-done return

Done, sorry.

mandrigin · 2018-02-07T10:48:27Z

@canercidam thanks for making Status better! 🥇

canercidam · 2018-02-07T10:56:37Z

@mandrigin A pleasure! And thanks for your time and efforts. @adambabik @mandrigin

canercidam added 4 commits February 4, 2018 17:34

option to sync and exit using LES

32c86f8

regenerate mocks

0b70886

fix mistyped comment

0c2f0c8

increase linter deadline

f3f7544

adambabik suggested changes Feb 4, 2018

View reviewed changes

use Sync and StopNode instead of SyncAndStopNode

ec2f582

dshulyak reviewed Feb 5, 2018

View reviewed changes

mandrigin suggested changes Feb 5, 2018

View reviewed changes

canercidam added 5 commits February 5, 2018 14:28

change flag name and refactor

cc1ffb4

use tickerResolution and add comment for timeout

663e065

simplify

06ff7c6

remove return param

4f68b07

add log line

db607f8

adambabik suggested changes Feb 5, 2018

View reviewed changes

mandrigin suggested changes Feb 5, 2018

View reviewed changes

change to EnsureSync with context.Context

e1becdf

mandrigin suggested changes Feb 5, 2018

View reviewed changes

adambabik suggested changes Feb 5, 2018

View reviewed changes

return exit code

3483730

mandrigin approved these changes Feb 5, 2018

View reviewed changes

adambabik approved these changes Feb 6, 2018

View reviewed changes

reword and move <-done

2121458

adambabik reviewed Feb 6, 2018

View reviewed changes

return on node manager stop error

732b743

mandrigin merged commit ca719af into status-im:develop Feb 7, 2018

canercidam mentioned this pull request Feb 12, 2018

LevelDB error when running e2e tests on the public network #652

Closed

Sync blockchain before running e2e tests on public testnet #568 #612

Sync blockchain before running e2e tests on public testnet #568 #612

Conversation

canercidam commented Feb 4, 2018 • edited Loading

adambabik left a 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

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

canercidam commented Feb 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandrigin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canercidam Feb 5, 2018 • edited Loading

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

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

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

adambabik Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandrigin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canercidam commented Feb 5, 2018

mandrigin left a 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

Choose a reason for hiding this comment

adambabik left a 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

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

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

mandrigin left a comment

Choose a reason for hiding this comment

dshulyak commented Feb 5, 2018

canercidam commented Feb 5, 2018

adambabik left a comment

Choose a reason for hiding this comment

canercidam commented Feb 4, 2018 •

edited

Loading

canercidam Feb 5, 2018 •

edited

Loading

adambabik Feb 5, 2018 •

edited

Loading