mirror of
https://github.com/sourcegraph/sourcegraph.git
synced 2026-02-06 17:11:49 +00:00
gitserver: Remove CloneableLimiter (#59935)
IMO, this is an unnecessary optimization that increases complexity and in the current implementation locks for longer than it needs to, because the lock in Blocking clone mode is only returned when the clone has completed, limiting the concurrency more than desired. There are also the clone limiter AND RPS limiter still in place, so we got more than enough rate limiters in place here, IMO.
This commit is contained in:
Erik Seliger
GitHub
parent
53f69fedec
commit
ff1332f0d8
@ -170,11 +170,9 @@ type Server struct {
|
||||
canceled bool
|
||||
wg sync.WaitGroup // tracks running background jobs
|
||||
|
||||
// cloneLimiter and cloneableLimiter limits the number of concurrent
|
||||
// clones and ls-remotes respectively. Use s.acquireCloneLimiter() and
|
||||
// s.acquireCloneableLimiter() instead of using these directly.
|
||||
cloneLimiter *limiter.MutableLimiter
|
||||
cloneableLimiter *limiter.MutableLimiter
|
||||
// cloneLimiter limits the number of concurrent
|
||||
// clones. Use s.acquireCloneLimiter() and instead of using it directly.
|
||||
cloneLimiter *limiter.MutableLimiter
|
||||
|
||||
// RPSLimiter limits the remote code host git operations done per second
|
||||
// per gitserver instance
|
||||
@ -235,13 +233,11 @@ func (s *Server) Handler() http.Handler {
|
||||
// Max concurrent clones also means repo updates.
|
||||
maxConcurrentClones := conf.GitMaxConcurrentClones()
|
||||
s.cloneLimiter = limiter.NewMutable(maxConcurrentClones)
|
||||
s.cloneableLimiter = limiter.NewMutable(maxConcurrentClones)
|
||||
|
||||
// TODO: Remove side-effects from this Handler method.
|
||||
conf.Watch(func() {
|
||||
limit := conf.GitMaxConcurrentClones()
|
||||
s.cloneLimiter.SetLimit(limit)
|
||||
s.cloneableLimiter.SetLimit(limit)
|
||||
})
|
||||
|
||||
mux := http.NewServeMux()
|
||||
@ -433,12 +429,6 @@ func (s *Server) acquireCloneLimiter(ctx context.Context) (context.Context, cont
|
||||
return s.cloneLimiter.Acquire(ctx)
|
||||
}
|
||||
|
||||
func (s *Server) acquireCloneableLimiter(ctx context.Context) (context.Context, context.CancelFunc, error) {
|
||||
lsRemoteQueue.Inc()
|
||||
defer lsRemoteQueue.Dec()
|
||||
return s.cloneableLimiter.Acquire(ctx)
|
||||
}
|
||||
|
||||
func (s *Server) IsRepoCloneable(ctx context.Context, repo api.RepoName) (protocol.IsRepoCloneableResponse, error) {
|
||||
// We use an internal actor here as the repo may be private. It is safe since all
|
||||
// we return is a bool indicating whether the repo is cloneable or not. Perhaps
|
||||
@ -794,17 +784,6 @@ func (s *Server) CloneRepo(ctx context.Context, repo api.RepoName, opts CloneOpt
|
||||
return "", err
|
||||
}
|
||||
|
||||
// isCloneable causes a network request, so we limit the number that can
|
||||
// run at one time. We use a separate semaphore to cloning since these
|
||||
// checks being blocked by a few slow clones will lead to poor feedback to
|
||||
// users. We can defer since the rest of the function does not block this
|
||||
// goroutine.
|
||||
ctx, cancel, err := s.acquireCloneableLimiter(ctx)
|
||||
if err != nil {
|
||||
return "", err // err will be a context error
|
||||
}
|
||||
defer cancel()
|
||||
|
||||
if err = s.RPSLimiter.Wait(ctx); err != nil {
|
||||
return "", err
|
||||
}
|
||||
@ -1209,10 +1188,6 @@ var (
|
||||
Name: "src_gitserver_clone_queue",
|
||||
Help: "number of repos waiting to be cloned.",
|
||||
})
|
||||
lsRemoteQueue = promauto.NewGauge(prometheus.GaugeOpts{
|
||||
Name: "src_gitserver_lsremote_queue",
|
||||
Help: "number of repos waiting to check existence on remote code host (git ls-remote).",
|
||||
})
|
||||
repoClonedCounter = promauto.NewCounter(prometheus.CounterOpts{
|
||||
Name: "src_gitserver_repo_cloned",
|
||||
Help: "number of successful git clones run",
|
||||
|
||||
@ -322,7 +322,6 @@ func makeTestServer(ctx context.Context, t *testing.T, repoDir, remote string, d
|
||||
ctx: ctx,
|
||||
Locker: NewRepositoryLocker(),
|
||||
cloneLimiter: limiter.NewMutable(1),
|
||||
cloneableLimiter: limiter.NewMutable(1),
|
||||
RPSLimiter: ratelimit.NewInstrumentedLimiter("GitserverTest", rate.NewLimiter(rate.Inf, 10)),
|
||||
RecordingCommandFactory: wrexec.NewRecordingCommandFactory(nil, 0),
|
||||
Perforce: perforce.NewService(ctx, obctx, logger, db, list.New()),
|
||||
|
||||
@ -1474,39 +1474,6 @@ Generated query for warning alert: `max((sum(src_gitserver_clone_queue)) >= 25)`
|
||||
|
||||
<br />
|
||||
|
||||
## gitserver: repository_existence_check_queue_size
|
||||
|
||||
<p class="subtitle">repository existence check queue size</p>
|
||||
|
||||
**Descriptions**
|
||||
|
||||
- <span class="badge badge-warning">warning</span> gitserver: 25+ repository existence check queue size
|
||||
|
||||
**Next steps**
|
||||
|
||||
- **Check the code host status indicator for errors:** on the Sourcegraph app homepage, when signed in as an admin click the cloud icon in the top right corner of the page.
|
||||
- **Check if the issue continues to happen after 30 minutes**, it may be temporary.
|
||||
- **Check the gitserver logs for more information.**
|
||||
- Learn more about the related dashboard panel in the [dashboards reference](./dashboards.md#gitserver-repository-existence-check-queue-size).
|
||||
- **Silence this alert:** If you are aware of this alert and want to silence notifications for it, add the following to your site configuration and set a reminder to re-evaluate the alert:
|
||||
|
||||
```json
|
||||
"observability.silenceAlerts": [
|
||||
"warning_gitserver_repository_existence_check_queue_size"
|
||||
]
|
||||
```
|
||||
|
||||
<sub>*Managed by the [Sourcegraph Source team](https://handbook.sourcegraph.com/departments/engineering/teams/source).*</sub>
|
||||
|
||||
<details>
|
||||
<summary>Technical details</summary>
|
||||
|
||||
Generated query for warning alert: `max((sum(src_gitserver_lsremote_queue)) >= 25)`
|
||||
|
||||
</details>
|
||||
|
||||
<br />
|
||||
|
||||
## gitserver: gitserver_site_configuration_duration_since_last_successful_update_by_instance
|
||||
|
||||
<p class="subtitle">maximum duration since last successful site configuration update (all "gitserver" instances)</p>
|
||||
|
||||
23
doc/admin/observability/dashboards.md
generated
23
doc/admin/observability/dashboards.md
generated
@ -5671,25 +5671,6 @@ Query: `sum(src_gitserver_clone_queue)`
|
||||
|
||||
<br />
|
||||
|
||||
#### gitserver: repository_existence_check_queue_size
|
||||
|
||||
<p class="subtitle">Repository existence check queue size</p>
|
||||
|
||||
Refer to the [alerts reference](./alerts.md#gitserver-repository-existence-check-queue-size) for 1 alert related to this panel.
|
||||
|
||||
To see this panel, visit `/-/debug/grafana/d/gitserver/gitserver?viewPanel=100071` on your Sourcegraph instance.
|
||||
|
||||
<sub>*Managed by the [Sourcegraph Source team](https://handbook.sourcegraph.com/departments/engineering/teams/source).*</sub>
|
||||
|
||||
<details>
|
||||
<summary>Technical details</summary>
|
||||
|
||||
Query: `sum(src_gitserver_lsremote_queue)`
|
||||
|
||||
</details>
|
||||
|
||||
<br />
|
||||
|
||||
#### gitserver: echo_command_duration_test
|
||||
|
||||
<p class="subtitle">Echo test command duration</p>
|
||||
@ -5704,7 +5685,7 @@ If this value is consistently high, consider the following:
|
||||
|
||||
This panel has no related alerts.
|
||||
|
||||
To see this panel, visit `/-/debug/grafana/d/gitserver/gitserver?viewPanel=100080` on your Sourcegraph instance.
|
||||
To see this panel, visit `/-/debug/grafana/d/gitserver/gitserver?viewPanel=100071` on your Sourcegraph instance.
|
||||
|
||||
<sub>*Managed by the [Sourcegraph Source team](https://handbook.sourcegraph.com/departments/engineering/teams/source).*</sub>
|
||||
|
||||
@ -5727,7 +5708,7 @@ It does not indicate any problems with the instance.
|
||||
|
||||
This panel has no related alerts.
|
||||
|
||||
To see this panel, visit `/-/debug/grafana/d/gitserver/gitserver?viewPanel=100090` on your Sourcegraph instance.
|
||||
To see this panel, visit `/-/debug/grafana/d/gitserver/gitserver?viewPanel=100072` on your Sourcegraph instance.
|
||||
|
||||
<sub>*Managed by the [Sourcegraph Source team](https://handbook.sourcegraph.com/departments/engineering/teams/source).*</sub>
|
||||
|
||||
|
||||
@ -234,21 +234,6 @@ func GitServer() *monitoring.Dashboard {
|
||||
- **Check which repositories need cloning**, by visiting e.g. https://sourcegraph.example.com/site-admin/repositories?filter=not-cloned
|
||||
`,
|
||||
},
|
||||
{
|
||||
Name: "repository_existence_check_queue_size",
|
||||
Description: "repository existence check queue size",
|
||||
Query: "sum(src_gitserver_lsremote_queue)",
|
||||
Warning: monitoring.Alert().GreaterOrEqual(25),
|
||||
Panel: monitoring.Panel().LegendFormat("queue size"),
|
||||
Owner: monitoring.ObservableOwnerSource,
|
||||
NextSteps: `
|
||||
- **Check the code host status indicator for errors:** on the Sourcegraph app homepage, when signed in as an admin click the cloud icon in the top right corner of the page.
|
||||
- **Check if the issue continues to happen after 30 minutes**, it may be temporary.
|
||||
- **Check the gitserver logs for more information.**
|
||||
`,
|
||||
},
|
||||
},
|
||||
{
|
||||
{
|
||||
Name: "echo_command_duration_test",
|
||||
Description: "echo test command duration",
|
||||
@ -266,8 +251,6 @@ func GitServer() *monitoring.Dashboard {
|
||||
- **Kubernetes and Docker Compose:** Check that you are running a similar number of git server replicas and that their CPU/memory limits are allocated according to what is shown in the [Sourcegraph resource estimator](../deploy/resource_estimator.md).
|
||||
`,
|
||||
},
|
||||
},
|
||||
{
|
||||
{
|
||||
Name: "src_gitserver_repo_count",
|
||||
Description: "number of repositories on gitserver",
|
||||
|
||||
Loading…
Reference in New Issue
Block a user