Merge branch 'remove-projects_ci_pipeline_schedules_project_id-fk' into 'master'

Swap FK ci_pipeline_schedules.project_id to projects for LFK

See merge request gitlab-org/gitlab!79270

app/services/ci/pipeline_schedule_service.rb

@@ -3,6 +3,8 @@
 module Ci
   class PipelineScheduleService < BaseService
     def execute(schedule)
+      return unless project.persisted?
+
       # Ensure `next_run_at` is set properly before creating a pipeline.
       # Otherwise, multiple pipelines could be created in a short interval.
       schedule.schedule_next_run!

app/workers/pipeline_schedule_worker.rb

@@ -13,6 +13,8 @@ class PipelineScheduleWorker # rubocop:disable Scalability/IdempotentWorker
   def perform
     Ci::PipelineSchedule.runnable_schedules.preloaded.find_in_batches do |schedules|
       schedules.each do |schedule|
+        next unless schedule.project
+
         with_context(project: schedule.project, user: schedule.owner) do
           Ci::PipelineScheduleService.new(schedule.project, schedule.owner).execute(schedule)
         end

app/workers/run_pipeline_schedule_worker.rb

@@ -15,7 +15,7 @@ class RunPipelineScheduleWorker # rubocop:disable Scalability/IdempotentWorker
     schedule = Ci::PipelineSchedule.find_by_id(schedule_id)
     user = User.find_by_id(user_id)
 
-    return unless schedule && user
+    return unless schedule && schedule.project && user
 
     run_pipeline_schedule(schedule, user)
   end

db/post_migrate/20220126203421_remove_projects_ci_pipeline_schedules_project_id_fk.rb

+# frozen_string_literal: true
+
+class RemoveProjectsCiPipelineSchedulesProjectIdFk < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    return unless foreign_key_exists?(:ci_pipeline_schedules, :projects, name: "fk_8ead60fcc4")
+
+    with_lock_retries do
+      execute('LOCK projects, ci_pipeline_schedules IN ACCESS EXCLUSIVE MODE') if transaction_open?
+
+      remove_foreign_key_if_exists(:ci_pipeline_schedules, :projects, name: "fk_8ead60fcc4")
+    end
+  end
+
+  def down
+    add_concurrent_foreign_key(:ci_pipeline_schedules, :projects, name: "fk_8ead60fcc4", column: :project_id, target_column: :id, on_delete: :cascade)
+  end
+end

db/schema_migrations/20220126203421

+07f837ddde21e36d1ca6a471dd96350d3020bd30204fca0e093983810c94e54d
\ No newline at end of file

db/structure.sql

@@ -29580,9 +29580,6 @@ ALTER TABLE ONLY releases
 ALTER TABLE ONLY protected_tags
     ADD CONSTRAINT fk_8e4af87648 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
 
-ALTER TABLE ONLY ci_pipeline_schedules
-    ADD CONSTRAINT fk_8ead60fcc4 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
-
 ALTER TABLE ONLY todos
     ADD CONSTRAINT fk_91d1f47b13 FOREIGN KEY (note_id) REFERENCES notes(id) ON DELETE CASCADE;
 

lib/gitlab/database/gitlab_loose_foreign_keys.yml

@@ -167,6 +167,9 @@ ci_pipeline_schedules:
   - table: users
     column: owner_id
     on_delete: async_nullify
+  - table: projects
+    column: project_id
+    on_delete: async_delete
 merge_trains:
   - table: ci_pipelines
     column: pipeline_id

spec/lib/gitlab/database/no_cross_db_foreign_keys_spec.rb

@@ -19,7 +19,6 @@ RSpec.describe 'cross-database foreign keys' do
       ci_pending_builds.namespace_id
       ci_pending_builds.project_id
       ci_pipeline_schedules.owner_id
-      ci_pipeline_schedules.project_id
       ci_pipelines.project_id
       ci_resource_groups.project_id
       ci_runner_namespaces.namespace_id

spec/models/ci/pipeline_schedule_spec.rb

@@ -227,4 +227,11 @@ RSpec.describe Ci::PipelineSchedule do
       it { is_expected.to eq(144) }
     end
   end
+
+  context 'loose foreign key on ci_pipeline_schedules.project_id' do
+    it_behaves_like 'cleanup by a loose foreign key' do
+      let!(:parent) { create(:project) }
+      let!(:model) { create(:ci_pipeline_schedule, project: parent) }
+    end
+  end
 end

spec/services/ci/pipeline_schedule_service_spec.rb

@@ -32,5 +32,22 @@ RSpec.describe Ci::PipelineScheduleService do
         expect { subject }.not_to raise_error
       end
     end
+
+    context 'when the project is missing' do
+      before do
+        project.delete
+      end
+
+      it 'does not raise an exception' do
+        expect { subject }.not_to raise_error
+      end
+
+      it 'does not run RunPipelineScheduleWorker' do
+        expect(RunPipelineScheduleWorker)
+          .not_to receive(:perform_async).with(schedule.id, schedule.owner.id)
+
+        subject
+      end
+    end
   end
 end

spec/workers/pipeline_schedule_worker_spec.rb

@@ -103,4 +103,14 @@ RSpec.describe PipelineScheduleWorker do
       expect { subject }.not_to raise_error
     end
   end
+
+  context 'when the project is missing' do
+    before do
+      project.delete
+    end
+
+    it 'does not raise an exception' do
+      expect { subject }.not_to raise_error
+    end
+  end
 end

spec/workers/run_pipeline_schedule_worker_spec.rb

@@ -10,12 +10,25 @@ RSpec.describe RunPipelineScheduleWorker do
 
     let(:worker) { described_class.new }
 
-    context 'when a project not found' do
+    context 'when a schedule not found' do
       it 'does not call the Service' do
         expect(Ci::CreatePipelineService).not_to receive(:new)
         expect(worker).not_to receive(:run_pipeline_schedule)
 
-        worker.perform(100000, user.id)
+        worker.perform(non_existing_record_id, user.id)
+      end
+    end
+
+    context 'when a schedule project is missing' do
+      before do
+        project.delete
+      end
+
+      it 'does not call the Service' do
+        expect(Ci::CreatePipelineService).not_to receive(:new)
+        expect(worker).not_to receive(:run_pipeline_schedule)
+
+        worker.perform(pipeline_schedule.id, user.id)
       end
     end
 
@@ -24,7 +37,7 @@ RSpec.describe RunPipelineScheduleWorker do
         expect(Ci::CreatePipelineService).not_to receive(:new)
         expect(worker).not_to receive(:run_pipeline_schedule)
 
-        worker.perform(pipeline_schedule.id, 10000)
+        worker.perform(pipeline_schedule.id, non_existing_record_id)
       end
     end