-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix(appset): Fix perpetual appset reconciliation #19822
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #19822 +/- ##
=========================================
Coverage ? 55.84%
=========================================
Files ? 320
Lines ? 44381
Branches ? 0
=========================================
Hits ? 24786
Misses ? 17027
Partials ? 2568 ☔ View full report in Codecov by Sentry. |
d83db00
to
998da98
Compare
I see there is a license check problem, if I read correctly, the problem also appears on other PRs, is there anything needed from us to fix it? |
6aed467
to
aa67e2d
Compare
Golang maps do not guarantee the order of the application resources from the applicationset which causes rapid sync activity for the applicationset as the objects and hence their resourceVersions are updated after each reconcile loop. This then triggers reconciliation of all objects watching the ApplicationSet. In order to prevent this behaviour, ensure that the ApplicationSet reconciler provides an idempotent list of resources, ensuring objects are not updated. Fixes: argoproj#19757 Co-authored-by: Fabian Selles <fabian.sellesrosa@gmail.com> Co-authored-by: Ariadna Rouco <ariadna.rouco@adevinta.com> Signed-off-by: Thibault Jamet <thibault.jamet@adevinta.com> Signed-off-by: Fabián Sellés <fabian.selles@adevinta.com>
aa67e2d
to
d39b7c7
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.
Looks good to me 👍 Thanks for implementing a fix for #19757
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
Would be great to get a patch release for this, it is very noisy inside our etcd 😓 |
yes please! 👆 🙏 |
/cherry-pick release-2.12 |
Golang maps do not guarantee the order of the application resources from the applicationset which causes rapid sync activity for the applicationset as the objects and hence their resourceVersions are updated after each reconcile loop. This then triggers reconciliation of all objects watching the ApplicationSet. In order to prevent this behaviour, ensure that the ApplicationSet reconciler provides an idempotent list of resources, ensuring objects are not updated. Fixes: #19757 Signed-off-by: Thibault Jamet <thibault.jamet@adevinta.com> Signed-off-by: Fabián Sellés <fabian.selles@adevinta.com> Co-authored-by: Fabian Selles <fabian.sellesrosa@gmail.com> Co-authored-by: Ariadna Rouco <ariadna.rouco@adevinta.com>
Golang maps do not guarantee the order of the application resources from the applicationset which causes rapid sync activity for the applicationset as the objects and hence their resourceVersions are updated after each reconcile loop. This then triggers reconciliation of all objects watching the ApplicationSet. In order to prevent this behaviour, ensure that the ApplicationSet reconciler provides an idempotent list of resources, ensuring objects are not updated. Fixes: #19757 Signed-off-by: Thibault Jamet <thibault.jamet@adevinta.com> Signed-off-by: Fabián Sellés <fabian.selles@adevinta.com> Co-authored-by: Thibault Jamet <tjamet@users.noreply.github.com> Co-authored-by: Fabian Selles <fabian.sellesrosa@gmail.com> Co-authored-by: Ariadna Rouco <ariadna.rouco@adevinta.com>
ScmProvider does not guarantee order of the application resources from the applicationset which causes rapid sync activity for the applicationset as the objects and hence their resourceVersions are updated after each reconcile loop.
This then triggers reconciliation of all objects watching the ApplicationSet.
In order to prevent this behaviour, ensure that the ApplicationSet reconciler provides an idempotent list of resources, ensuring objects are not updated.
Fixes: #19757
This PR is an alternate approach to #19676.
In #19676, the approach is to only reconcile based on generation changes, i.e.
spec
changes as defined by the generation implementation in CRDs.This means that, with this PR, any change of metadata (labels, annotations, ...) or status will be taken into consideration on the next run (triggered by requeueAfter, 3mn by default in my trials), which is the way new SCM repos or folders are discovered from my understanding.
This PR takes a different approach, ensuring that status updates are idempotent across reconcile loops and hence prevents from triggering any reconciliation in other reconcilers unless there is an actual change on the ApplicationSet.
Checklist: