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

Switch to shallow git clone and add unshallow feature #6464

Merged
merged 2 commits into from
Nov 4, 2021

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Oct 30, 2021

Description

In #4847 we tried to improve the initial clone of the workspace git repository using the flag --filter=blob:none.

The CPU and bandwidth utilization were reduced drastically, also impacting the initial load of the workspace.
Even with these benefits, we had to roll back the change due to the side effects the flag introduces in the local git repository.

This pull request is another attempt to improve the initial clone of the git repository by using a shallow clone (flag --depth=1)

By default, all workspaces will do a shallow clone and after 10s the start of the workspace, we fetch the latest 20 commits

Using https://aledbf-unshallow.staging.gitpod-dev.com/#https://gitlab.com/gitlab-org/gitlab/ as an example:

Initial size:

gitpod /workspace/gitlab $ du -sm
1309    .

Only the last commit is present:

gitpod /workspace/gitlab $ git history
git: 'history' is not a git command. See 'git --help'.
gitpod /workspace/gitlab $ git status
Refresh index: 100% (50406/50406), done.
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
gitpod /workspace/gitlab $ git log
commit aa1f905ed933a204190bff0626cefd52d2a22372 (grafted, HEAD -> master, origin/master, origin/HEAD)
Author: Suzanne Selhorn <[email protected]>
Date:   Sat Oct 30 17:43:23 2021 +0000

    Merge branch 'docs-deprecation-notice-cert-based-clusters-2' into 'master'
    
    Doc: Add deprecation notice to CI envs doc
    
    See merge request gitlab-org/gitlab!73368

After a couple of minutes (usually takes ~4m to unshallow)

gitpod /workspace/gitlab $ du -sm
3016    .

From the workspace logs:

{"level":"info","message":"workspace configured with unshallow feature","op":"unshallow","ring":1,"serviceContext":{"service":"workspacekit","version":""},"severity":"INFO","time":"2021-10-30T23:26:22Z"}
{"level":"info","message":"waiting 5m0s before running the unshallow script","op":"unshallow","ring":1,"serviceContext":{"service":"workspacekit","version":""},"severity":"INFO","time":"2021-10-30T23:26:22Z"}
{"level":"info","message":"unshallow of local repository took 3m43.210101577s","op":"unshallow","ring":1,"serviceContext":{"service":"workspacekit","version":""},"severity":"INFO","time":"2021-10-30T23:35:05Z"}

Related Issue(s)

Fixes #5231

How to test

Open https://aledbf-unshallow.staging.gitpod-dev.com/#https://gitlab.com/aledbf/gitlab

Release Notes

Switch to shallow git clone and add unshallow feature

Documentation

@aledbf aledbf requested a review from csweichel October 30, 2021 21:37
@aledbf aledbf changed the title WIP Switch to git shallow clone and add unshallow feature WIP Switch to shallow git clone and add unshallow feature Oct 30, 2021
@aledbf
Copy link
Member Author

aledbf commented Oct 30, 2021

/werft run

👍 started the job as gitpod-build-aledbf-unshallow.1

@codecov
Copy link

codecov bot commented Oct 30, 2021

Codecov Report

Merging #6464 (aa08cbd) into main (21fda09) will increase coverage by 13.56%.
The diff coverage is 4.34%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #6464       +/-   ##
===========================================
+ Coverage   19.04%   32.61%   +13.56%     
===========================================
  Files           2       86       +84     
  Lines         168    16459    +16291     
===========================================
+ Hits           32     5368     +5336     
- Misses        134    10616    +10482     
- Partials        2      475      +473     
Flag Coverage Δ
components-content-service-app 14.43% <14.28%> (?)
components-content-service-lib 14.43% <14.28%> (?)
components-image-builder-app 34.44% <ø> (?)
components-image-builder-mk3-app 35.03% <ø> (?)
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
components-supervisor-app 36.50% <0.00%> (?)
components-ws-daemon-app 22.04% <ø> (?)
components-ws-daemon-lib 22.04% <ø> (?)
components-ws-manager-app 39.58% <ø> (?)
components-ws-proxy-app 69.77% <ø> (?)
components-ws-proxy-lib 69.62% <ø> (?)
installer-app 7.45% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/content-service/pkg/initializer/git.go 0.00% <0.00%> (ø)
components/supervisor/pkg/supervisor/supervisor.go 6.12% <0.00%> (ø)
components/content-service/pkg/git/git.go 36.99% <100.00%> (ø)
components/local-app/pkg/auth/pkce.go
components/local-app/pkg/auth/auth.go
components/ws-daemon/pkg/content/tar.go 46.71% <0.00%> (ø)
installer/pkg/common/render.go 16.94% <0.00%> (ø)
components/ws-daemon/pkg/content/archive.go 57.95% <0.00%> (ø)
installer/pkg/components/ws-manager/role.go 0.00% <0.00%> (ø)
components/ws-daemon/pkg/content/config.go 33.33% <0.00%> (ø)
... and 81 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21fda09...aa08cbd. Read the comment docs.

@aledbf aledbf force-pushed the aledbf/unshallow branch 4 times, most recently from 4a91eb5 to 08b78ca Compare October 30, 2021 23:22
@aledbf aledbf changed the title WIP Switch to shallow git clone and add unshallow feature Switch to shallow git clone and add unshallow feature Oct 30, 2021
@aledbf
Copy link
Member Author

aledbf commented Oct 30, 2021

Another example running the script manually:

https://aledbf-unshallow.staging.gitpod-dev.com/#https://github.com/gitpod-io/openvscode-server

gitpod /workspace/openvscode-server $ du -sm
1487    .
gitpod /workspace/openvscode-server $ if git fetch --help | grep -c -- '--unshallow'; then git fetch --unshallow; else git fetch --depth 1000000000; fi
remote: Enumerating objects: 44275, done.
remote: Counting objects: 100% (39826/39826), done.
remote: Compressing objects: 100% (18082/18082), done.
remote: Total 37766 (delta 25267), reused 29453 (delta 18892), pack-reused 0
Receiving objects: 100% (37766/37766), 16.45 MiB | 18.00 MiB/s, done.
Resolving deltas: 100% (25267/25267), completed with 361 local objects.
gitpod /workspace/openvscode-server $ du -sm
1762    .

@aledbf aledbf marked this pull request as ready for review October 31, 2021 00:43
@csweichel
Copy link
Contributor

I very much like the unshallow idea - it nicely solves some of the issues we had seen.

I wonder though if we can up with more convenient ways to decide when to unshallow. The two things that shallow cloning broke were "forking" and "git blame":

  • forking: when a user cannot write to a repo its likely they'll need to fork. We could naively always unshallow when a user clones a repo they cannot write to (server can decide - send an unshallow flag as part of the content initializer). Better yet, we might be able to hook into the forking process somehow and unshallow prior to the fork. To start, we could go with the first option.
  • git blame: is much harder to detect. Short of placing something like a "git facade" in the workspace, or worse yet, relying on seccomp-notify based execve hacks, I cannot think of a good way to hook into that. The time-based mechanism does not help unfortunately, because there's no guarantee that a blame won't happen prior to the unshallow. Maybe @akosyakov has an idea?

The proposed time-based mechanism would certainly help reduce the load on the cluster, I fret it offloads the complexity onto the user though and does not solve the two cases above. Unshallowing the copy though IMHO is an awesome idea.

@akosyakov
Copy link
Member

git blame: is much harder to detect. Short of placing something like a "git facade" in the workspace, or worse yet, relying on seccomp-notify based execve hacks, I cannot think of a good way to hook into that. The time-based mechanism does not help unfortunately, because there's no guarantee that a blame won't happen prior to the unshallow. Maybe @akosyakov has an idea?

If you mean whether there is a way to hook on VS Code level, unfortunately not. GitLens execs git directly. Besides we are going to have other IDEs where we don't have any control of core features.

@csweichel
Copy link
Contributor

If you mean whether there is a way to hook on VS Code level, unfortunately not. GitLens execs git directly. Besides we are going to have other IDEs where we don't have any control of core features.

Can we assume that everyone is going to exec git? If so, we could add our own facade to git which triggers the unshallow when needed (e.g. prior to a git blame).

@akosyakov
Copy link
Member

Can we assume that everyone is going to exec git? If so, we could add our own facade to git which triggers the unshallow when needed (e.g. prior to a git blame).

To my knowledge yes. Don't know what Jetbrains product will do, but maybe it is too early to worry. We can enable it first on some repos before making a default?

@aledbf
Copy link
Member Author

aledbf commented Nov 1, 2021

Reading the comments I think my goals are not clear:

  • git repositories use shallow clone (no exceptions)
  • by default, there is no unshallow.
  • unshallow can be achieved:
    • using the unshallowAfter attribute in .gitpod.yml
    • running git --unshallow manually
    • defining a task in .gitpod.yml that runs git --unshallow
  • the change in the behavior should be reflected in the docs

@akosyakov
Copy link
Member

@aledbf What you propose sounds good as the end goal, but I am not sure about usability consequences of rolling it for everybody at once. It would be better if we use it ourself for a month on gitpod-io/gitpod and gitpod/openvscode-server to learn.

@JanKoehnlein
Copy link
Contributor

I agree with Anton. We should start with opt-in for shallow cloning and enable it on gitpod and open-vscode-server to test ourselves.

@csweichel
Copy link
Contributor

csweichel commented Nov 1, 2021

We have tested shallow clones for about a month in production already, and have found two issues only: forks and blame. Also, considering the real-world performance improvements we've seen, this is a change that we don't want to delay for a month with little gain. The goal must be to make shallow clones the default, because only then do we reap their benefits.

That said, we'll want to have a good answer to the issues we've found before. Making this opt-out today would make this a potentially breaking change; documentation would certainly help. Making this opt-in would be the low-risk path but would not give us the benefits we hope to achieve.

IMHO making this this feature opt-in makes little sense, because we're unlikely to see high uptake. I'd follow Alejandro's proposal with two alterations:

  • when a user cannot write a repo, we don't do a shallow clone. Because we know that forks and shallow clones don't go along so well, and users who cannot write to a repo are likely to fork, it wouldn't make sense to shallow clone here.
  • the time-based unshallow doesn't seem sensible to me because it doesn't solve any of the two issues we've identified. I'd get rid of it completely.

At the end it's mainly a communication issue, because with the outline above we don't expect adverse effects on most users. We'd need to make them aware of shallow clones, their implications, and how to remedy them. Also, worst case if we do see too many problems in practice, by having this a flag on the initializer (see below) we can quickly remove this behaviour.

Technical details

Instead of providing a time-based unshallow mechanism, I reckon we'd want to make this part of the Git content initializer spec, i.e. add a bool unshallow_clone = 7 to GitInitializer. server would only set this flag in the fork case outlined above.

@geropl
Copy link
Member

geropl commented Nov 2, 2021

👍 For not doing the time-based unshallow bc it adds user-facing complexity.

Regarding "opt-out vs opt-in": I agree that it does not make sense to go with opt-in only and wait for adoption. But this feature might break peoples workflows in a lot of ways, and just because we did not find more issues doesn't mean other will run into some. Especially as we cannot detect the general case and offer to unshallow.

At the end it's mainly a communication issue

💯 Another options to solve the UX problem:

  • make the git clone time transparent to the user (startup screen?) and make it opt-in to generate adoption.
  • make it opt-out and have a banner (?) on workspaces/workspace start that explains the change

@csweichel
Copy link
Contributor

@csweichel
Copy link
Contributor

/hold cancel

@iQQBot
Copy link
Contributor

iQQBot commented Nov 4, 2021

lgtm

@csweichel
Copy link
Contributor

@gitpod-io/engineering-ide please have a look as well, particularly towards the supervisor changes

@iQQBot
Copy link
Contributor

iQQBot commented Nov 4, 2021

Can I test this in preview environment?

Currently show Your account has been blocked.

@csweichel

@csweichel
Copy link
Contributor

Can I test this in preview environment?

Currently show Your account has been blocked.

@csweichel

Sorry for that. That's the default behaviour in preview environments. I've unblocked your user.

@iQQBot
Copy link
Contributor

iQQBot commented Nov 4, 2021

Thanks @csweichel looks good, in image builder stage, can we skip fetch to a depth of 20 commits ?

@csweichel
Copy link
Contributor

Thanks @csweichel looks good, in image builder stage, can we skip fetch to a depth of 20 commits ?

We could indeed do that, and we've discussed not fetching the history for any headless workspace (prebuilds, image-builds). However, this looks like a potential future optimisation. Considering that variance that would introduce, we'd like to see the effect of this change under load first.

@corneliusludmann
Copy link
Contributor

/approve

👍

@csweichel
Copy link
Contributor

/approve

@aledbf aledbf added approved and removed approved labels Nov 4, 2021
@iQQBot
Copy link
Contributor

iQQBot commented Nov 4, 2021

Seem @roboquat doesn't work

@csweichel
Copy link
Contributor

/approve

@csweichel
Copy link
Contributor

/lgtm

@roboquat roboquat added the lgtm label Nov 4, 2021
@roboquat
Copy link
Contributor

roboquat commented Nov 4, 2021

LGTM label has been added.

Git tree hash: c4004c1c50d6c1ebbc1981c68e7fd2068fb48d2f

@roboquat
Copy link
Contributor

roboquat commented Nov 4, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: corneliusludmann, csweichel

Associated issue: #5231

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:

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

@roboquat roboquat merged commit 636ef16 into main Nov 4, 2021
@roboquat roboquat deleted the aledbf/unshallow branch November 4, 2021 21:43
@jeanp413
Copy link
Member

jeanp413 commented Nov 6, 2021

Is there a way to tell gitpod to always do a deep clone, like for example defining an env variable in my gitpod settings?
I like to use the timeline view in vscode to check the latest changes in a specific file and now I have to unshallow the repo manually every time I open a new workspace 😬

@csweichel
Copy link
Contributor

There isn't at the moment. It's not clear to me where such an "unshallow: always" setting should live: in the .gitpod.yml (repo scoped), or at the user-level (valid for all repos).

@svenefftinge wdyt?

@svenefftinge
Copy link
Member

For @jeanp413 use case it should be user-level. Would it make sense to build a small vscode extension that does this?
Doesn't even need to be gitpod specific.

@jeanp413
Copy link
Member

yeah, a vscode extension (or a contributed configuration option in the gitpod-web extension) that calls git fetch --unshallow automatically on startup should do the trick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reintroduce partial clone
10 participants