-
Notifications
You must be signed in to change notification settings - Fork 5k
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: disable reconciliation if timeout.reconciliation is set to 0 #6406
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6406 +/- ##
=======================================
Coverage 41.01% 41.01%
=======================================
Files 152 152
Lines 20088 20088
=======================================
Hits 8239 8239
Misses 10716 10716
Partials 1133 1133 Continue to review full report at Codecov.
|
var resyncDuration time.Duration | ||
if appResyncPeriod == 0 { | ||
// Re-sync should be disabled if period is 0. Set duration to a very long duration | ||
resyncDuration = time.Hour * 24 * 365 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at the code, but how does the controller figure out when to reconcile again? I am assuming it takes the timestamp from application's .status.reconciledAt
and checks whether now - reconciledAt > resyncDuration
, right?
I am wondering whether it's sufficient to set the resyncDuration
to a year. I am imagining the following (admittedly, very far edge) scenario:
- user has 1000 apps that get synced once
- user disables resync
- no further syncs happen (e.g. applications were synced once, never changed)
- after a year, appSyncPeriod expires and apps will get resynced
That might end up in some very nasty surprise. A year is not that long of a period. Maybe we should change it to 100 years? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. 100 years sounds pretty safe :) updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the surprise will be for future generations still using argocd :)
Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
0489ddb
to
54d01a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Signed-off-by: Alexander Matyushentsev AMatyushentsev@gmail.com
Closes #1399
Closes #6357
PR allows disabling app reconciliation if
timeout.reconciliation
setting is set to 0