cardano-node: implement --shutdown-on-block-synced #3932

deepfire · 2022-05-27T23:52:34Z

This:

adds --shutdown-on-block-synced BLOCKNO option, in parallel to --shutdown-on-slot-synced SLOTNO
integrates it into the workbench

Jimbo4350 · 2022-05-30T14:53:05Z

cardano-node/src/Cardano/Node/Handlers/Shutdown.hs

@@ -51,14 +50,18 @@ data ShutdownTrace
  -- ^ Received shutdown request but found unexpected input in --shutdown-ipc FD:
  | RequestingShutdown Text
  -- ^ Ringing the node shutdown doorbell for reason
-  | ShutdownArmedAtSlot SlotNo
-  -- ^ Will terminate upon reaching maxSlot
+  | ShutdownArmedAtSlotBlock (Maybe SlotNo) (Maybe BlockNo)


Rather than (Maybe SlotNo) (Maybe BlockNo) this would be better with a sum type e.g data ShutdownMethod = ShutdownAtSlot SlotNo | ShutdownAtBlock BlockNo | NoShutdown and we can rename ShutdownArmedAtSlot to ShutdownNodeAt or something like that.

@Jimbo4350, good point -- I was considering that as well!

@Jimbo4350, done.

Jimbo4350 · 2022-06-02T19:43:20Z

cardano-node/src/Cardano/Node/Handlers/Shutdown.hs

    void $ forkLinkedWatcher registry "slotLimitTerminator" Watcher {
-        wFingerprint = id
+        wFingerprint = identity


Why the change to identity? This doesn't look necessary.

~~@Jimbo4350, sorry, Cardano.Prelude import confused me, undone.~~

Because I had to change the prelude to Cardano.Prelude, which has it as identity

Why did you have to make this change (importing Cardano.Prelude)? You shouldn't have to.

I needed Generic, and so instead of adding GHC.Generics, I switched the prelude, which allowed dumping a bunch of imports: 2c020c8#diff-9a71b006f6ddf2ab12e67e4d7e2951abf353bc8841f8a2638a1336a76b2ae43bL23-L32

Jimbo4350

A couple of comments

Jimbo4350 · 2022-06-02T19:45:32Z

cardano-node/src/Cardano/Node/Handlers/Shutdown.hs

+import           Ouroboros.Network.Block (HasHeader, BlockNo (..), SlotNo (..), pointSlot)
+
+
+data SlotOrBlock f


I understand you are trying to reuse SlotOrBlock but I would argue you still have to create the AndWithOrigin type. I think this makes the code a little more difficult to understand vs the following:

diff --git a/cardano-node/src/Cardano/Node/Handlers/Shutdown.hs b/cardano-node/src/Cardano/Node/Handlers/Shutdown.hs index 39f028deb..83457bbcc 100644 --- a/cardano-node/src/Cardano/Node/Handlers/Shutdown.hs +++ b/cardano-node/src/Cardano/Node/Handlers/Shutdown.hs @@ -26,9 +26,9 @@ module Cardano.Node.Handlers.Shutdown ) where +import Cardano.Prelude import Data.Aeson (FromJSON, ToJSON) import Generic.Data.Orphans () -import Cardano.Prelude import Data.Text (pack) import qualified GHC.IO.Handle.FD as IO (fdToHandle) @@ -43,29 +43,27 @@ import Ouroboros.Consensus.Block (Header) import qualified Ouroboros.Consensus.Storage.ChainDB as ChainDB import Ouroboros.Consensus.Util.ResourceRegistry (ResourceRegistry) import Ouroboros.Consensus.Util.STM (Watcher (..), forkLinkedWatcher) -import Ouroboros.Network.Block (HasHeader, BlockNo (..), SlotNo (..), pointSlot) +import Ouroboros.Network.Block (BlockNo (..), HasHeader, SlotNo (..), pointSlot) -data SlotOrBlock f - = ASlot !(f SlotNo) - | ABlock !(f BlockNo) - deriving (Generic) +data SlotOrBlock + = ASlot !SlotNo + | ABlock !BlockNo + deriving (Generic, Eq, Show) -deriving instance Eq (SlotOrBlock Identity) -deriving instance Show (SlotOrBlock Identity) -deriving instance FromJSON (SlotOrBlock Identity) -deriving instance ToJSON (SlotOrBlock Identity) +deriving instance FromJSON SlotOrBlock +deriving instance ToJSON SlotOrBlock -parseShutdownOnLimit :: Opt.Parser (Maybe (SlotOrBlock Identity)) +parseShutdownOnLimit :: Opt.Parser (Maybe SlotOrBlock) parseShutdownOnLimit = - optional (Opt.option (ASlot . Identity . SlotNo <$> Opt.auto) ( + optional (Opt.option (ASlot . SlotNo <$> Opt.auto) ( Opt.long "shutdown-on-slot-synced" <> Opt.metavar "SLOT" <> Opt.help "Shut down the process after ChainDB is synced up to the specified slot" <> Opt.hidden )) <|> - optional (Opt.option (ABlock . Identity . BlockNo <$> Opt.auto) ( + optional (Opt.option (ABlock . BlockNo <$> Opt.auto) ( Opt.long "shutdown-on-block-synced" <> Opt.metavar "BLOCK" <> Opt.help "Shut down the process after ChainDB is synced up to the specified block" @@ -81,21 +79,23 @@ data ShutdownTrace -- ^ Received shutdown request but found unexpected input in --shutdown-ipc FD: | RequestingShutdown Text -- ^ Ringing the node shutdown doorbell for reason - | ShutdownArmedAt (SlotOrBlock Identity) + | ShutdownArmedAt SlotOrBlock -- ^ Will terminate upon reaching a ChainDB sync limit deriving (Generic, FromJSON, ToJSON) deriving instance FromJSON BlockNo deriving instance ToJSON BlockNo -newtype AndWithOrigin a = AndWithOrigin (a, WithOrigin a) deriving (Eq) +data AndWithOrigin + = AndWithOriginBlock (BlockNo, WithOrigin BlockNo) + | AndWithOriginSlot (SlotNo, WithOrigin SlotNo) -deriving instance Eq (SlotOrBlock AndWithOrigin) +deriving instance Eq AndWithOrigin data ShutdownConfig = ShutdownConfig { scIPC :: !(Maybe Fd) - , scOnSyncLimit :: !(Maybe (SlotOrBlock Identity)) + , scOnSyncLimit :: !(Maybe SlotOrBlock) } deriving (Eq, Show) @@ -142,24 +142,22 @@ maybeSpawnOnSlotSyncedShutdownHandler sc tr registry chaindb = traceWith tr (ShutdownArmedAt lim) spawnLimitTerminator lim where - spawnLimitTerminator :: SlotOrBlock Identity -> IO () + spawnLimitTerminator :: SlotOrBlock -> IO () spawnLimitTerminator limit = void $ forkLinkedWatcher registry "slotLimitTerminator" Watcher { wFingerprint = identity , wInitial = Nothing , wReader = case limit of - ASlot (Identity x) -> ASlot . AndWithOrigin . (x,) <$> - (pointSlot <$> ChainDB.getTipPoint chaindb) - ABlock (Identity x) -> ABlock . AndWithOrigin . (x,) <$> - ChainDB.getTipBlockNo chaindb + ASlot x -> AndWithOriginSlot . (x,) . pointSlot <$> ChainDB.getTipPoint chaindb + ABlock x -> AndWithOriginBlock . (x,) <$> ChainDB.getTipBlockNo chaindb , wNotify = \case - ASlot (AndWithOrigin (lim, At cur)) -> + (AndWithOriginSlot (lim, At cur)) -> when (cur >= lim) $ do traceWith tr (RequestingShutdown $ "spawnLimitTerminator: reached target slot " <> (pack . show) cur) throwIO ExitSuccess - ABlock (AndWithOrigin (lim, At cur)) -> + (AndWithOriginBlock (lim, At cur)) -> when (cur >= lim) $ do traceWith tr (RequestingShutdown $ "spawnLimitTerminator: reached target block " <> (pack . show) cur)

Also the name SlotOrBlock doesn't really indicate immediately what it's going to be used for. Why not ShutdownOnSlotOrBlock? Or something similar.

@Jimbo4350, done.

Jimbo4350 · 2022-06-02T20:47:30Z

cardano-node/src/Cardano/Node/Parsers.hs

@@ -93,7 +91,7 @@ nodeRunParser = do
             }
           , pncValidateDB = validate
           , pncShutdownConfig =
-               Last . Just $ ShutdownConfig (getLast shutdownIPC) (getLast shutdownOnSlot)
+               Last . Just $ ShutdownConfig (getLast shutdownIPC) (join $ getLast shutdownOnLimit)


I would also recommend this to avoid join and follow the pattern of the other options being parsed:

diff --git a/cardano-node/src/Cardano/Node/Handlers/Shutdown.hs b/cardano-node/src/Cardano/Node/Handlers/Shutdown.hs index 5e5324dae..3fba11ae6 100644 --- a/cardano-node/src/Cardano/Node/Handlers/Shutdown.hs +++ b/cardano-node/src/Cardano/Node/Handlers/Shutdown.hs @@ -49,27 +49,28 @@ import Ouroboros.Network.Block (BlockNo (..), HasHeader, SlotNo (..), data SlotOrBlock = ASlot !SlotNo | ABlock !BlockNo - | NoShutdown + | NoShutdownOnSlotOrBlock deriving (Generic, Eq, Show) deriving instance FromJSON SlotOrBlock deriving instance ToJSON SlotOrBlock -parseShutdownOnLimit :: Opt.Parser (Maybe SlotOrBlock) +parseShutdownOnLimit :: Opt.Parser SlotOrBlock parseShutdownOnLimit = - optional (Opt.option (ASlot . SlotNo <$> Opt.auto) ( + Opt.option (ASlot . SlotNo <$> Opt.auto) ( Opt.long "shutdown-on-slot-synced" <> Opt.metavar "SLOT" <> Opt.help "Shut down the process after ChainDB is synced up to the specified slot" <> Opt.hidden - )) + ) <|> - optional (Opt.option (ABlock . BlockNo <$> Opt.auto) ( + Opt.option (ABlock . BlockNo <$> Opt.auto) ( Opt.long "shutdown-on-block-synced" <> Opt.metavar "BLOCK" <> Opt.help "Shut down the process after ChainDB is synced up to the specified block" <> Opt.hidden - )) + ) + <|> pure NoShutdownOnSlotOrBlock data ShutdownTrace = ShutdownRequested @@ -90,6 +91,7 @@ deriving instance ToJSON BlockNo data AndWithOrigin = AndWithOriginBlock (BlockNo, WithOrigin BlockNo) | AndWithOriginSlot (SlotNo, WithOrigin SlotNo) + | WithoutOrigin deriving instance Eq AndWithOrigin @@ -152,6 +154,7 @@ maybeSpawnOnSlotSyncedShutdownHandler sc tr registry chaindb = case limit of ASlot x -> AndWithOriginSlot . (x,) . pointSlot <$> ChainDB.getTipPoint chaindb ABlock x -> AndWithOriginBlock . (x,) <$> ChainDB.getTipBlockNo chaindb + NoShutdownOnSlotOrBlock -> return WithoutOrigin , wNotify = \case (AndWithOriginSlot (lim, At cur)) -> when (cur >= lim) $ do @@ -163,5 +166,6 @@ maybeSpawnOnSlotSyncedShutdownHandler sc tr registry chaindb = traceWith tr (RequestingShutdown $ "spawnLimitTerminator: reached target block " <> (pack . show) cur) throwIO ExitSuccess + WithoutOrigin -> pure () _ -> pure () } diff --git a/cardano-node/src/Cardano/Node/Parsers.hs b/cardano-node/src/Cardano/Node/Parsers.hs index a7cdbfb65..dd3389b43 100644 --- a/cardano-node/src/Cardano/Node/Parsers.hs +++ b/cardano-node/src/Cardano/Node/Parsers.hs @@ -23,7 +23,8 @@ import Ouroboros.Consensus.Mempool.API (MempoolCapacityBytes (..), MempoolCapacityBytesOverride (..)) import Ouroboros.Consensus.Storage.LedgerDB.DiskPolicy (SnapshotInterval (..)) -import Cardano.Node.Configuration.NodeAddress +import Cardano.Node.Configuration.NodeAddress (NodeHostIPv4Address (NodeHostIPv4Address), + NodeHostIPv6Address (NodeHostIPv6Address), PortNumber, SocketPath (SocketPath)) import Cardano.Node.Configuration.POM (PartialNodeConfiguration (..), lastOption) import Cardano.Node.Configuration.Socket import Cardano.Node.Handlers.Shutdown @@ -91,7 +92,7 @@ nodeRunParser = do } , pncValidateDB = validate , pncShutdownConfig = - Last . Just $ ShutdownConfig (getLast shutdownIPC) (join $ getLast shutdownOnLimit) + Last . Just $ ShutdownConfig (getLast shutdownIPC) (getLast shutdownOnLimit) , pncProtocolConfig = mempty , pncMaxConcurrencyBulkSync = mempty , pncMaxConcurrencyDeadline = mempty

@Jimbo4350, done!

..and thank you for an actionable suggestion, too!

cardano-node/src/Cardano/Tracing/OrphanInstances/Common.hs

Jimbo4350 · 2022-06-02T20:50:59Z

I think you are using a different formatter to cardano-node. We use stylish-haskell.

deepfire · 2022-06-03T00:31:52Z

Ok, I need to integrate stylish-haskell into emacs.

Jimbo4350

LGTM!

deepfire · 2022-06-03T20:12:57Z

bors r+

iohk-bors · 2022-06-03T21:47:30Z

Canceled.

deepfire · 2022-06-04T02:01:13Z

bors r+

3932: cardano-node: implement --shutdown-on-block-synced r=deepfire a=deepfire This: 1. adds `--shutdown-on-block-synced BLOCKNO` option, in parallel to `--shutdown-on-slot-synced SLOTNO` 2. integrates it into the workbench Co-authored-by: Marcin Szamotulski <coot@coot.me> Co-authored-by: iohk-bors[bot] <43231472+iohk-bors[bot]@users.noreply.github.com> Co-authored-by: Kosyrev Serge <serge.kosyrev@iohk.io> Co-authored-by: Jordan Millar <jordan.millar@iohk.io>

iohk-bors · 2022-06-04T04:28:20Z

Timed out.

…tream changes

deepfire · 2022-06-05T21:58:53Z

All checks passed, node-side review green -- merging.

deepfire requested review from denisshevchenko, jutaro, MarcFontaine, mgmeier, cleverca22, DavidEichmann, dcoutts, Jimbo4350, erikd, newhoggy, nc6 and JaredCorduan as code owners May 27, 2022 23:52

deepfire force-pushed the node-shutdown-on-block-synced branch 2 times, most recently from 373511c to 0982bfc Compare May 28, 2022 13:42

Jimbo4350 reviewed May 30, 2022

View reviewed changes

deepfire force-pushed the node-shutdown-on-block-synced branch 13 times, most recently from 86f0236 to 5660b57 Compare June 1, 2022 18:01

Jimbo4350 reviewed Jun 2, 2022

View reviewed changes

Jimbo4350 requested changes Jun 2, 2022

View reviewed changes

deepfire force-pushed the node-shutdown-on-block-synced branch 2 times, most recently from b05d8f6 to fb042c6 Compare June 3, 2022 00:30

deepfire requested review from Jimbo4350 and kosyrevSerge June 3, 2022 01:10

deepfire assigned deepfire and Jimbo4350 Jun 3, 2022

Jimbo4350 approved these changes Jun 3, 2022

View reviewed changes

deepfire force-pushed the node-shutdown-on-block-synced branch from fb042c6 to f3e379b Compare June 3, 2022 21:47

deepfire and others added 3 commits June 6, 2022 00:08

cardano-node: implement --shutdown-on-block-synced

2c020c8

cardano-node: review suggestions by Jordan Millar

d9bbc81

cardano-node | shutdown: suggested nomenclature changes & other downs…

a3a58c7

…tream changes

deepfire force-pushed the node-shutdown-on-block-synced branch from f3e379b to a3a58c7 Compare June 5, 2022 21:08

deepfire merged commit 2b2d5dd into master Jun 5, 2022

iohk-bors bot deleted the node-shutdown-on-block-synced branch June 5, 2022 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cardano-node: implement --shutdown-on-block-synced #3932

cardano-node: implement --shutdown-on-block-synced #3932

deepfire commented May 27, 2022

Jimbo4350 May 30, 2022

deepfire May 30, 2022

deepfire May 31, 2022

Jimbo4350 Jun 2, 2022 •

edited

deepfire Jun 3, 2022 •

edited

Jimbo4350 Jun 3, 2022 •

edited

deepfire Jun 5, 2022

Jimbo4350 left a comment

Jimbo4350 Jun 2, 2022

Jimbo4350 Jun 2, 2022

deepfire Jun 3, 2022

Jimbo4350 Jun 2, 2022

deepfire Jun 3, 2022

deepfire Jun 3, 2022

Jimbo4350 commented Jun 2, 2022

deepfire commented Jun 3, 2022

Jimbo4350 left a comment

deepfire commented Jun 3, 2022

iohk-bors bot commented Jun 3, 2022

deepfire commented Jun 4, 2022

iohk-bors bot commented Jun 4, 2022

deepfire commented Jun 5, 2022

		import Ouroboros.Network.Block (HasHeader, BlockNo (..), SlotNo (..), pointSlot)


		data SlotOrBlock f

cardano-node: implement --shutdown-on-block-synced #3932

cardano-node: implement --shutdown-on-block-synced #3932

Conversation

deepfire commented May 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 Jun 2, 2022 • edited

Choose a reason for hiding this comment

deepfire Jun 3, 2022 • edited

Choose a reason for hiding this comment

Jimbo4350 Jun 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 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

Jimbo4350 commented Jun 2, 2022

deepfire commented Jun 3, 2022

Jimbo4350 left a comment

Choose a reason for hiding this comment

deepfire commented Jun 3, 2022

iohk-bors bot commented Jun 3, 2022

deepfire commented Jun 4, 2022

iohk-bors bot commented Jun 4, 2022

deepfire commented Jun 5, 2022

Jimbo4350 Jun 2, 2022 •

edited

deepfire Jun 3, 2022 •

edited

Jimbo4350 Jun 3, 2022 •

edited