From c600cf83488398cf66b10af85ed9490fe9457bd4 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 25 Jun 2016 23:29:28 -0700 Subject: [PATCH 1/2] Fix database migrations when Redis is not running If Redis were not running or USE_DB were set to false, the application settings retrieval would fail completely. This change only attempts to use the cache if the system actually wants to connect to the DB and rescues any failures in talking to Redis. Closes #17557 --- lib/gitlab/current_settings.rb | 10 +++++--- spec/lib/gitlab/current_settings_spec.rb | 29 ++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 spec/lib/gitlab/current_settings_spec.rb diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 28c34429c1..67d6be3515 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -9,10 +9,14 @@ module Gitlab end def ensure_application_settings! - settings = ::ApplicationSetting.cached + if connect_to_db? + begin + settings = ::ApplicationSetting.cached + # In case Redis isn't running or the Redis UNIX socket file is not available + rescue ::Redis::BaseError, ::Errno::ENOENT + settings = ::ApplicationSetting.last + end - if !settings && connect_to_db? - settings = ::ApplicationSetting.current settings ||= ::ApplicationSetting.create_from_defaults unless ActiveRecord::Migrator.needs_migration? end diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb new file mode 100644 index 0000000000..f2d2f7b5d3 --- /dev/null +++ b/spec/lib/gitlab/current_settings_spec.rb @@ -0,0 +1,29 @@ +require 'spec_helper' + +describe Gitlab::CurrentSettings do + describe '#current_application_settings' do + it 'attempts to use cached values first' do + allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(true) + expect(ApplicationSetting).to receive(:cached).and_call_original + expect(ApplicationSetting).not_to receive(:last) + + expect(current_application_settings).to be_a(ApplicationSetting) + end + + it 'does not attempt to connect to DB or Redis' do + allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(false) + expect(ApplicationSetting).not_to receive(:current) + expect(ApplicationSetting).not_to receive(:last) + + expect(current_application_settings).to eq fake_application_settings + end + + it 'falls back to DB if Redis fails' do + allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(true) + expect(ApplicationSetting).to receive(:cached).and_raise(::Redis::BaseError) + expect(ApplicationSetting).to receive(:last).and_call_original + + expect(current_application_settings).to be_a(ApplicationSetting) + end + end +end From d10642a4b86d91faf7a3ffa508b4e8067213b14c Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Wed, 29 Jun 2016 14:28:41 -0700 Subject: [PATCH 2/2] Handle case when Redis cache returns an empty setting --- lib/gitlab/current_settings.rb | 2 +- spec/lib/gitlab/current_settings_spec.rb | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 67d6be3515..54b46e5d23 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -11,7 +11,7 @@ module Gitlab def ensure_application_settings! if connect_to_db? begin - settings = ::ApplicationSetting.cached + settings = ::ApplicationSetting.current # In case Redis isn't running or the Redis UNIX socket file is not available rescue ::Redis::BaseError, ::Errno::ENOENT settings = ::ApplicationSetting.last diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb index f2d2f7b5d3..004341ffd0 100644 --- a/spec/lib/gitlab/current_settings_spec.rb +++ b/spec/lib/gitlab/current_settings_spec.rb @@ -4,7 +4,7 @@ describe Gitlab::CurrentSettings do describe '#current_application_settings' do it 'attempts to use cached values first' do allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(true) - expect(ApplicationSetting).to receive(:cached).and_call_original + expect(ApplicationSetting).to receive(:current).and_return(::ApplicationSetting.create_from_defaults) expect(ApplicationSetting).not_to receive(:last) expect(current_application_settings).to be_a(ApplicationSetting) @@ -18,9 +18,16 @@ describe Gitlab::CurrentSettings do expect(current_application_settings).to eq fake_application_settings end + it 'falls back to DB if Redis returns an empty value' do + allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(true) + expect(ApplicationSetting).to receive(:last).and_call_original + + expect(current_application_settings).to be_a(ApplicationSetting) + end + it 'falls back to DB if Redis fails' do allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(true) - expect(ApplicationSetting).to receive(:cached).and_raise(::Redis::BaseError) + expect(ApplicationSetting).to receive(:current).and_raise(::Redis::BaseError) expect(ApplicationSetting).to receive(:last).and_call_original expect(current_application_settings).to be_a(ApplicationSetting)