diff options
author | Jacob Vosmaer (GitLab) <jacob@gitlab.com> | 2018-05-09 17:09:30 +0300 |
---|---|---|
committer | Jacob Vosmaer (GitLab) <jacob@gitlab.com> | 2018-05-09 17:09:30 +0300 |
commit | 2b6afcbe172d178e1eaaddd6e5827f16c083ebd0 (patch) | |
tree | 77d0ea1633b9dcd6d1d209ac4a2d0ba6e277b14f | |
parent | 41b55bb0c68f149d7b695abd816ddf4f3d32b6db (diff) | |
parent | dda990a05f4e80e579d49c02b1bc58ee0d02b684 (diff) |
Merge branch 'remove-ruby-limiter' into 'master'
Remove ruby concurrency limiter
See merge request gitlab-org/gitaly!708
-rw-r--r-- | CHANGELOG.md | 2 | ||||
-rw-r--r-- | doc/configuration/README.md | 1 | ||||
-rw-r--r-- | internal/config/ruby.go | 8 | ||||
-rw-r--r-- | internal/rubyserver/concurrency_test.go | 37 | ||||
-rw-r--r-- | internal/rubyserver/rubyserver.go | 33 | ||||
-rwxr-xr-x | ruby/bin/gitaly-ruby | 4 |
6 files changed, 3 insertions, 82 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index aad074faa..776c892c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ UNRELEASED +- Remove ruby concurrency limiter + https://gitlab.com/gitlab-org/gitaly/merge_requests/708 - Introduce src-d/go-git as dependency https://gitlab.com/gitlab-org/gitaly/merge_requests/709 diff --git a/doc/configuration/README.md b/doc/configuration/README.md index e685d52c9..88ac51b1e 100644 --- a/doc/configuration/README.md +++ b/doc/configuration/README.md @@ -112,7 +112,6 @@ max\_rss limit. |restart_delay|string|no|Time memory must be high before a restart is triggered, in seconds. Default 5 minutes ("5m").| |num_workers|integer|no|Number of gitaly-ruby worker processes. Default 2, minimum 2.| |linguist_languages_path|string|no|Override for dynamic languages.json discovery. Default: "" (use dynamic discovery).| -|concurrency|integer|no|Maximum number of concurrent gitaly-ruby requests. Default 20.| ### Logging diff --git a/internal/config/ruby.go b/internal/config/ruby.go index 384ab5139..9ec0fd3b0 100644 --- a/internal/config/ruby.go +++ b/internal/config/ruby.go @@ -15,7 +15,6 @@ type Ruby struct { RestartDelayToml duration `toml:"restart_delay"` NumWorkers int `toml:"num_workers"` LinguistLanguagesPath string `toml:"linguist_languages_path"` - Concurrency int `toml:"concurrency"` } // This type is a trick to let our TOML library parse durations from strings. @@ -54,12 +53,5 @@ func ConfigureRuby() error { Config.Ruby.NumWorkers = minWorkers } - if Config.Ruby.Concurrency == 0 { - // You can benchmark this number with: - // - // go test -bench BenchmarkConcurrency -run '^$' ./internal/rubyserver/ - Config.Ruby.Concurrency = 20 - } - return validateIsDirectory(Config.Ruby.Dir, "gitaly-ruby.dir") } diff --git a/internal/rubyserver/concurrency_test.go b/internal/rubyserver/concurrency_test.go index 98b86f90a..e82311513 100644 --- a/internal/rubyserver/concurrency_test.go +++ b/internal/rubyserver/concurrency_test.go @@ -7,7 +7,6 @@ import ( "testing" "time" - "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/config" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "google.golang.org/grpc/codes" @@ -15,42 +14,6 @@ import ( "google.golang.org/grpc/status" ) -func TestConcurrency(t *testing.T) { - testhelper.ConfigureRuby() - config.Config.Ruby.Concurrency = 1 - - s, err := Start() - if err != nil { - t.Fatal(err) - } - defer s.Stop() - - require.NoError(t, waitPing(s), "wait for gitaly-ruby to boot") - - start := make(chan struct{}) - wg := &sync.WaitGroup{} - - for i := 0; i < 100; i++ { - wg.Add(1) - - go func() { - defer wg.Done() - - <-start - for j := 0; j < 10; j++ { - // Before we had gitaly-ruby rate limiting, this could result in - // ResourceExhausted errors. This test is meant to guard against that - // problem coming back. - require.NoError(t, makeRequest(s)) - } - }() - } - - close(start) // increase chances that goroutines all run at once - - wg.Wait() -} - func waitPing(s *Server) error { var err error for start := time.Now(); time.Since(start) < ConnectTimeout; time.Sleep(100 * time.Millisecond) { diff --git a/internal/rubyserver/rubyserver.go b/internal/rubyserver/rubyserver.go index 0c07e9875..8b9f159dc 100644 --- a/internal/rubyserver/rubyserver.go +++ b/internal/rubyserver/rubyserver.go @@ -14,7 +14,6 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/config" "gitlab.com/gitlab-org/gitaly/internal/helper" - "gitlab.com/gitlab-org/gitaly/internal/middleware/limithandler" "gitlab.com/gitlab-org/gitaly/internal/rubyserver/balancer" "gitlab.com/gitlab-org/gitaly/internal/supervisor" "gitlab.com/gitlab-org/gitaly/internal/version" @@ -70,7 +69,6 @@ type Server struct { workers []*worker clientConnMu sync.Mutex clientConn *grpc.ClientConn - limiter *limithandler.ConcurrencyLimiter } // Stop shuts down the gitaly-ruby helper process and cleans up resources. @@ -103,19 +101,6 @@ func Start() (*Server, error) { } cfg := config.Config - - // We need a wide margin between the concurrency limit enforced on the - // client side and the thread pool size in the gitaly-ruby grpc server. - // If the client side limit is too close to the server thread pool size, - // or if the client does not limit at all, we can get ResourceExhausted - // errors from gitaly-ruby when it runs out of threads in its pool. - // - // Our choice of 2 * cfg.Ruby.Concurrency is probably not the optimal - // formula but it is good enough. Ruby threads are not that expensive, - // and it is pointless to set up large thread pools (e.g. 100 threads) in - // a single Ruby process anyway because of its Global Interpreter Lock. - rubyThreadPoolSize := 2 * cfg.Ruby.Concurrency - env := append( os.Environ(), "GITALY_RUBY_GIT_BIN_PATH="+command.GitPath(), @@ -124,7 +109,6 @@ func Start() (*Server, error) { "GITALY_RUBY_GITLAB_SHELL_PATH="+cfg.GitlabShell.Dir, "GITALY_RUBY_GITALY_BIN_DIR="+cfg.BinDir, "GITALY_VERSION="+version.GetVersion(), - fmt.Sprintf("GITALY_RUBY_THREAD_POOL_SIZE=%d", rubyThreadPoolSize), ) if dsn := cfg.Logging.RubySentryDSN; dsn != "" { env = append(env, "SENTRY_DSN="+dsn) @@ -132,9 +116,7 @@ func Start() (*Server, error) { gitalyRuby := path.Join(cfg.Ruby.Dir, "bin/gitaly-ruby") - s := &Server{ - limiter: limithandler.NewLimiter(cfg.Ruby.Concurrency, limithandler.NewPromMonitor("gitaly-ruby", "")), - } + s := &Server{} for i := 0; i < cfg.Ruby.NumWorkers; i++ { name := fmt.Sprintf("gitaly-ruby.%d", i) socketPath := socketPath(i) @@ -230,19 +212,6 @@ func (s *Server) BlobServiceClient(ctx context.Context) (pb.BlobServiceClient, e } func (s *Server) getConnection(ctx context.Context) (*grpc.ClientConn, error) { - ourTurn := make(chan struct{}) - go s.limiter.Limit(ctx, "gitaly-ruby", func() (interface{}, error) { - close(ourTurn) - <-ctx.Done() - return nil, nil - }) - - select { - case <-ctx.Done(): - return nil, ctx.Err() - case <-ourTurn: - } - s.clientConnMu.Lock() conn := s.clientConn s.clientConnMu.Unlock() diff --git a/ruby/bin/gitaly-ruby b/ruby/bin/gitaly-ruby index ce16e8ad4..87acda775 100755 --- a/ruby/bin/gitaly-ruby +++ b/ruby/bin/gitaly-ruby @@ -24,11 +24,7 @@ def main FileUtils.mkdir_p(socket_dir) File.chmod(0700, socket_dir) - pool_size = ENV['GITALY_RUBY_THREAD_POOL_SIZE'].to_i - pool_size = 30 if pool_size <= 0 - s = GRPC::RpcServer.new( - pool_size: pool_size, poll_period: SHUTDOWN_TIMEOUT, interceptors: [ GitalyServer::SentryInterceptor.new, |