From 66478bb14d24e9687507e47c08e1af69427b48fe Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Fri, 11 Apr 2025 16:21:22 +0300 Subject: [PATCH 1/3] Do not remove old protected pipelines --- app/models/ci/pipeline.rb | 1 + app/workers/ci/destroy_old_pipelines_worker.rb | 8 +++++++- .../wip/ci_skip_old_protected_pipelines.yml | 9 +++++++++ spec/models/ci/pipeline_spec.rb | 12 ++++++++++++ spec/workers/ci/destroy_old_pipelines_worker_spec.rb | 11 +++++++++++ 5 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 config/feature_flags/wip/ci_skip_old_protected_pipelines.yml diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 0729dbe90f83fb..8f3af1666825a8 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -460,6 +460,7 @@ class Pipeline < Ci::ApplicationRecord scope :before_pipeline, ->(pipeline) { created_before_id(pipeline.id).outside_pipeline_family(pipeline) } scope :with_pipeline_source, ->(source) { where(source: source) } scope :preload_pipeline_metadata, -> { preload(:pipeline_metadata) } + scope :not_ref_protected, -> { where("#{quoted_table_name}.protected IS NOT true") } scope :outside_pipeline_family, ->(pipeline) do where.not(id: pipeline.same_family_pipeline_ids) diff --git a/app/workers/ci/destroy_old_pipelines_worker.rb b/app/workers/ci/destroy_old_pipelines_worker.rb index 546364b55f2c7e..8c7e94927dbcbe 100644 --- a/app/workers/ci/destroy_old_pipelines_worker.rb +++ b/app/workers/ci/destroy_old_pipelines_worker.rb @@ -18,7 +18,9 @@ def perform_work(*) Project.find_by_id(fetch_next_project_id).try do |project| with_context(project: project) do timestamp = project.ci_delete_pipelines_in_seconds.seconds.ago - pipelines = Ci::Pipeline.for_project(project.id).created_before(timestamp).limit(LIMIT).to_a + pipelines = Ci::Pipeline.for_project(project.id).created_before(timestamp) + pipelines = pipelines.not_ref_protected if skip_protected_pipelines?(project) + pipelines = pipelines.limit(LIMIT).to_a Ci::DestroyPipelineService.new(project, nil).unsafe_execute(pipelines) @@ -49,5 +51,9 @@ def fetch_next_project_id def queue_key Ci::ScheduleOldPipelinesRemovalCronWorker::QUEUE_KEY end + + def skip_protected_pipelines?(project) + Feature.enabled?(:ci_skip_old_protected_pipelines, project.root_namespace, type: :wip) + end end end diff --git a/config/feature_flags/wip/ci_skip_old_protected_pipelines.yml b/config/feature_flags/wip/ci_skip_old_protected_pipelines.yml new file mode 100644 index 00000000000000..a6e73dcc91dd79 --- /dev/null +++ b/config/feature_flags/wip/ci_skip_old_protected_pipelines.yml @@ -0,0 +1,9 @@ +--- +name: ci_skip_old_protected_pipelines +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/514867 +introduced_by_url: +rollout_issue_url: +milestone: '17.11' +group: group::ci platform +type: wip +default_enabled: false diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 2d55e89a11b853..7288371ad5515d 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -733,6 +733,18 @@ def transitionable?(from, to) end end + describe '.not_ref_protected' do + subject { described_class.not_ref_protected.order(:id) } + + let_it_be(:protected_pipeline) { create(:ci_pipeline, protected: true) } + let_it_be(:unprotected_pipeline) { create(:ci_pipeline, protected: false) } + let_it_be(:protected_unspecified_pipeline) { create(:ci_pipeline, protected: nil) } + + it 'contains all unprotected pipelines' do + is_expected.to contain_exactly(unprotected_pipeline, protected_unspecified_pipeline) + end + end + describe '.with_pipeline_source' do subject { described_class.with_pipeline_source(source) } diff --git a/spec/workers/ci/destroy_old_pipelines_worker_spec.rb b/spec/workers/ci/destroy_old_pipelines_worker_spec.rb index 1c307696e8ee13..fcce9a40c53785 100644 --- a/spec/workers/ci/destroy_old_pipelines_worker_spec.rb +++ b/spec/workers/ci/destroy_old_pipelines_worker_spec.rb @@ -43,6 +43,17 @@ expect { perform }.not_to raise_error end end + + context 'when protected pipelines are configured' do + let_it_be(:old_protected_pipeline) do + create(:ci_pipeline, project: project, created_at: 1.month.ago, protected: true) + end + + it 'keeps protected pipelines' do + expect { perform }.to change { project.all_pipelines.count }.by(-2) + expect(old_protected_pipeline.reload).to be_present + end + end end describe '#remaining_work_count' do -- GitLab From 10a9107c26d03a169ec663b0898af6a2b274f60c Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Mon, 14 Apr 2025 12:10:06 +0300 Subject: [PATCH 2/3] Apply reviewer feedback --- config/feature_flags/wip/ci_skip_old_protected_pipelines.yml | 2 +- spec/models/ci/pipeline_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config/feature_flags/wip/ci_skip_old_protected_pipelines.yml b/config/feature_flags/wip/ci_skip_old_protected_pipelines.yml index a6e73dcc91dd79..42fe37ede372d6 100644 --- a/config/feature_flags/wip/ci_skip_old_protected_pipelines.yml +++ b/config/feature_flags/wip/ci_skip_old_protected_pipelines.yml @@ -1,7 +1,7 @@ --- name: ci_skip_old_protected_pipelines feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/514867 -introduced_by_url: +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/187883 rollout_issue_url: milestone: '17.11' group: group::ci platform diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 7288371ad5515d..df7b936db48b98 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -734,7 +734,7 @@ def transitionable?(from, to) end describe '.not_ref_protected' do - subject { described_class.not_ref_protected.order(:id) } + subject { described_class.not_ref_protected } let_it_be(:protected_pipeline) { create(:ci_pipeline, protected: true) } let_it_be(:unprotected_pipeline) { create(:ci_pipeline, protected: false) } -- GitLab From 12d423389cd627360bfe8010f3b433126bdbb6fd Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Mon, 14 Apr 2025 12:12:05 +0300 Subject: [PATCH 3/3] Apply reviewer feedback --- config/feature_flags/wip/ci_skip_old_protected_pipelines.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/feature_flags/wip/ci_skip_old_protected_pipelines.yml b/config/feature_flags/wip/ci_skip_old_protected_pipelines.yml index 42fe37ede372d6..e228e6669b69cd 100644 --- a/config/feature_flags/wip/ci_skip_old_protected_pipelines.yml +++ b/config/feature_flags/wip/ci_skip_old_protected_pipelines.yml @@ -3,7 +3,7 @@ name: ci_skip_old_protected_pipelines feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/514867 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/187883 rollout_issue_url: -milestone: '17.11' +milestone: '18.0' group: group::ci platform type: wip default_enabled: false -- GitLab