Error reporting for network & consensus errors #4015

coot · 2022-09-20T08:51:58Z

Description

runRethrowPolicy logs the exception through a separate tracer:
RethrowPolicyTracer. A seprate tracer will allow to not use a configuration
to disable it in cardano-node.

Also added a top level handler for exceptions thrown by the consensus code.
This tracer should also not be configurable. To avoid duplication it does not
log diiffusion startup failure exceptions and exceptions that were rethrown
from connection handler thread (which are wrapped in ExceptionInHandler).

Fixes #4000.

rethrow-policies: fixed typos
connection-handler: rethrow exceptions using ExceptionInHandler
ouroboros-consensus: updated consensusRethrowPolicy
ouroboros-newtork-framework: changed ExceptionInHandler
ouroboros-consensus: added top level error tracer

Checklist

Branch
- Commit sequence broadly makes sense
- Commits have useful messages
- New tests are added if needed and existing tests are updated
- If this branch changes Consensus and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
- If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
- If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
Pull Request
- Self-reviewed the diff
- Useful pull request description at least containing the following information:
  - What does this PR change?
  - Why these changes were needed?
  - How does this affect downstream repositories and/or end-users?
  - Which ticket does this PR close (if any)? If it does, is it linked?
- Reviewer requested

bolt12

LGTM :)

dnadales · 2022-09-20T12:57:43Z

ouroboros-consensus/src/Ouroboros/Consensus/Node.hs

-        llrnRunDataDiffusion registry apps appsExtra
+    withRegistry $ \registry ->
+      handleJust
+             -- ignore exception thrown in connection handlers and diffusion


We should provide a rationale on why we ignore these errors.

They are already logged by the network layer. I'll update the comment.

Included, I also caught a freshman mistake, check the diff 😁

ouroboros-network-framework/src/Ouroboros/Network/RethrowPolicy.hs

dnadales · 2022-09-20T15:06:35Z

ouroboros-network-framework/src/Ouroboros/Network/RethrowPolicy.hs

@@ -86,12 +95,32 @@ data ErrorContext = OutboundError
 type RethrowPolicy_ = ErrorContext -> SomeException -> ErrorCommand

 newtype RethrowPolicy = RethrowPolicy {
-    runRethrowPolicy :: RethrowPolicy_
+    runRethrowPolicyPure :: RethrowPolicy_


Why the Pure suffix?

Because it is pure unlike runRethrowPolicy.

Initially, I thought that runRethrowPolicyPure won't be needed, but it's actually useful in consensusRethrowPolicy.

coot · 2022-09-22T07:18:41Z

@bolt12 I added a commit which ignores ExceptionInHandler in top level Diffusion.InitializationTracer (they are now logged by RethrowPolicyTracer).

coot · 2022-09-22T07:44:05Z

Hmm, I think this PR should be changed. Parts of it are inferior to what we already have, let me try to explain.

The diffusion already has a top level handler which logs all exceptions (the only confusing part about it is that the tracer is called InitializationTracer). If we introduce RethrowPolicyTracer we cannot distinguish errors thrown by connection handler which should kill the node (which ought to use Alert severity) from ones that just should kill the connection (which ought to have Info severity).

For this reason I will remove the part that introduced RethrowPolicyTracer.

This allows the consensus to use it's error policy to wrap around the `Ouroboros.Network.Consensus.Node.runWith` to catch all initialisation errors without interfering with all the exceptions rethrown by the network layer.

Made `ExceptionInHandler` an existential type. This allows to catch all exceptions regardless of peer address type.

Renamed InitialisationTracer as DiffusionTracer: it not only logs initialisation messages but also terminal errors.

coot · 2022-09-27T20:21:29Z

bors merge

iohk-bors · 2022-09-27T22:11:59Z

Build succeeded:

4120: Cherry picked network changes for cardano-node-1.35.5 release r=coot a=coot This cherry-picked patches from the following PRs: * #3794 * #3844 * #3785 * #3904 * #3915 * #3852 * #3970 * #3979 * #4015 * #4067 * #4004 * #4086 * #4113 * #4106 * #4127 * #4103 Also cherry-picked almost all the commits which modify GitHub actions: * 18c5244 Run GitHub Actions on pull requests * 3adf5a9 Use newer version of io-sim * ee9b7a6 Fix GH Actions Windows CI: switch from pkgconf to pkg-config * e6cf074 github-actions: use `ubuntu-latest` * 9a8b959 Updated versions of github actions * fc8f8f0 Fix GH Actions Windows CI caching * 7f07c40 Windows Github Actions now use MSYS2 * b21a7ce Fix chocolatey CI error * #4134 TODO: * [x] bump versions of packages * [x] input-output-hk/cardano-haskell-packages#84 Co-authored-by: Mark Tullsen <tullsen@galois.com> Co-authored-by: Marcin Szamotulski <coot@coot.me>

coot added networking consensus issues related to ouroboros-consensus labels Sep 20, 2022

coot requested review from nfrisby, dnadales, bolt12 and njd42 as code owners September 20, 2022 08:51

bolt12 approved these changes Sep 20, 2022

View reviewed changes

coot force-pushed the coot/rethrow-policy-tracer branch from bfdda22 to ee7f997 Compare September 20, 2022 09:14

dnadales reviewed Sep 20, 2022

View reviewed changes

coot force-pushed the coot/rethrow-policy-tracer branch from ee7f997 to 76a6cf8 Compare September 20, 2022 13:10

dnadales reviewed Sep 20, 2022

View reviewed changes

ouroboros-network-framework/src/Ouroboros/Network/RethrowPolicy.hs Outdated Show resolved Hide resolved

dnadales reviewed Sep 20, 2022

View reviewed changes

coot force-pushed the coot/rethrow-policy-tracer branch from 76a6cf8 to 0ab33f9 Compare September 20, 2022 16:08

dnadales approved these changes Sep 21, 2022

View reviewed changes

rethrow-policies: fixed typos

d8192c4

coot force-pushed the coot/rethrow-policy-tracer branch from 0ab33f9 to d84ff7a Compare September 22, 2022 06:50

coot added 5 commits September 22, 2022 09:45

connection-handler: rethrow exceptions using ExceptionInHandler

a9f06ad

This allows the consensus to use it's error policy to wrap around the `Ouroboros.Network.Consensus.Node.runWith` to catch all initialisation errors without interfering with all the exceptions rethrown by the network layer.

ouroboros-newtork-framework: changed ExceptionInHandler

440afba

Made `ExceptionInHandler` an existential type. This allows to catch all exceptions regardless of peer address type.

ouroboros-consensus: added top level error tracer

38bbd8e

diffusion: DiffusionTracer

e72c10e

Renamed InitialisationTracer as DiffusionTracer: it not only logs initialisation messages but also terminal errors.

Updated CHANGELOG files

cf8a8fb

coot force-pushed the coot/rethrow-policy-tracer branch from 04937ed to cf8a8fb Compare September 22, 2022 08:01

coot requested a review from bolt12 September 22, 2022 08:01

iohk-bors bot merged commit 9249a70 into master Sep 27, 2022

iohk-bors bot deleted the coot/rethrow-policy-tracer branch September 27, 2022 22:12

coot mentioned this pull request Oct 27, 2022

Cherry picked network changes for cardano-node-1.35.5 release #4114

Closed

coot mentioned this pull request Oct 28, 2022

Cherry picked network changes for cardano-node-1.35.5 release #4120

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error reporting for network & consensus errors #4015

Error reporting for network & consensus errors #4015

coot commented Sep 20, 2022 •

edited

Loading

bolt12 left a comment

dnadales Sep 20, 2022

coot Sep 20, 2022

coot Sep 20, 2022

dnadales Sep 20, 2022

coot Sep 20, 2022

coot commented Sep 22, 2022 •

edited

Loading

coot commented Sep 22, 2022

coot commented Sep 27, 2022

iohk-bors bot commented Sep 27, 2022

Error reporting for network & consensus errors #4015

Error reporting for network & consensus errors #4015

Conversation

coot commented Sep 20, 2022 • edited Loading

Description

Checklist

bolt12 left a comment

Choose a reason for hiding this comment

dnadales Sep 20, 2022

Choose a reason for hiding this comment

coot Sep 20, 2022

Choose a reason for hiding this comment

coot Sep 20, 2022

Choose a reason for hiding this comment

dnadales Sep 20, 2022

Choose a reason for hiding this comment

coot Sep 20, 2022

Choose a reason for hiding this comment

coot commented Sep 22, 2022 • edited Loading

coot commented Sep 22, 2022

coot commented Sep 27, 2022

iohk-bors bot commented Sep 27, 2022

coot commented Sep 20, 2022 •

edited

Loading

coot commented Sep 22, 2022 •

edited

Loading