gitserver: SRC_ENABLE_GC_AUTO to enable & disable gc on fetch#47852

abeatrix · 2023-02-17T21:47:14Z

Close https://github.com/sourcegraph/customer/issues/1955

Context: A customer is encountering an issue with one of their fast-moving repo that caused the repo unable to sync & index, making it unsearchable (context). We suspected the addition of --no-auto-gc in https://github.com/sourcegraph/sourcegraph/pull/36274 is the cause as the issue started after performing MVU, and the error went away when running git gc manually inside gitserver.

Included in this PR:

set SRC_ENABLE_GC_AUTO should enable and disable gc on fetch

Test plan

Ran locally to confirm setting SRC_ENABLE_GC_AUTO would toggle the command flag:

[    gitserver-1] 2023/02/17 20:54:11 Setting --auto-gc to fetch
[    gitserver-1] 2023/02/17 20:57:08 Setting --no-auto-gc to fetch

sourcegraph-bot · 2023-02-17T21:49:07Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff a071a07...d41d313.

Notify	File(s)
@indradhanush	cmd/gitserver/server/cleanup.go cmd/gitserver/server/refspecoverrides.go cmd/gitserver/server/serverutil_test.go cmd/gitserver/server/vcs_syncer_git.go
@sashaostrikov	cmd/gitserver/server/cleanup.go cmd/gitserver/server/refspecoverrides.go cmd/gitserver/server/serverutil_test.go cmd/gitserver/server/vcs_syncer_git.go

cmd/gitserver/server/serverutil.go

stefanhengl · 2023-02-20T07:29:34Z

cmd/gitserver/server/refspecoverrides.go

-	return exec.CommandContext(ctx, "git", append([]string{"fetch", "--no-auto-gc", "--progress", "--prune", remoteURL.String()}, refspecOverrides...)...)
+	// Perform automatic repository maintenance at the end of fetch
+	gc := "--auto-gc"
+	if e := os.Getenv("SRC_ENABLE_GC_AUTO"); e == "false" {


nit: maybe strconv.ParseBool would be better here? Especially because customers will set this, there might be a lot of variation in how they set it.

Why do we have to check the env var here again? Can't we check whether gitGCMode == gitGCModeGitAutoGC?

We already parse the env var here and already have some logic to set the GC mode here: https://github.com/sourcegraph/sourcegraph/blob/37458d2484d78de53765dfaea929be7c22e611cf/cmd/gitserver/server/cleanup.go#L56-L87

I think we should reuse that instead of adding another place where things can change.

cmd/gitserver/server/serverutil.go

stefanhengl · 2023-02-20T07:32:48Z

cmd/gitserver/server/vcs_syncer_git.go

 	} else {
+		// Perform automatic repository maintenance at the end of fetch
+		gc := "--auto-gc"
+		if e := os.Getenv("SRC_ENABLE_GC_AUTO"); e == "false" {


strconv.ParseBool?

Same as above: let's reuse gitGCMode or at least the parsing of the env var or move this check over to the the others and use .envGet.

indradhanush

Not a blocker, but could you please add a section to the end of this doc to document this new behaviour?

Can be a follow up PR as well. Leaving the comment so that it's tracked.

mrnugget

Thanks for hopping on this! Requesting changes because I think we should reuse the GC mode variable OR move the parsing of the env var closer to the others.

mrnugget · 2023-02-20T09:00:25Z

cmd/gitserver/server/refspecoverrides.go

-	return exec.CommandContext(ctx, "git", append([]string{"fetch", "--no-auto-gc", "--progress", "--prune", remoteURL.String()}, refspecOverrides...)...)
+	// Perform automatic repository maintenance at the end of fetch
+	gc := "--auto-gc"
+	if e := os.Getenv("SRC_ENABLE_GC_AUTO"); e == "false" {


Why do we have to check the env var here again? Can't we check whether gitGCMode == gitGCModeGitAutoGC?

We already parse the env var here and already have some logic to set the GC mode here: https://github.com/sourcegraph/sourcegraph/blob/37458d2484d78de53765dfaea929be7c22e611cf/cmd/gitserver/server/cleanup.go#L56-L87

I think we should reuse that instead of adding another place where things can change.

cmd/gitserver/server/serverutil.go

mrnugget · 2023-02-20T09:01:55Z

cmd/gitserver/server/vcs_syncer_git.go

 	} else {
+		// Perform automatic repository maintenance at the end of fetch
+		gc := "--auto-gc"
+		if e := os.Getenv("SRC_ENABLE_GC_AUTO"); e == "false" {


Same as above: let's reuse gitGCMode or at least the parsing of the env var or move this check over to the the others and use .envGet.

mrnugget · 2023-02-20T09:05:15Z

cmd/gitserver/server/vcs_syncer_git.go

 		cmd = refspecOverridesFetchCmd(ctx, remoteURL)
 	} else {
+		// Perform automatic repository maintenance at the end of fetch
+		gc := "--auto-gc"


Do we even need to set this explicitly? I kinda assumed that, depending on the GC mode we use, we set gc.auto = 1 or 0 in the git config. See: https://github.com/sourcegraph/sourcegraph/blob/37458d2484d78de53765dfaea929be7c22e611cf/cmd/gitserver/server/cleanup.go#L1402-L1420

abeatrix · 2023-02-20T17:03:20Z

Thank you all for the reviews! I wasn't aware of gitGCMode but what @mrnugget suggested makes sense to me. I do have a follow-up question to make sure I understand the current implementations correctly. It looks like we disable gc for gitGCModeJanitorAutoGC & gitGCModeMaintenance even when enableGCAuto is set to true, can someone explain to me if this is intended (or is this the issue that we are trying to fix)?

mrnugget · 2023-02-20T17:16:39Z

It looks like we disable gc for gitGCModeJanitorAutoGC & gitGCModeMaintenance even when enableGCAuto is set to true, can someone explain to me if this is intended (or is this the issue that we are trying to fix)?

I'll let @stefanhengl answer here (since he added it in https://github.com/sourcegraph/sourcegraph/commit/f461a4ae253faa2752d2bc64d049c90569de7a1f) but I think the thinking is this: if we use gitGCModeJanitorAutoGC or gitGCModeMaintenance we run our own garbage collection in janitor runs, every 1min, so we don't need to run auto-gc on commands. that's why we disable it, so a git fetch that triggers auto-gc can't clash with a janitor run in the background.

But: aren't we now discovering that even with janitor-git-gc-runs we still run into these problems and we should probably also use auto-gc even if gitGCModeJanitorAutoGC?

abeatrix · 2023-02-20T18:19:19Z

But: aren't we now discovering that even with janitor-git-gc-runs we still run into these problems and we should probably also use auto-gc even if gitGCModeJanitorAutoGC?

@mrnugget Yea that's what I believe is the issue as well and have updated to enable gc here for gitGCModeJanitorAutoGC, but want to make sure why it wasn't in the first place but what you explained clarified it for me :) Will wait for @stefanhengl to confirm

edit: @stefanhengl should we also remove the hard-coded "gc.auto=1" here?

stefanhengl · 2023-02-21T07:55:38Z

edit: @stefanhengl should we also remove the hard-coded "gc.auto=1" here?

From the docs

gc.auto::
When there are approximately more than this many loose
objects in the repository, git gc --auto will pack them.
Some Porcelain commands use this command to perform a
light-weight garbage collection from time to time. The
default value is 6700.

This is an aggressive setting for packing but AFAIK we never had trouble with it. Changing it might blow up dir sizes on gitserver. I never changed it and unless we have specific reason to remove it I wouldn't touch it.

stefanhengl · 2023-02-21T08:10:26Z

Thank you all for the reviews! I wasn't aware of gitGCMode but what @mrnugget suggested makes sense to me. I do have a follow-up question to make sure I understand the current implementations correctly. It looks like we disable gc for gitGCModeJanitorAutoGC & gitGCModeMaintenance even when enableGCAuto is set to true, can someone explain to me if this is intended (or is this the issue that we are trying to fix)?

Once it became obvious that sg maintenance caused repo corruption for customers with large repos I tried to lock down any unintentional garbage collection run by git porcelain commands IE I wanted to make sure our janitor job is the only one running gc. This whole approach failed and I recommend to take the original implementation (before I added sg maintenance) as baseline. The original implementation didn't set any repo config options like we do in gitSetAutoGC.

abeatrix added 2 commits February 17, 2023 13:25

disable gc auto based on env var value

076314a

Use protocol v2 for ls-remote

0303c5f

cla-bot bot added the cla-signed label Feb 17, 2023

abeatrix requested review from jhchabran, keegancsmith, mrnugget and stefanhengl February 17, 2023 21:47

jhchabran reviewed Feb 18, 2023

View reviewed changes

cmd/gitserver/server/serverutil.go Outdated Show resolved Hide resolved

stefanhengl approved these changes Feb 20, 2023

View reviewed changes

indradhanush reviewed Feb 20, 2023

View reviewed changes

mrnugget suggested changes Feb 20, 2023

View reviewed changes

Revert gc ls-remote

dbdf89a

Configure gc in gitSetAutoGC

df06b73

sashaostrikov approved these changes Feb 21, 2023

View reviewed changes

Move gitGCModeJanitorAutoGC back

d367d6c

abeatrix requested a review from mrnugget February 21, 2023 14:28

unset auto-gc for gitGCModeJanitorAutoGC

8eb519e

mrnugget approved these changes Feb 21, 2023

View reviewed changes

abeatrix added 2 commits February 21, 2023 11:17

Merge branch 'main' into bee-gc

9f65602

Update changelog

d41d313

abeatrix merged commit 382b481 into main Feb 21, 2023

abeatrix deleted the bee-gc branch February 21, 2023 19:43

Conversation

abeatrix commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

sourcegraph-bot commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

stefanhengl Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

mrnugget Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stefanhengl Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

mrnugget Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

indradhanush left a comment

Choose a reason for hiding this comment

Uh oh!

mrnugget left a comment

Choose a reason for hiding this comment

Uh oh!

mrnugget Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrnugget Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

mrnugget Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

abeatrix commented Feb 20, 2023

Uh oh!

mrnugget commented Feb 20, 2023

Uh oh!

abeatrix commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefanhengl commented Feb 21, 2023

Uh oh!

stefanhengl commented Feb 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

abeatrix commented Feb 17, 2023 •

edited

Loading

sourcegraph-bot commented Feb 17, 2023 •

edited

Loading

abeatrix commented Feb 20, 2023 •

edited

Loading