-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
lntest: shutdown all nodes at end of test #9209
Conversation
Fixes an issue where the standby nodes would keep running even after the test finished (successfully or with a failure).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to resuse the existing code
@@ -361,6 +361,8 @@ func (h *HarnessTest) Stop() { | |||
return | |||
} | |||
|
|||
h.shutdownAllNodes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, but I'm wondering why the subtest cleanup did not take care of the standby nodes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During a subtest the standby nodes are just reset, meaning their cached numbers (channels, UTXOs and so on) are reset, but the nodes aren't actually terminated. I think this has been this way for a while now, we just never noticed (or we did and didn't investigate).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I just realized we should only shutdown standby nodes here - the non-standbys have already been shut down during the cleanup in subtest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but there is no shutdownStandbyNodes()
, since shutdownNodes
just takes a boolean skipStandby
.
And if there are no non-standby nodes, shutting down all of them results in the same thing: just shutting down the standby ones.
Or would you like me to add a shutdownStandbyNodes()
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay double checked, indeed the subtest cleanup missed shutdown of standby nodes if the test succeeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think I recalled why I wrongfully removed it in the first place - when the test fails, we'll shut down the node twice, resulting in some error msgs being printed.
Or would you like me to add a shutdownStandbyNodes() function?
Nah I think you are right - there's map that tracks the active node, and we remove it from the map whenever we shut it down, so no need for this func.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I just tried this out by causing a test to fail and I didn't see duplicate or weird error messages... Maybe that was fixed in another way in the meantime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah likely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! LGTM🌸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Fixes an issue where the standby nodes would keep running even after the test finished (successfully or with a failure).