feat(controller): use manifest generate path during comparison (#14242) #15636

alexymantha · 2023-09-22T15:22:23Z

Adds a new function to the repo-server that compares 2 revisions and updates the cache with the new revision if no changes are detected. This leverages the manifest-generate-paths annotation that is already used by the webhook to determine what should be compared. The behaviour of doing the comparison and updating the cache is very similar to what is done with the webhooks.

The goal is to avoid unnecessary manifest generation when a new commit does not change any files in an application, it will reuse the already generated manifest instead.

The implementation is based on what was discussed here: https://cloud-native.slack.com/archives/C04SURUPDL2/p1686849819036059

Closes #14242

Checklist:

codecov · 2023-09-22T15:57:34Z

Codecov Report

Attention: Patch coverage is 81.87500% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 49.35%. Comparing base (f87897c) to head (37fdd8f).
Report is 18 commits behind head on master.

❗ Current head 37fdd8f differs from pull request most recent head 755bdc1. Consider uploading reports for the commit 755bdc1 to get more accurate results

Files	Patch %	Lines
reposerver/repository/repository.go	67.90%	19 Missing and 7 partials ⚠️
controller/state.go	85.71%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #15636      +/-   ##
==========================================
- Coverage   49.73%   49.35%   -0.38%     
==========================================
  Files         274      274              
  Lines       48948    48282     -666     
==========================================
- Hits        24343    23831     -512     
+ Misses      22230    22102     -128     
+ Partials     2375     2349      -26

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

crenshaw-dev · 2023-09-25T22:52:08Z

This is looking very cool btw. lmk if you need any help!

reposerver/repository/repository.go

alexymantha · 2023-09-27T20:43:41Z

util/webhook/webhook.go

@@ -364,70 +364,6 @@ func (a *ArgoCDWebhookHandler) storePreviouslyCachedManifests(app *v1alpha1.Appl
 	return nil
 }

-func getAppRefreshPaths(app *v1alpha1.Application) []string {


Moved to the app/path so it can be reused in the repo-server

alexymantha · 2023-09-27T21:00:04Z

Thanks @crenshaw-dev! I think it's ready for a first round of reviews

I'm also working on gathering some stats to see the performance improvements that this provides for monorepos. I will update as soon as I have some data

reposerver/repository/repository.proto

reposerver/repository/repository.go

reposerver/repository/repository_test.go

reposerver/repository/repository.go

Signed-off-by: Alexy Mantha <alexy@mantha.dev>

pasha-codefresh

Checked it locally, works really fine! Great job!

agaudreault · 2024-02-28T15:32:59Z

reposerver/repository/repository.go

+	err := s.cache.SetNewRevisionManifests(newRev, oldRev, request.ApplicationSource, request.RefSources, request, request.Namespace, request.TrackingMethod, request.AppLabelKey, request.AppName, repoRefs)
+	if err != nil {
+		if err == cache.ErrCacheMiss {
+			log.WithFields(log.Fields{"application": request.AppName, "appNamespace": request.Namespace}).Debugf("manifest cache miss during comparison for application %s in repo %s from revision %s", request.AppName, request.GetRepo().Repo, oldRev)


I was running some test locally and I got the following error.

The test was

create an app with the annotation and without auto-sync

Sync it to HEAD (08484ff536ba26aecf0103476e223297459c2fd3)

Commit to HEAD outside the path (4bf66a93e785b7af4e56d33ec13cbf344072a8b8)

Manual Refresh (15:02:57)

Manual Sync (15:07:04)

argocd-repo-server time="2024-02-28T15:02:57Z" level=debug msg="manifest cache updated for application app2 in repo https://github.com/agaudreault/gitops-tests from revision 08484ff536ba26aecf0103476e223297459c2fd3 to revision 4bf66a93e785b7af4e56d33ec13cbf344072a8b8" appNamespace=dev application=app2 argocd-repo-server time="2024-02-28T15:07:04Z" level=debug msg="no changes found for application app2 in repo https://github.com/agaudreault/gitops-tests from revision 08484ff536ba26aecf0103476e223297459c2fd3 to revision 4bf66a93e785b7af4e56d33ec13cbf344072a8b8" appNamespace=dev application=app2 argocd-repo-server time="2024-02-28T15:07:04Z" level=warning msg="error updating cached revision for repo https://github.com/agaudreault/gitops-tests with revision 4bf66a93e785b7af4e56d33ec13cbf344072a8b8: manifest cache move error for app2: ERR no such key" appNamespace=dev application=app2

I wonder if we should handle the warning in a different way. From what I understand, it will warn for cache miss as long as the app differs from the HEAD, which will be a lot.

The overall behaviour works as expected and the manifests are not regenerated during the manual refresh or sync.
The application status.operationState.sync.revision will be 4bf66a, but the value of status.operationState.syncResult.revision and status.sync.revision will be 08484f (what is displayed in the UI)

Hmm you're right, that was previously caught by err == cache.ErrCacheMiss but digging into the code it looks like, unlike the Get, the new Rename for the Redis cache does not handle the cache miss and instead returns the raw Redis error. It seems to be an inconsistency because the other caches (like the in-memory) do return ErrCacheMiss on a miss, I'll see if I can update the Redis cache to handle the miss properly

Added the cache miss in cache/redis.go

agaudreault · 2024-03-01T16:01:26Z

controller/state.go

+			_, err = repoClient.UpdateRevisionForPaths(context.Background(), &apiclient.UpdateRevisionForPathsRequest{
+				Repo:               repo,
+				Revision:           revisions[i],
+				SyncedRevision:     app.Status.Sync.Revision,


When it is multi-source, app.Status.Sync.Revision[i] should be used everywhere in the loop.

reposerver/repository/repository.go

Signed-off-by: Alexy Mantha <alexy@mantha.dev>

controller/state.go

reposerver/repository/repository.go

Signed-off-by: Alexy Mantha <alexy@mantha.dev>

crenshaw-dev

Just a few small things, I think. Testing now.

crenshaw-dev · 2024-04-04T19:10:06Z

reposerver/repository/repository.proto

+}
+
+message UpdateRevisionForPathsResponse {
+    bool changes = 1;


I'm wondering if we should just drop this field. It's not used anywhere besides the unit test. If what we really need from the API call is the side effect (updating the cache key), then we should probably just test for the side-effect.

crenshaw-dev · 2024-04-04T19:11:30Z

reposerver/repository/repository.go

+	}
+
+	// Update revision in refSource
+	if request.HasMultipleSources && request.ApplicationSource.Helm != nil {


Suggested change

}

// Update revision in refSource

if request.HasMultipleSources && request.ApplicationSource.Helm != nil {

// Update revision in refSource

Can we simplify like this?

crenshaw-dev · 2024-04-04T19:13:14Z

util/git/client.go

+// returns the meta-data for the commit
+func (m *nativeGitClient) ChangedFiles(revision string, targetRevision string) ([]string, error) {


Suggested change

// returns the meta-data for the commit

func (m *nativeGitClient) ChangedFiles(revision string, targetRevision string) ([]string, error) {

// ChangedFiles returns a list of files changed between two revisions

func (m *nativeGitClient) ChangedFiles(revision string, targetRevision string) ([]string, error) {

crenshaw-dev

Something else I noticed is that the feature won't work for skipping unrelated changes in external Helm values files. The referenced git repo's source revision is part of the Helm chart source's cache key, so when that SHA changes, the Helm chart source contents get re-generated. The Helm chart source generation code doesn't make any accommodations for the manifest-generate-path annotation.

I'll merge for now to get the change into 2.11 (overall it looks and behaves great). I'd just request a follow-up PR for the nitpicks (if they're accurate) and a docs PR to note that the shortcut doesn't work for external Helm values files.

…17761) * cleanup Signed-off-by: Alexy Mantha <alexy@mantha.dev> * update docs Signed-off-by: Alexy Mantha <alexy@mantha.dev> --------- Signed-off-by: Alexy Mantha <alexy@mantha.dev>

…roj#14242) (argoproj#15636) * squash commits Signed-off-by: Alexy Mantha <alexy@mantha.dev> * Update util/git/client.go Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fix error message Signed-off-by: Alexy Mantha <alexy@mantha.dev> * add git client options Signed-off-by: Alexy Mantha <alexy@mantha.dev> * Update generated code Signed-off-by: Alexy Mantha <alexy@mantha.dev> * run fmt Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fix tests Signed-off-by: Alexy Mantha <alexy@mantha.dev> * failed gen Signed-off-by: Alexy Mantha <alexy@mantha.dev> * tweak logs and rename cache Signed-off-by: Alexy Mantha <alexy@mantha.dev> * validate revisions Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fix tests Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fix tests Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fmt Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fix linting Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fixes from review Signed-off-by: Alexy Mantha <alexy@mantha.dev> * generate Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fix Signed-off-by: Alexy Mantha <alexy@mantha.dev> * use log context Signed-off-by: Alexy Mantha <alexy@mantha.dev> --------- Signed-off-by: Alexy Mantha <alexy@mantha.dev> Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

…#15636) (argoproj#17761) * cleanup Signed-off-by: Alexy Mantha <alexy@mantha.dev> * update docs Signed-off-by: Alexy Mantha <alexy@mantha.dev> --------- Signed-off-by: Alexy Mantha <alexy@mantha.dev>

…roj#14242) (argoproj#15636) * squash commits Signed-off-by: Alexy Mantha <alexy@mantha.dev> * Update util/git/client.go Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fix error message Signed-off-by: Alexy Mantha <alexy@mantha.dev> * add git client options Signed-off-by: Alexy Mantha <alexy@mantha.dev> * Update generated code Signed-off-by: Alexy Mantha <alexy@mantha.dev> * run fmt Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fix tests Signed-off-by: Alexy Mantha <alexy@mantha.dev> * failed gen Signed-off-by: Alexy Mantha <alexy@mantha.dev> * tweak logs and rename cache Signed-off-by: Alexy Mantha <alexy@mantha.dev> * validate revisions Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fix tests Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fix tests Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fmt Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fix linting Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fixes from review Signed-off-by: Alexy Mantha <alexy@mantha.dev> * generate Signed-off-by: Alexy Mantha <alexy@mantha.dev> * fix Signed-off-by: Alexy Mantha <alexy@mantha.dev> * use log context Signed-off-by: Alexy Mantha <alexy@mantha.dev> --------- Signed-off-by: Alexy Mantha <alexy@mantha.dev> Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

…#15636) (argoproj#17761) * cleanup Signed-off-by: Alexy Mantha <alexy@mantha.dev> * update docs Signed-off-by: Alexy Mantha <alexy@mantha.dev> --------- Signed-off-by: Alexy Mantha <alexy@mantha.dev>

crenshaw-dev mentioned this pull request Sep 27, 2023

auto-sync on monorepo effectively equals to self-heal #10679

Open

3 tasks

alexymantha force-pushed the enforce-manifest-generate-path branch 6 times, most recently from 3c917f8 to 89023d6 Compare September 27, 2023 20:35

alexymantha commented Sep 27, 2023

View reviewed changes

reposerver/repository/repository.go Outdated Show resolved Hide resolved

alexymantha commented Sep 27, 2023

View reviewed changes

alexymantha marked this pull request as ready for review September 27, 2023 20:58

alexymantha requested review from a team as code owners September 27, 2023 20:58

agaudreault reviewed Sep 28, 2023

View reviewed changes

reposerver/repository/repository.proto Outdated Show resolved Hide resolved

agaudreault reviewed Sep 28, 2023

View reviewed changes

reposerver/repository/repository.go Outdated Show resolved Hide resolved

agaudreault reviewed Sep 28, 2023

View reviewed changes

reposerver/repository/repository_test.go Outdated Show resolved Hide resolved

agaudreault reviewed Sep 28, 2023

View reviewed changes

reposerver/repository/repository.go Show resolved Hide resolved

agaudreault approved these changes Sep 29, 2023

View reviewed changes

alexymantha force-pushed the enforce-manifest-generate-path branch from 3c57949 to af8b9bd Compare October 3, 2023 18:17

alexymantha requested a review from a team as a code owner October 3, 2023 18:17

alexymantha force-pushed the enforce-manifest-generate-path branch 7 times, most recently from 766c74c to b3dfd50 Compare October 3, 2023 18:29

squash commits

4be21fc

Signed-off-by: Alexy Mantha <alexy@mantha.dev>

alexymantha requested a review from crenshaw-dev February 26, 2024 23:31

pasha-codefresh approved these changes Feb 27, 2024

View reviewed changes

agaudreault reviewed Feb 28, 2024

View reviewed changes

agaudreault reviewed Mar 1, 2024

View reviewed changes

alexymantha added 2 commits March 28, 2024 14:37

Merge branch 'master' into enforce-manifest-generate-path

5fab168

fixes from review

8aac2af

Signed-off-by: Alexy Mantha <alexy@mantha.dev>

alexymantha force-pushed the enforce-manifest-generate-path branch from a4c576a to 8aac2af Compare March 28, 2024 22:28

alexymantha added 2 commits March 29, 2024 10:58

Merge branch 'master' into enforce-manifest-generate-path

6d3aeb0

generate

9ff1ddf

Signed-off-by: Alexy Mantha <alexy@mantha.dev>

ishitasequeira reviewed Apr 1, 2024

View reviewed changes

controller/state.go Outdated Show resolved Hide resolved

reposerver/repository/repository.go Outdated Show resolved Hide resolved

alexymantha added 3 commits April 2, 2024 10:55

fix

c818325

Signed-off-by: Alexy Mantha <alexy@mantha.dev>

Merge branch 'master' into enforce-manifest-generate-path

99a33bf

use log context

91ef6e0

Signed-off-by: Alexy Mantha <alexy@mantha.dev>

alexymantha requested review from agaudreault and ishitasequeira April 2, 2024 19:08

Merge branch 'master' into enforce-manifest-generate-path

755bdc1

crenshaw-dev requested changes Apr 4, 2024

View reviewed changes

crenshaw-dev approved these changes Apr 4, 2024

View reviewed changes

crenshaw-dev merged commit 4e46a5e into argoproj:master Apr 4, 2024

alexymantha mentioned this pull request Apr 6, 2024

chore: cleanup follow up for enforce manifest generate path (#15636) #17761

Merged

14 tasks

codecae mentioned this pull request Apr 19, 2024

Changes to webhook.go prevent webhook events from Azure DevOps from triggering application refresh. #17903

Closed

This was referenced Jun 21, 2024

Bug: Webhook trigger from Bitbucket does not refresh Applications in 2.11.0 #18289

Closed

Monorepo deployment considerations with declarative GitOps using ArgoCD #9472

Closed

agaudreault mentioned this pull request Jul 29, 2024

REQUEST: Promotion to Approver for @agaudreault argoproj/argoproj#310

Closed

6 tasks

toyamagu-2021 mentioned this pull request Dec 1, 2024

Sync clash with failed to diff ... git diff ... when using ManifestGeneratePaths and syncedRevision proceeds targetRevision #21014

Closed

3 tasks

		// returns the meta-data for the commit
		func (m *nativeGitClient) ChangedFiles(revision string, targetRevision string) ([]string, error) {

feat(controller): use manifest generate path during comparison (#14242) #15636

feat(controller): use manifest generate path during comparison (#14242) #15636

Uh oh!

Conversation

alexymantha commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

crenshaw-dev commented Sep 25, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexymantha commented Sep 27, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pasha-codefresh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crenshaw-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crenshaw-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexymantha commented Sep 22, 2023 •

edited

Loading

codecov bot commented Sep 22, 2023 •

edited

Loading