Merge branch 'mk/feature-flag-package-file-verification' into 'master'

Geo: Feature flag Self-service framework verification

See merge request gitlab-org/gitlab!46998

ee/app/models/concerns/geo/verifiable_replicator.rb

@@ -7,15 +7,41 @@ module Geo
     include Delay
 
     class_methods do
+      # If replication is disabled, then so is verification.
+      def verification_enabled?
+        enabled? && verification_feature_flag_enabled?
+      end
+
+      # Overridden by PackageFileReplicator with its own feature flag so we can
+      # release verification for PackageFileReplicator alone, at first.
+      # This feature flag name is not dynamic like the replication feature flag,
+      # because Geo is proliferating too many permanent feature flags, and if
+      # there is a serious bug with verification that needs to be shut off
+      # immediately, then the replication feature flag can be disabled until it
+      # is fixed. This feature flag is intended to be removed after it is
+      # defaulted on.
+      # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46998 for more
+      def verification_feature_flag_enabled?
+        Feature.enabled?(:geo_framework_verification)
+      end
+
       def checksummed
         model.available_replicables.checksummed
       end
 
       def checksummed_count
+        # When verification is disabled, this returns nil.
+        # Bonus: This causes the progress bar to be hidden.
+        return unless verification_enabled?
+
         model.available_replicables.checksummed.count
       end
 
       def checksum_failed_count
+        # When verification is disabled, this returns nil.
+        # Bonus: This causes the progress bar to be hidden.
+        return unless verification_enabled?
+
         model.available_replicables.checksum_failed.count
       end
     end
@@ -41,6 +67,7 @@ module Geo
     end
 
     def needs_checksum?
+      return false unless self.class.verification_enabled?
       return true unless model_record.respond_to?(:needs_checksum?)
 
       model_record.needs_checksum?

ee/app/replicators/geo/package_file_replicator.rb

@@ -8,6 +8,12 @@ module Geo
       ::Packages::PackageFile
     end
 
+    # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46998 for
+    # reasoning about this override.
+    def self.verification_feature_flag_enabled?
+      Feature.enabled?(:geo_package_file_verification)
+    end
+
     def carrierwave_uploader
       model_record.file
     end

ee/changelogs/unreleased/mk-feature-flag-package-file-verification.yml

+---
+title: 'Geo: Disable Self-service framework verification by default, and add package
+  file verification feature flag'
+merge_request: 46998
+author:
+type: added

ee/config/feature_flags/development/geo_framework_verification.yml

+---
+name: geo_framework_verification
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46998
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/277400
+milestone: '13.6'
+type: development
+group: group::geo
+default_enabled: false

ee/config/feature_flags/development/geo_package_file_verification.yml

+---
+name: geo_package_file_verification
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46998
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/271680
+milestone: '13.6'
+type: development
+group: group::geo
+default_enabled: false

ee/lib/gitlab/geo/geo_node_status_check.rb

@@ -82,8 +82,11 @@ module Gitlab
         [].tap do |status|
           Gitlab::Geo.enabled_replicator_classes.each do |replicator_class|
             status.push current_node_status.synced_in_percentage_for(replicator_class)
+
+            if replicator_class.verification_enabled?
               status.push current_node_status.checksummed_in_percentage_for(replicator_class)
             end
+          end
 
           if Gitlab::Geo.repository_verification_enabled?
             status.push current_node_status.repositories_verified_in_percentage
@@ -280,7 +283,9 @@ module Gitlab
       end
 
       def print_replicators_checked_status
-        Gitlab::Geo.enabled_replicator_classes.each do |replicator_class|
+        verifiable_replicator_classes = Gitlab::Geo.enabled_replicator_classes.select(&:verification_enabled?)
+
+        verifiable_replicator_classes.each do |replicator_class|
           print "#{replicator_class.replicable_title_plural} Checked: ".rjust(GEO_STATUS_COLUMN_WIDTH)
           show_failed_value(replicator_class.checksum_failed_count)
           print "#{replicator_class.checksummed_count}/#{replicator_class.registry_count} "

ee/spec/lib/gitlab/geo/geo_node_status_check_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe Gitlab::Geo::GeoNodeStatusCheck do
       allow(Gitlab.config.geo.registry_replication).to receive(:enabled).and_return(true)
     end
 
-    it 'prints messages for all verification checks' do
+    it 'prints messages for all legacy replication and verification checks' do
       checks = [
         /Repositories: /,
         /Verified Repositories: /,
@@ -27,14 +27,55 @@ RSpec.describe Gitlab::Geo::GeoNodeStatusCheck do
         /Container repositories: /,
         /Design repositories: /,
         /Repositories Checked: /
-      ] + Gitlab::Geo.enabled_replicator_classes.map { |k| /#{k.replicable_title_plural} Checked:/ } +
-          Gitlab::Geo.enabled_replicator_classes.map { |k| /#{k.replicable_title_plural}:/ }
+      ]
 
       checks.each do |text|
         expect { subject.print_replication_verification_status }.to output(text).to_stdout
       end
     end
 
+    context 'Replicators' do
+      let(:replicators) { Gitlab::Geo.enabled_replicator_classes }
+
+      context 'replication' do
+        let(:checks) do
+          replicators.map { |k| /#{k.replicable_title_plural}:/ }
+        end
+
+        it 'prints messages for replication' do
+          checks.each do |text|
+            expect { subject.print_replication_verification_status }.to output(text).to_stdout
+          end
+        end
+      end
+
+      context 'verification' do
+        let(:checks) do
+          replicators.map { |k| /#{k.replicable_title_plural} Checked:/ }
+        end
+
+        context 'when verification is enabled' do
+          it 'prints messages for verification checks' do
+            checks.each do |text|
+              expect { subject.print_replication_verification_status }.to output(text).to_stdout
+            end
+          end
+        end
+
+        context 'when verification is disabled' do
+          it 'does not print messages for verification checks' do
+            replicators.each do |replicator|
+              allow(replicator).to receive(:verification_enabled?).and_return(false)
+            end
+
+            checks.each do |text|
+              expect { subject.print_replication_verification_status }.not_to output(text).to_stdout
+            end
+          end
+        end
+      end
+    end
+
     context 'when replication is up-to-date' do
       it 'returns true' do
         expect(subject.replication_verification_complete?).to be_truthy

ee/spec/models/geo_node_status_spec.rb

@@ -1146,6 +1146,7 @@ RSpec.describe GeoNodeStatus, :geo do
     end
   end
 
+  context 'Replicator stats' do
     where(:replicator, :model_factory, :registry_factory) do
       Geo::MergeRequestDiffReplicator      | :external_merge_request_diff | :geo_merge_request_diff_registry
       Geo::PackageFileReplicator           | :package_file                | :geo_package_file_registry
@@ -1155,92 +1156,130 @@ RSpec.describe GeoNodeStatus, :geo do
 
     with_them do
       let(:replicable_name) { replicator.replicable_name_plural }
-    let(:checksummed_count_method) { "#{replicable_name}_checksummed_count" }
-    let(:checksum_failed_count_method) { "#{replicable_name}_checksum_failed_count" }
-    let(:checksummed_in_percentage_method) { "#{replicable_name}_checksummed_in_percentage" }
+
+      context 'replication' do
         let(:registry_count_method) { "#{replicable_name}_registry_count" }
         let(:failed_count_method) { "#{replicable_name}_failed_count" }
         let(:synced_count_method) { "#{replicable_name}_synced_count" }
         let(:synced_in_percentage_method) { "#{replicable_name}_synced_in_percentage" }
 
-    describe '#<replicable_name>_checksummed_count' do
+        describe '#<replicable_name>_[registry|synced|failed]_count' do
+          context 'when package registries available' do
             before do
-        stub_current_geo_node(primary)
+              create(registry_factory, :failed)
+              create(registry_factory, :failed)
+              create(registry_factory, :synced)
             end
 
-      it 'returns the right number of checksummed replicables' do
-        create(model_factory, :checksummed)
-        create(model_factory, :checksummed)
-        create(model_factory, :checksum_failure)
+            it 'returns the right number of repos in registry' do
+              expect(subject.send(registry_count_method)).to eq(3)
+            end
 
-        expect(subject.send(checksummed_count_method)).to eq(2)
+            it 'returns the right number of failed and synced repos' do
+              expect(subject.send(failed_count_method)).to eq(2)
+              expect(subject.send(synced_count_method)).to eq(1)
+            end
+
+            it 'returns the percent of synced replicables' do
+              expect(subject.send(synced_in_percentage_method)).to be_within(0.01).of(33.33)
             end
           end
 
-    describe '#<replicable_name>_checksum_failed_count' do
+          context 'when no package registries available' do
+            it 'returns 0' do
+              expect(subject.send(registry_count_method)).to eq(0)
+              expect(subject.send(failed_count_method)).to eq(0)
+              expect(subject.send(synced_count_method)).to eq(0)
+            end
+
+            it 'returns 0' do
+              expect(subject.send(synced_in_percentage_method)).to eq(0)
+            end
+          end
+        end
+      end
+
+      context 'verification' do
+        let(:checksummed_count_method) { "#{replicable_name}_checksummed_count" }
+        let(:checksum_failed_count_method) { "#{replicable_name}_checksum_failed_count" }
+        let(:checksummed_in_percentage_method) { "#{replicable_name}_checksummed_in_percentage" }
+
+        context 'when verification is enabled' do
           before do
             stub_current_geo_node(primary)
+            allow(replicator).to receive(:verification_enabled?).and_return(true)
           end
 
-      it 'returns the right number of failed replicables' do
+          context 'when there are replicables' do
+            before do
+              create(model_factory, :checksummed)
               create(model_factory, :checksummed)
               create(model_factory, :checksum_failure)
-        create(model_factory, :checksum_failure)
+            end
 
-        expect(subject.send(checksum_failed_count_method)).to eq(2)
+            describe '#<replicable_name>_checksummed_count' do
+              it 'returns the right number of checksummed replicables' do
+                expect(subject.send(checksummed_count_method)).to eq(2)
               end
             end
 
-    describe '#<replicable_name>_checksummed_in_percentage' do
-      before do
-        stub_current_geo_node(primary)
+            describe '#<replicable_name>_checksum_failed_count' do
+              it 'returns the right number of failed replicables' do
+                expect(subject.send(checksum_failed_count_method)).to eq(1)
               end
-
-      it 'returns 0 when no replicables available' do
-        expect(subject.send(checksummed_in_percentage_method)).to eq(0)
             end
 
+            describe '#<replicable_name>_checksummed_in_percentage' do
               it 'returns the right percentage' do
-        create(model_factory, :checksummed)
-        create(model_factory, :checksummed)
-        create(model_factory, :checksummed)
-        create(model_factory, :checksum_failure)
+                expect(subject.send(checksummed_in_percentage_method)).to be_within(0.01).of(66.67)
+              end
+            end
+          end
 
-        expect(subject.send(checksummed_in_percentage_method)).to be_within(0.0001).of(75)
+          context 'when there are no replicables' do
+            describe '#<replicable_name>_checksummed_count' do
+              it 'returns 0' do
+                expect(subject.send(checksummed_count_method)).to eq(0)
               end
             end
 
-    describe '#<replicable_name>_[registry|synced|failed]_count' do
-      context 'when package registries available' do
-        before do
-          create(registry_factory, :failed)
-          create(registry_factory, :failed)
-          create(registry_factory, :synced)
+            describe '#<replicable_name>_checksum_failed_count' do
+              it 'returns 0' do
+                expect(subject.send(checksum_failed_count_method)).to eq(0)
+              end
             end
 
-        it 'returns the right number of repos in registry' do
-          expect(subject.send(registry_count_method)).to eq(3)
+            describe '#<replicable_name>_checksummed_in_percentage' do
+              it 'returns 0' do
+                expect(subject.send(checksummed_in_percentage_method)).to eq(0)
+              end
+            end
+          end
         end
 
-        it 'returns the right number of failed and synced repos' do
-          expect(subject.send(failed_count_method)).to eq(2)
-          expect(subject.send(synced_count_method)).to eq(1)
+        context 'when verification is disabled' do
+          before do
+            stub_current_geo_node(primary)
+            allow(replicator).to receive(:verification_enabled?).and_return(false)
           end
 
-        it 'returns the percent of synced replicables' do
-          expect(subject.send(synced_in_percentage_method)).to be_within(0.01).of(33.33)
+          describe '#<replicable_name>_checksummed_count' do
+            it 'returns nil' do
+              expect(subject.send(checksummed_count_method)).to be_nil
             end
           end
 
-      context 'when no package registries available' do
-        it 'returns 0' do
-          expect(subject.send(registry_count_method)).to eq(0)
-          expect(subject.send(failed_count_method)).to eq(0)
-          expect(subject.send(synced_count_method)).to eq(0)
+          describe '#<replicable_name>_checksum_failed_count' do
+            it 'returns nil' do
+              expect(subject.send(checksum_failed_count_method)).to be_nil
+            end
           end
 
+          describe '#<replicable_name>_checksummed_in_percentage' do
             it 'returns 0' do
-          expect(subject.send(synced_in_percentage_method)).to eq(0)
+              expect(subject.send(checksummed_in_percentage_method)).to eq(0)
+            end
+          end
         end
       end
     end

ee/spec/support/shared_examples/models/concerns/repository_replicator_strategy_shared_examples.rb

@@ -19,6 +19,7 @@ RSpec.shared_examples 'a repository replicator' do
   end
 
   it_behaves_like 'a replicator'
+  it_behaves_like 'a verifiable replicator'
 
   # This could be included in each model's spec, but including it here is DRYer.
   include_examples 'a replicable model'

ee/spec/support/shared_examples/models/concerns/verifiable_replicator_shared_examples.rb

@@ -5,6 +5,40 @@
 RSpec.shared_examples 'a verifiable replicator' do
   include EE::GeoHelpers
 
+  describe '.verification_enabled?' do
+    context 'when replication is enabled' do
+      before do
+        expect(described_class).to receive(:enabled?).and_return(true)
+      end
+
+      context 'when the verification feature flag is enabled' do
+        it 'returns true' do
+          allow(described_class).to receive(:verification_feature_flag_enabled?).and_return(true)
+
+          expect(described_class.verification_enabled?).to be_truthy
+        end
+      end
+
+      context 'when geo_framework_verification feature flag is disabled' do
+        it 'returns false' do
+          allow(described_class).to receive(:verification_feature_flag_enabled?).and_return(false)
+
+          expect(described_class.verification_enabled?).to be_falsey
+        end
+      end
+    end
+
+    context 'when replication is disabled' do
+      before do
+        expect(described_class).to receive(:enabled?).and_return(false)
+      end
+
+      it 'returns false' do
+        expect(described_class.verification_enabled?).to be_falsey
+      end
+    end
+  end
+
   describe '#after_verifiable_update' do
     it 'schedules the checksum calculation if needed' do
       expect(replicator).to receive(:schedule_checksum_calculation)
@@ -15,18 +49,20 @@ RSpec.shared_examples 'a verifiable replicator' do
   end
 
   describe '#calculate_checksum!' do
-    it 'calculates the checksum' do
+    before do
       model_record.save!
+    end
+
+    it 'calculates the checksum' do
+      expect(model_record).to receive(:calculate_checksum!).and_return('abc123')
 
       replicator.calculate_checksum!
 
-      expect(model_record.reload.verification_checksum).not_to be_nil
-      expect(model_record.reload.verified_at).not_to be_nil
+      expect(model_record.reload.verification_checksum).to eq('abc123')
+      expect(model_record.verified_at).not_to be_nil
     end
 
     it 'saves the error message and increments retry counter' do
-      model_record.save!
-
       allow(model_record).to receive(:calculate_checksum!) do
         raise StandardError.new('Failure to calculate checksum')
       end