Replace pages migraiton feature flag with the ENV variable

Currently we mark pages projects as not deployed if
feature flag is enabled.

We want to give users control over this behavior and
I'm afraid of just enabling feature flag for everyone.

In worst case scenario enabling this feature flag and running
migration may result in all pages projects marked as not deployed.
(e.g. if users run task on the node without NFS share attached)

So the idea is:
* we'll run the migration in background without this behavior enabled
* users will manually run the task and see all the errors logged
* if the decide that these errors are legit, they can mark project
as "pages not deployed" by re-running rake task with the ENV variable
enabled

app/services/pages/migrate_from_legacy_storage_service.rb

@@ -2,11 +2,12 @@
 
 module Pages
   class MigrateFromLegacyStorageService
-    def initialize(logger, migration_threads:, batch_size:, ignore_invalid_entries:)
+    def initialize(logger, migration_threads:, batch_size:, ignore_invalid_entries:, mark_projects_as_not_deployed:)
       @logger = logger
       @migration_threads = migration_threads
       @batch_size = batch_size
       @ignore_invalid_entries = ignore_invalid_entries
+      @mark_projects_as_not_deployed = mark_projects_as_not_deployed
 
       @migrated = 0
       @errored = 0
@@ -60,7 +61,9 @@ module Pages
     def migrate_project(project)
       result = nil
       time = Benchmark.realtime do
-        result = ::Pages::MigrateLegacyStorageToDeploymentService.new(project, ignore_invalid_entries: @ignore_invalid_entries).execute
+        result = ::Pages::MigrateLegacyStorageToDeploymentService.new(project,
+                                                                      ignore_invalid_entries: @ignore_invalid_entries,
+                                                                      mark_projects_as_not_deployed: @mark_projects_as_not_deployed).execute
       end
 
       if result[:status] == :success

app/services/pages/migrate_legacy_storage_to_deployment_service.rb

@@ -9,9 +9,10 @@ module Pages
 
     attr_reader :project
 
-    def initialize(project, ignore_invalid_entries: false)
+    def initialize(project, ignore_invalid_entries: false, mark_projects_as_not_deployed: false)
       @project = project
       @ignore_invalid_entries = ignore_invalid_entries
+      @mark_projects_as_not_deployed = mark_projects_as_not_deployed
     end
 
     def execute
@@ -36,10 +37,12 @@ module Pages
       archive_path = zip_result[:archive_path]
 
       unless archive_path
+        return error("Archive not created. Missing public directory in #{@project.pages_path}") unless @mark_projects_as_not_deployed
+
         project.set_first_pages_deployment!(nil)
 
         return success(
-          message: "Archive not created. Missing public directory in #{project.pages_path} ? Marked project as not deployed")
+          message: "Archive not created. Missing public directory in #{project.pages_path}? Marked project as not deployed")
       end
 
       deployment = nil

app/services/pages/zip_directory_service.rb

@@ -18,13 +18,7 @@ module Pages
     end
 
     def execute
-      unless resolve_public_dir
-        if Feature.enabled?(:pages_migration_mark_as_not_deployed)
-          return success
-        end
-
-        return error("Can not find valid public dir in #{@input_dir}")
-      end
+      return success unless resolve_public_dir
 
       output_file = File.join(real_dir, "@migrated.zip") # '@' to avoid any name collision with groups or projects
 

changelogs/unreleased/295187-roll-out-pages_migration_mark_as_not_deployed-feature-flag.yml

+---
+title: Allow users to mark pages projects as not deployed during migration to zip
+  storage
+merge_request: 55862
+author:
+type: added

config/feature_flags/development/pages_migration_mark_as_not_deployed.yml

----
-name: pages_migration_mark_as_not_deployed
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49473
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/295187
-milestone: '13.8'
-type: development
-group: group::release
-default_enabled: false

lib/tasks/gitlab/pages.rake

@@ -11,7 +11,8 @@ namespace :gitlab do
       result = ::Pages::MigrateFromLegacyStorageService.new(logger,
                                                             migration_threads: migration_threads,
                                                             batch_size: batch_size,
-                                                            ignore_invalid_entries: ignore_invalid_entries).execute
+                                                            ignore_invalid_entries: ignore_invalid_entries,
+                                                            mark_projects_as_not_deployed: mark_projects_as_not_deployed).execute
 
       logger.info("A total of #{result[:migrated] + result[:errored]} projects were processed.")
       logger.info("- The #{result[:migrated]} projects migrated successfully")
@@ -51,5 +52,11 @@ namespace :gitlab do
         ENV.fetch('PAGES_MIGRATION_IGNORE_INVALID_ENTRIES', 'false')
       )
     end
+
+    def mark_projects_as_not_deployed
+      Gitlab::Utils.to_boolean(
+        ENV.fetch('PAGES_MIGRATION_MARK_PROJECTS_AS_NOT_DEPLOYED', 'false')
+      )
+    end
   end
 end

spec/services/pages/migrate_from_legacy_storage_service_spec.rb

@@ -3,7 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe Pages::MigrateFromLegacyStorageService do
-  let(:service) { described_class.new(Rails.logger, migration_threads: 3, batch_size: 10, ignore_invalid_entries: false) }
+  let(:batch_size) { 10 }
+  let(:mark_projects_as_not_deployed) { false }
+  let(:service) { described_class.new(Rails.logger, migration_threads: 3, batch_size: batch_size, ignore_invalid_entries: false, mark_projects_as_not_deployed: mark_projects_as_not_deployed) }
 
   it 'does not try to migrate pages if pages are not deployed' do
     expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new)
@@ -11,33 +13,35 @@ RSpec.describe Pages::MigrateFromLegacyStorageService do
     expect(service.execute).to eq(migrated: 0, errored: 0)
   end
 
-  it 'uses multiple threads' do
-    projects = create_list(:project, 20)
-    projects.each do |project|
-      project.mark_pages_as_deployed
+  context 'when there is work for multiple threads' do
+    let(:batch_size) { 2 } # override to force usage of multiple threads
+
+    it 'uses multiple threads' do
+      projects = create_list(:project, 20)
+      projects.each do |project|
+        project.mark_pages_as_deployed
 
-      FileUtils.mkdir_p File.join(project.pages_path, "public")
-      File.open(File.join(project.pages_path, "public/index.html"), "w") do |f|
-        f.write("Hello!")
+        FileUtils.mkdir_p File.join(project.pages_path, "public")
+        File.open(File.join(project.pages_path, "public/index.html"), "w") do |f|
+          f.write("Hello!")
+        end
       end
-    end
 
-    service = described_class.new(Rails.logger, migration_threads: 3, batch_size: 2, ignore_invalid_entries: false)
+      threads = Concurrent::Set.new
 
-    threads = Concurrent::Set.new
+      expect(service).to receive(:migrate_project).exactly(20).times.and_wrap_original do |m, *args|
+        threads.add(Thread.current)
 
-    expect(service).to receive(:migrate_project).exactly(20).times.and_wrap_original do |m, *args|
-      threads.add(Thread.current)
+        # sleep to be 100% certain that once thread can't consume all the queue
+        # it works without it, but I want to avoid making this test flaky
+        sleep(0.01)
 
-      # sleep to be 100% certain that once thread can't consume all the queue
-      # it works without it, but I want to avoid making this test flaky
-      sleep(0.01)
+        m.call(*args)
+      end
 
-      m.call(*args)
+      expect(service.execute).to eq(migrated: 20, errored: 0)
+      expect(threads.length).to eq(3)
     end
-
-    expect(service.execute).to eq(migrated: 20, errored: 0)
-    expect(threads.length).to eq(3)
   end
 
   context 'when pages are marked as deployed' do
@@ -48,12 +52,24 @@ RSpec.describe Pages::MigrateFromLegacyStorageService do
     end
 
     context 'when pages directory does not exist' do
-      it 'counts project as migrated' do
-        expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false) do |service|
+      context 'when mark_projects_as_not_deployed is set' do
+        let(:mark_projects_as_not_deployed) { true }
+
+        it 'counts project as migrated' do
+          expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: true) do |service|
+            expect(service).to receive(:execute).and_call_original
+          end
+
+          expect(service.execute).to eq(migrated: 1, errored: 0)
+        end
+      end
+
+      it 'counts project as errored' do
+        expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: false) do |service|
           expect(service).to receive(:execute).and_call_original
         end
 
-        expect(service.execute).to eq(migrated: 1, errored: 0)
+        expect(service.execute).to eq(migrated: 0, errored: 1)
       end
     end
 
@@ -66,7 +82,7 @@ RSpec.describe Pages::MigrateFromLegacyStorageService do
       end
 
       it 'migrates pages projects without deployments' do
-        expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false) do |service|
+        expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: false) do |service|
           expect(service).to receive(:execute).and_call_original
         end
 

spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb

@@ -11,47 +11,47 @@ RSpec.describe Pages::MigrateLegacyStorageToDeploymentService do
       expect(zip_service).to receive(:execute).and_call_original
     end
 
-    expect(described_class.new(project, ignore_invalid_entries: true).execute[:status]).to eq(:success)
+    expect(described_class.new(project, ignore_invalid_entries: true).execute[:status]).to eq(:error)
   end
 
-  it 'marks pages as not deployed if public directory is absent' do
-    project.mark_pages_as_deployed
+  context 'when mark_projects_as_not_deployed is passed' do
+    let(:service) { described_class.new(project, mark_projects_as_not_deployed: true) }
 
-    expect(project.pages_metadatum.reload.deployed).to eq(true)
+    it 'marks pages as not deployed if public directory is absent and invalid entries are ignored' do
+      project.mark_pages_as_deployed
+      expect(project.pages_metadatum.reload.deployed).to eq(true)
 
-    expect(service.execute).to(
-      eq(status: :success,
-         message: "Archive not created. Missing public directory in #{project.pages_path} ? Marked project as not deployed")
-    )
+      expect(service.execute).to(
+        eq(status: :success,
+           message: "Archive not created. Missing public directory in #{project.pages_path}? Marked project as not deployed")
+      )
 
-    expect(project.pages_metadatum.reload.deployed).to eq(false)
-  end
-
-  it 'does not mark pages as not deployed if public directory is absent but pages_deployment exists' do
-    deployment = create(:pages_deployment, project: project)
-    project.update_pages_deployment!(deployment)
-    project.mark_pages_as_deployed
+      expect(project.pages_metadatum.reload.deployed).to eq(false)
+    end
 
-    expect(project.pages_metadatum.reload.deployed).to eq(true)
+    it 'does not mark pages as not deployed if public directory is absent but pages_deployment exists' do
+      deployment = create(:pages_deployment, project: project)
+      project.update_pages_deployment!(deployment)
+      project.mark_pages_as_deployed
+      expect(project.pages_metadatum.reload.deployed).to eq(true)
 
-    expect(service.execute).to(
-      eq(status: :success,
-         message: "Archive not created. Missing public directory in #{project.pages_path} ? Marked project as not deployed")
-    )
+      expect(service.execute).to(
+        eq(status: :success,
+           message: "Archive not created. Missing public directory in #{project.pages_path}? Marked project as not deployed")
+      )
 
-    expect(project.pages_metadatum.reload.deployed).to eq(true)
+      expect(project.pages_metadatum.reload.deployed).to eq(true)
+    end
   end
 
-  it 'does not mark pages as not deployed if public directory is absent but feature is disabled' do
-    stub_feature_flags(pages_migration_mark_as_not_deployed: false)
-
+  it 'does not mark pages as not deployed if public directory is absent but invalid entries are not ignored' do
     project.mark_pages_as_deployed
 
     expect(project.pages_metadatum.reload.deployed).to eq(true)
 
     expect(service.execute).to(
       eq(status: :error,
-         message: "Can't create zip archive: Can not find valid public dir in #{project.pages_path}")
+         message: "Archive not created. Missing public directory in #{project.pages_path}")
     )
 
     expect(project.pages_metadatum.reload.deployed).to eq(true)

spec/services/pages/zip_directory_service_spec.rb

@@ -33,15 +33,6 @@ RSpec.describe Pages::ZipDirectoryService do
       expect(archive).to be_nil
       expect(entries_count).to be_nil
     end
-
-    it 'returns error if pages_migration_mark_as_not_deployed is disabled' do
-      stub_feature_flags(pages_migration_mark_as_not_deployed: false)
-
-      expect(status).to eq(:error)
-      expect(message).to eq("Can not find valid public dir in #{service_directory}")
-      expect(archive).to be_nil
-      expect(entries_count).to be_nil
-    end
   end
 
   context "when work direcotry doesn't exist" do

spec/tasks/gitlab/pages_rake_spec.rb

@@ -14,7 +14,8 @@ RSpec.describe 'gitlab:pages' do
       expect_next_instance_of(::Pages::MigrateFromLegacyStorageService, anything,
                               migration_threads: 3,
                               batch_size: 10,
-                              ignore_invalid_entries: false) do |service|
+                              ignore_invalid_entries: false,
+                              mark_projects_as_not_deployed: false) do |service|
         expect(service).to receive(:execute).and_call_original
       end
 
@@ -27,7 +28,8 @@ RSpec.describe 'gitlab:pages' do
       expect_next_instance_of(::Pages::MigrateFromLegacyStorageService, anything,
                               migration_threads: 5,
                               batch_size: 10,
-                              ignore_invalid_entries: false) do |service|
+                              ignore_invalid_entries: false,
+                              mark_projects_as_not_deployed: false) do |service|
         expect(service).to receive(:execute).and_call_original
       end
 
@@ -40,7 +42,8 @@ RSpec.describe 'gitlab:pages' do
       expect_next_instance_of(::Pages::MigrateFromLegacyStorageService, anything,
                               migration_threads: 3,
                               batch_size: 100,
-                              ignore_invalid_entries: false) do |service|
+                              ignore_invalid_entries: false,
+                              mark_projects_as_not_deployed: false) do |service|
         expect(service).to receive(:execute).and_call_original
       end
 
@@ -53,7 +56,22 @@ RSpec.describe 'gitlab:pages' do
       expect_next_instance_of(::Pages::MigrateFromLegacyStorageService, anything,
                               migration_threads: 3,
                               batch_size: 10,
-                              ignore_invalid_entries: true) do |service|
+                              ignore_invalid_entries: true,
+                              mark_projects_as_not_deployed: false) do |service|
+        expect(service).to receive(:execute).and_call_original
+      end
+
+      subject
+    end
+
+    it 'uses PAGES_MIGRATION_MARK_PROJECTS_AS_NOT_DEPLOYED environment variable' do
+      stub_env('PAGES_MIGRATION_MARK_PROJECTS_AS_NOT_DEPLOYED', 'true')
+
+      expect_next_instance_of(::Pages::MigrateFromLegacyStorageService, anything,
+                              migration_threads: 3,
+                              batch_size: 10,
+                              ignore_invalid_entries: false,
+                              mark_projects_as_not_deployed: true) do |service|
         expect(service).to receive(:execute).and_call_original
       end