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

No way to disable git mirror #55

Open
artem-zinnatullin opened this issue Jan 27, 2023 · 5 comments
Open

No way to disable git mirror #55

artem-zinnatullin opened this issue Jan 27, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@artem-zinnatullin
Copy link
Contributor

Hi team, long time no see!

We've been exploring Partial Git Clones recently and found that Treeless git clones are super fast for us while providing a fully working Git repo.

Now I want to check if we actually gaining anything by using shared Git Mirror dir with Treeless clone or wasting more time on it due to what BuildKite agent does while using git mirror:

# Using git-mirrors experiment 🧪
--
  | $ cd /git-mirrors
  | # Updating existing repository mirror to find commit 25009bc1f51560bb81763bd7e24e309963647476
  | $ git --git-dir /git-mirrors/git-github-myrepo-git remote set-url origin git@github.com:myrepo.git
  | $ git --git-dir /git-mirrors/git-github-myrepo-git fetch origin master
  • mirror dir locking which forces jobs on one node into serial cloning and adds delay to job start.

From what I see in the code after trying to remove git-mirrors-host-path from our custom params is that k8s-buildkite-plugin then defaults to /git-mirrors and continues to use Git Mirror.

Thus, I propose a change when a null value on git-mirrors-host-path will disable git mirror feature, while setting a value will enable it with passed path.

The change however will be breaking since default behavior will change and some users might have relied on it, for them we will suggest explicitly passing the old default path /git-mirrors.

I can implement the change if we come to agreement!

wdyt?

Thanks!

@artem-zinnatullin artem-zinnatullin added the bug Something isn't working label Jan 27, 2023
@tgolsson
Copy link
Member

Hello @artem-zinnatullin! Thank you for the report.

As far as I can tell setting the host path to "" (the default) should end up with the actual job running with an emptyDir mirrors path, which shouldn't be shared:

local gitMirrorsVolume =
if env.BUILDKITE_PLUGIN_K8S_GIT_MIRRORS_HOST_PATH != ''
then { hostPath: { path: env.BUILDKITE_PLUGIN_K8S_GIT_MIRRORS_HOST_PATH, type: 'DirectoryOrCreate' } }
else { emptyDir: {} }

I've reviewed a few of my pipelines (all running with the default "" value) and they're are doing full clones in the job. Is this related to some bad interaction by just having the plugin? I'm not conceptually against disabling git-mirrors if not configured - this would mirror the de-facto behavior as I can tell.

Finally, just to validate - is the above log from the the "job" phase or the "setup" phase on the static Buildkite node? I'm a bit suspicious of it updating an existing repository if you haven't configured git-mirrors, from the above reasoning.

@artem-zinnatullin
Copy link
Contributor Author

@tgolsson

As far as I can tell setting the host path to "" (the default) should end up with the actual job running with an emptyDir mirrors path, which shouldn't be shared:

Yes, it does run with emptyDir and still does full git clone in there! Which is what I'm trying to disable because we're using treeless clones now and simply don't need git mirrors at all and they only slow us down :)

That's why I'm asking for a way to completely disable git mirror functionality, it's a feature of BuildKite Agent that is not enabled by default and I think it's reasonable for k8s integration to also not have it on by default.

Finally, just to validate - is the above log from the the "job" phase or the "setup" phase on the static Buildkite node? I'm a bit suspicious of it updating an existing repository if you haven't configured git-mirrors, from the above reasoning.

This is from the "job phase", git clones in "setup phase" is a separate issue I'm dealing with yeah! (Our repos grow bigger and bigger, cloning became an issue)

For context: we're one of the oldest users of this repo (2+ years), at some point I rewrote huge portion of this repo :)

@tgolsson
Copy link
Member

tgolsson commented Feb 9, 2023

Yes, it does run with emptyDir and still does full git clone in there! Which is what I'm trying to disable because we're using treeless clones now and simply don't need git mirrors at all and they only slow us down :)

Cool, makes sense. Out of curiosity, how do you configure treeless clones? Does it all happen in "userland"? Or do you configure buildkite to do this? Wondering whether we also need to make sure the job/init-container (if you use them?) has the proper config.

This is from the "job phase", git clones in "setup phase" is a separate issue I'm dealing with yeah! (Our repos grow bigger and bigger, cloning became an issue)

Hmm! I'm not quite following why it'd do a repeat/update then since the dir would be empty, but I assume it ends up cloning twice (maybe the treeless comes first?). Is there a simple repro/recipe I could use for this? I think it'd be a good case to add a self-test for too for the future.

@artem-zinnatullin
Copy link
Contributor Author

Out of curiosity, how do you configure treeless clones? Does it all happen in "userland"?

We pass BuildKite Agent env var in "userland" BUILDKITE_GIT_CLONE_FLAGS=--filter=tree:0

Wondering whether we also need to make sure the job/init-container (if you use them?) has the proper config.

Git mirroring happens in init container, so yeah, that's what I want to change (I'm willing to work on PR if we agree on design!)

Hmm! I'm not quite following why it'd do a repeat/update then since the dir would be empty, but I assume it ends up cloning twice (maybe the treeless comes first?). Is there a simple repro/recipe I could use for this? I think it'd be a good case to add a self-test for too for the future.

"Second" clone happens because EmbarkStudios/k8s-buildkite-plugin always passes --experiment=git-mirrors to buildkite-agent

args: ['bootstrap', '--experiment=git-mirrors', '--git-mirrors-path=/git-mirrors', '--ssh-keyscan', '--command', 'true'],
and this is what I want to allow users to disable

@tgolsson
Copy link
Member

OK; so I think this makes sense as a change and doesn't seem super-complex. I'm rewriting a bunch of the CI to do self-tests with the current commit so I'll see if I can replicate the negative case; and then we can think about a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants