Merge branch 'bvl-async-update-pages-config' into 'master'

Run pages configuration update in a worker

See merge request gitlab-org/gitlab!38524

app/services/projects/update_pages_configuration_service.rb

@@ -19,7 +19,7 @@ module Projects
       success
     rescue => e
       Gitlab::ErrorTracking.track_exception(e)
-      error(e.message)
+      error(e.message, pass_back: { exception: e })
     end
 
     private

app/services/projects/update_service.rb

@@ -142,8 +142,12 @@ module Projects
     end
 
     def update_pages_config
+      if Feature.enabled?(:async_update_pages_config, project)
+        PagesUpdateConfigurationWorker.perform_async(project.id)
+      else
         Projects::UpdatePagesConfigurationService.new(project).execute
       end
+    end
 
     def changing_pages_https_only?
       project.previous_changes.include?(:pages_https_only)

app/workers/all_queues.yml

@@ -1524,6 +1524,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: pages_update_configuration
+  :feature_category: :pages
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: phabricator_import_import_tasks
   :feature_category: :importers
   :has_external_dependencies:

app/workers/pages_update_configuration_worker.rb

+# frozen_string_literal: true
+
+class PagesUpdateConfigurationWorker
+  include ApplicationWorker
+
+  idempotent!
+  feature_category :pages
+
+  def perform(project_id)
+    project = Project.find_by_id(project_id)
+    return unless project
+
+    result = Projects::UpdatePagesConfigurationService.new(project).execute
+
+    # The ConfigurationService swallows all exceptions and wraps them in a status
+    # we need to keep this while the feature flag still allows running this
+    # service within a request.
+    # But we might as well take advantage of sidekiq retries here.
+    # We should let the service raise after we remove the feature flag
+    # https://gitlab.com/gitlab-org/gitlab/-/issues/230695
+    raise result[:exception] if result[:exception]
+  end
+end

config/feature_flags/development/async_update_pages_config.yml

+---
+name: async_update_pages_config
+introduced_by_url:
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/230695
+group: 'team::Scalability'
+type: development
+default_enabled: false

config/sidekiq_queues.yml

@@ -178,6 +178,8 @@
   - 1
 - - pages_domain_verification
   - 1
+- - pages_update_configuration
+  - 1
 - - personal_access_tokens
   - 1
 - - phabricator_import_import_tasks

spec/services/projects/update_pages_configuration_service_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Projects::UpdatePagesConfigurationService do
   let(:project) { create(:project) }
   let(:service) { described_class.new(project) }
 
-  describe "#update" do
+  describe "#execute" do
     let(:file) { Tempfile.new('pages-test') }
 
     subject { service.execute }
@@ -40,5 +40,14 @@ RSpec.describe Projects::UpdatePagesConfigurationService do
         expect(subject).to include(status: :success)
       end
     end
+
+    context 'when an error occurs' do
+      it 'returns an error object' do
+        e = StandardError.new("Failure")
+        allow(service).to receive(:reload_daemon).and_raise(e)
+
+        expect(subject).to eq(status: :error, message: "Failure", exception: e)
+      end
+    end
   end
 end

spec/services/projects/update_service_spec.rb

@@ -396,15 +396,16 @@ RSpec.describe Projects::UpdateService do
       end
     end
 
-    context 'when updating #pages_https_only', :https_pages_enabled do
-      subject(:call_service) do
-        update_project(project, admin, pages_https_only: false)
+    shared_examples 'updating pages configuration' do
+      it 'schedules the `PagesUpdateConfigurationWorker`' do
+        expect(PagesUpdateConfigurationWorker).to receive(:perform_async).with(project.id)
+
+        subject
       end
 
-      it 'updates the attribute' do
-        expect { call_service }
-          .to change { project.pages_https_only? }
-          .to(false)
+      context 'when `async_update_pages_config` is disabled' do
+        before do
+          stub_feature_flags(async_update_pages_config: false)
         end
 
         it 'calls Projects::UpdatePagesConfigurationService' do
@@ -413,8 +414,23 @@ RSpec.describe Projects::UpdateService do
                   .with(project)
                   .and_call_original
 
-        call_service
+          subject
+        end
+      end
+    end
+
+    context 'when updating #pages_https_only', :https_pages_enabled do
+      subject(:call_service) do
+        update_project(project, admin, pages_https_only: false)
+      end
+
+      it 'updates the attribute' do
+        expect { call_service }
+          .to change { project.pages_https_only? }
+          .to(false)
       end
+
+      it_behaves_like 'updating pages configuration'
     end
 
     context 'when updating #pages_access_level' do
@@ -428,14 +444,7 @@ RSpec.describe Projects::UpdateService do
           .to(ProjectFeature::ENABLED)
       end
 
-      it 'calls Projects::UpdatePagesConfigurationService' do
-        expect(Projects::UpdatePagesConfigurationService)
-          .to receive(:new)
-          .with(project)
-          .and_call_original
-
-        call_service
-      end
+      it_behaves_like 'updating pages configuration'
     end
 
     context 'when updating #emails_disabled' do

spec/workers/pages_update_configuration_worker_spec.rb

+# frozen_string_literal: true
+require "spec_helper"
+
+RSpec.describe PagesUpdateConfigurationWorker do
+  describe "#perform" do
+    let_it_be(:project) { create(:project) }
+
+    it "does not break if the project doesn't exist" do
+      expect { subject.perform(-1) }.not_to raise_error
+    end
+
+    it "calls the correct service" do
+      expect_next_instance_of(Projects::UpdatePagesConfigurationService, project) do |service|
+        expect(service).to receive(:execute).and_return({})
+      end
+
+      subject.perform(project.id)
+    end
+
+    it "raises an exception if the service returned an error" do
+      allow_next_instance_of(Projects::UpdatePagesConfigurationService) do |service|
+        allow(service).to receive(:execute).and_return({ exception: ":boom:" })
+      end
+
+      expect { subject.perform(project.id) }.to raise_error(":boom:")
+    end
+
+    it_behaves_like "an idempotent worker" do
+      let(:job_args) { [project.id] }
+      let(:pages_dir) { Dir.mktmpdir }
+      let(:config_path) { File.join(pages_dir, "config.json") }
+
+      before do
+        allow(Project).to receive(:find_by_id).with(project.id).and_return(project)
+        allow(project).to receive(:pages_path).and_return(pages_dir)
+
+        # Make sure _some_ config exists
+        FileUtils.touch(config_path)
+      end
+
+      after do
+        FileUtils.remove_entry(pages_dir)
+      end
+
+      it "only updates the config file once" do
+        described_class.new.perform(project.id)
+
+        expect(File.mtime(config_path)).not_to be_nil
+        expect { subject }.not_to change { File.mtime(config_path) }
+      end
+    end
+  end
+end