Don't exclude .git from rsync #117827

BenTheElder · 2023-05-05T22:02:35Z

What type of PR is this?

What this PR does / why we need it:

This would be something of a performance regression, but first I just want to see what breaks. Letting CI identify that with this PR.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-05-05T22:02:43Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

BenTheElder · 2023-05-05T22:09:32Z

Actually the performance doesn't feel bad versus build/run.sh before (which was already a bit slow), without measuring it.

Just syncing in .git fixes #117821 but I'm still not recalling what it was that depended on excluding it.

BenTheElder · 2023-05-06T01:26:03Z

locally this broke a few things with make verify, but they look pretty solvable (e.g. kubernetes/publishing-bot#345, we don't have pyyaml in the container but should probably consider not requiring it anyhow)

BenTheElder · 2023-05-06T01:28:16Z

build/common.sh

@@ -674,7 +674,6 @@ function kube::build::sync_to_container() {
  # necessary.
  kube::build::rsync \
    --delete \
-    --filter='H /.git' \
    --filter='- /_tmp/' \


copy_output filters /_temp/ but not /_tmp/ ...

We have a lot of thingsd encoded in .gitignore - I wonder if we can figure out how to respect that instead of encoding many of the same things again?

I'd like to focus on eliminating the rsync dance entirely.

... but I also don't think this will work very well unless rsync is taught to read .gitignore files directly, it seems pretty expensive to read .gitignore files recursively compared to the handful of exclusions here?

BenTheElder · 2023-05-06T01:32:48Z

hack/make-rules/../../hack/verify-golangci-lint.sh
hack/make-rules/../../hack/verify-openapi-spec.sh
hack/make-rules/../../hack/verify-shellcheck.sh
hack/make-rules/../../hack/verify-publishing-bot.py

publishing-bot is the pyyaml thing

shellcheck is trying to invoke docker since shellcheck binary isn't available. I wrote a new version of this for registry.k8s.io that doesn't need docker we can backport here.

openapi-spec failed on:

+++ [0505 22:17:30] Placing binaries
+++ [0505 22:17:32] Setting GOMAXPROCS: 48

You can use 'hack/install-etcd.sh' to install a copy in third_party/.
etcd must be in your PATH

+++ [0505 22:17:32] Clean up complete
+++ exit code: 1
+++ error: 1
FAILED verify-openapi-spec.sh 33s

which seems like something we should just do automatically instead of asking the user to do it ...

golangci-lint failed on:

level=error msg="Unable to load custom analyzer logcheck:../_output/local/bin/logcheck.so,
plugin.Open("/go/src/k8s.io/kubernetes/_output/local/bin/logcheck.so"): realpath failed"
level=error msg="Running error: unknown linters: 'logcheck', run 'golangci-lint help linters' to see the list of supported linters"

BenTheElder · 2023-05-08T02:09:42Z

filed kubernetes/publishing-bot#345 for publishing-bot verify

BenTheElder · 2023-05-08T02:10:05Z

#117831 for golangci-lint

saschagrunert

Is there any other way to discover the root dir without relying on .git / GIT_DISCOVERY_ACROSS_FILESYSTEM?

BenTheElder · 2023-06-06T20:33:22Z

Is there any other way to discover the root dir without relying on .git / GIT_DISCOVERY_ACROSS_FILESYSTEM?

I don't know why that's relevant? But yes there's other ways to find the root dir.

See #117821, the issue is not GIT_DISCOVERY_ACROSS_FILESYSTEM

BenTheElder · 2023-06-16T20:25:07Z

/test all

BenTheElder · 2023-06-16T20:48:47Z

/sig release testing
/kind cleanup

Will continue to iterate on this intermittently, at least some of the issues are fixed now.

See #117821 for why we need this to move forward.

Also, at some point #112862

BenTheElder · 2023-06-16T22:48:32Z

Ah, it always worked in CI, the issue is when using build/run.sh locally (CI doesn't use run.sh to execute verify scripts), but that already doesn't work for some verify scripts currently.

Still need to fix:

hack/make-rules/../../hack/verify-golangci-lint.sh
hack/make-rules/../../hack/verify-openapi-spec.sh
hack/make-rules/../../hack/verify-shellcheck.sh
hack/make-rules/../../hack/verify-publishing-bot.py

but only when using build/run.sh make verify.

Whereas on master / 1ff1a26 all of these fail without this patch build/run/.sh make verify fails:

hack/make-rules/../../hack/verify-codegen.sh
hack/make-rules/../../hack/verify-generated-stable-metrics.sh
hack/make-rules/../../hack/verify-golangci-lint.sh
hack/make-rules/../../hack/verify-internal-modules.sh
hack/make-rules/../../hack/verify-mocks.sh
hack/make-rules/../../hack/verify-openapi-spec.sh
hack/make-rules/../../hack/verify-shellcheck.sh
hack/make-rules/../../hack/verify-spelling.sh
hack/make-rules/../../hack/verify-yamlfmt.sh
hack/make-rules/../../hack/verify-publishing-bot.py

So there's no regression here, it should be less broken. (Note: tested on Linux but results should be same on mac)

BenTheElder · 2023-06-16T22:49:32Z

cc @thockin this is related to the git worktree work.

thockin · 2023-06-16T23:24:50Z

Alternately: git init && git add . && git commit -am "add"

BenTheElder · 2023-06-17T06:08:29Z

Alternately: git init && git add . && git commit -am "add"

We could, but I want to avoid load-bearing filtering in rsync anyhow because it's going to block migrating to a simple volume mount for #112862.

The remaining broken scripts are not broken by git, they're just broken in the build container for other reasons (assuming the host binary output path, trying to use docker themselves, etc)

thockin · 2023-06-18T05:27:11Z

The .git dir can be huge - the point of rsync was to allow remote builds and make mac's not suck. If you sync that, won't it be as bad as not using sync?

…

On Fri, Jun 16, 2023, 11:08 PM Benjamin Elder ***@***.***> wrote: Alternately: git init && git add . && git commit -am "add" We could, but I want to avoid load-bearing filtering in rsync anyhow because it's going to block migrating to a simple volume mount for #112862 <#112862>. The remaining broken scripts are not broken by git, they're just broken in the build container for other reasons (assuming the host binary output path, trying to use docker themselves, etc) — Reply to this email directly, view it on GitHub <#117827 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVC77HXFUPXGMMLZUCLXLVCWTANCNFSM6AAAAAAXXUD3BY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

BenTheElder · 2023-06-28T18:46:55Z

The .git dir can be huge - the point of rsync was to allow remote builds
and make mac's not suck.

We stopped supporting remote builds O(years) ago, that just leaves macOS.

If you sync that, won't it be as bad as not using sync?

Not necessarily, since you do a full read through once and then everything is in the linux VM, repeated I/O during builds across the VM boundary is still expensive (though I think fast enough these days that a hopefully soon follow-up is to eliminate the sync entirely and just "bind mount").

dims · 2023-10-23T15:03:44Z

/approve
/lgtm

/hold Ben, please remove hold when you feel this is ready to land

k8s-ci-robot · 2023-10-23T15:03:52Z

LGTM label has been added.

Git tree hash: 3a639fdea38eed0c8e5ca373b740ebbeaed039d0

k8s-ci-robot · 2023-10-23T15:04:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, dims

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:

~~build/OWNERS~~ [BenTheElder,dims]

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

thockin · 2023-10-23T15:30:07Z

What's the impact on build/test loops? I seem to recall trying this and it being bad, even on my Linux desktop

BenTheElder · 2023-10-23T16:11:34Z

I recall when I wrote this PR it wasn't bad [on linux] but I'd prefer to confirm hard numbers with the latest changes so ... let me report back later and leave the hold until then ...

BenTheElder · 2023-10-23T23:18:30Z

What's the impact on build/test loops? I seem to recall trying this and it being bad, even on my Linux desktop

If you're somehow running an extremely trivial command like build/run.sh git status after make clean but after having already downloaded the docker images etc you might notice a few extra seconds at most (Tested on a large Linux VM). This scenario is pretty unlikely though.

If you're running subsequent commands this is ~free because you've already synced in .git and it's not really changing.
For any less trivial containerized build commands e.g. build/run.sh make WHAT=cmd/kube-apiserver the variance in build times is higher than any measurable difference with/without this patch (~2m for a clean build, ~29s for calling it again no-op).

NOTE: build/run.sh git status won't even work without this patch anyhow ...

Ensuring we're not depending on a load-bearing "don't sync .git" is the first step towards eliminating rsync in favor of a source bind mount. We haven't supported remote rsync/docker hosts for years and mac+docker VFS has gotten faster.

Shipping this patch unbreaks a lot of containerized git interactions as well.

thockin · 2023-10-23T23:23:34Z

LGTM

BenTheElder · 2023-10-23T23:25:46Z

/hold cancel

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 5, 2023

k8s-ci-robot requested review from jeremyrickard and xmudrii May 5, 2023 22:03

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2023

BenTheElder commented May 6, 2023

View reviewed changes

saschagrunert reviewed Jun 5, 2023

View reviewed changes

dims removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 12, 2023

WIP: don't exclude .git from rsync

c1cced2

BenTheElder force-pushed the git-fun branch from e76ad05 to c1cced2 Compare June 16, 2023 20:27

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2023

BenTheElder changed the title ~~WIP: don't exclude .git from rsync~~ Don't exclude .git from rsync Jun 16, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 16, 2023

BenTheElder mentioned this pull request Jun 16, 2023

make verify doesn't work under build/run.sh due to git worktree usage while .git is not rsynced to build container #117821

Closed

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

k8s-ci-robot assigned dims Oct 23, 2023

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

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

k8s-ci-robot merged commit 13ee40c into kubernetes:master Oct 24, 2023
13 checks passed

k8s-ci-robot added this to the v1.29 milestone Oct 24, 2023

BenTheElder deleted the git-fun branch October 24, 2023 17:47

pohly mentioned this pull request Oct 25, 2023

logcheck doesn't work with dockerized build #117831

Closed

BenTheElder mentioned this pull request Jun 3, 2024

remove rsync from build #112862

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't exclude .git from rsync #117827

Don't exclude .git from rsync #117827

BenTheElder commented May 5, 2023 •

edited

k8s-ci-robot commented May 5, 2023

BenTheElder commented May 5, 2023

BenTheElder commented May 6, 2023

BenTheElder May 6, 2023 •

edited

thockin Oct 23, 2023

BenTheElder Oct 23, 2023

BenTheElder commented May 6, 2023

BenTheElder commented May 8, 2023

BenTheElder commented May 8, 2023

saschagrunert left a comment •

edited

BenTheElder commented Jun 6, 2023

BenTheElder commented Jun 16, 2023

BenTheElder commented Jun 16, 2023

BenTheElder commented Jun 16, 2023 •

edited

BenTheElder commented Jun 16, 2023

thockin commented Jun 16, 2023

BenTheElder commented Jun 17, 2023

thockin commented Jun 18, 2023 via email

BenTheElder commented Jun 28, 2023

dims commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

thockin commented Oct 23, 2023

BenTheElder commented Oct 23, 2023 •

edited

BenTheElder commented Oct 23, 2023 •

edited

thockin commented Oct 23, 2023

BenTheElder commented Oct 23, 2023

Don't exclude .git from rsync #117827

Don't exclude .git from rsync #117827

Conversation

BenTheElder commented May 5, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented May 5, 2023

BenTheElder commented May 5, 2023

BenTheElder commented May 6, 2023

BenTheElder May 6, 2023 • edited

Choose a reason for hiding this comment

thockin Oct 23, 2023

Choose a reason for hiding this comment

BenTheElder Oct 23, 2023

Choose a reason for hiding this comment

BenTheElder commented May 6, 2023

BenTheElder commented May 8, 2023

BenTheElder commented May 8, 2023

saschagrunert left a comment • edited

Choose a reason for hiding this comment

BenTheElder commented Jun 6, 2023

BenTheElder commented Jun 16, 2023

BenTheElder commented Jun 16, 2023

BenTheElder commented Jun 16, 2023 • edited

BenTheElder commented Jun 16, 2023

thockin commented Jun 16, 2023

BenTheElder commented Jun 17, 2023

thockin commented Jun 18, 2023 via email

BenTheElder commented Jun 28, 2023

dims commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

thockin commented Oct 23, 2023

BenTheElder commented Oct 23, 2023 • edited

BenTheElder commented Oct 23, 2023 • edited

thockin commented Oct 23, 2023

BenTheElder commented Oct 23, 2023

BenTheElder commented May 5, 2023 •

edited

BenTheElder May 6, 2023 •

edited

saschagrunert left a comment •

edited

BenTheElder commented Jun 16, 2023 •

edited

BenTheElder commented Oct 23, 2023 •

edited

BenTheElder commented Oct 23, 2023 •

edited