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.

Sign up for GitHub

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

Jump to bottom

Add --shutdown-on-slot-synced test and ensure ExitSuccess #3670

Merged
merged 3 commits into from Mar 24, 2022
Merged

Add --shutdown-on-slot-synced test and ensure ExitSuccess #3670

merged 3 commits into from Mar 24, 2022

Conversation

DavidEichmann
Copy link
Contributor

@DavidEichmann DavidEichmann commented Mar 2, 2022 •
edited

Fixes #3646

This does 2 things:

  1. Adds a test that starts a node with --shutdown-on-slot-synced x and checks that (A) the exit code is 0 and that (B) the chain tip slot is close to x.
  2. Rethrows the ExceptionInLinkedThread _ ExitSuccess exception as an an unwrapped ExitSuccess exception. This means the process's exit code will be 0 rather than 1 (this is asserted by the new test).

Without the changes of this PR, property 1.A fails (the exit code is 1) but 1.B succeeds.

newhoggy
newhoggy reviewed Mar 2, 2022
View reviewed changes
cardano-node/src/Cardano/Node/Handlers/TopLevel.hs
Just (ExceptionInLinkedThread _ eInner)
| Just ExitSuccess <- fromException eInner
-> throwIO ExitSuccess
_ -> throwIO full
Copy link
Contributor

Choose a reason for hiding this comment

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

How does propagating ExitSuccess this way change the observed behaviour of the program?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added info in the PR description. See point 2.

newhoggy
newhoggy reviewed Mar 2, 2022
View reviewed changes
cardano-node/src/Cardano/Node/Run.hs Outdated
@@ -105,6 +106,23 @@ runNode
:: PartialNodeConfiguration
-> IO ()
runNode cmdPc = do
-- Workaround to ensure that the main thread throws an async exception on
-- receiving a SIGTERM signal.
#ifdef UNIX
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this only work on POSIX?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so I've limited scope of the PR to posix, aiming to leave the windows behavior unchanged. I'm honestly not sure what the counterpart is on windows or if one even exits.

Copy link
Contributor

@deepfire deepfire Mar 3, 2022 •
edited

Choose a reason for hiding this comment

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

The primary user of this is system-level benchmarking -- and we don't really run that on Windows, since that platform is wallet-only, @newhoggy.

So we're perfectly fine with #ifdef UNIX

@newhoggy
Copy link
Contributor

newhoggy commented Mar 2, 2022 •
edited

Can you document in the PR how the test fails without the changes so it's easier to understand what this PR does to the behaviour of cardano-node?

@DavidEichmann DavidEichmann mentioned this pull request Mar 2, 2022
Merged
@DavidEichmann DavidEichmann force-pushed the davide/ShutdownOnSlotSynced_test branch from 015c47e to 1239b0d Compare March 2, 2022 16:49
@DavidEichmann
Copy link
Contributor Author

I've removed one commit that is already in another PR: #3641

deepfire
deepfire approved these changes Mar 3, 2022
View reviewed changes
Copy link
Contributor

@deepfire deepfire left a comment

Choose a reason for hiding this comment

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

Thanks a lot @DavidEichmann!

@DavidEichmann DavidEichmann force-pushed the davide/ShutdownOnSlotSynced_test branch from 1239b0d to f3e2451 Compare March 3, 2022 14:21
@DavidEichmann
Copy link
Contributor Author

I've increased the timeout of the test while checking for termination every second in hopes that the test will succeed on CI.

@DavidEichmann DavidEichmann force-pushed the davide/ShutdownOnSlotSynced_test branch 3 times, most recently from 757ca4a to 3d46a68 Compare March 7, 2022 17:27
@DavidEichmann DavidEichmann force-pushed the davide/ShutdownOnSlotSynced_test branch 2 times, most recently from 80f30c4 to e22d290 Compare March 17, 2022 16:43
@DavidEichmann
Copy link
Contributor Author

I've added a new commit "Allow tests to specify custom node CLI options for local testnet" and used this to greatly simplify the test added in this PR.

Jimbo4350
Jimbo4350 requested changes Mar 18, 2022
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Nice 👍. A couple of minor comments.

cardano-testnet/test/Spec/ShutdownOnSlotSynced.hs Outdated
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TypeApplications #-}
{-# LANGUAGE RecordWildCards #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use NamedFieldPuns instead? I find RecordWildCards too opaque.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just get rid of it and use projection functions

cardano-testnet/test/Spec/ShutdownOnSlotSynced.hs Outdated
mExitCodeRunning === Right ExitSuccess
log <- H.readFile nodeStdout
slotTip <- H.noteShow
$ read @Integer
Copy link
Contributor

@Jimbo4350 Jimbo4350 Mar 18, 2022 •
edited

Choose a reason for hiding this comment

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

Let's also catch this potential exception here and fail properly. *** Exception: Prelude.read: no parse can be a nightmare to debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what the "proper" way of handling this is, but I'll use fail and explicit error messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies, I wasn't clear. I mean fail using failMessage: https://github.com/input-output-hk/hedgehog-extras/blob/4190fe64c9269ff10cb7cb363b82c521b8e1093b/src/Hedgehog/Extras/Test/Base.hs#L128
So we can see in the output where the failure took place.

@DavidEichmann DavidEichmann force-pushed the davide/ShutdownOnSlotSynced_test branch from e22d290 to 8e13f1c Compare March 22, 2022 13:17
@DavidEichmann
Copy link
Contributor Author

I had to increase the running time of the test as it seems that some times the test would finish without any log messages re. chain tip.

@DavidEichmann
At the top level, rethrow ExceptionInLinkedThread _ ExitSuccess as …
feff556 
…`ExitSuccess`

This is needed to a achieve the correct exit code when using the
--shutdown-on-slot-synced option
@DavidEichmann DavidEichmann force-pushed the davide/ShutdownOnSlotSynced_test branch from 8e13f1c to 980bdc5 Compare March 22, 2022 15:31
@Jimbo4350 Jimbo4350 merged commit 91cc07a into master Mar 24, 2022
@iohk-bors iohk-bors bot deleted the davide/ShutdownOnSlotSynced_test branch March 24, 2022 15:44
@dorin100 dorin100 mentioned this pull request Jun 1, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@newhoggy newhoggy newhoggy approved these changes

@deepfire deepfire deepfire approved these changes

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

@erikd erikd Awaiting requested review from erikd

@nc6 nc6 Awaiting requested review from nc6

@JaredCorduan JaredCorduan Awaiting requested review from JaredCorduan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement shutdown-on-slot-synced test in cardano-node
4 participants
@DavidEichmann @newhoggy @deepfire @Jimbo4350