Merge branch '295206-follow-up-from-add-pages-migration-service' into 'master'

Refactor pages migration services

See merge request gitlab-org/gitlab!50624

app/services/pages/migrate_legacy_storage_to_deployment_service.rb

@@ -3,8 +3,8 @@
 module Pages
   class MigrateLegacyStorageToDeploymentService
     ExclusiveLeaseTakenError = Class.new(StandardError)
-    FailedToCreateArchiveError = Class.new(StandardError)
 
+    include BaseServiceUtility
     include ::Pages::LegacyStorageLease
 
     attr_reader :project
@@ -14,37 +14,43 @@ module Pages
     end
 
     def execute
-      migrated = try_obtain_lease do
+      result = try_obtain_lease do
         execute_unsafe
-
-        true
       end
 
-      raise ExclusiveLeaseTakenError, "Can't migrate pages for project #{project.id}: exclusive lease taken" unless migrated
+      raise ExclusiveLeaseTakenError, "Can't migrate pages for project #{project.id}: exclusive lease taken" if result.nil?
+
+      result
     end
 
     private
 
     def execute_unsafe
-      archive_path, entries_count = ::Pages::ZipDirectoryService.new(project.pages_path).execute
+      zip_result = ::Pages::ZipDirectoryService.new(project.pages_path).execute
+
+      if zip_result[:status] == :error
+        if !project.pages_metadatum&.reload&.pages_deployment &&
+           Feature.enabled?(:pages_migration_mark_as_not_deployed, project)
+          project.mark_pages_as_not_deployed
+        end
+
+        return error("Can't create zip archive: #{zip_result[:message]}")
+      end
+
+      archive_path = zip_result[:archive_path]
 
       deployment = nil
       File.open(archive_path) do |file|
         deployment = project.pages_deployments.create!(
           file: file,
-          file_count: entries_count,
+          file_count: zip_result[:entries_count],
           file_sha256: Digest::SHA256.file(archive_path).hexdigest
         )
       end
 
       project.set_first_pages_deployment!(deployment)
-    rescue ::Pages::ZipDirectoryService::InvalidArchiveError => e
-      if !project.pages_metadatum&.reload&.pages_deployment &&
-         Feature.enabled?(:pages_migration_mark_as_not_deployed, project)
-        project.mark_pages_as_not_deployed
-      end
 
-      raise FailedToCreateArchiveError, e
+      success
     ensure
       FileUtils.rm_f(archive_path) if archive_path
     end

app/services/pages/zip_directory_service.rb

@@ -2,11 +2,11 @@
 
 module Pages
   class ZipDirectoryService
+    include BaseServiceUtility
     include Gitlab::Utils::StrongMemoize
 
-    Error = Class.new(::StandardError)
-    InvalidArchiveError = Class.new(Error)
-    InvalidEntryError = Class.new(Error)
+    # used only to track exceptions in Sentry
+    InvalidEntryError = Class.new(StandardError)
 
     PUBLIC_DIR = 'public'
 
@@ -15,19 +15,19 @@ module Pages
     end
 
     def execute
-      raise InvalidArchiveError, "Invalid work directory: #{@input_dir}" unless valid_work_directory?
+      return error("Can not find valid public dir in #{@input_dir}") unless valid_path?(public_dir)
 
       output_file = File.join(real_dir, "@migrated.zip") # '@' to avoid any name collision with groups or projects
 
       FileUtils.rm_f(output_file)
 
-      count = 0
+      entries_count = 0
       ::Zip::File.open(output_file, ::Zip::File::CREATE) do |zipfile|
         write_entry(zipfile, PUBLIC_DIR)
-        count = zipfile.entries.count
+        entries_count = zipfile.entries.count
       end
 
-      [output_file, count]
+      success(archive_path: output_file, entries_count: entries_count)
     rescue => e
       FileUtils.rm_f(output_file) if output_file
       raise e
@@ -39,9 +39,6 @@ module Pages
       disk_file_path = File.join(real_dir, zipfile_path)
 
       unless valid_path?(disk_file_path)
-        # archive without public directory is completelly unusable
-        raise InvalidArchiveError, "Invalid public directory: #{disk_file_path}" if zipfile_path == PUBLIC_DIR
-
         # archive with invalid entry will just have this entry missing
         raise InvalidEntryError
       end
@@ -80,8 +77,7 @@ module Pages
     def valid_path?(disk_file_path)
       realpath = File.realpath(disk_file_path)
 
-      realpath == File.join(real_dir, PUBLIC_DIR) ||
-        realpath.start_with?(File.join(real_dir, PUBLIC_DIR + "/"))
+      realpath == public_dir || realpath.start_with?(public_dir + "/")
     # happens if target of symlink isn't there
     rescue => e
       Gitlab::ErrorTracking.track_exception(e, input_dir: real_dir, disk_file_path: disk_file_path)
@@ -89,18 +85,16 @@ module Pages
       false
     end
 
-    def valid_work_directory?
-      Dir.exist?(real_dir)
-    rescue => e
-      Gitlab::ErrorTracking.track_exception(e, input_dir: @input_dir)
-
-      false
-    end
-
     def real_dir
       strong_memoize(:real_dir) do
         File.realpath(@input_dir) rescue nil
       end
     end
+
+    def public_dir
+      strong_memoize(:public_dir) do
+        File.join(real_dir, PUBLIC_DIR) rescue nil
+      end
+    end
   end
 end

lib/tasks/gitlab/pages.rake

@@ -6,24 +6,29 @@ namespace :gitlab do
     task migrate_legacy_storage: :gitlab_environment do
       logger = Logger.new(STDOUT)
       logger.info('Starting to migrate legacy pages storage to zip deployments')
-      migrated_projects = 0
+      processed_projects = 0
 
       ProjectPagesMetadatum.only_on_legacy_storage.each_batch(of: 10) do |batch|
         batch.preload(project: [:namespace, :route, pages_metadatum: :pages_deployment]).each do |metadatum|
           project = metadatum.project
+
+          result = nil
           time = Benchmark.realtime do
-            ::Pages::MigrateLegacyStorageToDeploymentService.new(project).execute
+            result = ::Pages::MigrateLegacyStorageToDeploymentService.new(project).execute
           end
+          processed_projects += 1
 
-          migrated_projects += 1
-
+          if result[:status] == :success
             logger.info("project_id: #{project.id} #{project.pages_path} has been migrated in #{time} seconds")
+          else
+            logger.error("project_id: #{project.id} #{project.pages_path} failed to be migrated in #{time} seconds: #{result[:message]}")
+          end
         rescue => e
           logger.error("#{e.message} project_id: #{project&.id}")
           Gitlab::ErrorTracking.track_exception(e, project_id: project&.id)
         end
 
-        logger.info("#{migrated_projects} pages projects are migrated")
+        logger.info("#{processed_projects} pages projects are processed")
       end
     end
   end

spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb

@@ -11,9 +11,10 @@ RSpec.describe Pages::MigrateLegacyStorageToDeploymentService do
 
     expect(project.pages_metadatum.reload.deployed).to eq(true)
 
-    expect do
-      service.execute
-    end.to raise_error(described_class::FailedToCreateArchiveError)
+    expect(service.execute).to(
+      eq(status: :error,
+         message: "Can't create zip archive: Can not find valid public dir in #{project.pages_path}")
+    )
 
     expect(project.pages_metadatum.reload.deployed).to eq(false)
   end
@@ -25,9 +26,10 @@ RSpec.describe Pages::MigrateLegacyStorageToDeploymentService do
 
     expect(project.pages_metadatum.reload.deployed).to eq(true)
 
-    expect do
-      service.execute
-    end.to raise_error(described_class::FailedToCreateArchiveError)
+    expect(service.execute).to(
+      eq(status: :error,
+         message: "Can't create zip archive: Can not find valid public dir in #{project.pages_path}")
+    )
 
     expect(project.pages_metadatum.reload.deployed).to eq(true)
   end
@@ -39,9 +41,10 @@ RSpec.describe Pages::MigrateLegacyStorageToDeploymentService do
 
     expect(project.pages_metadatum.reload.deployed).to eq(true)
 
-    expect do
-      service.execute
-    end.to raise_error(described_class::FailedToCreateArchiveError)
+    expect(service.execute).to(
+      eq(status: :error,
+         message: "Can't create zip archive: Can not find valid public dir in #{project.pages_path}")
+    )
 
     expect(project.pages_metadatum.reload.deployed).to eq(true)
   end
@@ -49,7 +52,9 @@ RSpec.describe Pages::MigrateLegacyStorageToDeploymentService do
   it 'removes pages archive when can not save deployment' do
     archive = fixture_file_upload("spec/fixtures/pages.zip")
     expect_next_instance_of(::Pages::ZipDirectoryService) do |zip_service|
-      expect(zip_service).to receive(:execute).and_return([archive.path, 3])
+      expect(zip_service).to receive(:execute).and_return(status: :success,
+                                                          archive_path: archive.path,
+                                                          entries_count: 3)
     end
 
     expect_next_instance_of(PagesDeployment) do |deployment|
@@ -73,7 +78,7 @@ RSpec.describe Pages::MigrateLegacyStorageToDeploymentService do
 
     it 'creates pages deployment' do
       expect do
-        described_class.new(project).execute
+        expect(described_class.new(project).execute).to eq(status: :success)
       end.to change { project.reload.pages_deployments.count }.by(1)
 
       deployment = project.pages_metadatum.pages_deployment

spec/services/pages/zip_directory_service_spec.rb

@@ -3,13 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Pages::ZipDirectoryService do
-  it 'raises error if project pages dir does not exist' do
-    expect do
-      described_class.new("/tmp/not/existing/dir").execute
-    end.to raise_error(described_class::InvalidArchiveError)
-  end
-
-  context 'when work dir exists' do
   around do |example|
     Dir.mktmpdir do |dir|
       @work_dir = dir
@@ -21,21 +14,35 @@ RSpec.describe Pages::ZipDirectoryService do
     described_class.new(@work_dir).execute
   end
 
-    let(:archive) { result.first }
-    let(:entries_count) { result.second }
+  let(:status) { result[:status] }
+  let(:message) { result[:message] }
+  let(:archive) { result[:archive_path] }
+  let(:entries_count) { result[:entries_count] }
 
-    it 'raises error if there is no public directory and does not leave archive' do
-      expect { archive }.to raise_error(described_class::InvalidArchiveError)
+  it 'returns error if project pages dir does not exist' do
+    expect(
+      described_class.new("/tmp/not/existing/dir").execute
+    ).to eq(status: :error, message: "Can not find valid public dir in /tmp/not/existing/dir")
+  end
+
+  it 'returns nils if there is no public directory and does not leave archive' do
+    expect(status).to eq(:error)
+    expect(message).to eq("Can not find valid public dir in #{@work_dir}")
+    expect(archive).to eq(nil)
+    expect(entries_count).to eq(nil)
 
     expect(File.exist?(File.join(@work_dir, '@migrated.zip'))).to eq(false)
   end
 
-    it 'raises error if public directory is a symlink' do
+  it 'returns nils if public directory is a symlink' do
     create_dir('target')
     create_file('./target/index.html', 'hello')
     create_link("public", "./target")
 
-      expect { archive }.to raise_error(described_class::InvalidArchiveError)
+    expect(status).to eq(:error)
+    expect(message).to eq("Can not find valid public dir in #{@work_dir}")
+    expect(archive).to eq(nil)
+    expect(entries_count).to eq(nil)
   end
 
   context 'when there is a public directory' do
@@ -215,5 +222,4 @@ RSpec.describe Pages::ZipDirectoryService do
       yield zip_file
     end
   end
-  end
 end