summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--CHANGELOG.md4
-rw-r--r--app/controllers/projects/tree_controller.rb5
-rw-r--r--changelogs/unreleased/36549-circuit-breaker-handles-missing-storages.yml5
-rw-r--r--config/initializers/1_settings.rb4
-rw-r--r--db/migrate/20170828135939_migrate_user_external_mail_data.rb2
-rw-r--r--db/post_migrate/20170828170502_post_deploy_migrate_user_external_mail_data.rb2
-rw-r--r--doc/development/ux_guide/basics.md2
-rw-r--r--doc/install/requirements.md4
-rw-r--r--lib/gitlab/git/storage.rb1
-rw-r--r--lib/gitlab/git/storage/circuit_breaker.rb30
-rw-r--r--lib/gitlab/git/storage/health.rb2
-rw-r--r--lib/gitlab/git/storage/null_circuit_breaker.rb47
-rw-r--r--lib/gitlab/health_checks/fs_shards_check.rb2
-rw-r--r--spec/lib/gitlab/git/storage/circuit_breaker_spec.rb13
-rw-r--r--spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb77
-rw-r--r--spec/lib/gitlab/health_checks/fs_shards_check_spec.rb18
-rw-r--r--spec/support/stub_configuration.rb2
17 files changed, 189 insertions, 31 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 838a0a9bde9..6c671f8d53a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,10 @@
documentation](doc/development/changelog.md) for instructions on adding your own
entry.
+## 10.0.1 (2017-09-23)
+
+- [FIXED] Fix duplicate key errors in PostDeployMigrateUserExternalMailData migration.
+
## 10.0.0 (2017-09-22)
- [SECURITY] Upgrade brace-expansion NPM package due to security issue. !13665 (Markus Koller)
diff --git a/app/controllers/projects/tree_controller.rb b/app/controllers/projects/tree_controller.rb
index 1fc276b8c03..f3784f4e07c 100644
--- a/app/controllers/projects/tree_controller.rb
+++ b/app/controllers/projects/tree_controller.rb
@@ -35,7 +35,10 @@ class Projects::TreeController < Projects::ApplicationController
end
format.json do
- render json: TreeSerializer.new(project: @project, repository: @repository, ref: @ref).represent(@tree)
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/38261
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ render json: TreeSerializer.new(project: @project, repository: @repository, ref: @ref).represent(@tree)
+ end
end
end
end
diff --git a/changelogs/unreleased/36549-circuit-breaker-handles-missing-storages.yml b/changelogs/unreleased/36549-circuit-breaker-handles-missing-storages.yml
new file mode 100644
index 00000000000..f5ccb163d98
--- /dev/null
+++ b/changelogs/unreleased/36549-circuit-breaker-handles-missing-storages.yml
@@ -0,0 +1,5 @@
+---
+title: Allow the git circuit breaker to correctly handle missing repository storages
+merge_request: 14417
+author:
+type: fixed
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 94429ee91a9..27c1ecc7b23 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -499,9 +499,7 @@ Settings.backup['upload']['storage_class'] ||= nil
# Git
#
Settings['git'] ||= Settingslogic.new({})
-Settings.git['max_size'] ||= 20971520 # 20.megabytes
-Settings.git['bin_path'] ||= '/usr/bin/git'
-Settings.git['timeout'] ||= 10
+Settings.git['bin_path'] ||= '/usr/bin/git'
# Important: keep the satellites.path setting until GitLab 9.0 at
# least. This setting is fed to 'rm -rf' in
diff --git a/db/migrate/20170828135939_migrate_user_external_mail_data.rb b/db/migrate/20170828135939_migrate_user_external_mail_data.rb
index 592e141b7e6..395181a3b22 100644
--- a/db/migrate/20170828135939_migrate_user_external_mail_data.rb
+++ b/db/migrate/20170828135939_migrate_user_external_mail_data.rb
@@ -33,7 +33,7 @@ class MigrateUserExternalMailData < ActiveRecord::Migration
SELECT true
FROM user_synced_attributes_metadata
WHERE user_id = users.id
- AND provider = users.email_provider
+ AND provider = users.email_provider OR (provider IS NULL AND users.email_provider IS NULL)
)
AND id BETWEEN #{start_id} AND #{end_id}
EOF
diff --git a/db/post_migrate/20170828170502_post_deploy_migrate_user_external_mail_data.rb b/db/post_migrate/20170828170502_post_deploy_migrate_user_external_mail_data.rb
index fefd931e5d2..a475b242921 100644
--- a/db/post_migrate/20170828170502_post_deploy_migrate_user_external_mail_data.rb
+++ b/db/post_migrate/20170828170502_post_deploy_migrate_user_external_mail_data.rb
@@ -33,7 +33,7 @@ class PostDeployMigrateUserExternalMailData < ActiveRecord::Migration
SELECT true
FROM user_synced_attributes_metadata
WHERE user_id = users.id
- AND provider = users.email_provider
+ AND provider = users.email_provider OR (provider IS NULL AND users.email_provider IS NULL)
)
AND id BETWEEN #{start_id} AND #{end_id}
EOF
diff --git a/doc/development/ux_guide/basics.md b/doc/development/ux_guide/basics.md
index dcd5f677f25..e215026bcca 100644
--- a/doc/development/ux_guide/basics.md
+++ b/doc/development/ux_guide/basics.md
@@ -33,7 +33,7 @@ This is the typeface used for code blocks and references to commits, branches, a
## Icons
-GitLab has a strong, unique personality. When you look at any screen, you should know immediately know that it is GitLab.
+GitLab has a strong, unique personality. When you look at any screen, you should know immediately that it is GitLab.
Iconography is a powerful visual cue to the user and is a great way for us to reflect our particular sense of style.
- **Standard size:** 16px * 16px
diff --git a/doc/install/requirements.md b/doc/install/requirements.md
index f672b358096..17fe80fa93d 100644
--- a/doc/install/requirements.md
+++ b/doc/install/requirements.md
@@ -82,11 +82,11 @@ errors during usage.
We recommend having at least 2GB of swap on your server, even if you currently have
enough available RAM. Having swap will help reduce the chance of errors occurring
-if your available memory changes. We also recommend [configuring the kernels swappiness setting](https://askubuntu.com/a/103916)
+if your available memory changes. We also recommend [configuring the kernel's swappiness setting](https://askubuntu.com/a/103916)
to a low value like `10` to make the most of your RAM while still having the swap
available when needed.
-Notice: The 25 workers of Sidekiq will show up as separate processes in your process overview (such as top or htop) but they share the same RAM allocation since Sidekiq is a multithreaded application. Please see the section below about Unicorn workers for information about many you need of those.
+Notice: The 25 workers of Sidekiq will show up as separate processes in your process overview (such as top or htop) but they share the same RAM allocation since Sidekiq is a multithreaded application. Please see the section below about Unicorn workers for information about how many you need of those.
## Database
diff --git a/lib/gitlab/git/storage.rb b/lib/gitlab/git/storage.rb
index e28be4b8a38..08e6c29abad 100644
--- a/lib/gitlab/git/storage.rb
+++ b/lib/gitlab/git/storage.rb
@@ -11,6 +11,7 @@ module Gitlab
end
CircuitOpen = Class.new(Inaccessible)
+ Misconfiguration = Class.new(Inaccessible)
REDIS_KEY_PREFIX = 'storage_accessible:'.freeze
diff --git a/lib/gitlab/git/storage/circuit_breaker.rb b/lib/gitlab/git/storage/circuit_breaker.rb
index 9ea9367d4b7..1eaa2d83fb6 100644
--- a/lib/gitlab/git/storage/circuit_breaker.rb
+++ b/lib/gitlab/git/storage/circuit_breaker.rb
@@ -28,14 +28,26 @@ module Gitlab
def self.for_storage(storage)
cached_circuitbreakers = RequestStore.fetch(:circuitbreaker_cache) do
Hash.new do |hash, storage_name|
- hash[storage_name] = new(storage_name)
+ hash[storage_name] = build(storage_name)
end
end
cached_circuitbreakers[storage]
end
- def initialize(storage, hostname = Gitlab::Environment.hostname)
+ def self.build(storage, hostname = Gitlab::Environment.hostname)
+ config = Gitlab.config.repositories.storages[storage]
+
+ if !config.present?
+ NullCircuitBreaker.new(storage, hostname, error: Misconfiguration.new("Storage '#{storage}' is not configured"))
+ elsif !config['path'].present?
+ NullCircuitBreaker.new(storage, hostname, error: Misconfiguration.new("Path for storage '#{storage}' is not configured"))
+ else
+ new(storage, hostname)
+ end
+ end
+
+ def initialize(storage, hostname)
@storage = storage
@hostname = hostname
@@ -64,6 +76,10 @@ module Gitlab
recent_failure || too_many_failures
end
+ def failure_info
+ @failure_info ||= get_failure_info
+ end
+
# Memoizing the `storage_available` call means we only do it once per
# request when the storage is available.
#
@@ -121,10 +137,12 @@ module Gitlab
end
end
- def failure_info
- @failure_info ||= get_failure_info
+ def cache_key
+ @cache_key ||= "#{Gitlab::Git::Storage::REDIS_KEY_PREFIX}#{storage}:#{hostname}"
end
+ private
+
def get_failure_info
last_failure, failure_count = Gitlab::Git::Storage.redis.with do |redis|
redis.hmget(cache_key, :last_failure, :failure_count)
@@ -134,10 +152,6 @@ module Gitlab
FailureInfo.new(last_failure, failure_count.to_i)
end
-
- def cache_key
- @cache_key ||= "#{Gitlab::Git::Storage::REDIS_KEY_PREFIX}#{storage}:#{hostname}"
- end
end
end
end
diff --git a/lib/gitlab/git/storage/health.rb b/lib/gitlab/git/storage/health.rb
index 2d723147f4f..1564e94b7f7 100644
--- a/lib/gitlab/git/storage/health.rb
+++ b/lib/gitlab/git/storage/health.rb
@@ -78,7 +78,7 @@ module Gitlab
def failing_circuit_breakers
@failing_circuit_breakers ||= failing_on_hosts.map do |hostname|
- CircuitBreaker.new(storage_name, hostname)
+ CircuitBreaker.build(storage_name, hostname)
end
end
diff --git a/lib/gitlab/git/storage/null_circuit_breaker.rb b/lib/gitlab/git/storage/null_circuit_breaker.rb
new file mode 100644
index 00000000000..297c043d054
--- /dev/null
+++ b/lib/gitlab/git/storage/null_circuit_breaker.rb
@@ -0,0 +1,47 @@
+module Gitlab
+ module Git
+ module Storage
+ class NullCircuitBreaker
+ # These will have actual values
+ attr_reader :storage,
+ :hostname
+
+ # These will always have nil values
+ attr_reader :storage_path,
+ :failure_wait_time,
+ :failure_reset_time,
+ :storage_timeout
+
+ def initialize(storage, hostname, error: nil)
+ @storage = storage
+ @hostname = hostname
+ @error = error
+ end
+
+ def perform
+ @error ? raise(@error) : yield
+ end
+
+ def circuit_broken?
+ !!@error
+ end
+
+ def failure_count_threshold
+ 1
+ end
+
+ def last_failure
+ circuit_broken? ? Time.now : nil
+ end
+
+ def failure_count
+ circuit_broken? ? 1 : 0
+ end
+
+ def failure_info
+ Gitlab::Git::Storage::CircuitBreaker::FailureInfo.new(last_failure, failure_count)
+ end
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/health_checks/fs_shards_check.rb b/lib/gitlab/health_checks/fs_shards_check.rb
index a1905b091b4..afaa59b1018 100644
--- a/lib/gitlab/health_checks/fs_shards_check.rb
+++ b/lib/gitlab/health_checks/fs_shards_check.rb
@@ -125,7 +125,7 @@ module Gitlab
end
def storage_circuitbreaker_test(storage_name)
- Gitlab::Git::Storage::CircuitBreaker.new(storage_name).perform { "OK" }
+ Gitlab::Git::Storage::CircuitBreaker.build(storage_name).perform { "OK" }
rescue Gitlab::Git::Storage::Inaccessible
nil
end
diff --git a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
index c86353abb7c..98cf7966dad 100644
--- a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
+++ b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: true, broken_storage: true do
let(:storage_name) { 'default' }
- let(:circuit_breaker) { described_class.new(storage_name) }
+ let(:circuit_breaker) { described_class.new(storage_name, hostname) }
let(:hostname) { Gitlab::Environment.hostname }
let(:cache_key) { "storage_accessible:#{storage_name}:#{hostname}" }
@@ -22,7 +22,8 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
'failure_wait_time' => 30,
'failure_reset_time' => 1800,
'storage_timeout' => 5
- }
+ },
+ 'nopath' => { 'path' => nil }
)
end
@@ -59,6 +60,14 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
expect(breaker).to be_a(described_class)
expect(described_class.for_storage('default')).to eq(breaker)
end
+
+ it 'returns a broken circuit breaker for an unknown storage' do
+ expect(described_class.for_storage('unknown').circuit_broken?).to be_truthy
+ end
+
+ it 'returns a broken circuit breaker when the path is not set' do
+ expect(described_class.for_storage('nopath').circuit_broken?).to be_truthy
+ end
end
describe '#initialize' do
diff --git a/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb
new file mode 100644
index 00000000000..0e645008c88
--- /dev/null
+++ b/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb
@@ -0,0 +1,77 @@
+require 'spec_helper'
+
+describe Gitlab::Git::Storage::NullCircuitBreaker do
+ let(:storage) { 'default' }
+ let(:hostname) { 'localhost' }
+ let(:error) { nil }
+
+ subject(:breaker) { described_class.new(storage, hostname, error: error) }
+
+ context 'with an error' do
+ let(:error) { Gitlab::Git::Storage::Misconfiguration.new('error') }
+
+ describe '#perform' do
+ it { expect { breaker.perform { 'ok' } }.to raise_error(error) }
+ end
+
+ describe '#circuit_broken?' do
+ it { expect(breaker.circuit_broken?).to be_truthy }
+ end
+
+ describe '#last_failure' do
+ it { Timecop.freeze { expect(breaker.last_failure).to eq(Time.now) } }
+ end
+
+ describe '#failure_count' do
+ it { expect(breaker.failure_count).to eq(breaker.failure_count_threshold) }
+ end
+
+ describe '#failure_info' do
+ it { Timecop.freeze { expect(breaker.failure_info).to eq(Gitlab::Git::Storage::CircuitBreaker::FailureInfo.new(Time.now, breaker.failure_count_threshold)) } }
+ end
+ end
+
+ context 'not broken' do
+ describe '#perform' do
+ it { expect(breaker.perform { 'ok' }).to eq('ok') }
+ end
+
+ describe '#circuit_broken?' do
+ it { expect(breaker.circuit_broken?).to be_falsy }
+ end
+
+ describe '#last_failure' do
+ it { expect(breaker.last_failure).to be_nil }
+ end
+
+ describe '#failure_count' do
+ it { expect(breaker.failure_count).to eq(0) }
+ end
+
+ describe '#failure_info' do
+ it { expect(breaker.failure_info).to eq(Gitlab::Git::Storage::CircuitBreaker::FailureInfo.new(nil, 0)) }
+ end
+ end
+
+ describe '#failure_count_threshold' do
+ it { expect(breaker.failure_count_threshold).to eq(1) }
+ end
+
+ it 'implements the CircuitBreaker interface' do
+ ours = described_class.public_instance_methods
+ theirs = Gitlab::Git::Storage::CircuitBreaker.public_instance_methods
+
+ # These methods are not part of the public API, but are public to allow the
+ # CircuitBreaker specs to operate. They should be made private over time.
+ exceptions = %i[
+ cache_key
+ check_storage_accessible!
+ no_failures?
+ storage_available?
+ track_storage_accessible
+ track_storage_inaccessible
+ ]
+
+ expect(theirs - ours).to contain_exactly(*exceptions)
+ end
+end
diff --git a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb
index f5c9680bf59..73dd236a5c6 100644
--- a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb
+++ b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb
@@ -21,7 +21,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
let(:metric_class) { Gitlab::HealthChecks::Metric }
let(:result_class) { Gitlab::HealthChecks::Result }
- let(:repository_storages) { [:default] }
+ let(:repository_storages) { ['default'] }
let(:tmp_dir) { Dir.mktmpdir }
let(:storages_paths) do
@@ -64,7 +64,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
allow(described_class).to receive(:storage_circuitbreaker_test) { true }
end
- it { is_expected.to include(result_class.new(false, 'cannot stat storage', shard: :default)) }
+ it { is_expected.to include(result_class.new(false, 'cannot stat storage', shard: 'default')) }
end
context 'storage points to directory that has both read and write rights' do
@@ -72,7 +72,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
FileUtils.chmod_R(0755, tmp_dir)
end
- it { is_expected.to include(result_class.new(true, nil, shard: :default)) }
+ it { is_expected.to include(result_class.new(true, nil, shard: 'default')) }
it 'cleans up files used for testing' do
expect(described_class).to receive(:storage_write_test).with(any_args).and_call_original
@@ -85,7 +85,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
allow(described_class).to receive(:storage_read_test).with(any_args).and_return(false)
end
- it { is_expected.to include(result_class.new(false, 'cannot read from storage', shard: :default)) }
+ it { is_expected.to include(result_class.new(false, 'cannot read from storage', shard: 'default')) }
end
context 'write test fails' do
@@ -93,7 +93,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
allow(described_class).to receive(:storage_write_test).with(any_args).and_return(false)
end
- it { is_expected.to include(result_class.new(false, 'cannot write to storage', shard: :default)) }
+ it { is_expected.to include(result_class.new(false, 'cannot write to storage', shard: 'default')) }
end
end
end
@@ -109,7 +109,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
it 'provides metrics' do
metrics = described_class.metrics
- expect(metrics).to all(have_attributes(labels: { shard: :default }))
+ expect(metrics).to all(have_attributes(labels: { shard: 'default' }))
expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0))
expect(metrics).to include(an_object_having_attributes(name: :filesystem_readable, value: 0))
expect(metrics).to include(an_object_having_attributes(name: :filesystem_writable, value: 0))
@@ -128,7 +128,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
it 'provides metrics' do
metrics = described_class.metrics
- expect(metrics).to all(have_attributes(labels: { shard: :default }))
+ expect(metrics).to all(have_attributes(labels: { shard: 'default' }))
expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 1))
expect(metrics).to include(an_object_having_attributes(name: :filesystem_readable, value: 1))
expect(metrics).to include(an_object_having_attributes(name: :filesystem_writable, value: 1))
@@ -156,14 +156,14 @@ describe Gitlab::HealthChecks::FsShardsCheck do
describe '#readiness' do
subject { described_class.readiness }
- it { is_expected.to include(result_class.new(false, 'cannot stat storage', shard: :default)) }
+ it { is_expected.to include(result_class.new(false, 'cannot stat storage', shard: 'default')) }
end
describe '#metrics' do
it 'provides metrics' do
metrics = described_class.metrics
- expect(metrics).to all(have_attributes(labels: { shard: :default }))
+ expect(metrics).to all(have_attributes(labels: { shard: 'default' }))
expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0))
expect(metrics).to include(an_object_having_attributes(name: :filesystem_readable, value: 0))
expect(metrics).to include(an_object_having_attributes(name: :filesystem_writable, value: 0))
diff --git a/spec/support/stub_configuration.rb b/spec/support/stub_configuration.rb
index 45c10e78789..2dfb4d4a07f 100644
--- a/spec/support/stub_configuration.rb
+++ b/spec/support/stub_configuration.rb
@@ -42,7 +42,7 @@ module StubConfiguration
# Default storage is always required
messages['default'] ||= Gitlab.config.repositories.storages.default
messages.each do |storage_name, storage_settings|
- storage_settings['path'] ||= TestEnv.repos_path
+ storage_settings['path'] = TestEnv.repos_path unless storage_settings.key?('path')
storage_settings['failure_count_threshold'] ||= 10
storage_settings['failure_wait_time'] ||= 30
storage_settings['failure_reset_time'] ||= 1800
View Lorry Controller Status