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.

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

Outbound Governor changes #3580

Merged
merged 10 commits into from Jan 20, 2022
Merged

Outbound Governor changes #3580

merged 10 commits into from Jan 20, 2022

Conversation

coot
Copy link
Contributor

@coot coot commented Jan 19, 2022

  • Stop mux incase termination of miniprotocols timeout
  • Remove superfluous unregisterOutboundConnection call
  • Make keepalive and blockfetch cleanup uninterruptible
  • io-sim: export ppSimEvent
  • io-sim: organise io-sim export list
  • outbound-governor-test: added 'TraceEnvRequestPublicRootPeers'
  • outbound-governor-test: improved counterexample information
  • outbound-governor-test: added test cases from various issues
  • outbound-governor: change the order of events
  • ouroboros-governor: check the state when promoting a warm peer
  • peer-state-actions: remove no-op unregisterOutboundConnection

Splitting the export list into various sections, to make nicer haddocks.
* Add `MonadSay` tracer, this allows to have outbound governor events
  in `IOSim` trace.
* Added `IOSim` trace to `prop_governor_gossip_1hr`: note that the
  simulations written by the governor never terminate,  so
  we must limit the trace output.
Copy link
Contributor

@bolt12 bolt12 left a comment

Choose a reason for hiding this comment

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

Looks good to me! 😄

@coot coot force-pushed the coot/outbound-governor-changes branch from 8446c0c to aa716f5 Compare January 20, 2022 12:04
@coot
Copy link
Contributor Author

coot commented Jan 20, 2022

bors merge

Copy link
Contributor

@karknu karknu left a comment

Choose a reason for hiding this comment

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

LGTM.

@coot
Copy link
Contributor Author

coot commented Jan 20, 2022

bors cancel

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 20, 2022

Canceled.

The outbound governor should first update its state from potentially
blocking decisions (monitoring connections, jobs, target and local roots
peers) and then look into non-blocking internal decisions.

This patch also fixes `prop_governor_gossip_1hr` which did not account
local root peers when checking the target number of root peers.  This
made the test depend on the order of events: with just two root peers
Public and Local, and `targetNumberOfRootPeers` set to 1; if `Public` one
was found first then the governor would still be looking to satisfy its
local root peer target, and it would find the `Local` peer; the test
would pass.  However, if the events where ordered differently, and
`Local` peer is found first, then the governor was not looking for any
more public root peers, and the `Public` peer was never discovered.

Closes #3460, fixes #3550
Check whether a peer is hasn't become cold when we try to update the
state once. In rare cases it could become cold just after we promoted it
to a hot peer, but before the completion continuation was scheduled.
This is because there could be a race between an asynchronous demotion
and the promotion.

Fixes #3550
If a mini-protocol errored, or handshake errored, we don't need to
`unregisterOutboundConnection`.  The connection handler thrown an
exception (either mux does it, or in the other case the handler itself).
When this happens, the finally handler will govern the evolution of
state of the connection: it will set it to `TerminatingSt`, close the
connection and wait for the `TIME_WAIT` timer, when eventually the
connection will be forgotten.
The `prop_governor_nofail` is a pure test which throws
`AssertionFailed`.  Run it in `IO` using `evaluate` to catch the errors
and print the trace log if it fails.  This relies on the property that
the trace will also encounter the same assertion failure.
As in the `outbound-governor: check the state when promoting a warm
peer` commit, but for the error handler.

Fixes #3494, fixes #3515.
@coot coot force-pushed the coot/outbound-governor-changes branch from aa716f5 to 0a25a5f Compare January 20, 2022 14:25
@coot
Copy link
Contributor Author

coot commented Jan 20, 2022

bors merge

iohk-bors bot added a commit that referenced this pull request Jan 20, 2022
3580: Outbound Governor changes r=coot a=coot

- Stop mux incase termination of miniprotocols timeout
- Remove superfluous unregisterOutboundConnection call
- Make keepalive and blockfetch cleanup uninterruptible
- io-sim: export ppSimEvent
- io-sim: organise io-sim export list
- outbound-governor-test: added 'TraceEnvRequestPublicRootPeers'
- outbound-governor-test: improved counterexample information
- outbound-governor-test: added test cases from various issues
- outbound-governor: change the order of events
- ouroboros-governor: check the state when promoting a warm peer
- peer-state-actions: remove no-op unregisterOutboundConnection


Co-authored-by: Marcin Szamotulski <coot@coot.me>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 20, 2022

Build failed:

@coot
Copy link
Contributor Author

coot commented Jan 20, 2022

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 20, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit fe18c8c into master Jan 20, 2022
@iohk-bors iohk-bors bot deleted the coot/outbound-governor-changes branch January 20, 2022 15:56
karknu added a commit that referenced this pull request Jan 21, 2022
3580: Outbound Governor changes r=coot a=coot

- Stop mux incase termination of miniprotocols timeout
- Remove superfluous unregisterOutboundConnection call
- Make keepalive and blockfetch cleanup uninterruptible
- io-sim: export ppSimEvent
- io-sim: organise io-sim export list
- outbound-governor-test: added 'TraceEnvRequestPublicRootPeers'
- outbound-governor-test: improved counterexample information
- outbound-governor-test: added test cases from various issues
- outbound-governor: change the order of events
- ouroboros-governor: check the state when promoting a warm peer
- peer-state-actions: remove no-op unregisterOutboundConnection

Co-authored-by: Marcin Szamotulski <coot@coot.me>
@karknu karknu mentioned this pull request Jan 21, 2022
4 tasks
coot added a commit that referenced this pull request May 16, 2022
3580: Outbound Governor changes r=coot a=coot

- Stop mux incase termination of miniprotocols timeout
- Remove superfluous unregisterOutboundConnection call
- Make keepalive and blockfetch cleanup uninterruptible
- io-sim: export ppSimEvent
- io-sim: organise io-sim export list
- outbound-governor-test: added 'TraceEnvRequestPublicRootPeers'
- outbound-governor-test: improved counterexample information
- outbound-governor-test: added test cases from various issues
- outbound-governor: change the order of events
- ouroboros-governor: check the state when promoting a warm peer
- peer-state-actions: remove no-op unregisterOutboundConnection


Co-authored-by: Marcin Szamotulski <coot@coot.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
outbound-governor Issues / PRs related to outbound-governor peer-state-actions Issues related to PeerStateActions
Projects
None yet
3 participants