diff options
author | Shinya Maeda <shinya@gitlab.com> | 2019-06-19 19:12:48 +0700 |
---|---|---|
committer | Shinya Maeda <shinya@gitlab.com> | 2019-06-24 14:53:36 +0700 |
commit | a22e68bf46e660aa21bf2ae0073a51b9a950f870 (patch) | |
tree | 9c5fd271bdffae9583279ec4a7d93ed7af42cd06 | |
parent | 97e8f494427582251af55056d25459bf038dabbc (diff) | |
download | gitlab-ce-fix-pipeline-schedule-edge-case.tar.gz |
Fix pipeline schedule edge casefix-pipeline-schedule-edge-case
If pipeline schedule is to run at the exact same time with when cron
worker runs, the pipeline schedule will not be executed at the
ideal timing.
We fix this bug by comparing the exact matching of ideal and
cron worker's next run at.
-rw-r--r-- | app/models/ci/pipeline_schedule.rb | 23 | ||||
-rw-r--r-- | changelogs/unreleased/fix-pipeline-schedule-edge-case.yml | 6 | ||||
-rw-r--r-- | spec/models/ci/pipeline_schedule_spec.rb | 51 |
3 files changed, 42 insertions, 38 deletions
diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb index 6a4241c94bc..ba8cea0cea9 100644 --- a/app/models/ci/pipeline_schedule.rb +++ b/app/models/ci/pipeline_schedule.rb @@ -55,15 +55,20 @@ module Ci # This way, a schedule like `*/1 * * * *` won't be triggered in a short interval # when PipelineScheduleWorker runs irregularly by Sidekiq Memory Killer. def set_next_run_at - self.next_run_at = Gitlab::Ci::CronParser.new(Settings.cron_jobs['pipeline_schedule_worker']['cron'], - Time.zone.name) - .next_time_from(ideal_next_run_at) + now = Time.zone.now + ideal_next_run = ideal_next_run_from(now) + + self.next_run_at = if ideal_next_run == cron_worker_next_run_from(now) + ideal_next_run + else + cron_worker_next_run_from(ideal_next_run) + end end def schedule_next_run! save! # with set_next_run_at rescue ActiveRecord::RecordInvalid - update_attribute(:next_run_at, nil) # update without validation + update_column(:next_run_at, nil) # update without validation end def job_variables @@ -72,9 +77,15 @@ module Ci private - def ideal_next_run_at + def ideal_next_run_from(start_time) Gitlab::Ci::CronParser.new(cron, cron_timezone) - .next_time_from(Time.zone.now) + .next_time_from(start_time) + end + + def cron_worker_next_run_from(start_time) + Gitlab::Ci::CronParser.new(Settings.cron_jobs['pipeline_schedule_worker']['cron'], + Time.zone.name) + .next_time_from(start_time) end end end diff --git a/changelogs/unreleased/fix-pipeline-schedule-edge-case.yml b/changelogs/unreleased/fix-pipeline-schedule-edge-case.yml new file mode 100644 index 00000000000..2b7e3611567 --- /dev/null +++ b/changelogs/unreleased/fix-pipeline-schedule-edge-case.yml @@ -0,0 +1,6 @@ +--- +title: Fix pipeline schedule does not run correctly when it's scheduled at the same + time with the cron worker +merge_request: 29848 +author: +type: fixed diff --git a/spec/models/ci/pipeline_schedule_spec.rb b/spec/models/ci/pipeline_schedule_spec.rb index d7b81caddf5..aee43025288 100644 --- a/spec/models/ci/pipeline_schedule_spec.rb +++ b/spec/models/ci/pipeline_schedule_spec.rb @@ -88,23 +88,8 @@ describe Ci::PipelineSchedule do describe '#set_next_run_at' do let(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly) } - let(:ideal_next_run_at) { pipeline_schedule.send(:ideal_next_run_at) } - - let(:expected_next_run_at) do - Gitlab::Ci::CronParser.new(Settings.cron_jobs['pipeline_schedule_worker']['cron'], Time.zone.name) - .next_time_from(ideal_next_run_at) - end - - let(:cron_worker_next_run_at) do - Gitlab::Ci::CronParser.new(Settings.cron_jobs['pipeline_schedule_worker']['cron'], Time.zone.name) - .next_time_from(Time.zone.now) - end - - context 'when creates new pipeline schedule' do - it 'updates next_run_at automatically' do - expect(pipeline_schedule.next_run_at).to eq(expected_next_run_at) - end - end + let(:ideal_next_run_at) { pipeline_schedule.send(:ideal_next_run_from, Time.zone.now) } + let(:cron_worker_next_run_at) { pipeline_schedule.send(:cron_worker_next_run_from, Time.zone.now) } context 'when PipelineScheduleWorker runs at a specific interval' do before do @@ -129,7 +114,7 @@ describe Ci::PipelineSchedule do let(:pipeline_schedule) { create(:ci_pipeline_schedule, :every_minute) } it "updates next_run_at to the sidekiq worker's execution time" do - Timecop.freeze(2019, 06, 19, 12, 00) do + Timecop.freeze(Time.parse("2019-06-01 12:18:00+0000")) do expect(pipeline_schedule.next_run_at).to eq(cron_worker_next_run_at) end end @@ -157,9 +142,8 @@ describe Ci::PipelineSchedule do let(:new_cron) { '0 0 1 1 *' } it 'updates next_run_at automatically' do - pipeline_schedule.update!(cron: new_cron) - - expect(pipeline_schedule.next_run_at).to eq(expected_next_run_at) + expect { pipeline_schedule.update!(cron: new_cron) } + .to change { pipeline_schedule.next_run_at } end end end @@ -167,21 +151,24 @@ describe Ci::PipelineSchedule do describe '#schedule_next_run!' do let!(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly) } - context 'when reschedules after 10 days from now' do - let(:future_time) { 10.days.from_now } - let(:ideal_next_run_at) { pipeline_schedule.send(:ideal_next_run_at) } + before do + pipeline_schedule.update_column(:next_run_at, nil) + end - let(:expected_next_run_at) do - Gitlab::Ci::CronParser.new(Settings.cron_jobs['pipeline_schedule_worker']['cron'], Time.zone.name) - .next_time_from(ideal_next_run_at) + it 'updates next_run_at' do + expect { pipeline_schedule.schedule_next_run! } + .to change { pipeline_schedule.next_run_at } + end + + context 'when record is invalid' do + before do + allow(pipeline_schedule).to receive(:save!) { raise ActiveRecord::RecordInvalid.new(pipeline_schedule) } end - it 'points to proper next_run_at' do - Timecop.freeze(future_time) do - pipeline_schedule.schedule_next_run! + it 'nullifies the next run at' do + pipeline_schedule.schedule_next_run! - expect(pipeline_schedule.next_run_at).to eq(expected_next_run_at) - end + expect(pipeline_schedule.next_run_at).to be_nil end end end |