Merge branch '50345-hashed-storage-feature-flag' into 'master'

Feature flag to disable Hashed Storage migration when renaming a repository

Closes #50345

See merge request gitlab-org/gitlab-ce!21291

8 files changed, 125 insertions, 3 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 8f631d7f0ed..178d70757a1 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -2072,13 +2072,19 @@ class Project < ActiveRecord::Base
   private
 
   def rename_or_migrate_repository!
-    if Gitlab::CurrentSettings.hashed_storage_enabled? && storage_version != LATEST_STORAGE_VERSION
+    if Gitlab::CurrentSettings.hashed_storage_enabled? &&
+        storage_upgradable? &&
+        Feature.disabled?(:skip_hashed_storage_upgrade) # kill switch in case we need to disable upgrade behavior
       ::Projects::HashedStorageMigrationService.new(self, full_path_was).execute
     else
       storage.rename_repo
     end
   end
 
+  def storage_upgradable?
+    storage_version != LATEST_STORAGE_VERSION
+  end
+
   def after_rename_repository(full_path_before, path_before)
     execute_rename_repository_hooks!(full_path_before)
 
diff --git a/changelogs/unreleased/50345-hashed-storage-feature-flag.yml b/changelogs/unreleased/50345-hashed-storage-feature-flag.yml
new file mode 100644
index 00000000000..4c5182b843b
--- /dev/null
+++ b/changelogs/unreleased/50345-hashed-storage-feature-flag.yml
@@ -0,0 +1,5 @@
+---
+title: Feature flag to disable Hashed Storage migration when renaming a repository
+merge_request: 21291
+author:
+type: added
diff --git a/doc/development/feature_flags.md b/doc/development/feature_flags.md
index 09ea8c05be6..702caacc74f 100644
--- a/doc/development/feature_flags.md
+++ b/doc/development/feature_flags.md
@@ -57,3 +57,15 @@ end
 Features that are developed and are intended to be merged behind a feature flag
 should not include a changelog entry. The entry should be added in the merge
 request removing the feature flags.
+
+### Specs
+
+In the test environment `Feature.enabled?` is stubbed to always respond to `true`,
+so we make sure behavior under feature flag doesn't go untested in some non-specific
+contexts.
+ 
+If you need to test the feature flag in a different state, you need to stub it with: 
+
+```ruby
+stub_feature_flags(my_feature_flag: false)
+```
diff --git a/lib/feature.rb b/lib/feature.rb
index 09c5ef3ad94..24dbcb32fc0 100644
--- a/lib/feature.rb
+++ b/lib/feature.rb
@@ -47,7 +47,8 @@ class Feature
     end
 
     def disabled?(key, thing = nil)
-      !enabled?(key, thing)
+      # we need to make different method calls to make it easy to mock / define expectations in test mode
+      thing.nil? ? !enabled?(key) : !enabled?(key, thing)
     end
 
     def enable(key, thing = true)
diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb
index f313e675654..8bb5a843484 100644
--- a/spec/lib/feature_spec.rb
+++ b/spec/lib/feature_spec.rb
@@ -1,6 +1,13 @@
 require 'spec_helper'
 
 describe Feature do
+  before do
+    # We mock all calls to .enabled? to return true in order to force all
+    # specs to run the feature flag gated behavior, but here we need a clean
+    # behavior from the class
+    allow(described_class).to receive(:enabled?).and_call_original
+  end
+
   describe '.get' do
     let(:feature) { double(:feature) }
     let(:key) { 'my_feature' }
@@ -106,4 +113,63 @@ describe Feature do
       it_behaves_like 'a memoized Flipper instance'
     end
   end
+
+  describe '.enabled?' do
+    it 'returns false for undefined feature' do
+      expect(described_class.enabled?(:some_random_feature_flag)).to be_falsey
+    end
+
+    it 'returns false for existing disabled feature in the database' do
+      described_class.disable(:disabled_feature_flag)
+
+      expect(described_class.enabled?(:disabled_feature_flag)).to be_falsey
+    end
+
+    it 'returns true for existing enabled feature in the database' do
+      described_class.enable(:enabled_feature_flag)
+
+      expect(described_class.enabled?(:enabled_feature_flag)).to be_truthy
+    end
+
+    context 'with an individual actor' do
+      CustomActor = Struct.new(:flipper_id)
+
+      let(:actor) { CustomActor.new(flipper_id: 'CustomActor:5') }
+      let(:another_actor) { CustomActor.new(flipper_id: 'CustomActor:10') }
+
+      before do
+        described_class.enable(:enabled_feature_flag, actor)
+      end
+
+      it 'returns true when same actor is informed' do
+        expect(described_class.enabled?(:enabled_feature_flag, actor)).to be_truthy
+      end
+
+      it 'returns false when different actor is informed' do
+        expect(described_class.enabled?(:enabled_feature_flag, another_actor)).to be_falsey
+      end
+
+      it 'returns false when no actor is informed' do
+        expect(described_class.enabled?(:enabled_feature_flag)).to be_falsey
+      end
+    end
+  end
+
+  describe '.disable?' do
+    it 'returns true for undefined feature' do
+      expect(described_class.disabled?(:some_random_feature_flag)).to be_truthy
+    end
+
+    it 'returns true for existing disabled feature in the database' do
+      described_class.disable(:disabled_feature_flag)
+
+      expect(described_class.disabled?(:disabled_feature_flag)).to be_truthy
+    end
+
+    it 'returns false for existing enabled feature in the database' do
+      described_class.enable(:enabled_feature_flag)
+
+      expect(described_class.disabled?(:enabled_feature_flag)).to be_falsey
+    end
+  end
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 8cb706b54b0..7cfffbde42f 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2989,6 +2989,7 @@ describe Project do
         # call. This makes testing a bit easier.
         allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
         allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
+        stub_feature_flags(skip_hashed_storage_upgrade: false)
       end
 
       it 'renames a repository' do
@@ -3160,6 +3161,7 @@ describe Project do
         # call. This makes testing a bit easier.
         allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
         allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
+        stub_feature_flags(skip_hashed_storage_upgrade: false)
       end
 
       context 'migration to hashed storage' do
diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb
index 9572b4110d5..695b9980548 100644
--- a/spec/services/projects/update_service_spec.rb
+++ b/spec/services/projects/update_service_spec.rb
@@ -249,9 +249,20 @@ describe Projects::UpdateService do
           expect(project.errors.messages[:base]).to include('There is already a repository with that name on disk')
         end
 
-        context 'when hashed storage enabled' do
+        it 'renames the project without upgrading it' do
+          result = update_project(project, admin, path: 'new-path')
+
+          expect(result).not_to include(status: :error)
+          expect(project).to be_valid
+          expect(project.errors).to be_empty
+          expect(project.disk_path).to include('new-path')
+          expect(project.reload.hashed_storage?(:repository)).to be_falsey
+        end
+
+        context 'when hashed storage is enabled' do
           before do
             stub_application_setting(hashed_storage_enabled: true)
+            stub_feature_flags(skip_hashed_storage_upgrade: false)
           end
 
           it 'migrates project to a hashed storage instead of renaming the repo to another legacy name' do
@@ -262,6 +273,22 @@ describe Projects::UpdateService do
             expect(project.errors).to be_empty
             expect(project.reload.hashed_storage?(:repository)).to be_truthy
           end
+
+          context 'when skip_hashed_storage_upgrade feature flag is enabled' do
+            before do
+              stub_feature_flags(skip_hashed_storage_upgrade: true)
+            end
+
+            it 'renames the project without upgrading it' do
+              result = update_project(project, admin, path: 'new-path')
+
+              expect(result).not_to include(status: :error)
+              expect(project).to be_valid
+              expect(project.errors).to be_empty
+              expect(project.disk_path).to include('new-path')
+              expect(project.reload.hashed_storage?(:repository)).to be_falsey
+            end
+          end
         end
       end
 
diff --git a/spec/support/helpers/stub_feature_flags.rb b/spec/support/helpers/stub_feature_flags.rb
index b96338bf548..c54a871b157 100644
--- a/spec/support/helpers/stub_feature_flags.rb
+++ b/spec/support/helpers/stub_feature_flags.rb
@@ -1,4 +1,7 @@
 module StubFeatureFlags
+  # Stub Feature flags with `flag_name: true/false`
+  #
+  # @param [Hash] features where key is feature name and value is boolean whether enabled or not
   def stub_feature_flags(features)
     features.each do |feature_name, enabled|
       allow(Feature).to receive(:enabled?).with(feature_name) { enabled }