GitRepositoryReconciler no-op clone improvements #721
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce a new field in the GitRepositoryReconciler to set the enabled features. This makes it test friendly compared to using global flags for setting and checking flags in the tests. Enable default feature gates in all the GitRepo reconciler tests. Signed-off-by: Sunny <[email protected]>
Introduce concrete and partial commits. Concrete commits have all the information from remote including the hash and commit content. Partial commits are based on locally available copy of a repo, they may only contain the commit hash and reference. IsConcreteCommit() can be used to find out if a given commit is based on local information or full remote repo information. Update go-git and libgit2 branch/tag clone optimization to return a partial commit and no error. Update and simplify the go-git and libgit2 tests for the same. Signed-off-by: Sunny <[email protected]>
darkowlzz
added
enhancement
darkowlzz
force-pushed
the
gitrepo-no-op-clone-improv
branch
2 times, most recently
from
aa9edfd to
0fbc735
Compare
Update the GitRepositoryReconciler to attempt git clone optimization only when an artifact is already present in storage and do not skip reconciliation. When a partial commit is received, due to no-op clone, copy the existing artifact to the source build directory and populate all the associated fields in reconcileSource(). This ensure that the reconciliation is not skipped and allows other subreconcilers to operate on other parts of the GitRepo object like include, ignore, etc, when attributes related to them change but the remote repository has not changed. Remove git.NoChangeError as no-op clone now returns a partial commit and no error. Signed-off-by: Sunny <[email protected]>
darkowlzz
force-pushed
the
gitrepo-no-op-clone-improv
branch
from
0fbc735 to
500c668
Compare
|
This is not ready yet. Realized that the existing artifact that's copied contains the final content after processing included and ignored conditions. reconcileSource() needs to write the original source into the source build directory before processing include and ignored conditions. |
pjbgf
reviewed
May 16, 2022
|
Closing in favor of #724 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The no-op git clone implementation introduced some bugs in the GitRepo reconciler due to the way it's currently integrated in the reconciliation loop. Skipping the reconciliation in the middle leaves other critical subreconcilers to not operate on the object. This causes inconsistent behavior and sometimes persistent reconciliation failure.
This change updates the GitRepo reconciler to attempt no-op git clone optimization only when an artifact is already present in the storage. The git packages are also updated to not return error anymore but a partial commit which contains commit hash and reference. On full clone, concrete commit is returned which contains all the commit metadata and content fetched from remote repository. With these, the GitRepo reconciler receives a partial commit when no-op clone takes place. In
reconcileSource()subreconciler, it then populates the source build directory with the existing artifact content and lets the rest of the reconciliation continue. The other subreconcilers then continue building the source with any changes in the GitRepo attributes, like include, ignore, etc, when attributes related to them change but the remote repository has not changed.Previous logs and events related to no-op clone are no longer present by default because we are doing full reconciliation every time now. When log level is set to debug, the no-op clone is logged:
{"level":"debug","ts":"2022-05-15T00:43:26.373+0530","logger":"controller.gitrepository","msg":"no changes since last reconciliation, observed revision '6.1.4/bf09377bfd5d3bcac1e895fa8ce52dc76695c060'","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"GitRepository","name":"podinfo","namespace":"default"} {"level":"info","ts":"2022-05-15T00:43:26.377+0530","logger":"controller.gitrepository","msg":"artifact up-to-date with remote revision: '6.1.4/bf09377bfd5d3bcac1e895fa8ce52dc76695c060'","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"GitRepository","name":"podinfo","namespace":"default"}Failure recovery notification message for no-op clone results in:
as the partial commit doesn't contain the commit message information.
Since it's not ideal to use global flags to set feature gates in the tests, the GitRepositoryReconciler now has a
featuresfield containing all the enabled features. This makes it easier to configure the feature per test without affecting other tests. All the tests now have the default feature gates enabled.These improvements are based on a series of attempts to find and fix the issues mentioned above, see https://github.com/fluxcd/source-controller/compare/gitrepo-rec-fixes?expand=1 . Some of the ideas from there may be added in the future separately.