Executable capability registration logic update to match that of trigger capability plus expand test suite to test behaviour #15593

ettec · 2024-12-10T10:47:21Z

Reuse the registration logic from trigger capabiliies in executable capabilties and expand the test cases to contain workflow/capability node bouncing and registration tests to ensure registration logic behaves as expected.

https://smartcontract-it.atlassian.net/browse/CAPPL-314

depends on

add registration refresh and expiry to executable capability remote config chainlink-common#968

github-actions · 2024-12-10T10:48:42Z

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

#added For any new functionality added.
#breaking_change For any functionality that requires manual action for the node to boot.
#bugfix For bug fixes.
#changed For any change to the existing functionality.
#db_update For any feature that introduces updates to database schema.
#deprecation_notice For any upcoming deprecation functionality.
#internal For changesets that need to be excluded from the final changelog.
#nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
#removed For any functionality/config that is removed.
#updated For any functionality that is updated.
#wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

🎖️ No JIRA issue number found in: PR title, commit message, or branch name. Please include the issue ID in one of these.

github-actions · 2024-12-10T10:59:13Z

AER Report: CI Core

aer_workflow , commit , Scheduled Run Frequency , Clean Go Tidy & Generate , Detect Changes , Flakeguard Root Project / Get Tests To Run , GolangCI Lint , Core Tests (go_core_tests) , Core Tests (go_core_tests_integration) , Core Tests (go_core_ccip_deployment_tests) , Core Tests (go_core_fuzz) , Core Tests (go_core_race_tests) , test-scripts , Flakeguard Deployment Project , Flakeguard Root Project / Run Tests (github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/registration, ubuntu... , Flakeguard Root Project / Run Tests (github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/executable, ubuntu-l... , lint , Flakeguard Root Project / Report , SonarQube Scan , Flakey Test Detection

1. GolangCI Lint failed due to typechecking error and permission denied

[A 1 <= 10 words sentence that describes the error]:[job id where the error happened]

Source of Error:

Golang Lint ()	2024-12-18T19:51:43.5438824Z level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix / does not contain main module or its selected dependencies"
Golang Lint ()	2024-12-18T19:51:43.5440257Z level=error msg="Running error: can't create output for golangci-lint-report.xml: open golangci-lint-report.xml: permission denied"
Golang Lint ()	2024-12-18T19:51:43.5474725Z ##[error]golangci-lint exit with code 3

Why: The GolangCI Lint action failed because it could not find the main module or its dependencies in the root directory (/). Additionally, it encountered a permission denied error when trying to create the golangci-lint-report.xml file.

Suggested fix: Ensure that the working directory is correctly set to the directory containing the go.mod file. Also, check and adjust file permissions to allow the creation of the golangci-lint-report.xml file.

2. Cat command failed due to missing file

[A 1 <= 10 words sentence that describes the error]:[job id where the error happened]

Source of Error:

Golang Lint ()	2024-12-18T19:51:43.5584137Z ##[group]Run cat .//golangci-lint-report.xml
Golang Lint ()	2024-12-18T19:51:43.5584636Z �[36;1mcat .//golangci-lint-report.xml�[0m
Golang Lint ()	2024-12-18T19:51:43.5687336Z cat: .//golangci-lint-report.xml: No such file or directory
Golang Lint ()	2024-12-18T19:51:43.5693336Z ##[error]Process completed with exit code 1.

Why: The cat command failed because the golangci-lint-report.xml file was not created due to the previous permission denied error.

Suggested fix: Fix the permission issue and ensure the golangci-lint-report.xml file is created successfully before attempting to read it with cat.

3. Upload artifact failed due to missing file

[A 1 <= 10 words sentence that describes the error]:[job id where the error happened]

Source of Error:

Golang Lint ()	2024-12-18T19:51:43.5882247Z ##[group]Run actions/[email protected]
Golang Lint ()	2024-12-18T19:51:43.5882738Z with:
Golang Lint ()	2024-12-18T19:51:43.5883040Z name: golangci-lint-report-
Golang Lint ()	2024-12-18T19:51:43.5883428Z path: .//golangci-lint-report.xml
Golang Lint ()	2024-12-18T19:51:43.8408851Z ##[warning]No files were found with the provided path: .//golangci-lint-report.xml. No artifacts will be uploaded.

Why: The artifact upload step failed because the golangci-lint-report.xml file was not found, likely due to the previous permission denied error preventing its creation.

Suggested fix: Resolve the permission issue to ensure the golangci-lint-report.xml file is created, then verify the file path is correct before attempting to upload the artifact.

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

bolekk

relayed some high-level comments offline

core/capabilities/remote/trigger_subscriber.go

…e capability

cl-sonarqube-production · 2024-12-18T20:02:11Z

Quality Gate passed

Issues
2 New issues
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

bolekk · 2024-12-19T01:27:25Z

core/capabilities/remote/trigger_subscriber.go

 	"github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/types"
 	"github.com/smartcontractkit/chainlink/v2/core/logger"
 	p2ptypes "github.com/smartcontractkit/chainlink/v2/core/services/p2p/types"
 )

+const (
+	defaultSendChannelBufferSize = 1000


why not keep consts together like they were before?

bolekk · 2024-12-19T01:28:29Z

core/capabilities/remote/trigger_subscriber.go

+	s.wg.Add(1)
+	err := s.registrationClient.Start(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to start capability register: %w", err)


"capability register" -> "capability registration client"

bolekk · 2024-12-19T01:31:19Z

core/capabilities/remote/trigger_subscriber.go

@@ -106,78 +106,58 @@ func (s *triggerSubscriber) Info(ctx context.Context) (commoncap.CapabilityInfo,
 }

 func (s *triggerSubscriber) RegisterTrigger(ctx context.Context, request commoncap.TriggerRegistrationRequest) (<-chan commoncap.TriggerResponse, error) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
 	rawRequest, err := pb.MarshalTriggerRegistrationRequest(request)
 	if err != nil {
 		return nil, err
 	}
 	if request.Metadata.WorkflowID == "" {
 		return nil, errors.New("empty workflowID")
 	}


Lock() was here before on purpose - we don't have to unmarshal the request under lock

If you can, maybe send out a fix for the race condition in a separate PR, so those two independent changes are not bundled together.

bolekk · 2024-12-19T01:34:13Z

core/capabilities/remote/trigger_subscriber.go

-			registration, found := s.registeredWorkflows[workflowID]
-			s.mu.RUnlock()
-			if !found {
+			registered := s.isWorkflowRegistered(workflowID)


you don't need the "registered" variable, just if !s.isWorkflow...

bolekk · 2024-12-19T01:35:51Z

core/capabilities/remote/trigger_subscriber.go

-	// Registrations will quickly expire on all remote nodes.
-	// Alternatively, we could send UnregisterTrigger messages right away.
+
+	responseChannel <- response


We do this under a lock - what if the reader is slow? It will block the whole subscriber object.

bolekk · 2024-12-19T01:39:05Z

core/capabilities/remote/registration/client.go

+	// unregister messages to the nodes
+}
+
+func (r *Client) registrationLoop() {


this is pretty much the same as the current registrationLoop() in triggerSubscriber, right?

bolekk · 2024-12-19T01:40:13Z

core/capabilities/remote/registration/client.go

+	registrationRequest []byte
+}
+
+type registerDispatcher interface {


You should be able to just use the types.Dispatcher interface. Is there some unexpected circular dependency?

bolekk · 2024-12-19T01:43:44Z

core/capabilities/remote/trigger_publisher.go

+
+	registrationServer := registration.NewServer(lggr, publisher, capInfo, config.RegistrationExpiry, workflowDONs, "TriggerPublisher")
+
+	publisher.registrationServer = registrationServer


just put those two lines above object creation and return it like it was before - why make an exception for this one field?

bolekk · 2024-12-19T01:50:45Z

core/capabilities/remote/executable/server.go

-		r.lggr.Errorw("invalid message id", "err", err, "id", remote.SanitizeLogString(string(msg.MessageId)))
-		return
-	}
+		req, err := pb.UnmarshalRegisterToWorkflowRequest(msg.Payload)


Some tabs vs spaces problem here?

ettec marked this pull request as ready for review December 10, 2024 11:34

ettec requested review from a team as code owners December 10, 2024 11:34

ettec requested a review from ilija42 December 10, 2024 11:34

ilija42 requested a review from EasterTheBunny December 11, 2024 20:14

ettec added the build-publish Build and Publish image to SDLC label Dec 16, 2024

ettec force-pushed the register-to-workflow branch 2 times, most recently from 8236209 to 9bb5804 Compare December 16, 2024 21:01

bolekk reviewed Dec 17, 2024

View reviewed changes

core/capabilities/remote/trigger_subscriber.go Outdated Show resolved Hide resolved

ettec force-pushed the register-to-workflow branch from 2dc55be to 7e69460 Compare December 17, 2024 12:19

ettec mentioned this pull request Dec 18, 2024

Preperation for register unregister execution capability #15760

Merged

ettec force-pushed the register-to-workflow branch from eea3387 to 775a25b Compare December 18, 2024 19:22

reuse capability registration logic from the trigger in the executabl…

3b27019

…e capability

ettec force-pushed the register-to-workflow branch from 775a25b to 3b27019 Compare December 18, 2024 19:40

post rebase tidy

0c09a0f

bolekk reviewed Dec 19, 2024

View reviewed changes

ettec closed this Dec 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Executable capability registration logic update to match that of trigger capability plus expand test suite to test behaviour #15593

Executable capability registration logic update to match that of trigger capability plus expand test suite to test behaviour #15593

ettec commented Dec 10, 2024

github-actions bot commented Dec 10, 2024

github-actions bot commented Dec 10, 2024 •

edited

Loading

bolekk left a comment

cl-sonarqube-production bot commented Dec 18, 2024

bolekk Dec 19, 2024

bolekk Dec 19, 2024

bolekk Dec 19, 2024

bolekk Dec 19, 2024

bolekk Dec 19, 2024

bolekk Dec 19, 2024

bolekk Dec 19, 2024

bolekk Dec 19, 2024

bolekk Dec 19, 2024

bolekk Dec 19, 2024


		registrationServer := registration.NewServer(lggr, publisher, capInfo, config.RegistrationExpiry, workflowDONs, "TriggerPublisher")

		publisher.registrationServer = registrationServer

Executable capability registration logic update to match that of trigger capability plus expand test suite to test behaviour #15593

Executable capability registration logic update to match that of trigger capability plus expand test suite to test behaviour #15593

Conversation

ettec commented Dec 10, 2024

github-actions bot commented Dec 10, 2024

github-actions bot commented Dec 10, 2024 • edited Loading

AER Report: CI Core

1. GolangCI Lint failed due to typechecking error and permission denied

2. Cat command failed due to missing file

3. Upload artifact failed due to missing file

AER Report: Operator UI CI ran successfully ✅

bolekk left a comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Dec 18, 2024

Quality Gate passed

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 Dec 10, 2024 •

edited

Loading