From 968674e41798c437b9ebf4a9731fe2f2a4f07024 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 12 Jun 2019 15:32:29 +0200 Subject: Move Gitaly feature flag logic to Feature::Gitaly The GitalyClient held a lot of logic which was all very tightly coupled. In this instance the feature logic was extracted to make it do just a little less and create a bit more focus in the GitalyClient's responsibilies. --- lib/feature/gitaly.rb | 25 +++++++++++++++++++++++++ lib/gitlab/gitaly_client.rb | 21 +++------------------ spec/lib/feature/gitaly_spec.rb | 32 ++++++++++++++++++++++++++++++++ spec/lib/gitlab/gitaly_client_spec.rb | 14 -------------- 4 files changed, 60 insertions(+), 32 deletions(-) create mode 100644 lib/feature/gitaly.rb create mode 100644 spec/lib/feature/gitaly_spec.rb diff --git a/lib/feature/gitaly.rb b/lib/feature/gitaly.rb new file mode 100644 index 00000000000..33868e49f14 --- /dev/null +++ b/lib/feature/gitaly.rb @@ -0,0 +1,25 @@ +class Feature + class Gitaly + CATFILE_CACHE = 'catfile-cache'.freeze + + # Server feature flags should use '_' to separate words. + SERVER_FEATURE_FLAGS = [CATFILE_CACHE].freeze + + class << self + def enabled?(feature_flag) + Feature::FlipperFeature.table_exists? && Feature.enabled?("gitaly_#{feature_flag}") + rescue ActiveRecord::NoDatabaseError + false + end + + def server_feature_flags + @server_feature_flags ||= + begin + SERVER_FEATURE_FLAGS.map do |f| + ["gitaly-feature-#{f.tr('_', '-')}", enabled?(f).to_s] + end.to_h + end + end + end + end +end diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index e683d4e5bbe..f3b0e65400b 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -31,10 +31,6 @@ module Gitlab MAXIMUM_GITALY_CALLS = 30 CLIENT_NAME = (Sidekiq.server? ? 'gitlab-sidekiq' : 'gitlab-web').freeze - SERVER_FEATURE_CATFILE_CACHE = 'catfile-cache'.freeze - # Server feature flags should use '_' to separate words. - SERVER_FEATURE_FLAGS = [SERVER_FEATURE_CATFILE_CACHE].freeze - MUTEX = Mutex.new define_histogram :gitaly_controller_action_duration_seconds do @@ -223,9 +219,9 @@ module Gitlab metadata['call_site'] = feature.to_s if feature metadata['gitaly-servers'] = address_metadata(remote_storage) if remote_storage metadata['x-gitlab-correlation-id'] = Labkit::Correlation::CorrelationId.current_id if Labkit::Correlation::CorrelationId.current_id - metadata['gitaly-session-id'] = session_id if feature_enabled?(SERVER_FEATURE_CATFILE_CACHE) + metadata['gitaly-session-id'] = session_id if Feature::Gitaly.enabled?(Feature::Gitaly::CATFILE_CACHE) - metadata.merge!(server_feature_flags) + metadata.merge!(Feature::Gitaly.server_feature_flags) result = { metadata: metadata } @@ -244,11 +240,6 @@ module Gitlab Gitlab::SafeRequestStore[:gitaly_session_id] ||= SecureRandom.uuid end - def self.server_feature_flags - SERVER_FEATURE_FLAGS.map do |f| - ["gitaly-feature-#{f.tr('_', '-')}", feature_enabled?(f).to_s] - end.to_h - end def self.token(storage) params = Gitlab.config.repositories.storages[storage] @@ -257,12 +248,6 @@ module Gitlab params['gitaly_token'].presence || Gitlab.config.gitaly['token'] end - def self.feature_enabled?(feature_name) - Feature::FlipperFeature.table_exists? && Feature.enabled?("gitaly_#{feature_name}") - rescue ActiveRecord::NoDatabaseError - false - end - # Ensures that Gitaly is not being abuse through n+1 misuse etc def self.enforce_gitaly_request_limits(call_site) # Only count limits in request-response environments (not sidekiq for example) @@ -295,7 +280,7 @@ module Gitlab # check if the limit is being exceeded while testing in those environments # In that case we can use a feature flag to indicate that we do want to # enforce request limits. - return true if feature_enabled?('enforce_requests_limits') + return true if Feature::Gitaly.enabled?('enforce_requests_limits') !(Rails.env.production? || ENV["GITALY_DISABLE_REQUEST_LIMITS"]) end diff --git a/spec/lib/feature/gitaly_spec.rb b/spec/lib/feature/gitaly_spec.rb new file mode 100644 index 00000000000..12923e4ec41 --- /dev/null +++ b/spec/lib/feature/gitaly_spec.rb @@ -0,0 +1,32 @@ +require 'spec_helper' + +describe Feature::Gitaly do + let(:feature_flag) { "mepmep" } + + describe ".enabled?" do + context 'when the gate is closed' do + before do + allow(Feature).to receive(:enabled?).with("gitaly_mepmep").and_return(false) + end + + it 'returns false' do + expect(described_class.enabled?(feature_flag)).to be(false) + end + end + end + + describe ".server_feature_flags" do + before do + stub_const("#{described_class}::SERVER_FEATURE_FLAGS", [feature_flag]) + allow(Feature).to receive(:enabled?).with("gitaly_mepmep").and_return(false) + end + + subject { described_class.server_feature_flags } + + it { is_expected.to be_a(Hash) } + + context 'when one flag is disabled' do + it { is_expected.to eq("gitaly-feature-mepmep" => "false") } + end + end +end diff --git a/spec/lib/gitlab/gitaly_client_spec.rb b/spec/lib/gitlab/gitaly_client_spec.rb index da1eb0c2618..eed233f1f3e 100644 --- a/spec/lib/gitlab/gitaly_client_spec.rb +++ b/spec/lib/gitlab/gitaly_client_spec.rb @@ -330,20 +330,6 @@ describe Gitlab::GitalyClient do end end - describe 'feature_enabled?' do - let(:feature_name) { 'my_feature' } - let(:real_feature_name) { "gitaly_#{feature_name}" } - - before do - allow(Feature).to receive(:enabled?).and_return(false) - end - - it 'returns false' do - expect(Feature).to receive(:enabled?).with(real_feature_name) - expect(described_class.feature_enabled?(feature_name)).to be(false) - end - end - describe 'timeouts' do context 'with default values' do before do -- cgit v1.2.1 From 4dfaaf40b9c6754157adff3704a8036b440148b3 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 12 Jun 2019 16:20:01 +0200 Subject: Turn on Cat-File cache by default The feature flag has been introduced an was turned off by default, now the it will default to be turned on. That change would still allow users to turn this feature off by leveraging the Rails console by running: `Feature.disable("gitaly_catfile-cache")` Another option is to manage the number of items the LRU cache will contain, by updating the `config.toml` for Gitaly. This would be the `catfile_cache_size`: https://gitlab.com/gitlab-org/gitaly/blob/0dcb5c579e63754f557aef91a4fa7a00e5b8b127/config.toml.example#L27 Closes: https://gitlab.com/gitlab-org/gitaly/issues/1712 --- lib/feature/gitaly.rb | 22 ++++++++++------ lib/gitlab/gitaly_client.rb | 1 - lib/gitlab/gitaly_client/storage_settings.rb | 2 +- spec/lib/feature/gitaly_spec.rb | 30 ++++++++++++++-------- .../gitlab/gitaly_client/storage_settings_spec.rb | 12 ++++++--- spec/support/helpers/cycle_analytics_helpers.rb | 2 +- 6 files changed, 43 insertions(+), 26 deletions(-) diff --git a/lib/feature/gitaly.rb b/lib/feature/gitaly.rb index 33868e49f14..d7a8f8a0b9e 100644 --- a/lib/feature/gitaly.rb +++ b/lib/feature/gitaly.rb @@ -1,24 +1,30 @@ +# frozen_string_literal: true + +require 'set' + class Feature class Gitaly + # Server feature flags should use '_' to separate words. + # CATFILE_CACHE sets an incorrect example CATFILE_CACHE = 'catfile-cache'.freeze - # Server feature flags should use '_' to separate words. SERVER_FEATURE_FLAGS = [CATFILE_CACHE].freeze + DEFAULT_ON_FLAGS = Set.new([CATFILE_CACHE]).freeze class << self def enabled?(feature_flag) - Feature::FlipperFeature.table_exists? && Feature.enabled?("gitaly_#{feature_flag}") + return false unless Feature::FlipperFeature.table_exists? + + default_on = DEFAULT_ON_FLAGS.include?(feature_flag) + Feature.enabled?("gitaly_#{feature_flag}", default_enabled: default_on) rescue ActiveRecord::NoDatabaseError false end def server_feature_flags - @server_feature_flags ||= - begin - SERVER_FEATURE_FLAGS.map do |f| - ["gitaly-feature-#{f.tr('_', '-')}", enabled?(f).to_s] - end.to_h - end + SERVER_FEATURE_FLAGS.map do |f| + ["gitaly-feature-#{f.tr('_', '-')}", enabled?(f).to_s] + end.to_h end end end diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index f3b0e65400b..47976389af6 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -240,7 +240,6 @@ module Gitlab Gitlab::SafeRequestStore[:gitaly_session_id] ||= SecureRandom.uuid end - def self.token(storage) params = Gitlab.config.repositories.storages[storage] raise "storage not found: #{storage.inspect}" if params.nil? diff --git a/lib/gitlab/gitaly_client/storage_settings.rb b/lib/gitlab/gitaly_client/storage_settings.rb index 78ef6bfc0ec..7d1206e551b 100644 --- a/lib/gitlab/gitaly_client/storage_settings.rb +++ b/lib/gitlab/gitaly_client/storage_settings.rb @@ -34,7 +34,7 @@ module Gitlab 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) && Feature::Gitaly.enabled?(DISK_ACCESS_DENIED_FLAG) rescue false # Err on the side of caution, don't break gitlab for people end diff --git a/spec/lib/feature/gitaly_spec.rb b/spec/lib/feature/gitaly_spec.rb index 12923e4ec41..0e24a927d4c 100644 --- a/spec/lib/feature/gitaly_spec.rb +++ b/spec/lib/feature/gitaly_spec.rb @@ -1,32 +1,40 @@ require 'spec_helper' describe Feature::Gitaly do - let(:feature_flag) { "mepmep" } + let(:feature_flag) { "mep_mep" } + + before do + stub_const("#{described_class}::SERVER_FEATURE_FLAGS", [feature_flag]) + end describe ".enabled?" do context 'when the gate is closed' do before do - allow(Feature).to receive(:enabled?).with("gitaly_mepmep").and_return(false) + stub_feature_flags(gitaly_mep_mep: false) end it 'returns false' do expect(described_class.enabled?(feature_flag)).to be(false) end end - end - describe ".server_feature_flags" do - before do - stub_const("#{described_class}::SERVER_FEATURE_FLAGS", [feature_flag]) - allow(Feature).to receive(:enabled?).with("gitaly_mepmep").and_return(false) + context 'when the flag defaults to on' do + it 'returns true' do + expect(described_class.enabled?(feature_flag)).to be(true) + end end + end - subject { described_class.server_feature_flags } + describe ".server_feature_flags" do + context 'when one flag is disabled' do + before do + stub_feature_flags(gitaly_mep_mep: false) + end - it { is_expected.to be_a(Hash) } + subject { described_class.server_feature_flags } - context 'when one flag is disabled' do - it { is_expected.to eq("gitaly-feature-mepmep" => "false") } + it { is_expected.to be_a(Hash) } + it { is_expected.to eq("gitaly-feature-mep-mep" => "false") } end end end diff --git a/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb b/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb index bb10be2a4dc..f2f53982b09 100644 --- a/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb +++ b/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb @@ -28,12 +28,16 @@ describe Gitlab::GitalyClient::StorageSettings do 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 + context 'when Rugged is enabled', :enable_rugged do + it 'returns false' do + expect(described_class.disk_access_denied?).to be_falsey + end end - it 'returns true' do - expect(described_class.disk_access_denied?).to be_truthy + context 'when Rugged is disabled' do + it 'returns true' do + expect(described_class.disk_access_denied?).to be_truthy + end end end end diff --git a/spec/support/helpers/cycle_analytics_helpers.rb b/spec/support/helpers/cycle_analytics_helpers.rb index 33648292037..100e439ef44 100644 --- a/spec/support/helpers/cycle_analytics_helpers.rb +++ b/spec/support/helpers/cycle_analytics_helpers.rb @@ -10,7 +10,7 @@ module CycleAnalyticsHelpers repository = project.repository oldrev = repository.commit(branch_name)&.sha || Gitlab::Git::BLANK_SHA - if Timecop.frozen? && Gitlab::GitalyClient.feature_enabled?(:operation_user_commit_files) + if Timecop.frozen? mock_gitaly_multi_action_dates(repository, commit_time) end -- cgit v1.2.1