[cardano-testnet] Fix flaky test workspace cleanup and node port allocation

[carbolymer]

Description

This PR fixes testnet startup nondeterministic issues:

1. workspace cleanup for successful tests: sometimes the directory could not get removed. Fixed by waiting and retrying.
2. on MacOS, when starting node with a random port number we were receiving "port in use" error, due to holding to the port by darwin kernel, resulting in port being stuck in TIME_WAIT state. Fixed by waiting and retrying.
3. stemming from nr 2: the operating system was also holding the lock to the stderr log file, resulting in node start retry failure. Fixed by additional manual closing of the file handle and using a different name of the log file.

Checklist

• Commit sequence broadly makes sense and commits have useful messages
• New tests are added if needed and existing tests are updated. These may include:
  • golden tests
  • property tests
  • roundtrip tests
  • integration tests
    See Runnings tests for more details
• Any changes are noted in the CHANGELOG.md for affected package
• The version bounds in .cabal files are updated
• CI passes. See note on CI. The following CI checks are required:
  • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
  • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
  • Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
• Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

[carbolymer]

Uhm, seems fix does not work. Still failing with:

               ┏━━ test/cardano-testnet-test/Cardano/Testnet/Test/SanityCheck.hs ━━━
            41 ┃ hprop_ledger_events_sanity_check :: Property
            42 ┃ hprop_ledger_events_sanity_check = integrationWorkspace "ledger-events-sanity-check" $ \tempAbsBasePath' -> H.runWithDefaultWatchdog_ $ do
               ┃ │ Workspace: /private/tmp/tmp.01xnrEP0ti/ledger-events-sanity-check-test-4333b7de3f5722d3
               ┃ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
               ┃ │ ━━━ Exception (IOException) ━━━
               ┃ │ /private/tmp/tmp.01xnrEP0ti/ledger-events-sanity-check-test-4333b7de3f5722d3/logs: removePathForcibly:removePathForcibly:removeDirectory: unsatisfied constraints (Directory not empty)
            43 ┃   -- Start a local test net
            44 ┃   conf <- mkConf tempAbsBasePath'

Most likely because epoch state logging gets in the way and gets cleaned up in MonadResouce after the workspace cleanup code.

[carbolymer]

Failures because of slow port closing after bind when reserving port numbers:

             324 ┃     let keysWithPorts = L.zip3 [1..] poolKeys portNumbers
            325 ┃     ePoolNodes <- H.forConcurrently keysWithPorts $ \(i, key, (portReleaseKey, port)) -> do
            326 ┃       let nodeName = mkNodeName i
            327 ┃           keyDir = tmpAbsPath </> poolKeyDir i
            328 ┃       H.note_ $ "Node name: " <> nodeName
                ┃       │ Node name: pool1
                ┃       │ Node name: pool2
                ┃       │ Node name: pool3
            329 ┃       eRuntime <- runExceptT $
            330 ┃         startNode (TmpAbsolutePath tmpAbsPath) nodeName testnetDefaultIpv4Address port (Just portReleaseKey) testnetMagic
            331 ┃           [ "run"
            332 ┃           , "--config", unFile configurationFile
            333 ┃           , "--topology", keyDir </> "topology.json"
            334 ┃           , "--database-path", keyDir </> "db"
            335 ┃           , "--shelley-kes-key", keyDir </> "kes.skey"
            336 ┃           , "--shelley-vrf-key", keyDir </> "vrf.skey"
            337 ┃           , "--byron-delegation-certificate", keyDir </> "byron-delegation.cert"
            338 ┃           , "--byron-signing-key", keyDir </> "byron-delegate.key"
            339 ┃           , "--shelley-operational-certificate", keyDir </> "opcert.cert"
            340 ┃           ]
            341 ┃       pure $ flip PoolNode key <$> eRuntime
            342 ┃ 
            343 ┃     let (failedNodes, poolNodes) = partitionEithers ePoolNodes
            344 ┃     unless (null failedNodes) $ do
            345 ┃       H.noteShow_ . vsep $ prettyError <$> failedNodes
                ┃       │ Cardano node process did not start: 
                ┃       │ cardano-node: DiffusionError Network.Socket.bind: resource busy (Address already in use)
                ┃       │ Cardano node process did not start: 
                ┃       │ cardano-node: DiffusionError Network.Socket.bind: resource busy (Address already in use)
            346 ┃       H.failure
                ┃       ^^^^^^^^^

This happens on slow machines, like macos runners.

[carbolymer]

New failure after fixing the previous two:

File error: /private/tmp/tmp.WyCYpKhEZj/propose-new-constitution-spo-test-52485b463ccb6b99/logs/pool2/stderr.log: openFile: resource busy (file is locked)
              

[Jimbo4350]

Nice work 👍 . One suggestion surrounding exponential backoff. waitForPortClosed will loop forever; we should probably fail at some point.

[Jimbo4350]

Thoughts on using an exponential back off so this doesn't loop forever?

[carbolymer]

It's wrapped in timeout, so it will be interrupted eventually. I'll use here retrying instead. It will simplify the code.

[Jimbo4350]

what do you mean by "supplied block"?

[carbolymer]

Oh, the comment doesn't make much sense. I've copied haddock as-is from https://hackage.haskell.org/package/hedgehog-extras-0.6.5.0/docs/Hedgehog-Extras-Test-Base.html#v:workspace
I'll upstream it after merging this PR. I'll fix it in hedgehog-extras

[Jimbo4350]

Another spot for exponential backoff? I see retry has the exponentialBackoff combinator :)

[carbolymer]

I'm not sure we will gain much with exponential backoff. The operation is not expensive so we're not using CPU usage or anything.

I'd say exponential backoff would make sense in resource intensive operations, like network communication.

[Jimbo4350]

Potential spot for exponential backoff

[carbolymer]

Actually, we're not retrying anything here. I'm assuming if the file exists, it'll be locked for a long time, and just try to create a file with the next name.