inrepoconfig: optimize secondary clones #30400

listx · 2023-08-17T00:53:33Z

The only data we need from a clone inrepoconfig repo is the .prow contents (aside: we clone it in the first place so that we can merge to the base branch).

The underlying Git client actually uses 2 clones --- first a mirror clone (git clone --mirror ...), and then a secondary local clone into a temporary folder (git clone ...). This secondary clone is the one we are optimizing here.

This adds 2 optimizations:

(1) Avoid populating object files with "--shared" flag. This way, the .git folder of the secondary clone can just have a pointer to the mirror clone. This saves a lot of disk space for large repos (not to mention that both the CPU time and disk usage for creating this clone is vastly reduced, especially when combined with the next optimization).

(2) Avoid a full checkout with sparse-checkout (supported since Git 2.25). This way, we can avoid deflating Git blob objects into file paths on disk for everything except (a) files at the toplevel and (b) files specified by the "git sparse-checkout set [...]" command. The toplevel files are included as part of "cone mode" which is the recommended mode for sparse checkouts. In our case we specify the ".prow" folder as the only path to make sure we get those inrepoconfig files checked out.

/cc @cjwagner @airbornepony @timwangmusic

k8s-ci-robot · 2023-08-17T00:53:37Z

@listx: GitHub didn't allow me to request PR reviews from the following users: airbornepony.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

The only data we need from a clone inrepoconfig repo is the .prow contents (aside: we clone it in the first place so that we can merge to the base branch).

The underlying Git client actually uses 2 clones --- first a mirror clone (git clone --mirror ...), and then a secondary local clone into a temporary folder (git clone ...). This secondary clone is the one we are optimizing here.

This adds 2 optimizations:

(1) Avoid populating object files with "--shared" flag. This way, the .git folder of the secondary clone can just have a pointer to the mirror clone. This saves a lot of disk space for large repos (not to mention that both the CPU time and disk usage for creating this clone is vastly reduced, especially when combined with the next optimization).

(2) Avoid a full checkout with sparse-checkout (supported since Git 2.25). This way, we can avoid deflating Git blob objects into file paths on disk for everything except (a) files at the toplevel and (b) files specified by the "git sparse-checkout set [...]" command. The toplevel files are included as part of "cone mode" which is the recommended mode for sparse checkouts. In our case we specify the ".prow" folder as the only path to make sure we get those inrepoconfig files checked out.

/cc @cjwagner @airbornepony @timwangmusic

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

listx · 2023-08-17T00:53:50Z

/hold

listx · 2023-08-17T04:07:22Z

/retest

listx · 2023-08-17T16:10:48Z

FTR the unit tests were previously failing because the version of git we use in CI is 2.30.2, which is older and does not exhibit the same behavior as I've seen in, for example 2.41. Notably in 2.30.2 the toplevel paths are not automatically populated in a sparse checkout. I've added a TODO comment for this because I don't think it's easy to update the version of git in CI (we'd have to update the bootstrap image, which uses debian and we know how long debian takes to update). Another solution would be to add a custom apt repo to get a newer git version for the bootstrap image, but the fix I've done here is simpler with virtually no practical difference.

EDIT: Currently the git base image we use for some of our components (e.g. gerrit) uses version 2.32.0 which also exhibits the same "toplevel is not populated by default" behavior.

prow/git/v2/client_factory.go

airbornepony · 2023-08-17T17:51:52Z

prow/config/inrepoconfig.go

+		// We only need ".prow" because the default "cone mode" of sparse
+		// checkouts already include files at the toplevel (which would include
+		// ".prow.yaml").
+		SparseCheckoutDirs: &[]string{".prow"},


Use inRepoConfigDirName instead of the literal. Also, does the TODO above apply, that you need the top-level file?

Oops, will update.

airbornepony · 2023-08-17T17:52:47Z

prow/git/v2/interactor_test.go

+		{
+			name:     "blank RepoOpts",
+			dir:      "/else",
+			from:     "/somewhere",


Using strings "/dir" and "/from" improves readability of expected calls below.

Will update to use "/mirrorclone" and "/secondaryclone" for even more readability.

No functional change.

airbornepony · 2023-08-17T19:31:30Z

prow/git/v2/client_factory.go

+	repoOpts := RepoOpts{
+		SparseCheckoutDirs:         nil,
+		ShareObjectsWithSourceRepo: false,
+	}


This is actually just RepoOpts{}, and it could be inlined in the call below.

I did this originally to make things more explicit (to show the reader "hey, we're not using these options"). I'm OK with inlining it. Will update.

listx · 2023-08-17T20:21:01Z

I've updated the PR to use Git 2.36+ semantics. So the unit test will fail in CI (because it's using Git 2.30.2). Once the CI image is updated (part of it is in #30409), we can retest and the unit test should pass again.

listx · 2023-08-17T21:32:33Z

I've made it use pre-Git-2.36 semantics, because we don't want to be blocked by the various image upgrade processes. We can do the Git upgrade sometime later when we bump our Git version in kubekins-e2e (CI) and prow components.

cjwagner

I only have minor nits. LGTM.

cjwagner · 2023-08-17T21:58:37Z

prow/git/v2/adapter.go

@@ -59,6 +59,12 @@ func (a *clientFactoryAdapter) ClientFor(org, repo string) (RepoClient, error) {
 	return &repoClientAdapter{Repo: r}, err
 }

+// v1 clients do not support customizing pre-repo options.


Maybe we should just always return an "unsupported" error from this function then. That way if it is accidentally used, the problem won't be subtle.

I did try this already, but then we have to delete one of our tests because it will fail:

test-infra/prow/config/inrepoconfig_test.go

Line 35 in 1e81b5a

testDefaultProwYAMLGetter(localgit.New, t)

. Doing it the current way still lets that test pass (and I didn't think too deeply here but it felt cleaner to do this than to delete that test altogether).

I think we need to remove that test then, right? If that causes the test to fail that is because this is a breaking change and we don't support the prow YAML getter with the v1 client anymore.

So I think we either need to always return an error from the unsupported function and remove the obsolete test, or properly implement this in the v1 client.

I mean, technically the v1 client is supported for the prow YAML getter. But it will be less optimized than the v2 client, because the v1 client won't use the --sparse or --shared flags.

Granted, we are going to try to disable repo serialization altogether, so when we cross that bridge (soon), the v1 client will be basically unusable for the prow YAML getter. So I am OK with removing the obsolete test. Will update (err, this PR is merged now, so will have to do it as a follow-up).

That followup is part of #30428. It's kind of blocked on #30520 though.

cjwagner · 2023-08-17T22:02:20Z

prow/git/v2/interactor.go

+	cloneArgs = append(cloneArgs, from)
+	cloneArgs = append(cloneArgs, i.dir)


nit: These could be combined.

Indeed. I.e., cloneArgs = append(cloneArgs, []string{from, i.dir}...), I think. Will update.

cjwagner · 2023-08-17T22:06:21Z

prow/git/v2/client_factory.go

+	// reachable from the root. If non-nil and non-empty, then those additional
+	// directories from the root are also checked out (populated) in the working
+	// tree, recursively.
+	SparseCheckoutDirs *[]string


nit: We don't need to make this a pointer value. Slices can already be nil, right?

Ah... true. I forgot. I.e.,

var foo []string // foo is nil foo := []string{} // foo is an empty slice (not nil)

Will update.

The only data we need from a clone inrepoconfig repo is the .prow contents (aside: we clone it in the first place so that we can merge to the base branch). The underlying Git client actually uses 2 clones --- first a mirror clone (git clone --mirror ...), and then a secondary local clone into a temporary folder (git clone ...). This secondary clone is the one we are optimizing here. This adds 2 optimizations: (1) Avoid populating object files with "--shared" flag. This way, the .git folder of the secondary clone can just have a pointer to the mirror clone. This saves a lot of disk space for large repos (not to mention that both the CPU time and disk usage for creating this clone is vastly reduced, especially when combined with the next optimization). (2) Avoid a full checkout with sparse-checkout (supported since Git 2.25). This way, we can avoid deflating Git blob objects into file paths on disk for everything except (a) files at the toplevel and (b) files specified by the "git sparse-checkout set <PATH> [...<PATH>]" command. The toplevel files are included as part of "cone mode" which is the recommended mode for sparse checkouts. In our case we specify the ".prow" folder (inRepoConfigDirName) which is the only path to make sure we get those inrepoconfig files checked out. Sadly, for Git versions 2.32 (which we currently use in our components), the toplevel files are *not* included by default. So we have to specify it separately, which is why we have inRepoConfigFileName also in SparseCheckoutDirs. In Git 2.41 (probably since 2.36) specifying inRepoConfigFileName will make Git exit with an error because Git will see that the `.prow.yaml` is a file, not a folder [1]. So before we upgrade our components to use Git 2.36+ we have to delete the inRepoConfigFileName from this list. [1] git/git@4ce5043

cjwagner

One remaining style nit that doesn't affect behavior. Lets get this merged.
/unhold

cjwagner · 2023-08-18T00:30:16Z

prow/git/v2/interactor.go

+		cloneArgs = append(cloneArgs, "--sparse")
+	}
+
+	cloneArgs = append(cloneArgs, []string{from, i.dir}...)


Suggested change

cloneArgs = append(cloneArgs, []string{from, i.dir}...)

cloneArgs = append(cloneArgs, from, i.dir)

k8s-ci-robot · 2023-08-18T00:33:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, listx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~prow/OWNERS~~ [cjwagner,listx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ests Before 916ed2b (Merge pull request kubernetes#30400 from listx/inrepoconfig-optimizations, 2023-08-17), we were under the impression that the secondary clones were unavoidably large. And so, we created the InRepoConfigGitCache in 3098b88 (Merge pull request would never delete and recreate the secondary clone for every request --- instead we would reuse the existing secondary clones across requests if those requests were for the same repo. This resulted in the need to ensure that only one such request would touch the associated secondary clone at any given time, which meant that we had to serialize the use of these secondary clones. But because the inrepoconfig repos we clone are so cheap now, there's no need to try to reuse the same one during the lifetime of a process (this is the point of InRepoConfigGitCache). We just create and delete each time we need to service a request. In manual tests, we saw reductions in size from 3.1GB to just 19MB for the disk space used for a secondary clone for a large repo using inrepoconfigs (where the bulk of the 19MB came from other unrelated files in the toplevel folder of the repo, due to the way sparse checkouts work). Note that we already have a call to repo.Clean() inside prowYAMLGetter, so that will delete the repo after we read the inrepoconfig values. However, the main "gitcache" directory (holding the mirror clone) will still be around and will not be deleted.

…ests Before 916ed2b (Merge pull request kubernetes#30400 from listx/inrepoconfig-optimizations, 2023-08-17), we were under the impression that the secondary clones were unavoidably large. So we created the InRepoConfigGitCache in 3098b88 (Merge pull request kubernetes#23264 from cjwagner/inrepo-config-cache, 2021-08-18) to make it so we would never delete and recreate the secondary clone for every request --- instead we would reuse the existing secondary clones across requests if those requests were for the same repo. This resulted in the need to ensure that only one such request would touch the associated secondary clone at any given time, which meant that we had to serialize the use of these secondary clones. But because the inrepoconfig repos we clone are so cheap now, there's no need to try to reuse the same one during the lifetime of a process. We just create and delete each time we need to service a request. In manual tests, we saw reductions in size from 3.1GB to just 19MB for the disk space used for a secondary clone for a large repo using inrepoconfigs (where the bulk of the 19MB came from other unrelated files in the toplevel folder of the repo, due to the way sparse checkouts work). Note that we already have a call to repo.Clean() inside prowYAMLGetter, so that will delete the repo after we read the inrepoconfig values. However, the main "gitcache" directory (holding the mirror clone) will still be around and will not be deleted.

…ests Before 916ed2b (Merge pull request kubernetes#30400 from listx/inrepoconfig-optimizations, 2023-08-17), we were under the impression that the secondary clones of inrepoconfig repos were unavoidably large. So we created the InRepoConfigGitCache wrapper in 3098b88 (Merge pull request kubernetes#23264 from cjwagner/inrepo-config-cache, 2021-08-18) to make it so we would never delete and recreate the secondary clone for every request (the default behavior of the wrapped git v2 client); instead, we would reuse the existing secondary clones across requests if those requests were for the same repo. This resulted in the need to ensure that only one such request would touch the associated secondary clone at any given time, which meant that we had to serialize the use of these secondary clones. But because the secondary clones are so cheap now, there's no need to try to reuse the same one during the lifetime of a process. We just create and delete each time we need to service a request. In manual tests, we saw reductions in size from 3.1GB to just 19MB for the disk space used for a secondary clone for a large repo using inrepoconfigs (where the bulk of the 19MB came from other unrelated files in the toplevel folder of the repo, due to the way sparse checkouts work). Note that we already have a call to repo.Clean() inside prowYAMLGetter, so that will delete the repo after we read the inrepoconfig values. However, the main "gitcache" directory (holding the mirror clone) will still be around and will not be deleted.

…ests Before 916ed2b (Merge pull request kubernetes#30400 from listx/inrepoconfig-optimizations, 2023-08-17), we were under the impression that the secondary clones of inrepoconfig repos were unavoidably large. So we created the InRepoConfigGitCache wrapper in 3098b88 (Merge pull request kubernetes#23264 from cjwagner/inrepo-config-cache, 2021-08-18) to make it so we would never delete and recreate the secondary clone for every request (the default behavior of the wrapped git v2 client); instead, we would reuse the existing secondary clones across requests if those requests were for the same repo. This resulted in the need to ensure that only one such request would touch the associated secondary clone at any given time, which meant that we had to serialize the use of these secondary clones. It turns out that the secondary clone optimizations in 916ed2b make these clones cheaper on disk by multiple orders of magnitude. In manual tests, we saw reductions in size from 3.1GB to just 19MB for the disk space used for a secondary clone for a large repo using inrepoconfigs (where the bulk of the 19MB came from other unrelated files in the toplevel folder of the repo, due to the way sparse checkouts work). Because the secondary clones are so cheap now, there's no need to try to reuse the same one during the lifetime of a process. We just create and delete each time we need to service a request. Note that we already have a call to repo.Clean() inside prowYAMLGetter, so that will delete the repo after we read the inrepoconfig values. However, the main "gitcache" directory (holding the mirror clone) will still be around and will not be deleted.

xrstf · 2023-10-20T09:47:21Z

In case anyone else is searching for it: This change broke the generated tests in our product, as we had a symlink from .prow/generated.yaml outside of the .prow/ directory. IMHO this could have been listed in the announcements as a breaking change.

listx · 2023-10-20T18:30:14Z

@xrstf Thanks for the report. I did not foresee that users used symlinks like that though, which is why I didn't make an announcement.

WRT technical reasons behind the breakage, I think this is because when we do sparse checkouts, we only populate the .prow/ files and the files under the project root (which will include .prow.yaml). So even though the symlink will exist, its destination (if outside the .prow/ folder and not at the project root) won't be populated by the sparse checkout.

I'll make an announcement in our docs page.

listx · 2023-10-20T18:37:01Z

See kubernetes-sigs/prow#65 for the announcement update to our docs.

xrstf · 2023-10-21T21:15:58Z

Thanks :-) I admit that our constellation with symlinks was kinda ... unique and we happily changed our repo structure to benefit from the sparse checkouts.

k8s-ci-robot requested review from cjwagner and timwangmusic August 17, 2023 00:53

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2023

listx force-pushed the inrepoconfig-optimizations branch from 070db4b to b507e30 Compare August 17, 2023 03:54

listx force-pushed the inrepoconfig-optimizations branch 3 times, most recently from 859d799 to 760cf04 Compare August 17, 2023 16:02

timwangmusic reviewed Aug 17, 2023

View reviewed changes

prow/git/v2/client_factory.go Show resolved Hide resolved

airbornepony reviewed Aug 17, 2023

View reviewed changes

TestInteractor_Clone: rename source/dest dirs for readability

21f8136

No functional change.

listx force-pushed the inrepoconfig-optimizations branch 2 times, most recently from 6270c39 to 91e0dcb Compare August 17, 2023 18:36

airbornepony reviewed Aug 17, 2023

View reviewed changes

listx mentioned this pull request Aug 17, 2023

bootstrap image: update to bookworm #30409

Merged

listx force-pushed the inrepoconfig-optimizations branch 2 times, most recently from cea211c to d44c183 Compare August 17, 2023 21:29

cjwagner approved these changes Aug 17, 2023

View reviewed changes

k8s-ci-robot assigned cjwagner Aug 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2023

listx force-pushed the inrepoconfig-optimizations branch from d44c183 to d4701fb Compare August 17, 2023 23:40

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2023

listx force-pushed the inrepoconfig-optimizations branch from d4701fb to 8e5361f Compare August 17, 2023 23:50

cjwagner approved these changes Aug 18, 2023

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 18, 2023

k8s-ci-robot merged commit 916ed2b into kubernetes:master Aug 18, 2023
6 of 7 checks passed

listx mentioned this pull request Aug 18, 2023

Inrepoconfig (non-performance-related) cleanups #30414

Closed

listx mentioned this pull request Aug 18, 2023

inrepoconfig: retire secondary clone serialization for same-repo requests #30403

Merged

xrstf mentioned this pull request Oct 20, 2023

Fix broken CI kubermatic/kubeone#2934

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inrepoconfig: optimize secondary clones #30400

inrepoconfig: optimize secondary clones #30400

listx commented Aug 17, 2023

k8s-ci-robot commented Aug 17, 2023

listx commented Aug 17, 2023

listx commented Aug 17, 2023

listx commented Aug 17, 2023 •

edited

airbornepony Aug 17, 2023

listx Aug 17, 2023

listx Aug 17, 2023

airbornepony Aug 17, 2023

listx Aug 17, 2023

listx Aug 17, 2023

airbornepony Aug 17, 2023

listx Aug 17, 2023

listx Aug 17, 2023

listx commented Aug 17, 2023

listx commented Aug 17, 2023

cjwagner left a comment

cjwagner Aug 17, 2023

listx Aug 17, 2023 •

edited

cjwagner Aug 18, 2023

listx Aug 18, 2023

listx Aug 26, 2023

cjwagner Aug 17, 2023

listx Aug 17, 2023

listx Aug 17, 2023

cjwagner Aug 17, 2023

listx Aug 17, 2023

listx Aug 17, 2023

cjwagner left a comment

cjwagner Aug 18, 2023

k8s-ci-robot commented Aug 18, 2023

xrstf commented Oct 20, 2023

listx commented Oct 20, 2023

listx commented Oct 20, 2023

xrstf commented Oct 21, 2023

		cloneArgs = append(cloneArgs, from)
		cloneArgs = append(cloneArgs, i.dir)

	cloneArgs = append(cloneArgs, []string{from, i.dir}...)
	cloneArgs = append(cloneArgs, from, i.dir)

inrepoconfig: optimize secondary clones #30400

inrepoconfig: optimize secondary clones #30400

Conversation

listx commented Aug 17, 2023

k8s-ci-robot commented Aug 17, 2023

listx commented Aug 17, 2023

listx commented Aug 17, 2023

listx commented Aug 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

listx commented Aug 17, 2023

listx commented Aug 17, 2023

cjwagner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

listx Aug 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjwagner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 18, 2023

xrstf commented Oct 20, 2023

listx commented Oct 20, 2023

listx commented Oct 20, 2023

xrstf commented Oct 21, 2023

listx commented Aug 17, 2023 •

edited

listx Aug 17, 2023 •

edited