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

feat: Add sync-dependencies #514

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

feat: Add sync-dependencies #514

wants to merge 4 commits into from

Conversation

alexec
Copy link

@alexec alexec commented Mar 19, 2023

Depended on by argo-cd#3517

This PR introduces a new feature: sync dependencies.

Sync dependencies give a new way to order sync operations that is influenced by sync waves and sync hooks (all of which I wrote, so I know it extremely well).

Like sync waves, a sync operation will only progress when all the dependents have completed.

Dependencies between objects is specified by the new argocd.argoproj.io/sync-dependencies annotation.

# The dependencies are specified as a comma-separated list of objects.
# The format of each object is <group>/<kind>/<namespace>/<name>
# The group and kind are optional. If not specified, they'll match any group or kind.
# The namespace is optional. If not specified, the namespace is assumed to be the same as the namespace of the object.
# The name is required.
argocd.argoproj.io/sync-dependencies: a,b,c

Reasons to do this:

  • The 3rd most popular enhancement request for Argo CD
  • Explanation can be found in argo-cd#3517.
  • Implementation is contained within the GitOps engine.

Reasons not to do this:

  • Impacts core GitOps engine code (must be mitigated by exceptional testing).
  • Another gun to shoot yourself in the foot with.

Open questions:

  • Sync waves, hooks, and dependencies have a certain level of opacity to them. By which I mean, that it is not always clear what they're doing unless you're familiar with the app. I wonder if we should introduce a new "waiting" state for resources?
  • Should dependencies take precedence over waves? I think this is yes.
  • How should dependencies be described? I've used annotation, is that right? Is that the right structure?
  • Are the rules that match tasks to dependencies?

Signed-off-by: Alex Collins <alex_collins@intuit.com>
Copy link
Author

Choose a reason for hiding this comment

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

In syncTasks we need to check for cyclic-dependencies.

pkg/sync/sync_context.go Outdated Show resolved Hide resolved
// delete all completed hooks which have appropriate delete policy
sc.deleteHooks(hooksPendingDeletionSuccessful)
sc.setOperationPhase(common.OperationSucceeded, "successfully synced (all tasks run)")
} else {
sc.setRunningPhase(remainingTasks, false)
sc.setRunningPhase(tasks, false)
Copy link
Author

@alexec alexec Mar 19, 2023

Choose a reason for hiding this comment

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

Looks like a 2+ year old bug.

pkg/sync/sync_context_test.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 19, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (ed70eac) 55.75% compared to head (745fb5a) 55.61%.
Report is 18 commits behind head on master.

❗ Current head 745fb5a differs from pull request most recent head de3f8c9. Consider uploading reports for the commit de3f8c9 to get more accurate results

Files Patch % Lines
pkg/sync/sync_context.go 64.28% 4 Missing and 1 partial ⚠️
pkg/sync/sync_tasks.go 94.73% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #514      +/-   ##
==========================================
- Coverage   55.75%   55.61%   -0.15%     
==========================================
  Files          41       42       +1     
  Lines        4525     4531       +6     
==========================================
- Hits         2523     2520       -3     
- Misses       1808     1817       +9     
  Partials      194      194              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Alex Collins <alex_collins@intuit.com>
alexec added 2 commits March 19, 2023 18:14
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Comment on lines +789 to +790
sc.log.WithValues("tasks", tasks).V(1).Info("tasks after sorting")

Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
sc.log.WithValues("tasks", tasks).V(1).Info("tasks after sorting")

@alexec alexec requested review from alexmt and jessesuen March 20, 2023 16:12
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
8.0% 8.0% Duplication

@alexec alexec closed this Mar 30, 2023
@alexec alexec reopened this Feb 19, 2024
Copy link

Quality Gate Passed Quality Gate passed

Issues
3 New issues

Measures
0 Security Hotspots
No data about Coverage
8.0% Duplication on New Code

See analysis details on SonarCloud

@reggie-k
Copy link
Member

I wonder whether we can hit a scenario of a dependent resource that has an auto-generated and thus unknown ahead of time name?

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.

2 participants