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 0000000000000000000000000000000000000000..f138c8ea4972252411bcb8e1ab9b38a54889f299 --- /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/models/ee/project_import_state.rb b/ee/app/models/ee/project_import_state.rb index 25b988105b3b5dd809bddbb2f387e7bb357e017c..baa2a1376d97b93a0e17d8db755b6ca412416184 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/app/workers/project_import_schedule_worker.rb b/ee/app/workers/project_import_schedule_worker.rb index 8df985fdd1c3d5aee005eea9ebd6052a10c359f3..c090ca21b91afe6298e05cebfcbddb7162feb882 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 d2c0ae0fce8d889dd7c2e30df199f9e14ef4b8cc..b6f2744dfa90092079025b214d168c30d1d9d75b 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 ae0952e53a68327f10bee4fc605906008ef0c6bf..e7d494c26dd8f5f47ea8bcda8b82ef34408c44dd 100644 --- a/ee/spec/workers/project_import_schedule_worker_spec.rb +++ b/ee/spec/workers/project_import_schedule_worker_spec.rb @@ -3,12 +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) } + before do + project.update!(mirror: true, mirror_user: project.owner) + end + let(:job_args) { [project.id] } before do @@ -70,4 +74,58 @@ end end end + + context 'when project does not support mirroring' do + let!(:import_state) { create(:import_state, :finished, project: project) } + + before do + 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 + + 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 diff --git a/ee/spec/workers/update_all_mirrors_worker_spec.rb b/ee/spec/workers/update_all_mirrors_worker_spec.rb index 2dac72c1a21d3f14f12f6bb0a8e12d88fe289258..bf1fdc2af749b517d0abdbe629ba030e59a52f88 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