Auto 4601/add log retry integration test #10355

EasterTheBunny · 2023-08-25T15:47:06Z

This PR offers a proposal for completing an integration test that runs log
trigger upkeeps, a full check pipeline, fails mercury server calls, and allows
the application to retry the upkeeps.

Additions include:

mock mercury server that can have a custom handler per test
upkeep setup helpers for creating/registering log triggered upkeeps
integration test for log event retry flow

github-actions · 2023-08-25T15:47:29Z

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

github-actions · 2023-08-25T15:47:29Z

I see that you haven't updated any README files. Would it make sense to do so?

amirylm · 2023-08-30T07:41:19Z

core/services/ocr2/plugins/ocr2keeper/evm21/encoding/packer.go

@@ -66,6 +66,17 @@ func (p *abiPacker) UnpackCheckResult(payload ocr2keepers.UpkeepPayload, raw str
 	return result, nil
 }

+func (p *abiPacker) UnpackGetUpkeepPrivilegeConfig(resp []byte) ([]byte, error) {


amirylm · 2023-08-30T07:42:51Z

core/services/ocr2/plugins/ocr2keeper/evm21/feed_lookup.go

@@ -217,10 +217,37 @@ func (r *EvmRegistry) allowedToUseMercury(opts *bind.CallOpts, upkeepId *big.Int
 		return encoding.NoPipelineError, encoding.UpkeepFailureReasonNone, false, allowed.(bool), nil
 	}

-	cfg, err := r.registry.GetUpkeepPrivilegeConfig(opts, upkeepId)
+	payload, err := r.abi.Pack("getUpkeepPrivilegeConfig", upkeepId)


extract to packer.PackGetUpkeepPrivilegeConfig

infiloop2 · 2023-08-30T17:22:21Z

core/services/ocr2/plugins/ocr2keeper/evm21/feed_lookup.go

+		// pack error, no retryable
+		r.lggr.Warnf("failed to pack getUpkeepPrivilegeConfig data for upkeepId %s: %s", upkeepId, err)
+
+		return encoding.RpcFlakyFailure, encoding.UpkeepFailureReasonNone, true, false, fmt.Errorf("failed to pack upkeepId: %w", err)


will need to fix this error instead of RpcFlakyFailure

I can create a new error type that makes more sense in this case.

This might fit in here but best to confirm with felix https://github.com/smartcontractkit/chainlink/blob/develop/core/services/ocr2/plugins/ocr2keeper/evm21/encoding/interface.go#L39

agree that this should be PackUnpackDecodeFailed

and this should be not retryable

infiloop2 · 2023-08-30T17:22:58Z

core/services/ocr2/plugins/ocr2keeper/evm21/feed_lookup.go

+		return encoding.RpcFlakyFailure, encoding.UpkeepFailureReasonNone, true, false, fmt.Errorf("failed to get upkeep privilege config: %v", err)
+	}
+
+	cfg, err := r.packer.UnpackGetUpkeepPrivilegeConfig(b)
 	if err != nil {
 		return encoding.RpcFlakyFailure, encoding.UpkeepFailureReasonNone, true, false, fmt.Errorf("failed to get upkeep privilege config: %v", err)


infiloop2 · 2023-08-30T17:25:39Z

core/services/ocr2/plugins/ocr2keeper/evm21/feed_lookup.go

@@ -217,21 +217,43 @@ func (r *EvmRegistry) allowedToUseMercury(opts *bind.CallOpts, upkeepId *big.Int
 		return encoding.NoPipelineError, encoding.UpkeepFailureReasonNone, false, allowed.(bool), nil
 	}

-	cfg, err := r.registry.GetUpkeepPrivilegeConfig(opts, upkeepId)
+	payload, err := r.packer.PackGetUpkeepPrivilegeConfig(upkeepId)


let's ensure we get Felix's review on this file if we go ahead with this approach

I'll ping him on it

FelixFan1992 · 2023-08-30T18:08:33Z

core/services/ocr2/plugins/ocr2keeper/evm21/feed_lookup.go

+		return encoding.RpcFlakyFailure, encoding.UpkeepFailureReasonNone, true, false, fmt.Errorf("failed to get upkeep privilege config: %v", err)
+	}
+
+	cfg, err := r.packer.UnpackGetUpkeepPrivilegeConfig(b)
 	if err != nil {
 		return encoding.RpcFlakyFailure, encoding.UpkeepFailureReasonNone, true, false, fmt.Errorf("failed to get upkeep privilege config: %v", err)


unpack error should be not retryable

github-actions · 2023-09-07T18:14:18Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6113328010

github-actions · 2023-09-08T15:15:29Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6123461550

github-actions · 2023-09-12T14:10:40Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6160657318

github-actions · 2023-09-12T15:06:49Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6161345141

github-actions · 2023-09-12T15:41:10Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6161752423

github-actions · 2023-09-12T15:42:02Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6161752820

shileiwill · 2023-09-12T17:02:52Z

core/services/ocr2/plugins/ocr2keeper/evm21/encoding/packer_test.go

+				// the actual struct to unmarshal into is not available to this
+				// package so basic json encoding is the limit of the following test
+				var data map[string]interface{}
+				err = json.Unmarshal(b, &data)


does it make sense to compare the output bytes of unpack() and our expected bytes?

I can add a test that b adheres to the correct json structure in byte form. It's a little brittle but it should work.

shileiwill · 2023-09-12T17:15:47Z

core/services/ocr2/plugins/ocr2keeper/evm21/streams_lookup.go

+		return encoding.PackUnpackDecodeFailed, encoding.UpkeepFailureReasonNone, false, false, fmt.Errorf("failed to pack upkeepId: %w", err)
+	}
+
+	var b hexutil.Bytes


super nit, i would recommend a more meaningful name, e.g. result or resultBytes.

shileiwill · 2023-09-12T17:16:41Z

core/services/ocr2/plugins/ocr2keeper/evm21/streams_lookup.go

 	if len(cfg) == 0 {
 		r.mercury.allowListCache.Set(upkeepId.String(), false, cache.DefaultExpiration)
 		return encoding.NoPipelineError, encoding.UpkeepFailureReasonMercuryAccessNotAllowed, false, false, fmt.Errorf("upkeep privilege config is empty")
 	}

 	var a UpkeepPrivilegeConfig
-	err = json.Unmarshal(cfg, &a)


ditto, how about something like privilegeConfig?

shileiwill · 2023-09-12T17:47:29Z

core/services/ocr2/plugins/ocr2keeper/integration_21_test.go

+		// the simulation here should force the streams lookup process to return
+		// retryable 2 times.
+		// the total count of failures should be (upkeepCount * 3 * tryCount)
+		if count <= mercuryFailCount {


i didnt get this math. mercury, by default, retries 3 times before returning a retryable, so for 1 upkeep, we should hit the mercury server 5 times(1st is regular query, 2,3,4 are retries, and then the 5th will return a retryable) in order to get 1 retryable?

For each call to CheckUpkeeps, mercury is tried 3 times in total for each upkeep. The expectation is that for each upkeep, 2 calls to CheckUpkeeps should result in a Retryable result and one should succeed. So for each upkeep that should be 3 'tries' * 2 'retries' for the failure condition and one more 'try' for the success condition. I think it's right?

shileiwill · 2023-09-12T17:49:14Z

core/services/ocr2/plugins/ocr2keeper/integration_21_test.go

+
+	// using sleep here is not the right way to go, but it currently work on
+	// a local machine. this does have issues in CI
+	time.Sleep(10 * time.Second)


curious, what is the issue in CI?

Let's avoid the sleep as it might be flaky, can't we wait for something to happen?

Actually I'll try removing this. Amir did some updates where this timer wasn't needed.

shileiwill · 2023-09-12T17:56:56Z

core/services/ocr2/plugins/ocr2keeper/integration_21_test.go

+
+func (c *feedLookupUpkeepController) RegisterAndFund(
+	t *testing.T,
+	f registerAndFundFunc,


is it possible to move the type registerAndFundFunc and the function definition in core/services/ocr2/plugins/ocr2keeper/integration_21_test.go to here? So we can remove the additional layer?

This commit offers a proposal for completing an integration test that runs log trigger upkeeps, a full check pipeline, fails mercury server calls, and allows the application to retry the upkeeps. Additions include: - mock mercury server that can have a custom handler per test - upkeep setup helpers for creating/registering log triggered upkeeps - integration test for log event retry flow

cl-sonarqube-production · 2023-09-13T16:24:23Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

84.0% Coverage
0.0% Duplication

EasterTheBunny force-pushed the AUTO-4601/add-log-retry-integration-test branch from 91cbaaa to 96390f0 Compare August 25, 2023 15:57

amirylm reviewed Aug 30, 2023

View reviewed changes

EasterTheBunny force-pushed the AUTO-4601/add-log-retry-integration-test branch from 9261892 to 8f0d764 Compare August 30, 2023 15:21

infiloop2 reviewed Aug 30, 2023

View reviewed changes

FelixFan1992 reviewed Aug 30, 2023

View reviewed changes

EasterTheBunny force-pushed the AUTO-4601/add-log-retry-integration-test branch from 255f4b2 to 9b02e7d Compare September 7, 2023 18:13

EasterTheBunny force-pushed the AUTO-4601/add-log-retry-integration-test branch from 9b02e7d to 6ab372d Compare September 8, 2023 15:14

EasterTheBunny force-pushed the AUTO-4601/add-log-retry-integration-test branch from 6174617 to 13c47c7 Compare September 12, 2023 14:10

EasterTheBunny marked this pull request as ready for review September 12, 2023 15:44

EasterTheBunny requested a review from a team as a code owner September 12, 2023 15:44

shileiwill reviewed Sep 12, 2023

View reviewed changes

EasterTheBunny added 8 commits September 13, 2023 11:05

successful run of retry integration test

2bbfc70

cleaned up test and packer and added tests

32021ce

corrected pipeline error types and retryable states

12c046d

fix test and streams name change

2841753

fix streams lookup test

743afae

fix AllowedToUseMercury test

8b07fcd

fix mercury test

02d9a11

apply code suggestions

b99cb4a

EasterTheBunny force-pushed the AUTO-4601/add-log-retry-integration-test branch from 32b60c5 to b99cb4a Compare September 13, 2023 16:05

EasterTheBunny enabled auto-merge September 13, 2023 16:53

shileiwill approved these changes Sep 13, 2023

View reviewed changes

EasterTheBunny added this pull request to the merge queue Sep 13, 2023

Merged via the queue into develop with commit 50134ee Sep 13, 2023
98 checks passed

EasterTheBunny deleted the AUTO-4601/add-log-retry-integration-test branch September 13, 2023 17:15

snehaagni mentioned this pull request Oct 18, 2023

release/2.6.0 -> master #10997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto 4601/add log retry integration test #10355

Auto 4601/add log retry integration test #10355

EasterTheBunny commented Aug 25, 2023

github-actions bot commented Aug 25, 2023

github-actions bot commented Aug 25, 2023

amirylm Aug 30, 2023

amirylm Aug 30, 2023

infiloop2 Aug 30, 2023

EasterTheBunny Aug 30, 2023

infiloop2 Aug 30, 2023

FelixFan1992 Aug 30, 2023

FelixFan1992 Aug 30, 2023

infiloop2 Aug 30, 2023

infiloop2 Aug 30, 2023

EasterTheBunny Aug 30, 2023

FelixFan1992 Aug 30, 2023

github-actions bot commented Sep 7, 2023

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 12, 2023

github-actions bot commented Sep 12, 2023

github-actions bot commented Sep 12, 2023

github-actions bot commented Sep 12, 2023

shileiwill Sep 12, 2023

EasterTheBunny Sep 12, 2023

shileiwill Sep 12, 2023

shileiwill Sep 12, 2023

shileiwill Sep 12, 2023

EasterTheBunny Sep 12, 2023

shileiwill Sep 12, 2023

amirylm Sep 12, 2023

EasterTheBunny Sep 12, 2023

shileiwill Sep 12, 2023

cl-sonarqube-production bot commented Sep 13, 2023

Auto 4601/add log retry integration test #10355

Auto 4601/add log retry integration test #10355

Conversation

EasterTheBunny commented Aug 25, 2023

github-actions bot commented Aug 25, 2023

github-actions bot commented Aug 25, 2023

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

github-actions bot commented Sep 7, 2023

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 12, 2023

github-actions bot commented Sep 12, 2023

github-actions bot commented Sep 12, 2023

github-actions bot commented Sep 12, 2023

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

cl-sonarqube-production bot commented Sep 13, 2023