[CAPPL-399] Syncer limits #16234

agparadiso · 2025-02-05T15:18:41Z

Description

This pr adds the following limits:

number of engines the syncer can host (global count limit)
number of workflows an owner can create (per owner limit)

CAPPL-399

Requires

Supports

github-actions · 2025-02-05T15:41:39Z

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

cedric-cordenier · 2025-02-10T10:47:01Z

core/config/toml/types.go

+	NetworkID              *string
+	ChainID                *string
+	GlobalEngineCountLimit *int32
+	WorkflowsPerOwnerLimit *int32


Can we put these two fields in a new top-level field called Workflows? I think putting it in ExternalRegistry isn't quite right (that refers to the CapabilitiesRegistry btw, not the WorklfowRegistry)

cedric-cordenier · 2025-02-10T10:48:01Z

core/services/chainlink/application.go

@@ -291,6 +292,14 @@ func NewApplication(opts ApplicationOpts) (Application, error) {
 		return nil, fmt.Errorf("could not instantiate workflow rate limiter: %w", err)
 	}

+	workflowSyncerLimiter, err := syncerlimiter.NewWorkflowSyncerLimiter(syncerlimiter.Config{


Just workflowLimiter, rather than workflowSyncerLimiter, it should apply to all workflows, not just those belonging to the workflowSyncer

cedric-cordenier · 2025-02-10T10:48:40Z

core/services/workflows/engine.go

@@ -1339,6 +1353,15 @@ func NewEngine(ctx context.Context, cfg Config) (engine *Engine, err error) {
 	// - that the resulting graph is strongly connected (i.e. no disjointed subgraphs exist)
 	// - etc.

+	// validate if adding another engine would exceed either the global or per owner count limit
+	ownerAllow, globalAllow := cfg.WorkflowSyncerLimiter.Allow(cfg.WorkflowOwner)


Can we put this in Start() instead so it's symmetric with Close :) ?

core/services/workflows/syncerlimiter/limiter.go

…creation

jmank88 · 2025-02-11T14:28:42Z

core/config/docs/core.toml

+[Capabilities.WorkflowRegistry.Limits]
+# WorkflowsGlobalLimit is the maximum number of workflows that can be registered globally.
+WorkflowsGlobalLimit = 50 # Default


Do we need to say Limit twice? Or could we drop either the extra wrapper or the suffix?

cedric-cordenier · 2025-02-13T10:09:50Z

core/config/capabilities_config.go

@@ -27,6 +27,8 @@ type CapabilitiesWorkflowRegistry interface {
 	MaxBinarySize() utils.FileSize
 	MaxConfigSize() utils.FileSize
 	RelayID() types.RelayID
+	WorkflowsGlobal() int32
+	WorkflowsPerOwner() int32


@agparadiso Is this correct? Looking at the docs there should be a struct beneath it called limits I think?

true, this was working because underneath I was calling Limits:

func (c *capabilitiesWorkflowRegistry) WorkflowsGlobal() int32 { return *c.c.Limits.WorkflowsGlobal } func (c *capabilitiesWorkflowRegistry) WorkflowsPerOwner() int32 { return *c.c.Limits.WorkflowsPerOwner }

will modify it, thanks!

cedric-cordenier · 2025-02-13T10:14:30Z

core/config/docs/core.toml

@@ -470,6 +470,12 @@ MaxEncryptedSecretsSize = '26.40kb' # Default
 # MaxConfigSize is the maximum size of a config that can be fetched from the given config url.
 MaxConfigSize = '50.00kb' # Default

+[Capabilities.WorkflowRegistry.Limits]


I still think we should move this to something that doesn't imply it's a limit that only applies to the WorkflowRegistry

eg. [Workflows.Limits]

Addressed 👍🏼

cedric-cordenier · 2025-02-25T10:16:57Z

core/config/docs/core.toml

+# Global is the maximum number of workflows that can be registered globally.
+Global = 50 # Default
+# PerOwner is the maximum number of workflows that can be registered per owner.
+PerOwner = 5 # Default


@agparadiso Can we set this to 200 Global and 200 PerOwner by default? This will be what we need for ST3

cedric-cordenier · 2025-02-25T10:17:49Z

core/services/workflows/engine.go

+		// validate if adding another workflow would exceed either the global or per owner engine count limit
+		ownerAllow, globalAllow := e.workflowLimits.Allow(e.workflow.owner)
+		if !globalAllow {
+			e.metrics.with(platform.KeyWorkflowID, e.workflow.id, platform.KeyWorkflowOwner, e.workflow.owner).incrementWorkflowLimitGlobalCounter(ctx)


We also should log a message via Beholder here and below on line 166

github-actions · 2025-02-26T17:33:25Z

Flakeguard Summary

Ran new or updated tests between develop and 4e954bb (CAPPL-399_syncer_limits).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

1 Results

Name	Pass Ratio	Panicked?	Timed Out?	Race?	Runs	Successes	Failures	Skips	Package	Package Panicked?	Avg Duration	Code Owners
TestWorkflowsConfig	0%	false	false	false	3	0	3	0	github.com/smartcontractkit/chainlink/v2/core/services/chainlink	false	10ms	Unknown

Artifacts

For detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json.

cl-sonarqube-production · 2025-02-26T17:57:49Z

Quality Gate passed

Issues
1 New issue
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

agparadiso force-pushed the CAPPL-399_syncer_limits branch 3 times, most recently from c22722f to 10ba0d2 Compare February 7, 2025 17:57

cedric-cordenier reviewed Feb 10, 2025

View reviewed changes

core/services/workflows/syncerlimiter/limiter.go Show resolved Hide resolved

agparadiso force-pushed the CAPPL-399_syncer_limits branch from 10ba0d2 to 88e3844 Compare February 10, 2025 11:02

agparadiso added 10 commits February 10, 2025 12:53

feat: WorkflowRegistry global limit

2804d7c

feat: implement limit of workflows per owner

516e0b5

fix: default values not loading

b65dfaf

feat: refactor GlobalEngineCountLimit to affect also delegate engine …

de44148

…creation

feat: encapsulate limiter as syncerlimiter and add per owner limits

3667b1a

chore: rename workflowSyncerLimiter to workflowLimiter

8d3fecf

feat: move WorkflowLimiter to start method and add monitoring

6815a46

chore: align naming GlobalEngineCountLimit to WorkflowsGlobalLimit

12a3834

chore: rename WorkflowLimiter to WorkflowLimits

8a03a29

feat: group workflow limits on a top-level field

5dd51d6

agparadiso force-pushed the CAPPL-399_syncer_limits branch 4 times, most recently from 7da41ca to 3544aea Compare February 10, 2025 17:11

fix: tests

2b8f98c

agparadiso force-pushed the CAPPL-399_syncer_limits branch from 3544aea to 2b8f98c Compare February 10, 2025 20:40

agparadiso marked this pull request as ready for review February 11, 2025 09:57

agparadiso requested review from a team as code owners February 11, 2025 09:57

agparadiso requested a review from cedric-cordenier February 11, 2025 09:57

jmank88 reviewed Feb 11, 2025

View reviewed changes

agparadiso added 3 commits February 12, 2025 12:19

fix: limit amount of workflows on Start method

1a0c288

chore: rename WorkflowsGlobalLimit and WorkflowsPerOwnerLimit

0ae4b76

test: simplify tests by checking return err

a1dcf8f

agparadiso requested a review from jmank88 February 12, 2025 16:32

cedric-cordenier reviewed Feb 13, 2025

View reviewed changes

fix: rename limits and place it under [Workflows.Limits]

0dff894

agparadiso force-pushed the CAPPL-399_syncer_limits branch from 3353bab to c5e8a20 Compare February 17, 2025 12:34

Merge branch 'develop' into CAPPL-399_syncer_limits

891f2ee

agparadiso force-pushed the CAPPL-399_syncer_limits branch from c5e8a20 to 891f2ee Compare February 17, 2025 14:53

agparadiso requested a review from cedric-cordenier February 17, 2025 15:20

cedric-cordenier reviewed Feb 25, 2025

View reviewed changes

chore: change defaults to 200 global and perOwner

2cdcf9c

agparadiso force-pushed the CAPPL-399_syncer_limits branch from 4e954bb to 2cdcf9c Compare February 26, 2025 17:32

chore: log a message when limit is reached on beholder

92f8544

cedric-cordenier approved these changes Feb 27, 2025

View reviewed changes

ettec approved these changes Feb 27, 2025

View reviewed changes

cedric-cordenier added this pull request to the merge queue Feb 27, 2025

Merged via the queue into develop with commit f50d567 Feb 27, 2025
182 of 184 checks passed

cedric-cordenier deleted the CAPPL-399_syncer_limits branch February 27, 2025 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CAPPL-399] Syncer limits #16234

[CAPPL-399] Syncer limits #16234

agparadiso commented Feb 5, 2025 •

edited

Loading

github-actions bot commented Feb 5, 2025 •

edited

Loading

cedric-cordenier Feb 10, 2025

cedric-cordenier Feb 10, 2025

cedric-cordenier Feb 10, 2025

jmank88 Feb 11, 2025

cedric-cordenier Feb 13, 2025

agparadiso Feb 14, 2025

cedric-cordenier Feb 13, 2025

agparadiso Feb 17, 2025

cedric-cordenier Feb 25, 2025

cedric-cordenier Feb 25, 2025

github-actions bot commented Feb 26, 2025

cl-sonarqube-production bot commented Feb 26, 2025

[CAPPL-399] Syncer limits #16234

[CAPPL-399] Syncer limits #16234

Conversation

agparadiso commented Feb 5, 2025 • edited Loading

Description

Requires

Supports

github-actions bot commented Feb 5, 2025 • edited Loading

AER Report: CI Core ran successfully ✅

AER Report: Operator UI CI ran successfully ✅

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 Feb 26, 2025

Flakeguard Summary

Found Flaky Tests ❌

Artifacts

cl-sonarqube-production bot commented Feb 26, 2025

Quality Gate passed

agparadiso commented Feb 5, 2025 •

edited

Loading

github-actions bot commented Feb 5, 2025 •

edited

Loading