CCIP-1730 Implementing Healthy function in LogPoller #584

mateusz-sekara · 2024-03-06T10:48:22Z

Required for finality violation checks #583

core/chains/evm/logpoller/log_poller.go

makramkd

Is it possible to go from finalityViolated == true to finalityViolated == false during normal operation without restarting and changing finality depth manually?

mateusz-sekara · 2024-03-07T09:00:58Z

Is it possible to go from finalityViolated == true to finalityViolated == false during normal operation without restarting and changing finality depth manually?

Deleting blocks and logs from the database doesn't require a restart. Please see the test, this case is included there ;)

core/chains/evm/logpoller/log_poller.go

core/chains/evm/logpoller/log_poller_test.go

makramkd · 2024-03-07T14:23:38Z

core/chains/evm/logpoller/log_poller.go

@@ -203,6 +213,13 @@ func (filter *Filter) Contains(other *Filter) bool {
 	return true
 }

+func (lp *logPoller) Ready() error {


This PR looks good overall but what's the difference between Ready and Healthy in this context? We already have a HealthReport method that calls lp.Healthy() but since that's not implemented on the LP itself it calls it on the StateMachine object from chainlink-common which is embedded in the LP struct.

The added "benefit" of implementing Healthy is that HealthReport will return a meaningful report now since we return whether finality has been violated.

Keep in mind I'm not 100% sure how these functions are consumed outside of this particular context we want to consume them in now.

Oh nice catch, I wasn't aware of healthy function when going through the interfaces. @reductionista could you please advise which is better for this case?

I believe healthy/ready are inherited from k8s probes. If not ready k8s won't send traffic yet and if not healthy k8s will restart the node. We only expose the ready one to k8s directly

ccip/core/web/health_controller.go

Line 27 in 2f10153

// NOTE: We only implement the k8s readiness check, *not* the liveness check. Liveness checks are only recommended in cases

and have a separate health report. Healthy definitely makes more sense here

It seems that Healthy() error is not present in LogPoller's interface, probably it has to be added first 🤔

It's in the services.StateMachine that's embedded in the logPoller struct - we have to override it to get useful behavior IIUC.

Ah you mean the LogPoller interface - that's a good point. We should add it there if we want clients to call it.

Yeah, already added it, I'm just a bit worried about what @connorwstein said about that function being used for k8s tooling. If that's the case, then we don't want to restart pod whenever LogPoller is marked as not healthy.

Who is the SME on this? Maybe someone from infra or maybe core?

makramkd · 2024-03-07T14:25:04Z

core/chains/evm/logpoller/log_poller.go

@@ -924,6 +943,7 @@ func (lp *logPoller) findBlockAfterLCA(ctx context.Context, current *evmtypes.He
 	lp.lggr.Criticalw("Reorg greater than finality depth detected", "finalityTag", lp.useFinalityTag, "current", current.Number, "latestFinalized", latestFinalizedBlockNumber)
 	rerr := errors.New("Reorg greater than finality depth")
 	lp.SvcErrBuffer.Append(rerr)


It would be nice if we can use this but the semantics seem weird, it seems like the buffer is flushed whenever you read it, so it's not really super usable for our use case.

connorwstein · 2024-03-07T15:09:45Z

core/chains/evm/logpoller/log_poller.go

@@ -203,6 +213,13 @@ func (filter *Filter) Contains(other *Filter) bool {
 	return true
 }

+func (lp *logPoller) Ready() error {


I believe healthy/ready are inherited from k8s probes. If not ready k8s won't send traffic yet and if not healthy k8s will restart the node. We only expose the ready one to k8s directly

ccip/core/web/health_controller.go

Line 27 in 2f10153

// NOTE: We only implement the k8s readiness check, *not* the liveness check. Liveness checks are only recommended in cases

and have a separate health report. Healthy definitely makes more sense here

connorwstein · 2024-03-07T15:24:11Z

core/chains/evm/logpoller/log_poller.go

@@ -758,9 +775,11 @@ func (lp *logPoller) getCurrentBlockMaybeHandleReorg(ctx context.Context, curren
 			// We return an error here which will cause us to restart polling from lastBlockSaved + 1
 			return nil, err2
 		}
+		lp.finalityViolated.Store(false)


I think still an edge case here: say a finality violation is detected and you wipe the db. We'll hit the "first poll ever on new chain" return case here and leave the bool true. I think you want a defer where any non-error return value from this function we set to false

So you suggest marking it as false at the end of PollAndSave tick? It would be definitely easier if errors were bubbled up, but still doable

imagining something like this, can be a follow up

func (lp *logPoller) getCurrentBlockMaybeHandleReorg(ctx context.Context, currentBlockNumber int64, currentBlock *evmtypes.Head) (*evmtypes.Head, err error) { defer func() { // Note careful about shadowing etc if err != nil { lp.finalityViolated.Store(false) } }()

connorwstein · 2024-03-07T19:40:28Z

core/chains/evm/logpoller/log_poller.go

@@ -758,9 +775,11 @@ func (lp *logPoller) getCurrentBlockMaybeHandleReorg(ctx context.Context, curren
 			// We return an error here which will cause us to restart polling from lastBlockSaved + 1
 			return nil, err2
 		}
+		lp.finalityViolated.Store(false)


imagining something like this, can be a follow up

func (lp *logPoller) getCurrentBlockMaybeHandleReorg(ctx context.Context, currentBlockNumber int64, currentBlock *evmtypes.Head) (*evmtypes.Head, err error) { defer func() { // Note careful about shadowing etc if err != nil { lp.finalityViolated.Store(false) } }()

mateusz-sekara had a problem deploying to sdlc March 6, 2024 10:48 — with GitHub Actions Failure

mateusz-sekara force-pushed the logpoller-reorg-check branch from 0fcf6c3 to 815df30 Compare March 6, 2024 10:53

mateusz-sekara temporarily deployed to sdlc March 6, 2024 10:53 — with GitHub Actions Inactive

mateusz-sekara changed the title ~~PoC exposing isHealthy function in LogPoller~~ CCIP-1730 PoC exposing isHealthy function in LogPoller Mar 6, 2024

mateusz-sekara temporarily deployed to sdlc March 6, 2024 11:17 — with GitHub Actions Inactive

mateusz-sekara force-pushed the logpoller-reorg-check branch from f40277b to 5609cc5 Compare March 6, 2024 11:19

mateusz-sekara temporarily deployed to sdlc March 6, 2024 11:19 — with GitHub Actions Inactive

mateusz-sekara force-pushed the logpoller-reorg-check branch from 5609cc5 to f7387ea Compare March 6, 2024 14:51

mateusz-sekara temporarily deployed to sdlc March 6, 2024 14:51 — with GitHub Actions Inactive

mateusz-sekara temporarily deployed to sdlc March 6, 2024 14:54 — with GitHub Actions Inactive

mateusz-sekara force-pushed the logpoller-reorg-check branch from 4514f12 to c6620fa Compare March 6, 2024 14:55

mateusz-sekara temporarily deployed to sdlc March 6, 2024 14:55 — with GitHub Actions Inactive

reductionista reviewed Mar 6, 2024

View reviewed changes

core/chains/evm/logpoller/log_poller.go Outdated Show resolved Hide resolved

mateusz-sekara temporarily deployed to sdlc March 7, 2024 08:10 — with GitHub Actions Inactive

mateusz-sekara force-pushed the logpoller-reorg-check branch from 6166a8a to 0a1aca4 Compare March 7, 2024 08:12

mateusz-sekara temporarily deployed to sdlc March 7, 2024 08:12 — with GitHub Actions Inactive

mateusz-sekara changed the title ~~CCIP-1730 PoC exposing isHealthy function in LogPoller~~ CCIP-1730 Exposing isHealthy function in LogPoller Mar 7, 2024

mateusz-sekara requested a review from reductionista March 7, 2024 08:12

mateusz-sekara added 3 commits March 7, 2024 09:12

PoC exposing isHealthy function in LogPoller

0b642ea

Test

b5ca33d

Recovery example in tests

2841718

mateusz-sekara force-pushed the logpoller-reorg-check branch from 0a1aca4 to 2841718 Compare March 7, 2024 08:13

mateusz-sekara temporarily deployed to sdlc March 7, 2024 08:13 — with GitHub Actions Inactive

mateusz-sekara marked this pull request as ready for review March 7, 2024 08:13

mateusz-sekara requested a review from a team as a code owner March 7, 2024 08:13

Reusing Ready function instead of isHealthy

8e60ec1

mateusz-sekara temporarily deployed to sdlc March 7, 2024 08:40 — with GitHub Actions Inactive

mateusz-sekara changed the title ~~CCIP-1730 Exposing isHealthy function in LogPoller~~ CCIP-1730 Implementing ready function in LogPoller Mar 7, 2024

makramkd reviewed Mar 7, 2024

View reviewed changes

mateusz-sekara requested review from connorwstein, reductionista and makramkd and removed request for reductionista March 7, 2024 12:57

roman-kashitsyn reviewed Mar 7, 2024

View reviewed changes

core/chains/evm/logpoller/log_poller.go Outdated Show resolved Hide resolved

core/chains/evm/logpoller/log_poller.go Show resolved Hide resolved

mateusz-sekara temporarily deployed to sdlc March 7, 2024 14:04 — with GitHub Actions Inactive

mateusz-sekara requested a review from roman-kashitsyn March 7, 2024 14:06

roman-kashitsyn reviewed Mar 7, 2024

View reviewed changes

core/chains/evm/logpoller/log_poller_test.go Outdated Show resolved Hide resolved

Post review fixes

b3f1c9b

mateusz-sekara force-pushed the logpoller-reorg-check branch from c4a14b0 to b3f1c9b Compare March 7, 2024 14:13

mateusz-sekara temporarily deployed to sdlc March 7, 2024 14:13 — with GitHub Actions Inactive

Merge branch 'ccip-develop' into logpoller-reorg-check

104eaa1

mateusz-sekara temporarily deployed to sdlc March 7, 2024 14:14 — with GitHub Actions Inactive

roman-kashitsyn approved these changes Mar 7, 2024

View reviewed changes

makramkd reviewed Mar 7, 2024

View reviewed changes

connorwstein requested changes Mar 7, 2024

View reviewed changes

Ready replaced with Healthy

48d2284

mateusz-sekara temporarily deployed to sdlc March 7, 2024 15:37 — with GitHub Actions Inactive

mateusz-sekara changed the title ~~CCIP-1730 Implementing ready function in LogPoller~~ CCIP-1730 Implementing Healthy function in LogPoller Mar 7, 2024

mateusz-sekara requested review from makramkd and connorwstein March 7, 2024 17:05

reductionista approved these changes Mar 7, 2024

View reviewed changes

mateusz-sekara temporarily deployed to sdlc March 7, 2024 19:21 — with GitHub Actions Inactive

connorwstein approved these changes Mar 7, 2024

View reviewed changes

mateusz-sekara merged commit 9f70ea4 into ccip-develop Mar 7, 2024
134 of 135 checks passed

mateusz-sekara deleted the logpoller-reorg-check branch March 7, 2024 19:40

mateusz-sekara mentioned this pull request Mar 25, 2024

CCIP-1906 Raising a flag when finality is violated smartcontractkit/chainlink#12564

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCIP-1730 Implementing Healthy function in LogPoller #584

CCIP-1730 Implementing Healthy function in LogPoller #584

mateusz-sekara commented Mar 6, 2024 •

edited

Loading

makramkd left a comment

mateusz-sekara commented Mar 7, 2024

makramkd Mar 7, 2024

makramkd Mar 7, 2024

makramkd Mar 7, 2024

mateusz-sekara Mar 7, 2024

connorwstein Mar 7, 2024

mateusz-sekara Mar 7, 2024

makramkd Mar 7, 2024

makramkd Mar 7, 2024

mateusz-sekara Mar 7, 2024

makramkd Mar 8, 2024

makramkd Mar 7, 2024

connorwstein Mar 7, 2024

connorwstein Mar 7, 2024

mateusz-sekara Mar 7, 2024

connorwstein Mar 7, 2024

connorwstein Mar 7, 2024

CCIP-1730 Implementing Healthy function in LogPoller #584

CCIP-1730 Implementing Healthy function in LogPoller #584

Conversation

mateusz-sekara commented Mar 6, 2024 • edited Loading

makramkd left a comment

Choose a reason for hiding this comment

mateusz-sekara commented Mar 7, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateusz-sekara commented Mar 6, 2024 •

edited

Loading