Skip to content

git: add Depth to CloneConfig - #1329

Open
ruslan-shaydullin wants to merge 1 commit into
fluxcd:mainfrom
ruslan-shaydullin:git-clone-depth
Open

ruslan-shaydullin wants to merge 1 commit into
fluxcd:mainfrom
ruslan-shaydullin:git-clone-depth

Conversation

@ruslan-shaydullin

Copy link
Copy Markdown

CloneConfig only offers ShallowClone, which clones a single commit. A consumer that needs a bounded amount of history has to choose between one commit and the full history.

This adds CloneConfig.Depth: the number of commits to clone along each parent chain from the tip of a branch, tag or semver tag. It takes precedence over ShallowClone, which keeps its meaning of depth 1, and a negative value is rejected before any remote operation. Checkouts by commit and by reference names that are neither a branch nor a tag are not touched here. Without Depth nothing changes and the existing tests pass as they are.

The consumer is a change in source-controller for fluxcd/source-controller#2010: a .spec.verify.forwardOnly option on GitRepository that rejects a revision which does not descend from the last verified one. To find the previous revision the controller clones the last 100 commits instead of the tip and, if that is not enough, discards the clone and clones the last 500 into a fresh directory. Fetching more history into the existing clone was ruled out by the comment in client.go and by the outcome of #1289: a fresh clone works on every provider. A draft PR in source-controller follows once this one has a number.

The next step on the same field is fluxcd/source-controller#2146, applying Depth to a checkout by commit when a branch is set. That is why this PR pins no behaviour for commit checkouts.

One question for review: the ancestry walk itself (breadth first over both parents of a merge, tolerant of shallow boundaries) currently lives in the controller. If you would rather have it in git/gogit, I have a variant ready with an exported FindAncestor(path, head, ancestor git.Hash, limit int) and can switch the same day.

A note from measuring this client on argoproj/argo-cd (11.7k commits) with go-git v5.19.3, which may interest you: the peak memory of a shallow clone grows with the depth, from 0.75 GiB at depth 1 to 1.7 GiB at 500 and 2.1 GiB at 1000, and from a depth of roughly 150 on it exceeds a full clone (1.1 GiB), while traffic and disk stay far below it (62 MB at 500 against 174 MB). The controller sizes its two depths with that in mind.

Tests: TestClone_depth covers branch, tag, semver and refs/heads/* and refs/tags/* names with Depth: 2 on three commits (exactly two arrive, the third gives ErrObjectNotFound, one entry in .git/shallow), Depth without ShallowClone, ShallowClone without Depth, and a negative depth that creates nothing. make test-git passes.

CloneConfig only offers ShallowClone, which clones a single commit.
Consumers that need a bounded amount of history, such as a controller
checking whether a new revision descends from the last verified one,
had to choose between a single commit and the full history.

Depth limits a branch, tag or semver clone to the given depth from the
tip and takes precedence over ShallowClone, which keeps its meaning of
depth 1. This change does not touch checkouts by commit or by other
reference names. A negative depth is rejected before any remote
operation.

Signed-off-by: Ruslan Shaydullin <shaydullin.r.d@outlook.com>
@bb-Ricardo

Copy link
Copy Markdown
Contributor

Thank you very much for putting the effort in to get this implemented.

I'm just a bit worried that the clone depth of, for example, 500 would exhaust the configured controllers memory limit and trigger an OOM eviction if a special crafted git history would need to be cloned and checked.

I was kind of waiting that the go-git partial clone would make progress and land in v6. This would be the most efficient way to clone the history without the binary blobs as they are not needed for signature verification.

What is your opinion on that?

@ruslan-shaydullin

Copy link
Copy Markdown
Author

Thanks @bb-Ricardo, and thanks for the groundwork in #2010. The two options you listed there were a configurable depth or a partial clone. This PR is the first one, except the depth isn't left to the user.

I looked at partial clone first. pkg is on go-git v5.19.3, which has no Filter in CloneOptions. v6.0.0-beta.1 has it, but fetching promised objects on demand isn't implemented yet, and go-git/go-git#1381 was dropped from the v6 milestone at the start of October. So waiting for it means waiting for a stable v6 and for Flux to move to it. When that happens the two should combine well: a depth-limited blob:none clone would bring only commits and trees, and Depth would still bound how far back the controller looks.

You're right about memory, and that's why the source-controller side (fluxcd/source-controller#2188) is bounded the way it is. The numbers in the description are from argo-cd: 0.75 GiB peak at depth 1 and 1.7 GiB at 500. The controller clones 100 commits first. It clones 500 only when the last verified revision isn't within those 100, and only after the signature of the new tip has been verified with a trusted key, so a crafted history pushed without a trusted key never gets the deeper clone. A baseline more than 500 commits back is rejected without cloning further, and while the reference still points at the rejected revision it's retried without a clone. The option is off unless it's set on a GitRepository.

Would that be enough for you, or would you prefer a lower second depth, say 300? I'm happy to move the deep clone to blob:none once partial clones are usable in go-git.

@bb-Ricardo

Copy link
Copy Markdown
Contributor

On the other hand, this is "only" implementing the possibility to specify the clone depth.
The controller can decide about the desired depth when calling the clone methods.

For me this would be fine for now but I'm not a member of this project.
This would need to be decided by the maintainers.

And once this is merged (hopefully) we should straight away create a follow up issue to implement
partial clone option once this lends in the upstream go-git project, just to not forget about it
and improve performance.

@ruslan-shaydullin

Copy link
Copy Markdown
Author

Agreed. Once this is merged I'll open the follow-up issue for partial clone and link go-git/go-git#1381, so it gets picked up when that lands.

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.

2 participants