diff --git a/Gemfile b/Gemfile index 5cf417a67ae3d0c96b49af115fca1d97abfa8fac..ae71dea4779a9cace87100ec4f4cd10bdc9fe61a 100644 --- a/Gemfile +++ b/Gemfile @@ -483,7 +483,7 @@ gem 'ssh_data', '~> 1.3' gem 'spamcheck', '~> 0.1.0' # Gitaly GRPC protocol definitions -gem 'gitaly', '~> 15.3.0-rc3' +gem 'gitaly', '~> 15.3.0-rc4' # KAS GRPC protocol definitions gem 'kas-grpc', '~> 0.0.2' diff --git a/Gemfile.lock b/Gemfile.lock index f04445e1d571b764a99445b35f7e1f13fb2196db..7ec954d9cea096a3c81a5a52bb27cc8590fa8391 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -508,7 +508,7 @@ GEM rails (>= 3.2.0) git (1.11.0) rchardet (~> 1.8) - gitaly (15.3.0.pre.rc3) + gitaly (15.3.0.pre.rc4) grpc (~> 1.0) github-markup (1.7.0) gitlab (4.16.1) @@ -1573,7 +1573,7 @@ DEPENDENCIES gettext (~> 3.3) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.3) - gitaly (~> 15.3.0.pre.rc3) + gitaly (~> 15.3.0.pre.rc4) github-markup (~> 1.7.0) gitlab-chronic (~> 0.10.5) gitlab-dangerfiles (~> 3.5.0) diff --git a/ee/spec/workers/geo/repository_sync_worker_spec.rb b/ee/spec/workers/geo/repository_sync_worker_spec.rb index d9a17861a18ff2f25ac4c9095f61c763c9240456..712fef9f64e4b6c8effb964488266958216a73be 100644 --- a/ee/spec/workers/geo/repository_sync_worker_spec.rb +++ b/ee/spec/workers/geo/repository_sync_worker_spec.rb @@ -64,6 +64,10 @@ # hide the 'broken' storage for this spec stub_storage_settings({}) + expect_next_instance_of(Gitlab::GitalyClient::ServerService) do |service| + expect(service).to receive(:readiness_check).and_return({ success: true }) + end + expect(repository_worker).to receive(:perform_async).with(project_in_synced_group.repository.storage) expect(design_worker).to receive(:perform_async).with(project_in_synced_group.repository.storage) expect(repository_worker).not_to receive(:perform_async).with('unknown') diff --git a/ee/spec/workers/geo/repository_verification/primary/batch_worker_spec.rb b/ee/spec/workers/geo/repository_verification/primary/batch_worker_spec.rb index c24c1b4095cef76a6adc01ade335b16e841bf911..491ea9339977f4198dfad8b61d4799bfcd8e21f7 100644 --- a/ee/spec/workers/geo/repository_verification/primary/batch_worker_spec.rb +++ b/ee/spec/workers/geo/repository_verification/primary/batch_worker_spec.rb @@ -45,6 +45,10 @@ # hide the 'broken' storage for this spec stub_storage_settings({}) + expect_next_instance_of(Gitlab::GitalyClient::ServerService) do |service| + expect(service).to receive(:readiness_check).and_return({ success: true }) + end + expect(Geo::RepositoryVerification::Primary::ShardWorker).to receive(:perform_async).with(healthy_shard) expect(Geo::RepositoryVerification::Primary::ShardWorker).not_to receive(:perform_async).with('unknown') diff --git a/ee/spec/workers/geo/repository_verification/secondary/scheduler_worker_spec.rb b/ee/spec/workers/geo/repository_verification/secondary/scheduler_worker_spec.rb index 2890971c8b92d9c7be0ee839c27adac8080976b5..e306a2c2c831955c28ff4fbc391dbefff41d9280 100644 --- a/ee/spec/workers/geo/repository_verification/secondary/scheduler_worker_spec.rb +++ b/ee/spec/workers/geo/repository_verification/secondary/scheduler_worker_spec.rb @@ -38,6 +38,10 @@ # hide the 'broken' storage for this spec stub_storage_settings({}) + expect_next_instance_of(Gitlab::GitalyClient::ServerService) do |service| + expect(service).to receive(:readiness_check).and_return({ success: true }) + end + expect(Geo::RepositoryVerification::Secondary::ShardWorker).to receive(:perform_async).with(healthy_shard) expect(Geo::RepositoryVerification::Secondary::ShardWorker).not_to receive(:perform_async).with('unknown') diff --git a/lib/gitlab/gitaly_client/server_service.rb b/lib/gitlab/gitaly_client/server_service.rb index 36bda67c26e68bc02f7a2a83b3b801810d2ac7d6..48fd0e66354ffc88840eb436cdee4f3ea07b90a3 100644 --- a/lib/gitlab/gitaly_client/server_service.rb +++ b/lib/gitlab/gitaly_client/server_service.rb @@ -26,6 +26,19 @@ def storage_disk_statistics storage_specific(disk_statistics) end + def readiness_check + request = Gitaly::ReadinessCheckRequest.new(timeout: GitalyClient.medium_timeout) + response = GitalyClient.call(@storage, :server_service, :readiness_check, request, timeout: GitalyClient.default_timeout) + + return { success: true } if response.ok_response + + failed_checks = response.failure_response.failed_checks.map do |failed_check| + ["#{failed_check.name}: #{failed_check.error_message}"] + end + + { success: false, message: failed_checks.join("\n") } + end + private def storage_specific(response) diff --git a/lib/gitlab/health_checks/gitaly_check.rb b/lib/gitlab/health_checks/gitaly_check.rb index f5f142c251fdb236f7e70edb30bcd9223bea46b8..2bd8ea711b5188f85fed9e639fc9bdfd4b165a83 100644 --- a/lib/gitlab/health_checks/gitaly_check.rb +++ b/lib/gitlab/health_checks/gitaly_check.rb @@ -27,17 +27,35 @@ def metrics end def check(storage_name) - serv = Gitlab::GitalyClient::HealthCheckService.new(storage_name) - result = serv.check + storage_healthy = healthy(storage_name) + unless storage_healthy[:success] + return HealthChecks::Result.new( + name, + storage_healthy[:success], + storage_healthy[:message], + shard: storage_name + ) + end + storage_ready = ready(storage_name) HealthChecks::Result.new( name, - result[:success], - result[:message], + storage_ready[:success], + storage_ready[:message], shard: storage_name ) end + def healthy(storage_name) + serv = Gitlab::GitalyClient::HealthCheckService.new(storage_name) + serv.check + end + + def ready(storage_name) + serv = Gitlab::GitalyClient::ServerService.new(storage_name) + serv.readiness_check + end + private def metric_prefix diff --git a/spec/lib/gitlab/gitaly_client/server_service_spec.rb b/spec/lib/gitlab/gitaly_client/server_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..615f2ce0c217dcffb0cc75fe2789cb0b5218dcb9 --- /dev/null +++ b/spec/lib/gitlab/gitaly_client/server_service_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::GitalyClient::ServerService do + let(:storage) { 'default' } + + describe '#readiness_check' do + before do + ::Gitlab::GitalyClient.clear_stubs! + end + + let(:request) do + Gitaly::ReadinessCheckRequest.new(timeout: 30) + end + + subject(:readiness_check) { described_class.new(storage).readiness_check } + + it 'returns a positive success if no failures happened' do + expect_next_instance_of(Gitaly::ServerService::Stub) do |service| + response = Gitaly::ReadinessCheckResponse.new(ok_response: Gitaly::ReadinessCheckResponse::Ok.new) + expect(service).to receive(:readiness_check).with(request, kind_of(Hash)).and_return(response) + end + + expect(readiness_check[:success]).to eq(true) + end + + it 'returns a negative success and a compiled message if at least one failure happened' do + failure1 = Gitaly::ReadinessCheckResponse::Failure::Response.new(name: '1', error_message: 'msg 1') + failure2 = Gitaly::ReadinessCheckResponse::Failure::Response.new(name: '2', error_message: 'msg 2') + failures = Gitaly::ReadinessCheckResponse::Failure.new(failed_checks: [failure1, failure2]) + response = Gitaly::ReadinessCheckResponse.new(failure_response: failures) + + expect_next_instance_of(Gitaly::ServerService::Stub) do |service| + expect(service).to receive(:readiness_check).with(request, kind_of(Hash)).and_return(response) + end + + expect(readiness_check[:success]).to eq(false) + expect(readiness_check[:message]).to eq("1: msg 1\n2: msg 2") + end + end +end diff --git a/spec/lib/gitlab/health_checks/gitaly_check_spec.rb b/spec/lib/gitlab/health_checks/gitaly_check_spec.rb index 7c346e3eb69fee890ac73a2a4944ca989cfa5313..85c9658aa6d654b63c8e0432129d0bf0897af22a 100644 --- a/spec/lib/gitlab/health_checks/gitaly_check_spec.rb +++ b/spec/lib/gitlab/health_checks/gitaly_check_spec.rb @@ -14,20 +14,36 @@ subject { described_class.readiness } before do - expect(Gitlab::GitalyClient::HealthCheckService).to receive(:new).and_return(gitaly_check) + expect(Gitlab::GitalyClient::HealthCheckService).to receive(:new).and_return(healthy_check) end context 'Gitaly server is up' do - let(:gitaly_check) { double(check: { success: true }) } + before do + expect(Gitlab::GitalyClient::ServerService).to receive(:new).and_return(ready_check) + end + + let(:healthy_check) { double(check: { success: true }) } + let(:ready_check) { double(readiness_check: { success: true }) } it { is_expected.to eq([result_class.new('gitaly_check', true, nil, shard: 'default')]) } end context 'Gitaly server is down' do - let(:gitaly_check) { double(check: { success: false, message: 'Connection refused' }) } + let(:healthy_check) { double(check: { success: false, message: 'Connection refused' }) } it { is_expected.to eq([result_class.new('gitaly_check', false, 'Connection refused', shard: 'default')]) } end + + context 'Gitaly server is not ready' do + before do + expect(Gitlab::GitalyClient::ServerService).to receive(:new).and_return(ready_check) + end + + let(:healthy_check) { double(check: { success: true }) } + let(:ready_check) { double(readiness_check: { success: false, message: 'Clock is out of sync' }) } + + it { is_expected.to match_array([result_class.new('gitaly_check', false, 'Clock is out of sync', shard: 'default')]) } + end end describe '#metrics' do diff --git a/spec/lib/gitlab/health_checks/probes/collection_spec.rb b/spec/lib/gitlab/health_checks/probes/collection_spec.rb index 741c45d953c316620e98c6a96a7e96c9c4851a04..3ec54642e24e8be68323061d22e8fa72095c1ae6 100644 --- a/spec/lib/gitlab/health_checks/probes/collection_spec.rb +++ b/spec/lib/gitlab/health_checks/probes/collection_spec.rb @@ -24,6 +24,10 @@ end it 'responds with readiness checks data' do + expect_next_instance_of(Gitlab::GitalyClient::ServerService) do |service| + expect(service).to receive(:readiness_check).and_return({ success: true }) + end + expect(subject.http_status).to eq(200) expect(subject.json[:status]).to eq('ok') diff --git a/spec/requests/health_controller_spec.rb b/spec/requests/health_controller_spec.rb index f70faf5bb9c796267377e183ba9fe78e5eeb0e40..fb1c0f85215158378c0d24c87c7048adb3afcf97 100644 --- a/spec/requests/health_controller_spec.rb +++ b/spec/requests/health_controller_spec.rb @@ -127,6 +127,10 @@ end it 'responds with readiness checks data' do + expect_next_instance_of(Gitlab::GitalyClient::ServerService) do |service| + expect(service).to receive(:readiness_check).and_return({ success: true }) + end + subject expect(json_response['db_check']).to contain_exactly({ 'status' => 'ok' }) diff --git a/spec/workers/repository_check/dispatch_worker_spec.rb b/spec/workers/repository_check/dispatch_worker_spec.rb index 829abc7d895f986b5e6b2451acb56e6bd6acad50..146228c085286842a1b3249bcd4f70e0354b469b 100644 --- a/spec/workers/repository_check/dispatch_worker_spec.rb +++ b/spec/workers/repository_check/dispatch_worker_spec.rb @@ -22,6 +22,10 @@ end it 'dispatches work to RepositoryCheck::BatchWorker' do + expect_next_instance_of(Gitlab::GitalyClient::ServerService) do |service| + expect(service).to receive(:readiness_check).and_return({ success: true }) + end + expect(RepositoryCheck::BatchWorker).to receive(:perform_async).at_least(:once) subject.perform