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: use 'git show-ref' to both retrieve and store generated manifests #3578

Merged
merged 2 commits into from
May 13, 2020

Conversation

alexmt
Copy link
Collaborator

@alexmt alexmt commented May 13, 2020

Closes #3577

@alexmt alexmt requested review from jannfis and jessesuen May 13, 2020 01:56
@codecov
Copy link

codecov bot commented May 13, 2020

Codecov Report

Merging #3578 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3578      +/-   ##
==========================================
+ Coverage   43.43%   43.44%   +0.01%     
==========================================
  Files         184      184              
  Lines       20390    20392       +2     
  Branches      275      238      -37     
==========================================
+ Hits         8856     8859       +3     
- Misses      10516    10519       +3     
+ Partials     1018     1014       -4     
Impacted Files Coverage Δ
reposerver/repository/repository.go 60.03% <100.00%> (+0.21%) ⬆️
pkg/apis/application/v1alpha1/hack.go 0.00% <0.00%> (ø)
ui/src/app/applications/components/utils.tsx 49.83% <0.00%> (ø)
ui/src/app/shared/services/projects-service.ts 19.71% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd1de6e...cafea0a. Read the comment docs.

Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

Please see my comments in #3577

@alexmt
Copy link
Collaborator Author

alexmt commented May 13, 2020

Thank you for investigation, @jannfis ! I did not know how to even approach it.

I agree that problem is specific to v0.7.0 tag in rollouts repo. However I think we still need to have protection from this edge case.

Current Argo CD behavior is the following:

  • Run git show-ref -s <tag>, get commit a
  • Try to find cached manifests for commit a and get nothing
  • Run git checkout v0.7.1; git rev-parse HEAD, get commit b.
  • Generate manifests using commit b and store result using commit b as cache key. Commit b is used for caching "just-in-case", assuming that it must be the same as a.

Since a and b are different, Argo CD never caches manifests for application pointing to the affected git tag. As a result we have to keep running the expensive generation every couple minutes.

PR changes the last step behavior so that Argo CD uses commit a to cache generated manifests. This would solve the performance issue.

What do you think?

@alexmt
Copy link
Collaborator Author

alexmt commented May 13, 2020

To summarize,

Now Argo CD generates manifests for commit b instead of a ( because of incorrect git metadata ) and never caches results.

I want to cache results to stop the performance issues.

@jannfis
Copy link
Member

jannfis commented May 13, 2020

Understood, and I think you're correct. We should not have a performance penalty or overload on Git servers due to repository issues.

However, I think that we should emit a warning if the commit SHAs for the tag (i.e. show-ref -s <tag>) and the actual HEAD of the tag (i.e. rev-parse HEAD after checkout <tag>) differ - so that there is a chance it gets fixed at the root (because I think it is a "broken" repository)

@alexmt
Copy link
Collaborator Author

alexmt commented May 13, 2020

Nice suggestion! Added warning message.

Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

LGTM. thank you for adding the warning!

@alexmt alexmt merged commit 4f49168 into argoproj:master May 13, 2020
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.

Manifests are incorrectly cached if target revision is tag
2 participants