Merge branch...

Merge branch '4957-projectsyncworker-should-skip-projects-that-have-a-broken-gitaly-shard-3' into 'master'

Shard name vs. Shard updates, general tech debt cleanup

Closes #4957

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

ee/app/workers/geo/project_sync_worker.rb

@@ -39,8 +39,9 @@ module Geo
       registry.force_to_redownload_wiki = false
 
       if registry.changed? || registry.last_wiki_synced_at.nil? || registry.last_wiki_successful_sync_at.nil?
-        registry.last_wiki_synced_at = DateTime.now
-        registry.last_wiki_successful_sync_at = DateTime.now
+        now = DateTime.now
+        registry.last_wiki_synced_at = now
+        registry.last_wiki_successful_sync_at = now
 
         success = registry.save
         log_info("#{success ? 'Successfully marked' : 'Failed to mark'} disabled wiki as synced", registry_id: registry.id, project_id: registry.project_id)

ee/app/workers/geo/scheduler/per_shard_scheduler_worker.rb

@@ -12,33 +12,34 @@ module Geo
       ].freeze
 
       def perform
-        Gitlab::Geo::ShardHealthCache.update(eligible_shards)
+        Gitlab::Geo::ShardHealthCache.update(eligible_shard_names)
 
-        eligible_shards.each do |shard_name|
-          schedule_job(shard_name)
+        eligible_shard_names.each { |shard_name| schedule_job(shard_name) }
       end
+
+      def eligible_shard_names
+        healthy_shard_names
       end
 
       def schedule_job(shard_name)
         raise NotImplementedError
       end
 
-      def eligible_shards
-        healthy_shards
-      end
-
-      def healthy_shards
-        strong_memoize(:healthy_shards) do
+      def healthy_shard_names
+        strong_memoize(:healthy_shard_names) do
           # 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_ready_shards.map { |result| result.labels[:shard] }.compact.uniq
+        end
+      end
+
+      def ready_shards
         HEALTHY_SHARD_CHECKS.map(&:readiness)
-            .map { |check_result| check_result.select(&:success) }
-            .inject(:&)
-            .map { |check_result| check_result.labels[:shard] }
-            .compact
-            .uniq
       end
+
+      def healthy_ready_shards
+        ready_shards.map { |result| result.select(&:success) }.inject(:&)
       end
     end
   end

ee/app/workers/geo/scheduler/secondary/per_shard_scheduler_worker.rb

@@ -16,8 +16,8 @@ module Geo
           super
         end
 
-        def eligible_shards
-          selective_sync_filter(healthy_shards)
+        def eligible_shard_names
+          selective_sync_filter(healthy_shard_names)
         end
 
         def selective_sync_filter(shards)

ee/spec/workers/geo/project_sync_worker_spec.rb

@@ -15,7 +15,9 @@ RSpec.describe Geo::ProjectSyncWorker do
     end
 
     context 'when project could not be found' do
-      it 'does not raise an error' do
+      it 'logs an error and returns' do
+        expect(subject).to receive(:log_error).with("Couldn't find project, skipping syncing", project_id: 999)
+
         expect { subject.perform(999, Time.now) }.not_to raise_error
       end
     end

ee/spec/workers/geo/repository_sync_worker_spec.rb

@@ -9,7 +9,7 @@ describe Geo::RepositorySyncWorker, :geo, :clean_gitlab_redis_cache do
   let!(:project_in_synced_group) { create(:project, group: synced_group) }
   let!(:unsynced_project) { create(:project) }
 
-  let(:healthy_shard) { project_in_synced_group.repository.storage }
+  let(:healthy_shard_name) { project_in_synced_group.repository.storage }
 
   subject { described_class.new }
 
@@ -39,13 +39,13 @@ describe Geo::RepositorySyncWorker, :geo, :clean_gitlab_redis_cache do
       end
 
       it 'skips backfill for projects on shards excluded by selective sync' do
-        secondary.update!(selective_sync_type: 'shards', selective_sync_shards: [healthy_shard])
+        secondary.update!(selective_sync_type: 'shards', selective_sync_shards: [healthy_shard_name])
 
         # Report both shards as healthy
         expect(Gitlab::HealthChecks::FsShardsCheck).to receive(:readiness)
-          .and_return([result(true, healthy_shard), result(true, 'broken')])
+          .and_return([result(true, healthy_shard_name), result(true, 'broken')])
         expect(Gitlab::HealthChecks::GitalyCheck).to receive(:readiness)
-          .and_return([result(true, healthy_shard), result(true, 'broken')])
+          .and_return([result(true, healthy_shard_name), result(true, 'broken')])
 
         expect(Geo::RepositoryShardSyncWorker).to receive(:perform_async).with('default')
         expect(Geo::RepositoryShardSyncWorker).not_to receive(:perform_async).with('broken')
@@ -78,11 +78,11 @@ describe Geo::RepositorySyncWorker, :geo, :clean_gitlab_redis_cache do
 
         # Report only one healthy shard
         expect(Gitlab::HealthChecks::FsShardsCheck).to receive(:readiness)
-          .and_return([result(true, healthy_shard), result(true, 'broken')])
+          .and_return([result(true, healthy_shard_name), result(true, 'broken')])
         expect(Gitlab::HealthChecks::GitalyCheck).to receive(:readiness)
-          .and_return([result(true, healthy_shard), result(false, 'broken')])
+          .and_return([result(true, healthy_shard_name), result(false, 'broken')])
 
-        expect(Geo::RepositoryShardSyncWorker).to receive(:perform_async).with(healthy_shard)
+        expect(Geo::RepositoryShardSyncWorker).to receive(:perform_async).with(healthy_shard_name)
         expect(Geo::RepositoryShardSyncWorker).not_to receive(:perform_async).with('broken')
 
         subject.perform

ee/spec/workers/geo/scheduler/per_shard_scheduler_worker_spec.rb

 require 'spec_helper'
 
 describe Geo::Scheduler::PerShardSchedulerWorker do
-  it 'includes ::Gitlab::Geo::LogHelpers' do
+  it 'includes ApplicationWorker' do
+    expect(described_class).to include_module(ApplicationWorker)
+  end
+
+  it 'includes CronjobQueue' do
+    expect(described_class).to include_module(CronjobQueue)
+  end
+
+  it 'includes Gitlab::Utils::StrongMemoize' do
+    expect(described_class).to include_module(::Gitlab::Utils::StrongMemoize)
+  end
+
+  it 'includes Gitlab::Geo::LogHelpers' do
     expect(described_class).to include_module(::Gitlab::Geo::LogHelpers)
   end
 
-  it 'needs many other specs'
+  it 'includes FsShards and Gitaly health checks' do
+    expect(described_class::HEALTHY_SHARD_CHECKS).to include(
+      Gitlab::HealthChecks::FsShardsCheck,
+      Gitlab::HealthChecks::GitalyCheck
+    )
+  end
+
+  describe 'instance methods' do
+    subject(:per_shard_scheduler_worker) { described_class.new }
+
+    let(:default_shard_name) { 'default' }
+    let(:other_shard_name) { 'other' }
+    let(:unhealthy_shard_name) { 'unhealthy' }
+
+    let(:default_shard) { Gitlab::HealthChecks::Result.new(true, nil, shard: default_shard_name) }
+    let(:other_shard) { Gitlab::HealthChecks::Result.new(true, nil, shard: other_shard_name) }
+    let(:unhealthy_shard) { Gitlab::HealthChecks::Result.new(false, '14:Connect Failed', shard: unhealthy_shard_name) }
+
+    before do
+      allow(Gitlab::HealthChecks::FsShardsCheck).to receive(:readiness).and_return([default_shard, other_shard, unhealthy_shard])
+      allow(Gitlab::HealthChecks::GitalyCheck).to receive(:readiness).and_return([default_shard, other_shard, unhealthy_shard])
+    end
+
+    describe '#schedule_job' do
+      it "raises a NotImplementedError exception" do
+        expect do
+          per_shard_scheduler_worker.schedule_job(default_shard_name)
+        end.to raise_exception(NotImplementedError)
+      end
+    end
+
+    describe '#ready_shards' do
+      let(:ready_shards) { [[default_shard, other_shard, unhealthy_shard], [default_shard, other_shard, unhealthy_shard]] }
+
+      it "returns an array of ready shards" do
+        expect(per_shard_scheduler_worker.ready_shards).to eq(ready_shards)
+      end
+    end
+
+    describe '#healthy_ready_shards' do
+      let(:healthy_ready_shards) { [default_shard, other_shard] }
+
+      it "returns an array of healthy shard names" do
+        expect(per_shard_scheduler_worker.healthy_ready_shards).to eq(healthy_ready_shards)
+      end
+    end
+
+    describe '#healthy_shard_names' do
+      it "returns an array of healthy shard names" do
+        expect(per_shard_scheduler_worker.healthy_shard_names).to eq([default_shard_name, other_shard_name])
+      end
+    end
+  end
 end