From c4b5998499e497d892067751973b47a96035ba83 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Thu, 14 Jul 2022 11:58:12 +0200 Subject: [PATCH 1/2] Deactivate mirrors for projects without license Contrbutes to https://gitlab.com/gitlab-org/gitlab/-/issues/216783 **Problem** Projects without a correct license are picked up by UpdateAllMirrorsWorker but we skip their processing. It increases amount of work for the worker and impacts the performance. **Solution** Mark projects without a license as hard failed to exclude them from the UpdateAllMirrorsWorker query. EE: true --- ...rd_failure_for_mirrors_without_license.yml | 8 +++ .../workers/project_import_schedule_worker.rb | 16 +++++- ee/app/workers/update_all_mirrors_worker.rb | 8 ++- .../project_import_schedule_worker_spec.rb | 57 ++++++++++++++++++- .../workers/update_all_mirrors_worker_spec.rb | 19 +++++++ 5 files changed, 105 insertions(+), 3 deletions(-) create mode 100644 config/feature_flags/development/hard_failure_for_mirrors_without_license.yml diff --git a/config/feature_flags/development/hard_failure_for_mirrors_without_license.yml b/config/feature_flags/development/hard_failure_for_mirrors_without_license.yml new file mode 100644 index 00000000000000..f138c8ea497225 --- /dev/null +++ b/config/feature_flags/development/hard_failure_for_mirrors_without_license.yml @@ -0,0 +1,8 @@ +--- +name: hard_failure_for_mirrors_without_license +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92422 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/367851 +milestone: '15.2' +type: development +group: group::source code +default_enabled: false diff --git a/ee/app/workers/project_import_schedule_worker.rb b/ee/app/workers/project_import_schedule_worker.rb index 8df985fdd1c3d5..c090ca21b91afe 100644 --- a/ee/app/workers/project_import_schedule_worker.rb +++ b/ee/app/workers/project_import_schedule_worker.rb @@ -26,7 +26,21 @@ def perform(project_id) next end - project.import_state.schedule + if project.mirror? + project.import_state.schedule + else + # If the project does not support mirroring (missing license for example) + # then we mark it as hard failed to exclude from UpdateAllMirrorWorker query + if Feature.enabled?(:hard_failure_for_mirrors_without_license) + # We cannot mark it as failed without changing the status to "scheduled" + project.import_state.schedule + + project.import_state.set_max_retry_count + project.import_state.mark_as_failed('Project mirroring is not supported') + + log_extra_metadata_on_done(:mirroring_skipped, 'Project does not support mirroring') + end + end end end end diff --git a/ee/app/workers/update_all_mirrors_worker.rb b/ee/app/workers/update_all_mirrors_worker.rb index d2c0ae0fce8d88..b6f2744dfa9009 100644 --- a/ee/app/workers/update_all_mirrors_worker.rb +++ b/ee/app/workers/update_all_mirrors_worker.rb @@ -76,7 +76,13 @@ def schedule_mirrors! projects = pull_mirrors_batch(freeze_at: now, batch_size: batch_size, offset_at: last).to_a break if projects.empty? - projects_to_schedule = projects.lazy.select(&:mirror?).take(capacity).force + projects_to_schedule = + if check_mirror_plans_in_query? && Feature.enabled?(:hard_failure_for_mirrors_without_license) + projects.take(capacity) + else + projects.lazy.select(&:mirror?).take(capacity).force + end + capacity -= projects_to_schedule.size schedule_projects_in_batch(projects_to_schedule) diff --git a/ee/spec/workers/project_import_schedule_worker_spec.rb b/ee/spec/workers/project_import_schedule_worker_spec.rb index ae0952e53a6832..3d2696bc97477a 100644 --- a/ee/spec/workers/project_import_schedule_worker_spec.rb +++ b/ee/spec/workers/project_import_schedule_worker_spec.rb @@ -3,11 +3,16 @@ require 'spec_helper' RSpec.describe ProjectImportScheduleWorker do - let!(:project) { create(:project) } + let!(:project) { create(:project, :public) } describe '#perform' do it_behaves_like 'an idempotent worker' do let!(:import_state) { create(:import_state, :none, project: project) } + let(:mirror) { true } + + before do + project.update!(mirror: mirror, mirror_user: project.owner) + end let(:job_args) { [project.id] } @@ -40,6 +45,41 @@ expect(Gitlab::Mirror).to have_received(:untrack_scheduling).with(project.id).at_least(:once) end + + context 'when project does not support mirroring' do + let(:mirror) { false } + + it 'marks a project hard failed' do + expect(project).to receive(:add_import_job) + expect(import_state).to be_none + + subject + + expect(import_state).to be_failed + expect(import_state.last_error).to eq('Project mirroring is not supported') + end + + it 'does not send a notification' do + expect(NotificationService).not_to receive(:new) + + subject + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(hard_failure_for_mirrors_without_license: false) + end + + it 'skips the project' do + expect(project).not_to receive(:add_import_job) + expect(import_state).to be_none + + subject + + expect(import_state).to be_none + end + end + end end context 'project is not found' do @@ -70,4 +110,19 @@ end end end + + context 'when project does not support mirroring' do + let!(:import_state) { create(:import_state, :none, project: project) } + + before do + project.update!(mirror: false, mirror_user: project.owner) + end + + it 'logs the error' do + expect(subject).to receive(:log_extra_metadata_on_done) + .with(:mirroring_skipped, 'Project does not support mirroring').and_call_original + + subject.perform(project.id) + end + end end diff --git a/ee/spec/workers/update_all_mirrors_worker_spec.rb b/ee/spec/workers/update_all_mirrors_worker_spec.rb index 2dac72c1a21d3f..bf1fdc2af749b5 100644 --- a/ee/spec/workers/update_all_mirrors_worker_spec.rb +++ b/ee/spec/workers/update_all_mirrors_worker_spec.rb @@ -149,6 +149,10 @@ def expect_import_scheduled(*projects) projects.each { |project| expect_import_status(project, 'scheduled') } end + def expect_import_failed(*projects) + projects.each { |project| expect_import_status(project, 'failed') } + end + def expect_import_not_scheduled(*projects) projects.each { |project| expect_import_status(project, 'none') } end @@ -263,6 +267,21 @@ def scheduled_mirror(at:, licensed:, public: false, subgroup: nil) expect_mirror_scheduling_tracked([licensed_project1, licensed_project2, public_project]) end + + context 'when public project does not have a open source license' do + it 'marks the mirror as hard failed' do + project_without_opensource_license = scheduled_mirror(at: 9.weeks.ago, licensed: false, public: true) + project_without_opensource_license.project_setting.update!(legacy_open_source_license_available: false) + + schedule_mirrors!(capacity: 4) + + expect_import_not_scheduled(*unlicensed_projects) + expect_import_scheduled(licensed_project1, licensed_project2, public_project) + expect_import_failed(project_without_opensource_license) + + expect_mirror_scheduling_tracked([project_without_opensource_license, licensed_project1, licensed_project2, public_project]) + end + end end end -- GitLab From 2e82217b5dac33f05fa0adcddb7052e2eb69ac79 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Fri, 15 Jul 2022 16:37:56 +0200 Subject: [PATCH 2/2] Consider disabled mirrors for capacity calculations --- ee/app/models/ee/project_import_state.rb | 4 +- .../project_import_schedule_worker_spec.rb | 83 ++++++++++--------- 2 files changed, 45 insertions(+), 42 deletions(-) diff --git a/ee/app/models/ee/project_import_state.rb b/ee/app/models/ee/project_import_state.rb index 25b988105b3b5d..baa2a1376d97b9 100644 --- a/ee/app/models/ee/project_import_state.rb +++ b/ee/app/models/ee/project_import_state.rb @@ -39,7 +39,7 @@ module ProjectImportState end after_transition [:scheduled, :started] => [:finished, :failed] do |state, _| - ::Gitlab::Mirror.decrement_capacity(state.project_id) if state.mirror? + ::Gitlab::Mirror.decrement_capacity(state.project_id) if state.project.read_attribute('mirror') end before_transition started: :failed do |state, _| @@ -78,7 +78,7 @@ module ProjectImportState end after_transition [:finished, :failed] => [:scheduled, :started] do |state, _| - ::Gitlab::Mirror.increment_capacity(state.project_id) if state.mirror? + ::Gitlab::Mirror.increment_capacity(state.project_id) if state.project.read_attribute('mirror') end end end diff --git a/ee/spec/workers/project_import_schedule_worker_spec.rb b/ee/spec/workers/project_import_schedule_worker_spec.rb index 3d2696bc97477a..e7d494c26dd8f5 100644 --- a/ee/spec/workers/project_import_schedule_worker_spec.rb +++ b/ee/spec/workers/project_import_schedule_worker_spec.rb @@ -8,10 +8,9 @@ describe '#perform' do it_behaves_like 'an idempotent worker' do let!(:import_state) { create(:import_state, :none, project: project) } - let(:mirror) { true } before do - project.update!(mirror: mirror, mirror_user: project.owner) + project.update!(mirror: true, mirror_user: project.owner) end let(:job_args) { [project.id] } @@ -45,41 +44,6 @@ expect(Gitlab::Mirror).to have_received(:untrack_scheduling).with(project.id).at_least(:once) end - - context 'when project does not support mirroring' do - let(:mirror) { false } - - it 'marks a project hard failed' do - expect(project).to receive(:add_import_job) - expect(import_state).to be_none - - subject - - expect(import_state).to be_failed - expect(import_state.last_error).to eq('Project mirroring is not supported') - end - - it 'does not send a notification' do - expect(NotificationService).not_to receive(:new) - - subject - end - - context 'when feature flag is disabled' do - before do - stub_feature_flags(hard_failure_for_mirrors_without_license: false) - end - - it 'skips the project' do - expect(project).not_to receive(:add_import_job) - expect(import_state).to be_none - - subject - - expect(import_state).to be_none - end - end - end end context 'project is not found' do @@ -112,17 +76,56 @@ end context 'when project does not support mirroring' do - let!(:import_state) { create(:import_state, :none, project: project) } + let!(:import_state) { create(:import_state, :finished, project: project) } before do - project.update!(mirror: false, mirror_user: project.owner) + stub_licensed_features(repository_mirrors: false) + project.update!(mirror: true, mirror_user: project.owner) + end + + it 'marks a project hard failed' do + expect(import_state).to be_finished + + subject.perform(project.id) + import_state.reload + + expect(import_state).to be_failed + expect(import_state.last_error).to eq('Project mirroring is not supported') + end + + it 'does not send a notification' do + expect(NotificationService).not_to receive(:new) + + subject.perform(project.id) + end + + it 'changes the capacity' do + expect(Gitlab::Mirror).to receive(:increment_capacity).with(project.id) + expect(Gitlab::Mirror).to receive(:decrement_capacity).with(project.id) + + subject.perform(project.id) end it 'logs the error' do expect(subject).to receive(:log_extra_metadata_on_done) - .with(:mirroring_skipped, 'Project does not support mirroring').and_call_original + .with(:mirroring_skipped, 'Project does not support mirroring').and_call_original subject.perform(project.id) end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(hard_failure_for_mirrors_without_license: false) + end + + it 'skips the project' do + expect(import_state).to be_finished + + subject.perform(project.id) + import_state.reload + + expect(import_state).to be_finished + end + end end end -- GitLab