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

Optimise clone operations #665

Merged
merged 10 commits into from
May 11, 2022
Merged

Optimise clone operations #665

merged 10 commits into from
May 11, 2022

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented Apr 12, 2022

No-op reconciliations are very inefficient, as they carry out a full (or shallow for go-git) clone of the target repository even when no changes have taken place.

The PR aims to rather optimise the process by first checking whether the last revision is still the same at the target repository, and if that is so, short-circuit the reconciliation. If not, reuse the same connection to fetch and reset the repository to the upstream HEAD.

This is an opt-out feature, which can be disabled by starting the controller with the argument --feature-gates=OptimizedGitClones=false.

Remaining tasks:

  • libgit2 (all checkout variants)
  • go-git (all checkout variants)
  • review documentation
  • Create feature flag (should this be enabled via flag?)
  • Fix darkowlzz recommendations on controller

pkg/git/libgit2/checkout.go Outdated Show resolved Hide resolved
@pjbgf pjbgf added this to the GA milestone Apr 26, 2022
@pjbgf pjbgf added the area/git Git related issues and pull requests label Apr 27, 2022
Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

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

LGTM! ✨ 🚀

@hiddeco
Copy link
Member

hiddeco commented May 10, 2022

Have been thinking some more about putting this behind a feature flag @pjbgf. False-positives would not be that much of an issue, as we are optimizing :-). But locking in unexpected ways (and thereby not moving forward anymore), would be an issue. (Although by looking at the code, this does not seem very plausible.)

But to better be safe than sorry, let's feature flag it for at least one minor range. Then review as follow up on next next minor.


I have been thinking about the growth of feature flags, which then later need to be removed (and deprecated) from the arguments. For the Git improvement, we used an environment variable for this. However, I am wondering we couldn't (also) add a flag accepting an array of items. We could then iterate over the array, enabling the feature flags for that release. Any flag that is no longer marked as experimental can just stops being detected (but logged to encourage removal), without breaking the further Deployment if the user does not update their config.

This would allow for a less verbose declarative configuration, as environment variables in YAML are lengthy and nested.

@hiddeco hiddeco added enhancement New feature or request experimental Issues and pull requests related to experimental features labels May 10, 2022
@stefanprodan
Copy link
Member

However, I am wondering we couldn't (also) add a flag accepting an array of items.

We could use the same convention as Kubernetes uses for feature gates https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/

Paulo Gomes and others added 7 commits May 11, 2022 11:40
No-op reconciliations are very inefficient, as they carry out
a full clone operation of the target repository even when
no changes have taken place.

This change will execute a remote-ls operation, and cancel
the clone operation if the remote tip commit is still the same
as the one observed on the last reconcilation. In such cases,
an git.NoChangesError is returned.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
…luxcd#694)

* Check if revision has changed in gogit CheckoutBranch

Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
getBlankRepoAndRemote's callers are responsible for the disposal
of the returned objects. However, the caller does not expect to
need to dispose objects when err != nil, which may result to memory
leaks.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
@pjbgf pjbgf marked this pull request as ready for review May 11, 2022 11:42
@pjbgf
Copy link
Member Author

pjbgf commented May 11, 2022

Added the feature-gate and done some other housekeeping bits. PTAL

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
@stefanprodan
Copy link
Member

I think it should be --feature-gates=GitOptimizedClones=true to follow Kubernetes conventions:
https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/#feature-gates-for-alpha-or-beta-features

@pjbgf
Copy link
Member Author

pjbgf commented May 11, 2022

I think it should be --feature-gates=GitOptimizedClones=true to follow Kubernetes conventions:
https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/#feature-gates-for-alpha-or-beta-features

@stefanprodan the feature gate is managed via --feature-gates=OptimizedGitClones=true.

OptimizedGitClones decreases resource utilization for GitRepository
reconciliations. It supports both go-git and libgit2 implementations
when cloning repositories using branches or tags.

This is an opt-out feature, which can be disabled by starting the
controller with the argument '--feature-gates=OptimizedGitClones=false'.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @pjbgf 🏅

@stefanprodan stefanprodan removed the experimental Issues and pull requests related to experimental features label May 11, 2022
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

Great optimization! Just a minor comment.

pkg/git/gogit/checkout.go Outdated Show resolved Hide resolved
@makkes
Copy link
Member

makkes commented May 11, 2022

Bonus for using feature gates! 💪🏻

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
@pjbgf pjbgf merged commit 9c1bbc4 into fluxcd:main May 11, 2022
@pjbgf pjbgf deleted the optimise-clone branch May 11, 2022 16:00
@pjbgf
Copy link
Member Author

pjbgf commented May 11, 2022

@aryan9600 @somtochiama @darkowlzz thank you very much for all the help getting this over the line. 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/git Git related issues and pull requests enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants