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

fix(appset): Don't use revision cache when reconciling after webhook (#16062) #16241

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

dhruvang1
Copy link
Contributor

@dhruvang1 dhruvang1 commented Nov 5, 2023

Repo Server is using ref caching in GetGitFiles which prevents AppSet from seeing the new commit. There are few approaches to solve it:

  • Disable Ref caching for GetGitFiles.
    • For each AppSet reconciliation, both from webhook & periodic sync, poll the Git Server to return the latest refs.
  • Disable Ref caching only for webhook requests (Implemented here)
    • Add noRevisionCache parameter in GitFilesRequest. On reconciliation triggered from webhook, pass noRevisionCache: true to repo server to force the Git Server polling to get the latest commit.
  • Explicitly pass the commit sha in request (complex as an AppSet can have multiple git generator)
    • We store the commit sha obtained from the webhook in AppSet annotation. On next reconciliation, pass the commit sha as revision. Repo server has the logic to handle commit sha and no changes are required there.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.

Fixes #16062

…cile (argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
…rgoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
@dhruvang1 dhruvang1 force-pushed the dhruvang/fix-appset-webhook branch from 8569613 to 95ec130 Compare November 7, 2023 01:58
@dhruvang1 dhruvang1 changed the title fix(appset): store sha from webhook to get latest change during reconcile (#16062) fix(appset): Don't use revision cache when reconciling after webhook (#16062) Nov 7, 2023
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (58da6a3) 49.51% compared to head (95ec130) 49.52%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #16241   +/-   ##
=======================================
  Coverage   49.51%   49.52%           
=======================================
  Files         269      269           
  Lines       46990    46995    +5     
=======================================
+ Hits        23269    23274    +5     
  Misses      21437    21437           
  Partials     2284     2284           
Files Coverage Δ
applicationset/generators/git.go 84.50% <100.00%> (+0.14%) ⬆️
applicationset/services/repo_service.go 88.88% <100.00%> (+0.42%) ⬆️
reposerver/repository/repository.go 60.12% <100.00%> (+0.02%) ⬆️

... and 1 file with indirect coverage changes

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

@dhruvang1 dhruvang1 marked this pull request as ready for review November 7, 2023 02:11
@dhruvang1 dhruvang1 requested a review from a team as a code owner November 7, 2023 02:11
@ericblackburn
Copy link
Contributor

LGTM, but I am not a Code Owner

@dhruvang1
Copy link
Contributor Author

@crenshaw-dev could you take a look when you are available.

@Laakso
Copy link

Laakso commented Nov 23, 2023

Awesome! Thanks for the fix! We've been waiting for this :) ❤️

Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

LGTM!

@alexmt alexmt merged commit 7408292 into argoproj:master Dec 4, 2023
27 checks passed
@alexmt
Copy link
Collaborator

alexmt commented Dec 5, 2023

/cherry-pick release-2.9

@alexmt
Copy link
Collaborator

alexmt commented Dec 5, 2023

/cherry-pick release-2.8

gcp-cherry-pick-bot bot pushed a commit that referenced this pull request Dec 5, 2023
…16062) (#16241)

* fix(appset): store sha from webhook to get latest change during reconcile (#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

* fix(appset): Don't use revision cache when reconciling after webhook(#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

---------

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
Copy link

Cherry-pick failed with Merge error 7408292bb058fcb1feb62b27cc7b348b2a4b2c68 into temp-cherry-pick-680fdf-release-2.8

alexmt pushed a commit that referenced this pull request Dec 5, 2023
…16062) (#16241) (#16536)

* fix(appset): store sha from webhook to get latest change during reconcile (#16062)



* fix(appset): Don't use revision cache when reconciling after webhook(#16062)



---------

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
Co-authored-by: Dhruvang Makadia <dhruvang1@users.noreply.github.com>
alexmt pushed a commit to alexmt/argo-cd that referenced this pull request Dec 5, 2023
…rgoproj#16062) (argoproj#16241)

* fix(appset): store sha from webhook to get latest change during reconcile (argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

* fix(appset): Don't use revision cache when reconciling after webhook(argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

---------

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
alexmt added a commit that referenced this pull request Dec 5, 2023
…16062) (#16241) (#16543)

* fix(appset): store sha from webhook to get latest change during reconcile (#16062)



* fix(appset): Don't use revision cache when reconciling after webhook(#16062)



---------

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
Co-authored-by: Dhruvang Makadia <dhruvang1@users.noreply.github.com>
vladfr pushed a commit to vladfr/argo-cd that referenced this pull request Dec 13, 2023
…rgoproj#16062) (argoproj#16241)

* fix(appset): store sha from webhook to get latest change during reconcile (argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

* fix(appset): Don't use revision cache when reconciling after webhook(argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

---------

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
tesla59 pushed a commit to tesla59/argo-cd that referenced this pull request Dec 16, 2023
…rgoproj#16062) (argoproj#16241)

* fix(appset): store sha from webhook to get latest change during reconcile (argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

* fix(appset): Don't use revision cache when reconciling after webhook(argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

---------

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
@jmalloc
Copy link
Contributor

jmalloc commented Jan 2, 2024

Any chance of getting a tag of 2.9.x that includes this fix? 🙏

@pre pre mentioned this pull request Jan 30, 2024
3 tasks
JulienFuix pushed a commit to JulienFuix/argo-cd that referenced this pull request Feb 6, 2024
…rgoproj#16062) (argoproj#16241)

* fix(appset): store sha from webhook to get latest change during reconcile (argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

* fix(appset): Don't use revision cache when reconciling after webhook(argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

---------

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
@jolet
Copy link

jolet commented Mar 25, 2024

hi we are experiencing same issue with AppSets not reconciliating new Apps, currently we found temp fix is to do manual hard refresh on any existing App from AppSet which triggers reconcilliation immediately, but thats cumbersome and prone to pointing teams to overuse hard refresh for everything

lyda pushed a commit to lyda/argo-cd that referenced this pull request Mar 28, 2024
…rgoproj#16062) (argoproj#16241)

* fix(appset): store sha from webhook to get latest change during reconcile (argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

* fix(appset): Don't use revision cache when reconciling after webhook(argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

---------

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
Signed-off-by: Kevin Lyda <kevin@lyda.ie>
Hariharasuthan99 pushed a commit to AmadeusITGroup/argo-cd that referenced this pull request Jun 16, 2024
…rgoproj#16062) (argoproj#16241)

* fix(appset): store sha from webhook to get latest change during reconcile (argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

* fix(appset): Don't use revision cache when reconciling after webhook(argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

---------

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
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.

ApplicationSet not generating new Application on webhook with Git file generator
6 participants