Merge branch '6803-geo-make-repository-verification-feature-flag-by-default-opt-out' into 'master'

Resolve "Geo - Make Geo repository verification flag opt out by default"

Closes #6803

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

doc/administration/geo/disaster_recovery/background_verification.md

 # Automatic background verification **[PREMIUM ONLY]**
 
 NOTE: **Note:**
-Automatic background verification of repositories and wikis was added in GitLab
-EE 10.6, but is disabled by default. To enable it, run
-`sudo gitlab-rails runner 'Feature.enable(:geo_repository_verification)'` on
-the **primary**. Until [issue #5699][ee-5699] is completed, we need to reset
-the cache for this feature flag on each **secondary**, to do this run
- `sudo gitlab-rails runner 'Rails.cache.expire('flipper/v1/feature/geo_repository_verification', 0)'`.
+Automatic background verification of repositories and wikis was added in
+GitLab EE 10.6 but is enabled by default only on Gitlab EE 11.1. You can
+disable or enable this feature manually by following
+[these instructions][feature-flag].
 
 Automatic backgorund verification ensures that the transferred data matches a
 calculated checksum, proving that the content on the **secondary** matches that
@@ -30,6 +28,43 @@ sudo gitlab-rails runner 'Geo::ProjectRegistry.verification_failed.delete_all'
 If verification is lagging significantly behind replication, consider giving
 the node more time before scheduling a planned failover.
 
+### Disabling or enabling the automatic background verification
+
+The following commands are to be issues in a Rails console on
+the **primary**:
+
+```sh
+# Omnibus GitLab
+gitlab-rails console
+
+# Installation from source
+cd /home/git/gitlab
+sudo -u git -H bin/rails console RAILS_ENV=production
+```
+
+**To check if automatic background verification is enabled:**
+
+```ruby
+Feature.enabled?('geo_repository_verification')
+```
+
+**To disable automatic background verification:**
+
+```ruby
+Feature.disable('geo_repository_verification')
+```
+
+**To enable automatic background verification:**
+
+```ruby
+Feature.enable('geo_repository_verification')
+```
+
+NOTE: **Note:**
+Until [issue #5699][ee-5699] is completed, we need to reset the cache for this
+feature flag on each **secondary**, to do this run
+`sudo gitlab-rails runner 'Rails.cache.expire('flipper/v1/feature/geo_repository_verification', 0)'`.
+
 # Repository verification
 
 Visit the **Admin Area ➔ Geo nodes** dashboard on the **primary** and expand
@@ -59,13 +94,14 @@ for every node after every update to make sure that they are all in sync.
 
 Until [issue #5064][ee-5064] is completed, background verification doesn't cover
 CI job artifacts and traces, LFS objects, or user uploads in file storage.
-Verifytheir integrity manually by following [these instructions][foreground-verification]
+Verify their integrity manually by following [these instructions][foreground-verification]
 on both nodes, and comparing the output between them.
 
 Data in object storage is **not verified**, as the object store is responsible
 for ensuring the integrity of the data.
 
 [disaster-recovery]: index.md
+[feature-flag]: background_verification.md#enabling-or-disabling-the-automatic-background-verification
 [foreground-verification]: ../../raketasks/check.md
 [ee-5064]: https://gitlab.com/gitlab-org/gitlab-ee/issues/5064
 [ee-5699]: https://gitlab.com/gitlab-org/gitlab-ee/issues/5699

ee/app/models/geo_node_status.rb

@@ -130,7 +130,7 @@ class GeoNodeStatus < ActiveRecord::Base
   end
 
   def initialize_feature_flags
-    self.repository_verification_enabled = Feature.enabled?('geo_repository_verification')
+    self.repository_verification_enabled = Gitlab::Geo.repository_verification_enabled?
   end
 
   def update_cache!

ee/app/workers/geo/repository_verification/primary/batch_worker.rb

@@ -3,7 +3,7 @@ module Geo
     module Primary
       class BatchWorker < Geo::Scheduler::Primary::PerShardSchedulerWorker
         def perform
-          return unless Feature.enabled?('geo_repository_verification')
+          return unless Gitlab::Geo.repository_verification_enabled?
 
           super
         end

ee/app/workers/geo/repository_verification/secondary/scheduler_worker.rb

@@ -3,7 +3,7 @@ module Geo
     module Secondary
       class SchedulerWorker < Geo::Scheduler::Secondary::PerShardSchedulerWorker
         def perform
-          return unless Feature.enabled?('geo_repository_verification')
+          return unless Gitlab::Geo.repository_verification_enabled?
 
           super
         end

ee/changelogs/unreleased/6803-geo-make-repository-verification-feature-flag-by-default-opt-out.yml

+---
+title: Geo - Make Geo repository verification flag opt-out by default
+merge_request: 6369
+author:
+type: other

ee/lib/gitlab/geo.rb

@@ -109,5 +109,16 @@ module Gitlab
       # urlsafe_base64 may return a string of size * 4/3
       SecureRandom.urlsafe_base64(size)[0, size]
     end
+
+    def self.repository_verification_enabled?
+      feature = Feature.get(:geo_repository_verification)
+
+      # If the feature has been set, always evaluate
+      if Feature.persisted?(feature)
+        return feature.enabled?
+      else
+        true
+      end
+    end
   end
 end

ee/lib/tasks/geo.rake

@@ -252,7 +252,7 @@ namespace :geo do
     print "#{current_node_status.repositories_synced_count}/#{current_node_status.repositories_count} "
     puts using_percentage(current_node_status.repositories_synced_in_percentage)
 
-    if Feature.enabled?(:geo_repository_verification)
+    if Gitlab::Geo.repository_verification_enabled?
       print 'Verified Repositories: '.rjust(COLUMN_WIDTH)
       show_failed_value(current_node_status.repositories_verification_failed_count)
       print "#{current_node_status.repositories_verified_count}/#{current_node_status.repositories_count} "
@@ -264,7 +264,7 @@ namespace :geo do
     print "#{current_node_status.wikis_synced_count}/#{current_node_status.wikis_count} "
     puts using_percentage(current_node_status.wikis_synced_in_percentage)
 
-    if Feature.enabled?(:geo_repository_verification)
+    if Gitlab::Geo.repository_verification_enabled?
       print 'Verified Wikis: '.rjust(COLUMN_WIDTH)
       show_failed_value(current_node_status.wikis_verification_failed_count)
       print "#{current_node_status.wikis_verified_count}/#{current_node_status.wikis_count} "

ee/spec/lib/gitlab/geo_spec.rb

@@ -174,4 +174,34 @@ describe Gitlab::Geo, :geo do
       described_class.configure_cron_jobs!
     end
   end
+
+  describe '.repository_verification_enabled?' do
+    context "when the feature flag hasn't been set" do
+      it 'returns true' do
+        expect(described_class.repository_verification_enabled?).to eq true
+      end
+    end
+
+    context "when the feature flag has been set" do
+      before do
+        allow(Feature).to receive(:persisted?).and_return(true)
+      end
+
+      context "when the feature flag is set to enabled" do
+        it 'returns true' do
+          Feature.enable(:geo_repository_verification)
+
+          expect(described_class.repository_verification_enabled?).to eq true
+        end
+      end
+
+      context "when the feature flag is set to disabled" do
+        it 'returns false' do
+          Feature.disable(:geo_repository_verification)
+
+          expect(described_class.repository_verification_enabled?).to eq false
+        end
+      end
+    end
+  end
 end

ee/spec/models/geo_node_status_spec.rb

@@ -533,7 +533,6 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns the right number of checksummed repositories' do
-      stub_feature_flags(geo_repository_verification: true)
       create(:repository_state, :repository_verified)
       create(:repository_state, :repository_verified)
 
@@ -541,7 +540,7 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns existing value when feature flag is off' do
-      stub_feature_flags(geo_repository_verification: false)
+      allow(Gitlab::Geo).to receive(:repository_verification_enabled?).and_return(false)
       create(:geo_node_status, :healthy, geo_node: primary)
 
       expect(subject.repositories_checksummed_count).to eq(600)
@@ -554,7 +553,6 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns the right number of failed repositories' do
-      stub_feature_flags(geo_repository_verification: true)
       create(:repository_state, :repository_failed)
       create(:repository_state, :repository_failed)
 
@@ -562,7 +560,7 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns existing value when feature flag if off' do
-      stub_feature_flags(geo_repository_verification: false)
+      allow(Gitlab::Geo).to receive(:repository_verification_enabled?).and_return(false)
       create(:geo_node_status, :healthy, geo_node: primary)
 
       expect(subject.repositories_checksum_failed_count).to eq(120)
@@ -597,7 +595,6 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns the right number of checksummed wikis' do
-      stub_feature_flags(geo_repository_verification: true)
       create(:repository_state, :wiki_verified)
       create(:repository_state, :wiki_verified)
 
@@ -605,7 +602,7 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns existing value when feature flag if off' do
-      stub_feature_flags(geo_repository_verification: false)
+      allow(Gitlab::Geo).to receive(:repository_verification_enabled?).and_return(false)
       create(:geo_node_status, :healthy, geo_node: primary)
 
       expect(subject.wikis_checksummed_count).to eq(585)
@@ -618,7 +615,6 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns the right number of failed wikis' do
-      stub_feature_flags(geo_repository_verification: true)
       create(:repository_state, :wiki_failed)
       create(:repository_state, :wiki_failed)
 
@@ -626,7 +622,7 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns existing value when feature flag if off' do
-      stub_feature_flags(geo_repository_verification: false)
+      allow(Gitlab::Geo).to receive(:repository_verification_enabled?).and_return(false)
       create(:geo_node_status, :healthy, geo_node: primary)
 
       expect(subject.wikis_checksum_failed_count).to eq(55)
@@ -661,7 +657,6 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns the right number of verified repositories' do
-      stub_feature_flags(geo_repository_verification: true)
       create(:geo_project_registry, :repository_verified)
       create(:geo_project_registry, :repository_verified)
 
@@ -669,7 +664,7 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns existing value when feature flag if off' do
-      stub_feature_flags(geo_repository_verification: false)
+      allow(Gitlab::Geo).to receive(:repository_verification_enabled?).and_return(false)
       create(:geo_node_status, :healthy, geo_node: secondary)
 
       expect(subject.repositories_verified_count).to eq(501)
@@ -682,7 +677,6 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns the right number of repositories that checksum mismatch' do
-      stub_feature_flags(geo_repository_verification: true)
       create(:geo_project_registry, :repository_checksum_mismatch)
       create(:geo_project_registry, :repository_verification_failed)
       create(:geo_project_registry, :repository_verified)
@@ -691,7 +685,7 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns existing value when feature flag if off' do
-      stub_feature_flags(geo_repository_verification: false)
+      allow(Gitlab::Geo).to receive(:repository_verification_enabled?).and_return(false)
       create(:geo_node_status, :healthy, geo_node: secondary)
 
       expect(subject.repositories_checksum_mismatch_count).to eq(15)
@@ -704,7 +698,6 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns the right number of failed repositories' do
-      stub_feature_flags(geo_repository_verification: true)
       create(:geo_project_registry, :repository_verification_failed)
       create(:geo_project_registry, :repository_verification_failed)
 
@@ -712,7 +705,7 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns existing value when feature flag if off' do
-      stub_feature_flags(geo_repository_verification: false)
+      allow(Gitlab::Geo).to receive(:repository_verification_enabled?).and_return(false)
       create(:geo_node_status, :healthy, geo_node: secondary)
 
       expect(subject.repositories_verification_failed_count).to eq(100)
@@ -725,7 +718,6 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns the right number of verified wikis' do
-      stub_feature_flags(geo_repository_verification: true)
       create(:geo_project_registry, :wiki_verified)
       create(:geo_project_registry, :wiki_verified)
 
@@ -733,7 +725,7 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns existing value when feature flag if off' do
-      stub_feature_flags(geo_repository_verification: false)
+      allow(Gitlab::Geo).to receive(:repository_verification_enabled?).and_return(false)
       create(:geo_node_status, :healthy, geo_node: secondary)
 
       expect(subject.wikis_verified_count).to eq(499)
@@ -746,7 +738,6 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns the right number of wikis that checksum mismatch' do
-      stub_feature_flags(geo_repository_verification: true)
       create(:geo_project_registry, :wiki_checksum_mismatch)
       create(:geo_project_registry, :wiki_verification_failed)
       create(:geo_project_registry, :wiki_verified)
@@ -755,7 +746,7 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns existing value when feature flag if off' do
-      stub_feature_flags(geo_repository_verification: false)
+      allow(Gitlab::Geo).to receive(:repository_verification_enabled?).and_return(false)
       create(:geo_node_status, :healthy, geo_node: secondary)
 
       expect(subject.wikis_checksum_mismatch_count).to eq(10)
@@ -768,7 +759,6 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns the right number of failed wikis' do
-      stub_feature_flags(geo_repository_verification: true)
       create(:geo_project_registry, :wiki_verification_failed)
       create(:geo_project_registry, :wiki_verification_failed)
 
@@ -776,7 +766,7 @@ describe GeoNodeStatus, :geo do
     end
 
     it 'returns existing value when feature flag if off' do
-      stub_feature_flags(geo_repository_verification: false)
+      allow(Gitlab::Geo).to receive(:repository_verification_enabled?).and_return(false)
       create(:geo_node_status, :healthy, geo_node: secondary)
 
       expect(subject.wikis_verification_failed_count).to eq(99)

ee/spec/workers/geo/repository_verification/primary/batch_worker_spec.rb

@@ -17,66 +17,58 @@ describe Geo::RepositoryVerification::Primary::BatchWorker, :postgresql, :clean_
   end
 
   describe '#perform' do
-    context 'when geo_repository_verification is enabled' do
-      before do
-        stub_feature_flags(geo_repository_verification: true)
-      end
-
-      it 'skips backfill for repositories on other shards' do
-        create(:project, repository_storage: 'broken')
-        unhealthy_outdated = create(:project, repository_storage: 'broken')
+    it 'skips backfill for repositories on other shards' do
+      create(:project, repository_storage: 'broken')
+      unhealthy_outdated = create(:project, repository_storage: 'broken')
 
-        create(:repository_state, :repository_outdated, project: unhealthy_outdated)
+      create(:repository_state, :repository_outdated, project: unhealthy_outdated)
 
-        allow(Gitlab::GitalyClient).to receive(:call) do
-          raise GRPC::Unavailable.new('No Gitaly available')
-        end
+      allow(Gitlab::GitalyClient).to receive(:call) do
+        raise GRPC::Unavailable.new('No Gitaly available')
+      end
 
-        expect(Geo::RepositoryVerification::Primary::ShardWorker).not_to receive(:perform_async).with('broken')
+      expect(Geo::RepositoryVerification::Primary::ShardWorker).not_to receive(:perform_async).with('broken')
 
-        subject.perform
-      end
+      subject.perform
+    end
 
-      it 'skips backfill for projects on missing shards' do
-        missing_not_verified = create(:project)
-        missing_not_verified.update_column(:repository_storage, 'unknown')
-        missing_outdated = create(:project)
-        missing_outdated.update_column(:repository_storage, 'unknown')
+    it 'skips backfill for projects on missing shards' do
+      missing_not_verified = create(:project)
+      missing_not_verified.update_column(:repository_storage, 'unknown')
+      missing_outdated = create(:project)
+      missing_outdated.update_column(:repository_storage, 'unknown')
 
-        create(:repository_state, :repository_outdated, project: missing_outdated)
+      create(:repository_state, :repository_outdated, project: missing_outdated)
 
-        # hide the 'broken' storage for this spec
-        stub_storage_settings({})
+      # hide the 'broken' storage for this spec
+      stub_storage_settings({})
 
-        expect(Geo::RepositoryVerification::Primary::ShardWorker).to receive(:perform_async).with(healthy_shard)
-        expect(Geo::RepositoryVerification::Primary::ShardWorker).not_to receive(:perform_async).with('unknown')
+      expect(Geo::RepositoryVerification::Primary::ShardWorker).to receive(:perform_async).with(healthy_shard)
+      expect(Geo::RepositoryVerification::Primary::ShardWorker).not_to receive(:perform_async).with('unknown')
 
-        subject.perform
-      end
+      subject.perform
+    end
 
-      it 'skips backfill for projects with downed Gitaly server' do
-        create(:project, repository_storage: 'broken')
-        unhealthy_outdated = create(:project, repository_storage: 'broken')
+    it 'skips backfill for projects with downed Gitaly server' do
+      create(:project, repository_storage: 'broken')
+      unhealthy_outdated = create(:project, repository_storage: 'broken')
 
-        create(:repository_state, :repository_outdated, project: unhealthy_outdated)
+      create(:repository_state, :repository_outdated, project: unhealthy_outdated)
 
-        # Report only one healthy shard
-        expect(Gitlab::HealthChecks::GitalyCheck).to receive(:readiness)
-          .and_return([result(true, healthy_shard), result(false, 'broken')])
+      # Report only one healthy shard
+      expect(Gitlab::HealthChecks::GitalyCheck).to receive(:readiness)
+        .and_return([result(true, healthy_shard), result(false, 'broken')])
 
-        expect(Geo::RepositoryVerification::Primary::ShardWorker).to receive(:perform_async).with(healthy_shard)
-        expect(Geo::RepositoryVerification::Primary::ShardWorker).not_to receive(:perform_async).with('broken')
+      expect(Geo::RepositoryVerification::Primary::ShardWorker).to receive(:perform_async).with(healthy_shard)
+      expect(Geo::RepositoryVerification::Primary::ShardWorker).not_to receive(:perform_async).with('broken')
 
-        subject.perform
-      end
+      subject.perform
     end
 
     context 'when geo_repository_verification is disabled' do
-      before do
-        stub_feature_flags(geo_repository_verification: false)
-      end
-
       it 'does not schedule jobs' do
+        allow(Gitlab::Geo).to receive(:repository_verification_enabled?).and_return(false)
+
         expect(Geo::RepositoryVerification::Primary::ShardWorker)
           .not_to receive(:perform_async).with(healthy_shard)
 

ee/spec/workers/geo/repository_verification/secondary/scheduler_worker_spec.rb

@@ -17,68 +17,60 @@ describe Geo::RepositoryVerification::Secondary::SchedulerWorker, :postgresql, :
   end
 
   describe '#perform' do
-    context 'when geo_repository_verification is enabled' do
-      before do
-        stub_feature_flags(geo_repository_verification: true)
-      end
-
-      it 'skips verification for repositories on other shards' do
-        create(:project, repository_storage: 'broken')
+    it 'skips verification for repositories on other shards' do
+      create(:project, repository_storage: 'broken')
 
-        allow(Gitlab::GitalyClient).to receive(:call) do
-          raise GRPC::Unavailable.new('No Gitaly available')
-        end
+      allow(Gitlab::GitalyClient).to receive(:call) do
+        raise GRPC::Unavailable.new('No Gitaly available')
+      end
 
-        expect(Geo::RepositoryVerification::Secondary::ShardWorker).not_to receive(:perform_async).with('broken')
+      expect(Geo::RepositoryVerification::Secondary::ShardWorker).not_to receive(:perform_async).with('broken')
 
-        subject.perform
-      end
+      subject.perform
+    end
 
-      it 'skips verification for projects on missing shards' do
-        missing_not_verified = create(:project)
-        missing_not_verified.update_column(:repository_storage, 'unknown')
+    it 'skips verification for projects on missing shards' do
+      missing_not_verified = create(:project)
+      missing_not_verified.update_column(:repository_storage, 'unknown')
 
-        # hide the 'broken' storage for this spec
-        stub_storage_settings({})
+      # hide the 'broken' storage for this spec
+      stub_storage_settings({})
 
-        expect(Geo::RepositoryVerification::Secondary::ShardWorker).to receive(:perform_async).with(healthy_shard)
-        expect(Geo::RepositoryVerification::Secondary::ShardWorker).not_to receive(:perform_async).with('unknown')
+      expect(Geo::RepositoryVerification::Secondary::ShardWorker).to receive(:perform_async).with(healthy_shard)
+      expect(Geo::RepositoryVerification::Secondary::ShardWorker).not_to receive(:perform_async).with('unknown')
 
-        subject.perform
-      end
+      subject.perform
+    end
 
-      it 'skips verification for projects with downed Gitaly server' do
-        create(:project, repository_storage: 'broken')
+    it 'skips verification for projects with downed Gitaly server' do
+      create(:project, repository_storage: 'broken')
 
-        expect(Gitlab::HealthChecks::GitalyCheck).to receive(:readiness)
-          .and_return([result(true, healthy_shard), result(false, 'broken')])
+      expect(Gitlab::HealthChecks::GitalyCheck).to receive(:readiness)
+        .and_return([result(true, healthy_shard), result(false, 'broken')])
 
-        expect(Geo::RepositoryVerification::Secondary::ShardWorker).to receive(:perform_async).with(healthy_shard)
-        expect(Geo::RepositoryVerification::Secondary::ShardWorker).not_to receive(:perform_async).with('broken')
+      expect(Geo::RepositoryVerification::Secondary::ShardWorker).to receive(:perform_async).with(healthy_shard)
+      expect(Geo::RepositoryVerification::Secondary::ShardWorker).not_to receive(:perform_async).with('broken')
 
-        subject.perform
-      end
+      subject.perform
+    end
 
-      it 'skips verification for projects on shards excluded by selective sync' do
-        secondary.update!(selective_sync_type: 'shards', selective_sync_shards: [healthy_shard])
+    it 'skips verification for projects on shards excluded by selective sync' do
+      secondary.update!(selective_sync_type: 'shards', selective_sync_shards: [healthy_shard])
 
-        # Report both shards as healthy
-        expect(Gitlab::HealthChecks::GitalyCheck).to receive(:readiness)
-          .and_return([result(true, healthy_shard), result(true, 'broken')])
+      # Report both shards as healthy
+      expect(Gitlab::HealthChecks::GitalyCheck).to receive(:readiness)
+        .and_return([result(true, healthy_shard), result(true, 'broken')])
 
-        expect(Geo::RepositoryVerification::Secondary::ShardWorker).to receive(:perform_async).with(healthy_shard)
-        expect(Geo::RepositoryVerification::Secondary::ShardWorker).not_to receive(:perform_async).with('broken')
+      expect(Geo::RepositoryVerification::Secondary::ShardWorker).to receive(:perform_async).with(healthy_shard)
+      expect(Geo::RepositoryVerification::Secondary::ShardWorker).not_to receive(:perform_async).with('broken')
 
-        subject.perform
-      end
+      subject.perform
     end
 
     context 'when geo_repository_verification is disabled' do
-      before do
-        stub_feature_flags(geo_repository_verification: false)
-      end
-
       it 'does not schedule jobs' do
+        allow(Gitlab::Geo).to receive(:repository_verification_enabled?).and_return(false)
+
         expect(Geo::RepositoryVerification::Secondary::ShardWorker)
           .not_to receive(:perform_async).with(healthy_shard)