Commit fd50ba42 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'sh-rugged-find-commit' into 'master'

Bring back Rugged implementation of find_commit

See merge request gitlab-org/gitlab-ce!25477
parents d4a2cd79 fb6a4e21
---
title: Bring back Rugged implementation of find_commit
merge_request: 25477
author:
type: fixed
...@@ -37,6 +37,28 @@ options: ...@@ -37,6 +37,28 @@ options:
circumstances it could lead to data loss if a failure occurs before data has circumstances it could lead to data loss if a failure occurs before data has
synced. synced.
### Improving NFS performance with GitLab
If you are using NFS to share Git data, we recommend that you enable a
number of feature flags that will allow GitLab application processes to
access Git data directly instead of going through the [Gitaly
service](../gitaly/index.md). Depending on your workload and disk
performance, these flags may help improve performance. See [the
issue](https://gitlab.com/gitlab-org/gitlab-ce/issues/57317) for more
details.
To do this, run the Rake task:
```sh
gitlab-rake gitlab:features:enable_rugged
```
If you need to undo this setting for some reason, run:
```sh
gitlab-rake gitlab:features:disable_rugged
```
### Known issues ### Known issues
On some customer systems, we have seen NFS clients slow precipitously due to On some customer systems, we have seen NFS clients slow precipitously due to
......
...@@ -56,6 +56,44 @@ If your test-suite is failing with Gitaly issues, as a first step, try running: ...@@ -56,6 +56,44 @@ If your test-suite is failing with Gitaly issues, as a first step, try running:
rm -rf tmp/tests/gitaly rm -rf tmp/tests/gitaly
``` ```
## Legacy Rugged code
While Gitaly can handle all Git access, many of GitLab customers still
run Gitaly atop NFS. The legacy Rugged implementation for Git calls may
be faster than the Gitaly RPC due to N+1 Gitaly calls and other
reasons. See [the
issue](https://gitlab.com/gitlab-org/gitlab-ce/issues/57317) for more
details.
Until GitLab has eliminated most of these inefficiencies or the use of
NFS is discontinued for Git data, Rugged implementations of some of the
most commonly-used RPCs can be enabled via feature flags:
* `rugged_find_commit`
* `rugged_get_tree_entries`
* `rugged_tree_entry`
* `rugged_commit_is_ancestor`
A convenience Rake task can be used to enable or disable these flags
all together. To enable:
```sh
bundle exec rake gitlab:features:enable_rugged
```
To disable:
```sh
bundle exec rake gitlab:features:disable_rugged
```
Most of this code exists in the `lib/gitlab/git/rugged_impl` directory.
NOTE: **Note:** You should NOT need to add or modify code related to
Rugged unless explicitly discussed with the [Gitaly
Team](https://gitlab.com/groups/gl-gitaly/group_members). This code will
NOT work on GitLab.com or other GitLab instances that do not use NFS.
## `TooManyInvocationsError` errors ## `TooManyInvocationsError` errors
During development and testing, you may experience `Gitlab::GitalyClient::TooManyInvocationsError` failures. During development and testing, you may experience `Gitlab::GitalyClient::TooManyInvocationsError` failures.
......
...@@ -5,6 +5,7 @@ module Gitlab ...@@ -5,6 +5,7 @@ module Gitlab
module Git module Git
class Commit class Commit
include Gitlab::EncodingHelper include Gitlab::EncodingHelper
prepend Gitlab::Git::RuggedImpl::Commit
extend Gitlab::Git::WrapsGitalyErrors extend Gitlab::Git::WrapsGitalyErrors
attr_accessor :raw_commit, :head attr_accessor :raw_commit, :head
...@@ -62,15 +63,19 @@ module Gitlab ...@@ -62,15 +63,19 @@ module Gitlab
# This saves us an RPC round trip. # This saves us an RPC round trip.
return nil if commit_id.include?(':') return nil if commit_id.include?(':')
commit = wrapped_gitaly_errors do commit = find_commit(repo, commit_id)
repo.gitaly_commit_client.find_commit(commit_id)
end
decorate(repo, commit) if commit decorate(repo, commit) if commit
rescue Gitlab::Git::CommandError, Gitlab::Git::Repository::NoRepository, ArgumentError rescue Gitlab::Git::CommandError, Gitlab::Git::Repository::NoRepository, ArgumentError
nil nil
end end
def find_commit(repo, commit_id)
wrapped_gitaly_errors do
repo.gitaly_commit_client.find_commit(commit_id)
end
end
# Get last commit for HEAD # Get last commit for HEAD
# #
# Ex. # Ex.
...@@ -185,6 +190,10 @@ module Gitlab ...@@ -185,6 +190,10 @@ module Gitlab
@repository = repository @repository = repository
@head = head @head = head
init_commit(raw_commit)
end
def init_commit(raw_commit)
case raw_commit case raw_commit
when Hash when Hash
init_from_hash(raw_commit) init_from_hash(raw_commit)
...@@ -400,3 +409,5 @@ module Gitlab ...@@ -400,3 +409,5 @@ module Gitlab
end end
end end
end end
Gitlab::Git::Commit.singleton_class.prepend Gitlab::Git::RuggedImpl::Commit::ClassMethods
...@@ -4,6 +4,7 @@ module Gitlab ...@@ -4,6 +4,7 @@ module Gitlab
module Git module Git
class Ref class Ref
include Gitlab::EncodingHelper include Gitlab::EncodingHelper
include Gitlab::Git::RuggedImpl::Ref
# Branch or tag name # Branch or tag name
# without "refs/tags|heads" prefix # without "refs/tags|heads" prefix
......
...@@ -11,6 +11,7 @@ module Gitlab ...@@ -11,6 +11,7 @@ module Gitlab
include Gitlab::Git::WrapsGitalyErrors include Gitlab::Git::WrapsGitalyErrors
include Gitlab::EncodingHelper include Gitlab::EncodingHelper
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
include Gitlab::Git::RuggedImpl::Repository
SEARCH_CONTEXT_LINES = 3 SEARCH_CONTEXT_LINES = 3
REV_LIST_COMMIT_LIMIT = 2_000 REV_LIST_COMMIT_LIMIT = 2_000
......
# frozen_string_literal: true
# NOTE: This code is legacy. Do not add/modify code here unless you have
# discussed with the Gitaly team. See
# https://docs.gitlab.com/ee/development/gitaly.html#legacy-rugged-code
# for more details.
# rubocop:disable Gitlab/ModuleWithInstanceVariables
module Gitlab
module Git
module RuggedImpl
module Commit
module ClassMethods
extend ::Gitlab::Utils::Override
def rugged_find(repo, commit_id)
obj = repo.rev_parse_target(commit_id)
obj.is_a?(::Rugged::Commit) ? obj : nil
rescue ::Rugged::Error
nil
end
override :find_commit
def find_commit(repo, commit_id)
if Feature.enabled?(:rugged_find_commit)
rugged_find(repo, commit_id)
else
super
end
end
end
extend ::Gitlab::Utils::Override
override :init_commit
def init_commit(raw_commit)
case raw_commit
when ::Rugged::Commit
init_from_rugged(raw_commit)
else
super
end
end
def init_from_rugged(commit)
author = commit.author
committer = commit.committer
@raw_commit = commit
@id = commit.oid
@message = commit.message
@authored_date = author[:time]
@committed_date = committer[:time]
@author_name = author[:name]
@author_email = author[:email]
@committer_name = committer[:name]
@committer_email = committer[:email]
@parent_ids = commit.parents.map(&:oid)
end
end
end
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
# frozen_string_literal: true
# NOTE: This code is legacy. Do not add/modify code here unless you have
# discussed with the Gitaly team. See
# https://docs.gitlab.com/ee/development/gitaly.html#legacy-rugged-code
# for more details.
module Gitlab
module Git
module RuggedImpl
module Ref
def self.dereference_object(object)
object = object.target while object.is_a?(::Rugged::Tag::Annotation)
object
end
end
end
end
end
# frozen_string_literal: true
# NOTE: This code is legacy. Do not add/modify code here unless you have
# discussed with the Gitaly team. See
# https://docs.gitlab.com/ee/development/gitaly.html#legacy-rugged-code
# for more details.
# rubocop:disable Gitlab/ModuleWithInstanceVariables
module Gitlab
module Git
module RuggedImpl
module Repository
FEATURE_FLAGS = %i(rugged_find_commit).freeze
def alternate_object_directories
relative_object_directories.map { |d| File.join(path, d) }
end
ALLOWED_OBJECT_RELATIVE_DIRECTORIES_VARIABLES = %w[
GIT_OBJECT_DIRECTORY_RELATIVE
GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE
].freeze
def relative_object_directories
Gitlab::Git::HookEnv.all(gl_repository).values_at(*ALLOWED_OBJECT_RELATIVE_DIRECTORIES_VARIABLES).flatten.compact
end
def rugged
@rugged ||= ::Rugged::Repository.new(path, alternates: alternate_object_directories)
rescue ::Rugged::RepositoryError, ::Rugged::OSError
raise ::Gitlab::Git::Repository::NoRepository.new('no repository for such path')
end
def cleanup
@rugged&.close
end
# Return the object that +revspec+ points to. If +revspec+ is an
# annotated tag, then return the tag's target instead.
def rev_parse_target(revspec)
obj = rugged.rev_parse(revspec)
Ref.dereference_object(obj)
end
end
end
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
...@@ -32,11 +32,19 @@ module Gitlab ...@@ -32,11 +32,19 @@ module Gitlab
end end
def self.disk_access_denied? def self.disk_access_denied?
return false if rugged_enabled?
!temporarily_allowed?(ALLOW_KEY) && GitalyClient.feature_enabled?(DISK_ACCESS_DENIED_FLAG) !temporarily_allowed?(ALLOW_KEY) && GitalyClient.feature_enabled?(DISK_ACCESS_DENIED_FLAG)
rescue rescue
false # Err on the side of caution, don't break gitlab for people false # Err on the side of caution, don't break gitlab for people
end end
def self.rugged_enabled?
Gitlab::Git::RuggedImpl::Repository::FEATURE_FLAGS.any? do |flag|
Feature.enabled?(flag)
end
end
def initialize(storage) def initialize(storage)
raise InvalidConfigurationError, "expected a Hash, got a #{storage.class.name}" unless storage.is_a?(Hash) raise InvalidConfigurationError, "expected a Hash, got a #{storage.class.name}" unless storage.is_a?(Hash)
raise InvalidConfigurationError, INVALID_STORAGE_MESSAGE unless storage.has_key?('path') raise InvalidConfigurationError, INVALID_STORAGE_MESSAGE unless storage.has_key?('path')
......
namespace :gitlab do
namespace :features do
desc 'GitLab | Features | Enable direct Git access via Rugged for NFS'
task enable_rugged: :environment do
set_rugged_feature_flags(true)
puts 'All Rugged feature flags were enabled.'
end
task disable_rugged: :environment do
set_rugged_feature_flags(false)
puts 'All Rugged feature flags were disabled.'
end
end
def set_rugged_feature_flags(status)
Gitlab::Git::RuggedImpl::Repository::FEATURE_FLAGS.each do |flag|
if status
Feature.enable(flag)
else
Feature.disable(flag)
end
end
end
end
...@@ -5,12 +5,18 @@ ALLOWED = [ ...@@ -5,12 +5,18 @@ ALLOWED = [
'lib/gitlab/bare_repository_import/repository.rb', 'lib/gitlab/bare_repository_import/repository.rb',
# Needed to avoid using the git binary to validate a branch name # Needed to avoid using the git binary to validate a branch name
'lib/gitlab/git_ref_validator.rb' 'lib/gitlab/git_ref_validator.rb',
# Reverted Rugged calls due to Gitaly atop NFS performance
# See https://docs.gitlab.com/ee/development/gitaly.html#legacy-rugged-code.
'lib/gitlab/git/rugged_impl/',
'lib/gitlab/gitaly_client/storage_settings.rb'
].freeze ].freeze
rugged_lines = IO.popen(%w[git grep -i -n rugged -- app config lib], &:read).lines rugged_lines = IO.popen(%w[git grep -i -n rugged -- app config lib], &:read).lines
rugged_lines = rugged_lines.select { |l| /^[^:]*\.rb:/ =~ l } rugged_lines = rugged_lines.select { |l| /^[^:]*\.rb:/ =~ l }
rugged_lines = rugged_lines.reject { |l| l.start_with?(*ALLOWED) } rugged_lines = rugged_lines.reject { |l| l.start_with?(*ALLOWED) }
rugged_lines = rugged_lines.reject { |l| /(include|prepend) Gitlab::Git::RuggedImpl/ =~ l}
rugged_lines = rugged_lines.reject do |line| rugged_lines = rugged_lines.reject do |line|
code, _comment = line.split('# ', 2) code, _comment = line.split('# ', 2)
code !~ /rugged/i code !~ /rugged/i
......
...@@ -112,7 +112,7 @@ describe Gitlab::Git::Commit, :seed_helper do ...@@ -112,7 +112,7 @@ describe Gitlab::Git::Commit, :seed_helper do
end end
context 'Class methods' do context 'Class methods' do
describe '.find' do shared_examples '.find' do
it "should return first head commit if without params" do it "should return first head commit if without params" do
expect(described_class.last(repository).id).to eq( expect(described_class.last(repository).id).to eq(
rugged_repo.head.target.oid rugged_repo.head.target.oid
...@@ -154,6 +154,20 @@ describe Gitlab::Git::Commit, :seed_helper do ...@@ -154,6 +154,20 @@ describe Gitlab::Git::Commit, :seed_helper do
end end
end end
describe '.find with Gitaly enabled' do
it_should_behave_like '.find'
end
describe '.find with Rugged enabled', :enable_rugged do
it 'calls out to the Rugged implementation' do
allow_any_instance_of(Rugged).to receive(:rev_parse).with(SeedRepo::Commit::ID).and_call_original
described_class.find(repository, SeedRepo::Commit::ID)
end
it_should_behave_like '.find'
end
describe '.last_for_path' do describe '.last_for_path' do
context 'no path' do context 'no path' do
subject { described_class.last_for_path(repository, 'master') } subject { described_class.last_for_path(repository, 'master') }
......
...@@ -26,4 +26,14 @@ describe Gitlab::GitalyClient::StorageSettings do ...@@ -26,4 +26,14 @@ describe Gitlab::GitalyClient::StorageSettings do
end end
end end
end end
describe '.disk_access_denied?' do
it 'return false when Rugged is enabled', :enable_rugged do
expect(described_class.disk_access_denied?).to be_falsey
end
it 'returns true' do
expect(described_class.disk_access_denied?).to be_truthy
end
end
end end
...@@ -115,10 +115,17 @@ RSpec.configure do |config| ...@@ -115,10 +115,17 @@ RSpec.configure do |config|
TestEnv.clean_test_path TestEnv.clean_test_path
end end
config.before do config.before do |example|
# Enable all features by default for testing # Enable all features by default for testing
allow(Feature).to receive(:enabled?) { true } allow(Feature).to receive(:enabled?) { true }
enabled = example.metadata[:enable_rugged].present?
# Disable Rugged features by default
Gitlab::Git::RuggedImpl::Repository::FEATURE_FLAGS.each do |flag|
allow(Feature).to receive(:enabled?).with(flag).and_return(enabled)
end
# The following can be removed when we remove the staged rollout strategy # The following can be removed when we remove the staged rollout strategy
# and we can just enable it using instance wide settings # and we can just enable it using instance wide settings
# (ie. ApplicationSetting#auto_devops_enabled) # (ie. ApplicationSetting#auto_devops_enabled)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment