Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CAPPL-324] rehydrate artifacts from db if they haven't changed #15668

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

agparadiso
Copy link
Contributor

@agparadiso agparadiso commented Dec 12, 2024

Description

This fix introduces a modification in the way we fetch the artifacts (binary and config). Basically we only want to download them from the provided URLs if they have changed and fetch them from the DB if they haven't. We control if they changed or not by fetching them by the workflowID that is generated based on the content of these artifacts.

CAPPL-324

Requires

Supports

ctx context.Context,
payload WorkflowRegistryWorkflowRegisteredV1,
) ([]byte, []byte, error) {
spec, err := h.orm.GetWorkflowSpec(ctx, hex.EncodeToString(payload.WorkflowOwner), payload.WorkflowName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agparadiso It's important to fetch by the workflow ID and NOT by the owner + the name; this will guarantee that the contents of the artifacts are the same as what's in the registration event.

func (h *eventHandler) decodeStoredWorkflowArtifacts(
spec *job.WorkflowSpec,
) ([]byte, []byte, error) {
decodedBinary, err := hex.DecodeString(spec.Workflow)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only used in one place and the function is quite small, I'd recommend you inline it in the function that uses it.

The broader point here is: abstractions always have a cost (cognitive overhead); make sure that they are worth that cost whether in terms of documentation (a function name gets you that for free) or otherwise. In this case I think it's minimal since it's obvious that we are decoding already due to the call to hex.DecodeString.

}

// there is an update in the BinaryURL or ConfigURL, lets fetch it
if spec.ConfigURL != payload.ConfigURL || spec.BinaryURL != payload.BinaryURL {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're operating on the workflowID and it is the same, you actually wouldn't need this check.

The flow should basically be either get the artifacts from the DB and if that fails for any reason recreate it. I don't think you need anything else.

Copy link
Contributor

github-actions bot commented Dec 12, 2024

AER Report: CI Core

aer_workflow , commit , Detect Changes , Clean Go Tidy & Generate , Scheduled Run Frequency , Flakeguard Root Project / Get Tests To Run , lint , Core Tests (go_core_tests) , Core Tests (go_core_tests_integration) , Flakeguard Deployment Project , Core Tests (go_core_ccip_deployment_tests) , Core Tests (go_core_race_tests) , Core Tests (go_core_fuzz) , Flakeguard Root Project / Run Tests (github.com/smartcontractkit/chainlink/v2/core/services/workflows/syncer, ubuntu-latest) , Flakey Test Detection , SonarQube Scan , Flakeguard Root Project / Report

1. Test result parsing failure:Run tests with flakeguard

Source of Error:
Run tests with flakeguard	2024-12-13T11:46:24.0656983Z Error running tests: failed to parse test results: failed to attribute panic to test, using regex syncer\.(Test[^\.\(]+) on these strings:
Run tests with flakeguard	2024-12-13T11:46:24.0658868Z panic: Log in goroutine after Test_workflowRegisteredHandler/skips_fetch_if_secrets_url_is_missing has completed: 2024-12-13T11:42:59.900Z	DEBUG	CapabilitiesRegistry	capabilities/registry.go:69	get capability	{"version": "unset@unset", "id": "basic-test-trigger@1.0.0"}

Why: The error occurred because a panic was logged after the test Test_workflowRegisteredHandler/skips_fetch_if_secrets_url_is_missing had already completed. This caused the test result parser to fail to attribute the panic to the correct test.

Suggested fix: Ensure that all logging and operations within the test are completed before the test ends. Review the test and the code it calls to ensure no asynchronous operations are still running after the test completes.

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@agparadiso agparadiso force-pushed the CAPPL-324_rehydrate_from_db branch from 189a28a to 8c88fc4 Compare December 13, 2024 11:02
@agparadiso agparadiso changed the title fix: rehydrate artifacts from db if they haven't changed [CAPPL-324] rehydrate artifacts from db if they haven't changed Dec 13, 2024
@agparadiso agparadiso marked this pull request as ready for review December 13, 2024 14:48
@agparadiso agparadiso requested a review from a team as a code owner December 13, 2024 14:48
@cedric-cordenier cedric-cordenier added this pull request to the merge queue Dec 13, 2024
auto-merge was automatically disabled December 13, 2024 15:37

Pull Request is not mergeable

Merged via the queue into develop with commit bc77243 Dec 13, 2024
168 of 170 checks passed
@cedric-cordenier cedric-cordenier deleted the CAPPL-324_rehydrate_from_db branch December 13, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants