Simplify and clarify code for local-cluster #25

ghost · 2021-06-21T08:29:16Z

The local-cluster project was the first code we wrote and some stuff in it are redundant with stuff we wrote later, or ill-named.

Replace local-cluster Logging with Hydra.Logging
Move Ports utility module to Hydra.Network and use its feature there, namely in the NetworkSpec tests
Rename Lib -> CardanoCluster and Node -> CardanoNode

The changes introduced made it even more apparent how flaky our NetworkSpec tests were so this PR also fixes that by:

Having a simpler way to check we broadcast to all nodes in the tests
Increasing timeout of Ouroboros network tests to 30s to give time to the framework to retry failed connection attempts

ghost · 2021-06-21T08:30:09Z

This PR was triggered by this ticket in our board's Red Bin: https://miro.com/app/board/o9J_lRyWcSY=/?moveToWidget=3074457360130079706&cot=14

hydra-node/test/Hydra/NetworkSpec.hs

hydra-node/src/Hydra/Logging.hs

hydra-node/hydra-node.cabal

hydra-node/src/Hydra/Network/Ouroboros.hs

hydra-node/test/Hydra/NetworkSpec.hs

The local-cluster project was the first code we wrote and some stuff in it are redundant with stuff we wrote later, or ill-named. * Replace local-cluster Logging with Hydra.Logging * Move Ports utility module to Hydra.Network and use its feature there * Rename Lib -> CardanoCluster and Node -> CardanoNode

This does not seem to improve the stability of the tests but it's still better to not have hard coded ports like that lying around.

There is a fixed retry window of 10s in Ouroboros networking code that cannot be configured hence having a 10s timeout runs a high risk of failing because a node which fails to connect won't have the time to reconnect.

ch1bo

Only minor things remain, but can also be merged like this

ch1bo · 2021-06-25T05:30:02Z

hydra-node/src/Hydra/Network/Ouroboros.hs

+data WithHost trace = WithHost Host trace
+  deriving (Show)
+
+data TraceOuroborosNetwork msg
  = TraceSubscriptions (WithIPList (SubscriptionTrace SockAddr))
  | TraceErrorPolicy (WithAddr SockAddr ErrorPolicyTrace)
  | TraceAcceptPolicy AcceptConnectionsPolicyTrace


Seeing the naming convention of ouroboros-network of likely being XXXTrace for trace types. Shall we adopt that as well? We have at least TraceXXX and XXXLog in use.

No change required here, just starting the conversation :)

ch1bo · 2021-06-25T05:32:07Z

hydra-node/src/Hydra/Network/Ouroboros.hs

@@ -254,9 +256,14 @@ withOuroborosNetwork tracer localHost remoteHosts networkCallback between = do
      , recvMsgDone = pure ()
      }

-data TraceOuroborosNetwork
+data WithHost trace = WithHost Host trace


Isn't hlint warning that this could be a newtype? if yes, I'd follow that suggestion

ch1bo · 2021-06-25T05:32:18Z

hydra-node/src/Hydra/Network/ZeroMQ.hs

-  | SubscribedTo [String]
+  | MessageSent Host LByteString
+  | LogMessageReceived Host Text
+  | SubscribedTo Host [String]
  deriving (Show)


Could: use the WithHost type you introduced

ch1bo · 2021-06-25T05:40:08Z

hydra-node/test/Hydra/NetworkSpec.hs

+  parallelBroadcast :: IO () -> [(PortNumber, Network IO Integer, TQueue IO Integer)] -> IO ()
+  parallelBroadcast check [] = check
+  parallelBroadcast check ((port, network, _) : rest) =
+    withAsync (fromIntegral port `broadcastFrom` network) $ \_ -> parallelBroadcast check rest


Could: This function is passing check around to just have it as "core" to the withAsync chain.. an alternative (which reads a bit clearer to me, would be to race_ (parallelBroadcast networks) checkQueues above and consequently:

Suggested change

parallelBroadcast :: IO () -> [(PortNumber, Network IO Integer, TQueue IO Integer)] -> IO ()

parallelBroadcast check [] = check

parallelBroadcast check ((port, network, _) : rest) =

withAsync (fromIntegral port `broadcastFrom` network) $ \_ -> parallelBroadcast check rest

parallelBroadcast :: [(PortNumber, Network IO Integer, TQueue IO Integer)] -> IO ()

parallelBroadcast [] = pure ()

parallelBroadcast [(port, network, _)] = fromIntegral port `broadcastFrom` network

parallelBroadcast ((port, network, _) : rest) =

withAsync (fromIntegral port `broadcastFrom` network) $ \_ -> parallelBroadcast rest

ch1bo · 2021-06-25T05:46:15Z

local-cluster/local-cluster.cabal

@@ -84,6 +82,7 @@ library
    , hspec-expectations
    , http-conduit
    , hydra-prelude
+    , hydra-node


As we have this here anyway, I guess I'd prefer the Hydra.Network.Ports to be in hydra-node instead of the hydra-prelude. May I move it or WDYT?

ghost · 2021-06-25T06:50:44Z

@ch1bo All valid points, I will address them after the merge.

ghost requested review from KtorZ and ch1bo June 21, 2021 08:29

KtorZ approved these changes Jun 21, 2021

View reviewed changes

hydra-node/test/Hydra/NetworkSpec.hs Show resolved Hide resolved

ghost force-pushed the abailly-iohk/unify-local-cluster-code branch from c81b5a6 to 3b6f315 Compare June 21, 2021 11:35

KtorZ reviewed Jun 21, 2021

View reviewed changes

hydra-node/src/Hydra/Logging.hs Show resolved Hide resolved

ghost force-pushed the abailly-iohk/unify-local-cluster-code branch 3 times, most recently from d9afdd4 to 9a19190 Compare June 24, 2021 12:31

ghost requested a review from KtorZ June 24, 2021 12:46

KtorZ reviewed Jun 24, 2021

View reviewed changes

hydra-node/hydra-node.cabal Outdated Show resolved Hide resolved

hydra-node/src/Hydra/Network/Ouroboros.hs Outdated Show resolved Hide resolved

hydra-node/test/Hydra/NetworkSpec.hs Show resolved Hide resolved

hydra-node/test/Hydra/NetworkSpec.hs Outdated Show resolved Hide resolved

abailly added 11 commits June 24, 2021 15:40

Allocated random ports for Network tests

2525080

This does not seem to improve the stability of the tests but it's still better to not have hard coded ports like that lying around.

Reverse accumulated logs on error

e83f135

Use same retry logic for test with only 2 nodes

67a7cc3

Remove dependency on Hydra.Logging in local-cluster

3b0abba

Move Ports module to hydra-prelude

4cda36e

Increase timeout of Network tests to 30s

6f2b322

There is a fixed retry window of 10s in Ouroboros networking code that cannot be configured hence having a 10s timeout runs a high risk of failing because a node which fails to connect won't have the time to reconnect.

Fix compilation error

b151b43

Address review Comments

4206112

Silly typo repeating port number

96ad152

Fix compilation errors following rebase

af56a16

ghost force-pushed the abailly-iohk/unify-local-cluster-code branch from c26d3a2 to af56a16 Compare June 24, 2021 15:50

Merge branch 'master' into abailly-iohk/unify-local-cluster-code

4ace180

ch1bo approved these changes Jun 25, 2021

View reviewed changes

ghost merged commit a2ebd35 into master Jun 25, 2021

ghost deleted the abailly-iohk/unify-local-cluster-code branch June 25, 2021 06:51

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify and clarify code for local-cluster #25

Simplify and clarify code for local-cluster #25

ghost commented Jun 21, 2021 •

edited by ghost

Loading

ghost commented Jun 21, 2021

ch1bo left a comment

ch1bo Jun 25, 2021

ch1bo Jun 25, 2021

ch1bo Jun 25, 2021

ch1bo Jun 25, 2021

ch1bo Jun 25, 2021

ch1bo Jun 25, 2021

ghost commented Jun 25, 2021

-  parallelBroadcast :: IO () -> [(PortNumber, Network IO Integer, TQueue IO Integer)] -> IO ()
-  parallelBroadcast check [] = check
-  parallelBroadcast check ((port, network, _) : rest) =
-    withAsync (fromIntegral port `broadcastFrom` network) $ \_ -> parallelBroadcast check rest
+  parallelBroadcast :: [(PortNumber, Network IO Integer, TQueue IO Integer)] -> IO ()
+  parallelBroadcast [] = pure ()
+  parallelBroadcast [(port, network, _)] = fromIntegral port `broadcastFrom` network
+  parallelBroadcast ((port, network, _) : rest) =
+    withAsync (fromIntegral port `broadcastFrom` network) $ \_ -> parallelBroadcast rest

Simplify and clarify code for local-cluster #25

Simplify and clarify code for local-cluster #25

Conversation

ghost commented Jun 21, 2021 • edited by ghost Loading

ghost commented Jun 21, 2021

ch1bo left a comment

Choose a reason for hiding this comment

ch1bo Jun 25, 2021

Choose a reason for hiding this comment

ch1bo Jun 25, 2021

Choose a reason for hiding this comment

ch1bo Jun 25, 2021

Choose a reason for hiding this comment

ch1bo Jun 25, 2021

Choose a reason for hiding this comment

ch1bo Jun 25, 2021

Choose a reason for hiding this comment

ch1bo Jun 25, 2021

Choose a reason for hiding this comment

ghost commented Jun 25, 2021

ghost commented Jun 21, 2021 •

edited by ghost

Loading