Merge branch 'sh-gitaly-check-for-healthy-shards' into 'master'

Geo: Don't attempt to schedule a repository sync for downed Gitaly shards

Closes #4329

See merge request gitlab-org/gitlab-ee!4265

changelogs/unreleased-ee/sh-gitaly-check-for-healthy-shards.yml

+---
+title: 'Geo: Don''t attempt to schedule a repository sync for downed Gitaly shards'
+merge_request:
+author:
+type: changed

ee/app/workers/geo/repository_sync_worker.rb

@@ -3,6 +3,11 @@ module Geo
     include ApplicationWorker
     include CronjobQueue
 
+    HEALTHY_SHARD_CHECKS = [
+      Gitlab::HealthChecks::FsShardsCheck,
+      Gitlab::HealthChecks::GitalyCheck
+    ].freeze
+
     def perform
       return unless Gitlab::Geo.geo_database_configured?
       return unless Gitlab::Geo.secondary?
@@ -17,10 +22,13 @@ module Geo
     end
 
     def healthy_shards
-      Gitlab::HealthChecks::FsShardsCheck
-        .readiness
-        .select(&:success)
-        .map { |check| check.labels[:shard] }
+      # For now, we need to perform both Gitaly and direct filesystem checks to ensure
+      # the shard is healthy. We take the intersection of the successful checks
+      # as the healthy shards.
+      HEALTHY_SHARD_CHECKS.map(&:readiness)
+        .map { |check_result| check_result.select(&:success) }
+        .inject(:&)
+        .map { |check_result| check_result.labels[:shard] }
         .compact
         .uniq
     end

spec/ee/spec/workers/geo/repository_sync_worker_spec.rb

@@ -48,6 +48,27 @@ describe Geo::RepositorySyncWorker, :geo, :clean_gitlab_redis_cache do
 
         Sidekiq::Testing.inline! { subject.perform }
       end
+
+      it 'skips backfill for projects with downed Gitaly server' do
+        create(:project, group: synced_group, repository_storage: 'broken')
+        unhealthy_dirty = create(:project, group: synced_group, repository_storage: 'broken')
+        healthy_shard = project_in_synced_group.repository.storage
+
+        create(:geo_project_registry, :synced, :repository_dirty, project: unhealthy_dirty)
+
+        # Report only one healthy shard
+        allow(Gitlab::HealthChecks::FsShardsCheck).to receive(:readiness).and_return(
+          [Gitlab::HealthChecks::Result.new(true, nil, { shard: healthy_shard }),
+           Gitlab::HealthChecks::Result.new(true, nil, { shard: 'broken' })])
+        allow(Gitlab::HealthChecks::GitalyCheck).to receive(:readiness).and_return(
+          [Gitlab::HealthChecks::Result.new(true, nil, { shard: healthy_shard }),
+           Gitlab::HealthChecks::Result.new(false, nil, { shard: 'broken' })])
+
+        expect(Geo::RepositoryShardSyncWorker).to receive(:perform_async).with(healthy_shard)
+        expect(Geo::RepositoryShardSyncWorker).not_to receive(:perform_async).with('broken')
+
+        Sidekiq::Testing.inline! { subject.perform }
+      end
     end
   end
 end