From b21ee2ee36e1aaddbe0b3541a8cac5f117143b66 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 28 Jun 2017 11:29:04 +0200 Subject: Add initial stage_id background migration files --- ...858_migrate_stage_id_reference_in_background.rb | 12 ++++++++++ .../migrate_build_stage_id_reference.rb | 16 +++++++++++++ ...igrate_stage_id_reference_in_background_spec.rb | 26 ++++++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb create mode 100644 lib/gitlab/background_migration/migrate_build_stage_id_reference.rb create mode 100644 spec/migrations/migrate_stage_id_reference_in_background_spec.rb diff --git a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb new file mode 100644 index 00000000000..2eaa798d0aa --- /dev/null +++ b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb @@ -0,0 +1,12 @@ +class MigrateStageIdReferenceInBackground < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + end + + def down + # noop + end +end diff --git a/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb b/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb new file mode 100644 index 00000000000..b554c3e079b --- /dev/null +++ b/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb @@ -0,0 +1,16 @@ +module Gitlab + module BackgroundMigration + class MigrateBuildStageIdReference + class Build < ActiveRecord::Base + self.table_name = 'ci_builds' + end + + class Stage < ActiveRecord::Base + self.table_name = 'ci_stages' + end + + def perform(id) + end + end + end +end diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb new file mode 100644 index 00000000000..f86ef834afa --- /dev/null +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20170628080858_migrate_stage_id_reference_in_background') + +describe MigrateStageIdReferenceInBackground, :migration, :redis do + let(:jobs) { table(:ci_builds) } + let(:stages) { table(:ci_stages) } + let(:pipelines) { table(:ci_pipelines) } + let(:projects) { table(:projects) } + + before do + projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1') + pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') + + jobs.create!(id: 1, commit_id: 1, project_id: 123, stage_idx: 2, stage: 'build') + jobs.create!(id: 2, commit_id: 1, project_id: 123, stage_idx: 2, stage: 'build') + jobs.create!(id: 3, commit_id: 1, project_id: 123, stage_idx: 1, stage: 'test') + jobs.create!(id: 4, commit_id: 1, project_id: 123, stage_idx: 3, stage: 'deploy') + + stages.create(id: 101, pipeline_id: 1, project_id: 123, name: 'test') + stages.create(id: 102, pipeline_id: 1, project_id: 123, name: 'build') + stages.create(id: 103, pipeline_id: 1, project_id: 123, name: 'deploy') + end + + it 'schedules background migrations' do + end +end -- cgit v1.2.1 From 98992c4e4b3231a99eb5ff17c44e96fe79a6cff2 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 28 Jun 2017 12:01:52 +0200 Subject: Add initial build stage_id ref background migration --- ...8080858_migrate_stage_id_reference_in_background.rb | 10 ++++++++++ .../migrate_build_stage_id_reference.rb | 18 +++++++++++------- .../migrate_stage_id_reference_in_background_spec.rb | 5 +++++ 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb index 2eaa798d0aa..44bac4a8cc7 100644 --- a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb +++ b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb @@ -3,7 +3,17 @@ class MigrateStageIdReferenceInBackground < ActiveRecord::Migration DOWNTIME = false + disable_ddl_transaction! + + class Build < ActiveRecord::Base + self.table_name = 'ci_builds' + end + def up + Build.find_each do |build| + BackgroundMigrationWorker + .perform_async('MigrateBuildStageIdReference', [build.id]) + end end def down diff --git a/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb b/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb index b554c3e079b..87c6c4ed49f 100644 --- a/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb +++ b/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb @@ -1,15 +1,19 @@ module Gitlab module BackgroundMigration class MigrateBuildStageIdReference - class Build < ActiveRecord::Base - self.table_name = 'ci_builds' - end + def perform(id) + raise ArgumentError unless id.is_a?(Integer) - class Stage < ActiveRecord::Base - self.table_name = 'ci_stages' - end + sql = <<-SQL.strip_heredoc + UPDATE "ci_builds" SET "stage_id" = ( + SELECT id FROM ci_stages + WHERE ci_stages.pipeline_id = ci_builds.commit_id + AND ci_stages.name = ci_builds.stage + ) + WHERE "ci_builds"."id" = #{id} AND "ci_builds"."stage_id" IS NULL + SQL - def perform(id) + ActiveRecord::Base.connection.execute(sql) end end end diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index f86ef834afa..ea3a18802d9 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -22,5 +22,10 @@ describe MigrateStageIdReferenceInBackground, :migration, :redis do end it 'schedules background migrations' do + expect(jobs.where(stage_id: nil)).to be_present + + migrate! + + expect(jobs.where(stage_id: nil)).to be_empty end end -- cgit v1.2.1 From 6209ff671fdd025be31f9dcaf208a71b6ec2907d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 28 Jun 2017 12:20:45 +0200 Subject: Update `db/schema.rb` with a new schema version --- db/schema.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index 8c7440ee610..3a45e0fe562 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170622162730) do +ActiveRecord::Schema.define(version: 20170628080858) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" -- cgit v1.2.1 From 02bb40e2acd7b1838e47e1a2f8b9288e42e6ca53 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 28 Jun 2017 12:21:25 +0200 Subject: Find builds that require a migration in batches --- .../20170628080858_migrate_stage_id_reference_in_background.rb | 9 ++++++--- spec/migrations/migrate_stage_id_reference_in_background_spec.rb | 2 ++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb index 44bac4a8cc7..6b326bc0b69 100644 --- a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb +++ b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb @@ -2,6 +2,8 @@ class MigrateStageIdReferenceInBackground < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers DOWNTIME = false + BATCH_SIZE = 10000 + MIGRATION = 'MigrateBuildStageIdReference'.freeze disable_ddl_transaction! @@ -10,9 +12,10 @@ class MigrateStageIdReferenceInBackground < ActiveRecord::Migration end def up - Build.find_each do |build| - BackgroundMigrationWorker - .perform_async('MigrateBuildStageIdReference', [build.id]) + Build.find_in_batches(batch_size: BATCH_SIZE).with_index do |builds, batch| + migrations = builds.map { |build| [MIGRATION, [build.id]] } + + BackgroundMigrationWorker.perform_bulk(*migrations) end end diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index ea3a18802d9..d515eb42b9d 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -8,6 +8,8 @@ describe MigrateStageIdReferenceInBackground, :migration, :redis do let(:projects) { table(:projects) } before do + stub_const('MigrateStageIdReferenceInBackground::BATCH_SIZE', 1) + projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1') pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') -- cgit v1.2.1 From 5292eb651e1e3595e409a4c216eb0be3445a9319 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 28 Jun 2017 12:23:00 +0200 Subject: Schedule background migration only when it is needed --- .../20170628080858_migrate_stage_id_reference_in_background.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb index 6b326bc0b69..bfeb09f6da1 100644 --- a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb +++ b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb @@ -12,11 +12,13 @@ class MigrateStageIdReferenceInBackground < ActiveRecord::Migration end def up - Build.find_in_batches(batch_size: BATCH_SIZE).with_index do |builds, batch| - migrations = builds.map { |build| [MIGRATION, [build.id]] } + Build.where(stage_id: nil) + .find_in_batches(batch_size: BATCH_SIZE) + .with_index do |builds, batch| + migrations = builds.map { |build| [MIGRATION, [build.id]] } - BackgroundMigrationWorker.perform_bulk(*migrations) - end + BackgroundMigrationWorker.perform_bulk(*migrations) + end end def down -- cgit v1.2.1 From 187dd1005cd92c530146d7f5b0a89b368b09c3e9 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 28 Jun 2017 15:24:53 +0200 Subject: Add specs for delayed stage_id background migrations --- ...858_migrate_stage_id_reference_in_background.rb | 7 +-- ...igrate_stage_id_reference_in_background_spec.rb | 51 ++++++++++++++++++++-- 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb index bfeb09f6da1..a73456af386 100644 --- a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb +++ b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb @@ -15,9 +15,10 @@ class MigrateStageIdReferenceInBackground < ActiveRecord::Migration Build.where(stage_id: nil) .find_in_batches(batch_size: BATCH_SIZE) .with_index do |builds, batch| - migrations = builds.map { |build| [MIGRATION, [build.id]] } - - BackgroundMigrationWorker.perform_bulk(*migrations) + builds.each do |build| + schedule = (batch - 1) * 5.minutes + BackgroundMigrationWorker.perform_at(schedule, MIGRATION, [build.id]) + end end end diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index d515eb42b9d..8b497656377 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -1,7 +1,37 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20170628080858_migrate_stage_id_reference_in_background') -describe MigrateStageIdReferenceInBackground, :migration, :redis do +RSpec::Matchers.define :have_migrated do |*expected| + match do |migration| + BackgroundMigrationWorker.jobs.any? do |job| + job['enqueued_at'].present? && job['args'] == [migration, expected] + end + end + + failure_message do |migration| + <<-EOS + Background migration `#{migration}` with args `#{expected.inspect}` + not migrated! + EOS + end +end + +RSpec::Matchers.define :have_scheduled_migration do |time, *expected| + match do |migration| + BackgroundMigrationWorker.jobs.any? do |job| + job['args'] == [migration, expected] && job['at'] >= time + end + end + + failure_message do |migration| + <<-EOS + Background migration `#{migration}` with args `#{expected.inspect}` + not scheduled! + EOS + end +end + +describe MigrateStageIdReferenceInBackground, :migration do let(:jobs) { table(:ci_builds) } let(:stages) { table(:ci_stages) } let(:pipelines) { table(:ci_pipelines) } @@ -23,11 +53,24 @@ describe MigrateStageIdReferenceInBackground, :migration, :redis do stages.create(id: 103, pipeline_id: 1, project_id: 123, name: 'deploy') end + it 'correctly schedules background migrations' do + Sidekiq::Testing.fake! do + migrate! + + expect(described_class::MIGRATION).to have_migrated(1) + expect(described_class::MIGRATION).to have_migrated(2) + expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 3) + expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 4) + end + end + it 'schedules background migrations' do - expect(jobs.where(stage_id: nil)).to be_present + Sidekiq::Testing.inline! do + expect(jobs.where(stage_id: nil)).to be_present - migrate! + migrate! - expect(jobs.where(stage_id: nil)).to be_empty + expect(jobs.where(stage_id: nil)).to be_empty + end end end -- cgit v1.2.1 From af2f2dc5ed588d33919d5db3f684c165d7427ab7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 11:41:19 +0200 Subject: Make it possible to schedule bg migrations in bulk --- app/workers/background_migration_worker.rb | 18 +++++++++++-- doc/development/background_migrations.md | 19 +++++++++++--- spec/support/sidekiq.rb | 8 +++++- spec/workers/background_migration_worker_spec.rb | 33 +++++++++++++++++++++++- 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/app/workers/background_migration_worker.rb b/app/workers/background_migration_worker.rb index e85e221d353..751f37a3c39 100644 --- a/app/workers/background_migration_worker.rb +++ b/app/workers/background_migration_worker.rb @@ -2,18 +2,32 @@ class BackgroundMigrationWorker include Sidekiq::Worker include DedicatedSidekiqQueue - # Schedules a number of jobs in bulk + # Enqueues a number of jobs in bulk. # # The `jobs` argument should be an Array of Arrays, each sub-array must be in # the form: # # [migration-class, [arg1, arg2, ...]] - def self.perform_bulk(*jobs) + def self.perform_bulk(jobs) Sidekiq::Client.push_bulk('class' => self, 'queue' => sidekiq_options['queue'], 'args' => jobs) end + # Schedules a number of jobs in bulk, with a delay. + # + def self.perform_bulk_in(delay, jobs) + now = Time.now.to_f + schedule = now + delay.to_f + + raise ArgumentError if schedule <= now + + Sidekiq::Client.push_bulk('class' => self, + 'queue' => sidekiq_options['queue'], + 'args' => jobs, + 'at' => schedule) + end + # Performs the background migration. # # See Gitlab::BackgroundMigration.perform for more information. diff --git a/doc/development/background_migrations.md b/doc/development/background_migrations.md index 0239e6b3163..a58f161fc30 100644 --- a/doc/development/background_migrations.md +++ b/doc/development/background_migrations.md @@ -50,14 +50,14 @@ your migration: BackgroundMigrationWorker.perform_async('BackgroundMigrationClassName', [arg1, arg2, ...]) ``` -Usually it's better to schedule jobs in bulk, for this you can use +Usually it's better to enqueue jobs in bulk, for this you can use `BackgroundMigrationWorker.perform_bulk`: ```ruby BackgroundMigrationWorker.perform_bulk( - ['BackgroundMigrationClassName', [1]], - ['BackgroundMigrationClassName', [2]], - ... + [['BackgroundMigrationClassName', [1]], + ['BackgroundMigrationClassName', [2]], + ...] ) ``` @@ -68,6 +68,17 @@ consuming migrations it's best to schedule a background job using an updates. Removals in turn can be handled by simply defining foreign keys with cascading deletes. +If you would like to schedule jobs in bulk with a delay, you can use +`BackgroundMigrationWorker.perform_bulk_in`: + +```ruby +jobs = [['BackgroundMigrationClassName', [1]], + ['BackgroundMigrationClassName', [2]], + ...] + +BackgroundMigrationWorker.perform_bulk_in(5.minutes, jobs) +``` + ## Cleaning Up Because background migrations can take a long time you can't immediately clean diff --git a/spec/support/sidekiq.rb b/spec/support/sidekiq.rb index 575d3451150..f3819ed2353 100644 --- a/spec/support/sidekiq.rb +++ b/spec/support/sidekiq.rb @@ -1,5 +1,11 @@ -require 'sidekiq/testing/inline' +require 'sidekiq/testing' Sidekiq::Testing.server_middleware do |chain| chain.add Gitlab::SidekiqStatus::ServerMiddleware end + +RSpec.configure do |config| + config.after(:each, :sidekiq) do + Sidekiq::Worker.clear_all + end +end diff --git a/spec/workers/background_migration_worker_spec.rb b/spec/workers/background_migration_worker_spec.rb index 85939429feb..4f6e3474634 100644 --- a/spec/workers/background_migration_worker_spec.rb +++ b/spec/workers/background_migration_worker_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe BackgroundMigrationWorker do +describe BackgroundMigrationWorker, :sidekiq do describe '.perform' do it 'performs a background migration' do expect(Gitlab::BackgroundMigration) @@ -10,4 +10,35 @@ describe BackgroundMigrationWorker do described_class.new.perform('Foo', [10, 20]) end end + + describe '.perform_bulk' do + it 'enqueues background migrations in bulk' do + Sidekiq::Testing.fake! do + described_class.perform_bulk([['Foo', [1]], ['Foo', [2]]]) + + expect(described_class.jobs.count).to eq 2 + expect(described_class.jobs).to all(include('enqueued_at')) + end + end + end + + describe '.perform_bulk_in' do + context 'when delay is valid' do + it 'correctly schedules background migrations' do + Sidekiq::Testing.fake! do + described_class.perform_bulk_in(1.minute, [['Foo', [1]], ['Foo', [2]]]) + + expect(described_class.jobs.count).to eq 2 + expect(described_class.jobs).to all(include('at')) + end + end + end + + context 'when delay is invalid' do + it 'raises an ArgumentError exception' do + expect { described_class.perform_bulk_in(-60, [['Foo']]) } + .to raise_error(ArgumentError) + end + end + end end -- cgit v1.2.1 From 69736f3927160bd362e165b4cd9e78912a3c30c0 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 12:10:29 +0200 Subject: Use ActiveRecord 5 batching to schedule bg migration --- app/workers/background_migration_worker.rb | 2 +- config/initializers/ar5_batching.rb | 4 +- ...858_migrate_stage_id_reference_in_background.rb | 7 ++- ...igrate_stage_id_reference_in_background_spec.rb | 55 ++++++++++------------ 4 files changed, 32 insertions(+), 36 deletions(-) diff --git a/app/workers/background_migration_worker.rb b/app/workers/background_migration_worker.rb index 751f37a3c39..23c297de8bc 100644 --- a/app/workers/background_migration_worker.rb +++ b/app/workers/background_migration_worker.rb @@ -14,7 +14,7 @@ class BackgroundMigrationWorker 'args' => jobs) end - # Schedules a number of jobs in bulk, with a delay. + # Schedules multiple jobs in bulk, with a delay. # def self.perform_bulk_in(delay, jobs) now = Time.now.to_f diff --git a/config/initializers/ar5_batching.rb b/config/initializers/ar5_batching.rb index 35e8b3808e2..31efef83a6f 100644 --- a/config/initializers/ar5_batching.rb +++ b/config/initializers/ar5_batching.rb @@ -15,7 +15,7 @@ module ActiveRecord relation = relation.where(arel_table[primary_key].lteq(finish)) if finish batch_relation = relation - loop do + 1.step do |index| if load records = batch_relation.records ids = records.map(&:id) @@ -31,7 +31,7 @@ module ActiveRecord primary_key_offset = ids.last raise ArgumentError.new("Primary key not included in the custom select clause") unless primary_key_offset - yield yielded_relation + yield yielded_relation, index break if ids.length < of batch_relation = relation.where(arel_table[primary_key].gt(primary_key_offset)) diff --git a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb index a73456af386..9e95216b35a 100644 --- a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb +++ b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb @@ -13,10 +13,9 @@ class MigrateStageIdReferenceInBackground < ActiveRecord::Migration def up Build.where(stage_id: nil) - .find_in_batches(batch_size: BATCH_SIZE) - .with_index do |builds, batch| - builds.each do |build| - schedule = (batch - 1) * 5.minutes + .in_batches(of: BATCH_SIZE) do |relation, index| + schedule = index * 5.minutes + relation.each do |build| BackgroundMigrationWorker.perform_at(schedule, MIGRATION, [build.id]) end end diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index 8b497656377..d3645ec0395 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -1,44 +1,39 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20170628080858_migrate_stage_id_reference_in_background') -RSpec::Matchers.define :have_migrated do |*expected| - match do |migration| - BackgroundMigrationWorker.jobs.any? do |job| - job['enqueued_at'].present? && job['args'] == [migration, expected] +describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do + matcher :have_migrated do |*expected| + match do |migration| + BackgroundMigrationWorker.jobs.any? do |job| + job['enqueued_at'].present? && job['args'] == [migration, expected] + end end - end - failure_message do |migration| - <<-EOS - Background migration `#{migration}` with args `#{expected.inspect}` - not migrated! - EOS + failure_message do |migration| + "Migration `#{migration}` with args `#{expected.inspect}` not executed!" + end end -end -RSpec::Matchers.define :have_scheduled_migration do |time, *expected| - match do |migration| - BackgroundMigrationWorker.jobs.any? do |job| - job['args'] == [migration, expected] && job['at'] >= time + matcher :have_scheduled_migration do |delay, *expected| + match do |migration| + BackgroundMigrationWorker.jobs.any? do |job| + job['args'] == [migration, expected] && + job['at'].to_f == (delay.to_f + Time.now.to_f) + end end - end - failure_message do |migration| - <<-EOS - Background migration `#{migration}` with args `#{expected.inspect}` - not scheduled! - EOS + failure_message do |migration| + "Migration `#{migration}` with args `#{expected.inspect}` not scheduled!" + end end -end -describe MigrateStageIdReferenceInBackground, :migration do let(:jobs) { table(:ci_builds) } let(:stages) { table(:ci_stages) } let(:pipelines) { table(:ci_pipelines) } let(:projects) { table(:projects) } before do - stub_const('MigrateStageIdReferenceInBackground::BATCH_SIZE', 1) + stub_const('MigrateStageIdReferenceInBackground::BATCH_SIZE', 2) projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1') pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') @@ -55,12 +50,14 @@ describe MigrateStageIdReferenceInBackground, :migration do it 'correctly schedules background migrations' do Sidekiq::Testing.fake! do - migrate! + Timecop.freeze do + migrate! - expect(described_class::MIGRATION).to have_migrated(1) - expect(described_class::MIGRATION).to have_migrated(2) - expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 3) - expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 4) + expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 1) + expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 2) + expect(described_class::MIGRATION).to have_scheduled_migration(10.minutes, 3) + expect(described_class::MIGRATION).to have_scheduled_migration(10.minutes, 4) + end end end -- cgit v1.2.1 From 0c14b6f0985415bd9762886378b0ad5a009d80cd Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 12:14:41 +0200 Subject: Remove unused background migrations matcher --- .../migrate_stage_id_reference_in_background_spec.rb | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index d3645ec0395..3eeca2e9659 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -2,18 +2,6 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20170628080858_migrate_stage_id_reference_in_background') describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do - matcher :have_migrated do |*expected| - match do |migration| - BackgroundMigrationWorker.jobs.any? do |job| - job['enqueued_at'].present? && job['args'] == [migration, expected] - end - end - - failure_message do |migration| - "Migration `#{migration}` with args `#{expected.inspect}` not executed!" - end - end - matcher :have_scheduled_migration do |delay, *expected| match do |migration| BackgroundMigrationWorker.jobs.any? do |job| -- cgit v1.2.1 From 8bd9cb6c87a1a1c51830360fe1aa1a228f9c768e Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 12:14:54 +0200 Subject: Perform stage_id ref backgound migration in bulks --- .../20170628080858_migrate_stage_id_reference_in_background.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb index 9e95216b35a..c54e8bde095 100644 --- a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb +++ b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb @@ -15,9 +15,9 @@ class MigrateStageIdReferenceInBackground < ActiveRecord::Migration Build.where(stage_id: nil) .in_batches(of: BATCH_SIZE) do |relation, index| schedule = index * 5.minutes - relation.each do |build| - BackgroundMigrationWorker.perform_at(schedule, MIGRATION, [build.id]) - end + jobs = relation.pluck(:id).map { |id| [MIGRATION, [id]] } + + BackgroundMigrationWorker.perform_bulk_in(schedule, jobs) end end -- cgit v1.2.1 From 42556419c99522e921299a3e247f115f580be7f1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 12:26:37 +0200 Subject: Improve specs for background stage_id ref migration --- ...080858_migrate_stage_id_reference_in_background.rb | 11 +++++------ .../migrate_stage_id_reference_in_background_spec.rb | 19 +++++++++++++------ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb index c54e8bde095..1d95fc62c87 100644 --- a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb +++ b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb @@ -12,13 +12,12 @@ class MigrateStageIdReferenceInBackground < ActiveRecord::Migration end def up - Build.where(stage_id: nil) - .in_batches(of: BATCH_SIZE) do |relation, index| - schedule = index * 5.minutes - jobs = relation.pluck(:id).map { |id| [MIGRATION, [id]] } + Build.where(stage_id: nil).in_batches(of: BATCH_SIZE) do |relation, index| + schedule = index * 5.minutes + jobs = relation.pluck(:id).map { |id| [MIGRATION, [id]] } - BackgroundMigrationWorker.perform_bulk_in(schedule, jobs) - end + BackgroundMigrationWorker.perform_bulk_in(schedule, jobs) + end end def down diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index 3eeca2e9659..046d9b351a8 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20170628080858_migrate_stage_id_reference_in_background') describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do - matcher :have_scheduled_migration do |delay, *expected| + matcher :be_scheduled_migration do |delay, *expected| match do |migration| BackgroundMigrationWorker.jobs.any? do |job| job['args'] == [migration, expected] && @@ -24,16 +24,22 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do stub_const('MigrateStageIdReferenceInBackground::BATCH_SIZE', 2) projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1') + projects.create!(id: 345, name: 'gitlab2', path: 'gitlab2') + pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') + pipelines.create!(id: 2, project_id: 345, ref: 'feature', sha: 'cdf43c3c') jobs.create!(id: 1, commit_id: 1, project_id: 123, stage_idx: 2, stage: 'build') jobs.create!(id: 2, commit_id: 1, project_id: 123, stage_idx: 2, stage: 'build') jobs.create!(id: 3, commit_id: 1, project_id: 123, stage_idx: 1, stage: 'test') jobs.create!(id: 4, commit_id: 1, project_id: 123, stage_idx: 3, stage: 'deploy') + jobs.create!(id: 5, commit_id: 2, project_id: 345, stage_idx: 1, stage: 'test') stages.create(id: 101, pipeline_id: 1, project_id: 123, name: 'test') stages.create(id: 102, pipeline_id: 1, project_id: 123, name: 'build') stages.create(id: 103, pipeline_id: 1, project_id: 123, name: 'deploy') + + jobs.create!(id: 6, commit_id: 2, project_id: 345, stage_id: 101, stage_idx: 1, stage: 'test') end it 'correctly schedules background migrations' do @@ -41,10 +47,11 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do Timecop.freeze do migrate! - expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 1) - expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 2) - expect(described_class::MIGRATION).to have_scheduled_migration(10.minutes, 3) - expect(described_class::MIGRATION).to have_scheduled_migration(10.minutes, 4) + expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 1) + expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 2) + expect(described_class::MIGRATION).to be_scheduled_migration(10.minutes, 3) + expect(described_class::MIGRATION).to be_scheduled_migration(10.minutes, 4) + expect(BackgroundMigrationWorker.jobs.size).to eq 5 end end end @@ -55,7 +62,7 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do migrate! - expect(jobs.where(stage_id: nil)).to be_empty + expect(jobs.where(stage_id: nil)).to be_one end end end -- cgit v1.2.1 From 4687e07c6ea31821350aeb2c0dcb540f773b5268 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 14:31:12 +0200 Subject: Make `inline` a default sidekiq testing processing again --- spec/support/sidekiq.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/support/sidekiq.rb b/spec/support/sidekiq.rb index f3819ed2353..5478fea4e64 100644 --- a/spec/support/sidekiq.rb +++ b/spec/support/sidekiq.rb @@ -1,4 +1,4 @@ -require 'sidekiq/testing' +require 'sidekiq/testing/inline' Sidekiq::Testing.server_middleware do |chain| chain.add Gitlab::SidekiqStatus::ServerMiddleware -- cgit v1.2.1 From efa3511e0495f73b06e96e60538f8e719ffe97a4 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 15:20:26 +0200 Subject: Test if argument passed to a migration is present --- lib/gitlab/background_migration/migrate_build_stage_id_reference.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb b/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb index 87c6c4ed49f..711126ea0d3 100644 --- a/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb +++ b/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb @@ -2,7 +2,7 @@ module Gitlab module BackgroundMigration class MigrateBuildStageIdReference def perform(id) - raise ArgumentError unless id.is_a?(Integer) + raise ArgumentError unless id.present? sql = <<-SQL.strip_heredoc UPDATE "ci_builds" SET "stage_id" = ( -- cgit v1.2.1 From c7f6e5efb51d28242bfe5102b9db9cd7a6c1e24b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 15:22:06 +0200 Subject: Use integers to schedule delayed background migrations --- app/workers/background_migration_worker.rb | 4 ++-- spec/migrations/migrate_stage_id_reference_in_background_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/workers/background_migration_worker.rb b/app/workers/background_migration_worker.rb index 23c297de8bc..e6ca1159b38 100644 --- a/app/workers/background_migration_worker.rb +++ b/app/workers/background_migration_worker.rb @@ -17,8 +17,8 @@ class BackgroundMigrationWorker # Schedules multiple jobs in bulk, with a delay. # def self.perform_bulk_in(delay, jobs) - now = Time.now.to_f - schedule = now + delay.to_f + now = Time.now.to_i + schedule = now + delay.to_i raise ArgumentError if schedule <= now diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index 046d9b351a8..1bd2c14b61c 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -6,7 +6,7 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do match do |migration| BackgroundMigrationWorker.jobs.any? do |job| job['args'] == [migration, expected] && - job['at'].to_f == (delay.to_f + Time.now.to_f) + job['at'].to_f == (delay.to_i + Time.now.to_i) end end -- cgit v1.2.1 From 6db8253cb86f44a80282706cc3de3df954661434 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 15:26:47 +0200 Subject: Improve readability of build stage id migration query --- .../migrate_build_stage_id_reference.rb | 13 +++++++------ .../migrate_stage_id_reference_in_background_spec.rb | 4 ++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb b/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb index 711126ea0d3..c8669ca3272 100644 --- a/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb +++ b/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb @@ -5,12 +5,13 @@ module Gitlab raise ArgumentError unless id.present? sql = <<-SQL.strip_heredoc - UPDATE "ci_builds" SET "stage_id" = ( - SELECT id FROM ci_stages - WHERE ci_stages.pipeline_id = ci_builds.commit_id - AND ci_stages.name = ci_builds.stage - ) - WHERE "ci_builds"."id" = #{id} AND "ci_builds"."stage_id" IS NULL + UPDATE "ci_builds" + SET "stage_id" = + (SELECT id FROM ci_stages + WHERE ci_stages.pipeline_id = ci_builds.commit_id + AND ci_stages.name = ci_builds.stage) + WHERE "ci_builds"."id" = #{id} + AND "ci_builds"."stage_id" IS NULL SQL ActiveRecord::Base.connection.execute(sql) diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index 1bd2c14b61c..63787d71233 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -58,11 +58,11 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do it 'schedules background migrations' do Sidekiq::Testing.inline! do - expect(jobs.where(stage_id: nil)).to be_present + expect(jobs.where(stage_id: nil).count).to eq 5 migrate! - expect(jobs.where(stage_id: nil)).to be_one + expect(jobs.where(stage_id: nil).count).to eq 1 end end end -- cgit v1.2.1 From 9c2290315e354c854bcd2ef0fed784dc5cfb48e8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 15:36:39 +0200 Subject: Do not compare float with integer in migrations specs --- spec/migrations/migrate_stage_id_reference_in_background_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index 63787d71233..2e5504c849d 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -6,7 +6,7 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do match do |migration| BackgroundMigrationWorker.jobs.any? do |job| job['args'] == [migration, expected] && - job['at'].to_f == (delay.to_i + Time.now.to_i) + job['at'].to_i == (delay.to_i + Time.now.to_i) end end -- cgit v1.2.1 From 1edd063a6c338e889d95def3e31aaca5420bfa5a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 30 Jun 2017 12:05:33 +0200 Subject: Add description to exception in bg migrations worker --- app/workers/background_migration_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/background_migration_worker.rb b/app/workers/background_migration_worker.rb index e6ca1159b38..e7ed71a687c 100644 --- a/app/workers/background_migration_worker.rb +++ b/app/workers/background_migration_worker.rb @@ -20,7 +20,7 @@ class BackgroundMigrationWorker now = Time.now.to_i schedule = now + delay.to_i - raise ArgumentError if schedule <= now + raise ArgumentError, 'Delay time invalid!' if schedule <= now Sidekiq::Client.push_bulk('class' => self, 'queue' => sidekiq_options['queue'], -- cgit v1.2.1 From 1bb0448f86c106a70a82627b3ffe84ed06a59081 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 30 Jun 2017 12:06:29 +0200 Subject: Improve code examples in background migrations docs --- doc/development/background_migrations.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/doc/development/background_migrations.md b/doc/development/background_migrations.md index a58f161fc30..72a34aa7de9 100644 --- a/doc/development/background_migrations.md +++ b/doc/development/background_migrations.md @@ -56,8 +56,7 @@ Usually it's better to enqueue jobs in bulk, for this you can use ```ruby BackgroundMigrationWorker.perform_bulk( [['BackgroundMigrationClassName', [1]], - ['BackgroundMigrationClassName', [2]], - ...] + ['BackgroundMigrationClassName', [2]]] ) ``` @@ -73,8 +72,7 @@ If you would like to schedule jobs in bulk with a delay, you can use ```ruby jobs = [['BackgroundMigrationClassName', [1]], - ['BackgroundMigrationClassName', [2]], - ...] + ['BackgroundMigrationClassName', [2]]] BackgroundMigrationWorker.perform_bulk_in(5.minutes, jobs) ``` -- cgit v1.2.1 From 6997dfa3426b26f7eb8f294b261827ef0b6d823b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 30 Jun 2017 12:07:31 +0200 Subject: Sanitize id value passed to async background migration --- lib/gitlab/background_migration/migrate_build_stage_id_reference.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb b/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb index c8669ca3272..d1d0a968588 100644 --- a/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb +++ b/lib/gitlab/background_migration/migrate_build_stage_id_reference.rb @@ -2,15 +2,13 @@ module Gitlab module BackgroundMigration class MigrateBuildStageIdReference def perform(id) - raise ArgumentError unless id.present? - sql = <<-SQL.strip_heredoc UPDATE "ci_builds" SET "stage_id" = (SELECT id FROM ci_stages WHERE ci_stages.pipeline_id = ci_builds.commit_id AND ci_stages.name = ci_builds.stage) - WHERE "ci_builds"."id" = #{id} + WHERE "ci_builds"."id" = #{id.to_i} AND "ci_builds"."stage_id" IS NULL SQL -- cgit v1.2.1 From 134f204ed8f5dd80e44463338ae93f3d905ca7af Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 30 Jun 2017 13:03:47 +0200 Subject: Do not override original AR5 batching interface --- config/initializers/ar5_batching.rb | 4 ++-- .../20170628080858_migrate_stage_id_reference_in_background.rb | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/config/initializers/ar5_batching.rb b/config/initializers/ar5_batching.rb index 31efef83a6f..35e8b3808e2 100644 --- a/config/initializers/ar5_batching.rb +++ b/config/initializers/ar5_batching.rb @@ -15,7 +15,7 @@ module ActiveRecord relation = relation.where(arel_table[primary_key].lteq(finish)) if finish batch_relation = relation - 1.step do |index| + loop do if load records = batch_relation.records ids = records.map(&:id) @@ -31,7 +31,7 @@ module ActiveRecord primary_key_offset = ids.last raise ArgumentError.new("Primary key not included in the custom select clause") unless primary_key_offset - yield yielded_relation, index + yield yielded_relation break if ids.length < of batch_relation = relation.where(arel_table[primary_key].gt(primary_key_offset)) diff --git a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb index 1d95fc62c87..30849ea1361 100644 --- a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb +++ b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb @@ -12,9 +12,12 @@ class MigrateStageIdReferenceInBackground < ActiveRecord::Migration end def up - Build.where(stage_id: nil).in_batches(of: BATCH_SIZE) do |relation, index| - schedule = index * 5.minutes + index = 1 + + Build.where(stage_id: nil).in_batches(of: BATCH_SIZE) do |relation| jobs = relation.pluck(:id).map { |id| [MIGRATION, [id]] } + schedule = index * 5.minutes + index += 1 BackgroundMigrationWorker.perform_bulk_in(schedule, jobs) end -- cgit v1.2.1 From 9c7c95c768cd5294dd085c2fc2425fae91c4c689 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 30 Jun 2017 14:22:23 +0200 Subject: Add initial changes for stages statuses migration --- .../20170630105320_add_status_to_ci_stages.rb | 9 +++ .../20170630111158_migrate_stages_statuses.rb | 76 ++++++++++++++++++++++ db/schema.rb | 3 +- spec/migrations/migrate_stages_statuses_spec.rb | 50 ++++++++++++++ 4 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20170630105320_add_status_to_ci_stages.rb create mode 100644 db/post_migrate/20170630111158_migrate_stages_statuses.rb create mode 100644 spec/migrations/migrate_stages_statuses_spec.rb diff --git a/db/migrate/20170630105320_add_status_to_ci_stages.rb b/db/migrate/20170630105320_add_status_to_ci_stages.rb new file mode 100644 index 00000000000..d497a61a959 --- /dev/null +++ b/db/migrate/20170630105320_add_status_to_ci_stages.rb @@ -0,0 +1,9 @@ +class AddStatusToCiStages < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :ci_stages, :status, :integer + end +end diff --git a/db/post_migrate/20170630111158_migrate_stages_statuses.rb b/db/post_migrate/20170630111158_migrate_stages_statuses.rb new file mode 100644 index 00000000000..b4b76893595 --- /dev/null +++ b/db/post_migrate/20170630111158_migrate_stages_statuses.rb @@ -0,0 +1,76 @@ +class MigrateStagesStatuses < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + class Build < ActiveRecord::Base + self.table_name = 'ci_builds' + + scope :relevant, -> do + where(status: %w[pending running success failed canceled skipped manual]) + end + + scope :created, -> { where(status: 'created') } + scope :running, -> { where(status: 'running') } + scope :pending, -> { where(status: 'pending') } + scope :success, -> { where(status: 'success') } + scope :failed, -> { where(status: 'failed') } + scope :canceled, -> { where(status: 'canceled') } + scope :skipped, -> { where(status: 'skipped') } + scope :manual, -> { where(status: 'manual') } + + scope :failed_but_allowed, -> do + where(allow_failure: true, status: [:failed, :canceled]) + end + + scope :exclude_ignored, -> do + where("allow_failure = ? OR status IN (?)", + false, all_state_names - [:failed, :canceled, :manual]) + end + + def status_sql + scope_relevant = relevant.exclude_ignored + scope_warnings = relevant.failed_but_allowed + + builds = scope_relevant.select('count(*)').to_sql + created = scope_relevant.created.select('count(*)').to_sql + success = scope_relevant.success.select('count(*)').to_sql + manual = scope_relevant.manual.select('count(*)').to_sql + pending = scope_relevant.pending.select('count(*)').to_sql + running = scope_relevant.running.select('count(*)').to_sql + skipped = scope_relevant.skipped.select('count(*)').to_sql + canceled = scope_relevant.canceled.select('count(*)').to_sql + warnings = scope_warnings.select('count(*) > 0').to_sql + + "(CASE + WHEN (#{builds})=(#{skipped}) AND (#{warnings}) THEN 'success' + WHEN (#{builds})=(#{skipped}) THEN 'skipped' + WHEN (#{builds})=(#{success}) THEN 'success' + WHEN (#{builds})=(#{created}) THEN 'created' + WHEN (#{builds})=(#{success})+(#{skipped}) THEN 'success' + WHEN (#{builds})=(#{success})+(#{skipped})+(#{canceled}) THEN 'canceled' + WHEN (#{builds})=(#{created})+(#{skipped})+(#{pending}) THEN 'pending' + WHEN (#{running})+(#{pending})>0 THEN 'running' + WHEN (#{manual})>0 THEN 'manual' + WHEN (#{created})>0 THEN 'running' + ELSE 'failed' + END)" + end + end + + def up + execute <<-SQL.strip_heredoc + SQL + end + + def down + execute <<-SQL.strip_heredoc + UPDATE ci_stages SET status = null + SQL + end + + private + +end diff --git a/db/schema.rb b/db/schema.rb index 8c7440ee610..f34dd32fb74 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170622162730) do +ActiveRecord::Schema.define(version: 20170630111158) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -337,6 +337,7 @@ ActiveRecord::Schema.define(version: 20170622162730) do t.datetime "created_at" t.datetime "updated_at" t.string "name" + t.integer "status" end add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name", using: :btree diff --git a/spec/migrations/migrate_stages_statuses_spec.rb b/spec/migrations/migrate_stages_statuses_spec.rb new file mode 100644 index 00000000000..dc54f4acbf4 --- /dev/null +++ b/spec/migrations/migrate_stages_statuses_spec.rb @@ -0,0 +1,50 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20170630111158_migrate_stages_statuses.rb') + +describe MigrateStagesStatuses, :migration do + let(:jobs) { table(:ci_builds) } + let(:stages) { table(:ci_stages) } + let(:pipelines) { table(:ci_pipelines) } + let(:projects) { table(:projects) } + + STATUSES = { created: 0, pending: 1, running: 2, success: 3, + failed: 4, canceled: 5, skipped: 6, manual: 7 } + STAGES = { test: 1, build: 2, deploy: 3} + + before do + projects.create!(id: 1, name: 'gitlab1', path: 'gitlab1') + projects.create!(id: 2, name: 'gitlab2', path: 'gitlab2') + + pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') + pipelines.create!(id: 2, project_id: 456, ref: 'feature', sha: '21a3deb') + + create_job(project: 1, pipeline: 1, stage: 'test', status: 'success') + create_job(project: 1, pipeline: 1, stage: 'test', status: 'running') + create_job(project: 1, pipeline: 1, stage: 'build', status: 'success') + create_job(project: 1, pipeline: 1, stage: 'build', status: 'failed') + create_job(project: 2, pipeline: 2, stage: 'test', status: 'success') + create_job(project: 2, pipeline: 2, stage: 'test', status: 'succcss') + + stages.create!(id: 1, pipeline_id: 1, project_id: 1, status: nil) + stages.create!(id: 2, pipeline_id: 1, project_id: 1, status: nil) + stages.create!(id: 3, pipeline_id: 2, project_id: 2, status: nil) + end + + pending 'correctly migrates stages statuses' do + expect(stages.where(status: nil).count).to eq 3 + + migrate! + + expect(stages.where(status: nil)).to be_empty + expect(stages.all.order(:id, :asc).pluck(:stage)) + .to eq %w[running success failed] + end + + def create_job(project:, pipeline:, stage:, status:) + stage_idx = STAGES[stage.to_sym] + status_id = STATUSES[status.to_sym] + + jobs.create!(project_id: project, commit_id: pipeline, + stage_idx: stage_idx, stage: stage, status: status_id) + end +end -- cgit v1.2.1 From a078767223ca9e66a9d5dbf614b2efc8bf7c45d4 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 3 Jul 2017 13:00:38 +0200 Subject: Improve exception description in bg migrations --- app/workers/background_migration_worker.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/workers/background_migration_worker.rb b/app/workers/background_migration_worker.rb index e7ed71a687c..45ce49bb5c0 100644 --- a/app/workers/background_migration_worker.rb +++ b/app/workers/background_migration_worker.rb @@ -20,7 +20,9 @@ class BackgroundMigrationWorker now = Time.now.to_i schedule = now + delay.to_i - raise ArgumentError, 'Delay time invalid!' if schedule <= now + if schedule <= now + raise ArgumentError, 'The schedule time must be in the future!' + end Sidekiq::Client.push_bulk('class' => self, 'queue' => sidekiq_options['queue'], -- cgit v1.2.1 From f6966d96ec5941db364a2c8d9d2d80d3aa7d20f2 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 3 Jul 2017 13:02:51 +0200 Subject: Reduce a delay between stage_id scheduled migrations --- .../20170628080858_migrate_stage_id_reference_in_background.rb | 2 +- spec/migrations/migrate_stage_id_reference_in_background_spec.rb | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb index 30849ea1361..ebec4cb6bb7 100644 --- a/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb +++ b/db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb @@ -16,7 +16,7 @@ class MigrateStageIdReferenceInBackground < ActiveRecord::Migration Build.where(stage_id: nil).in_batches(of: BATCH_SIZE) do |relation| jobs = relation.pluck(:id).map { |id| [MIGRATION, [id]] } - schedule = index * 5.minutes + schedule = index * 2.minutes index += 1 BackgroundMigrationWorker.perform_bulk_in(schedule, jobs) diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index 2e5504c849d..a32a7fceb68 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -47,10 +47,10 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do Timecop.freeze do migrate! - expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 1) - expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 2) - expect(described_class::MIGRATION).to be_scheduled_migration(10.minutes, 3) - expect(described_class::MIGRATION).to be_scheduled_migration(10.minutes, 4) + expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 1) + expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 2) + expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 3) + expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 4) expect(BackgroundMigrationWorker.jobs.size).to eq 5 end end -- cgit v1.2.1 From 7103c4a707157594c261ba2f68fbb649ca4df769 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 4 Jul 2017 09:20:18 +0200 Subject: Extend stages statuses migration --- .../20170630111158_migrate_stages_statuses.rb | 55 +++++++++++++--------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/db/post_migrate/20170630111158_migrate_stages_statuses.rb b/db/post_migrate/20170630111158_migrate_stages_statuses.rb index b4b76893595..8c6de84adf5 100644 --- a/db/post_migrate/20170630111158_migrate_stages_statuses.rb +++ b/db/post_migrate/20170630111158_migrate_stages_statuses.rb @@ -5,13 +5,17 @@ class MigrateStagesStatuses < ActiveRecord::Migration disable_ddl_transaction! + STATUSES = { created: 0, pending: 1, running: 2, success: 3, + failed: 4, canceled: 5, skipped: 6, manual: 7 } + + class Stage < ActiveRecord::Base + self.table_name = 'ci_stages' + end + class Build < ActiveRecord::Base self.table_name = 'ci_builds' - scope :relevant, -> do - where(status: %w[pending running success failed canceled skipped manual]) - end - + scope :latest, -> { where(retried: [false, nil]) } scope :created, -> { where(status: 'created') } scope :running, -> { where(status: 'running') } scope :pending, -> { where(status: 'pending') } @@ -27,12 +31,12 @@ class MigrateStagesStatuses < ActiveRecord::Migration scope :exclude_ignored, -> do where("allow_failure = ? OR status IN (?)", - false, all_state_names - [:failed, :canceled, :manual]) + false, %w[created pending running success skipped]) end - def status_sql - scope_relevant = relevant.exclude_ignored - scope_warnings = relevant.failed_but_allowed + def self.status_sql + scope_relevant = latest.exclude_ignored + scope_warnings = latest.failed_but_allowed builds = scope_relevant.select('count(*)').to_sql created = scope_relevant.created.select('count(*)').to_sql @@ -45,24 +49,33 @@ class MigrateStagesStatuses < ActiveRecord::Migration warnings = scope_warnings.select('count(*) > 0').to_sql "(CASE - WHEN (#{builds})=(#{skipped}) AND (#{warnings}) THEN 'success' - WHEN (#{builds})=(#{skipped}) THEN 'skipped' - WHEN (#{builds})=(#{success}) THEN 'success' - WHEN (#{builds})=(#{created}) THEN 'created' - WHEN (#{builds})=(#{success})+(#{skipped}) THEN 'success' - WHEN (#{builds})=(#{success})+(#{skipped})+(#{canceled}) THEN 'canceled' - WHEN (#{builds})=(#{created})+(#{skipped})+(#{pending}) THEN 'pending' - WHEN (#{running})+(#{pending})>0 THEN 'running' - WHEN (#{manual})>0 THEN 'manual' - WHEN (#{created})>0 THEN 'running' - ELSE 'failed' + WHEN (#{builds})=(#{skipped}) AND (#{warnings}) THEN #{STATUSES[:success]} + WHEN (#{builds})=(#{skipped}) THEN #{STATUSES[:skipped]} + WHEN (#{builds})=(#{success}) THEN #{STATUSES[:success]} + WHEN (#{builds})=(#{created}) THEN #{STATUSES[:created]} + WHEN (#{builds})=(#{success})+(#{skipped}) THEN #{STATUSES[:success]} + WHEN (#{builds})=(#{success})+(#{skipped})+(#{canceled}) THEN #{STATUSES[:canceled]} + WHEN (#{builds})=(#{created})+(#{skipped})+(#{pending}) THEN #{STATUSES[:pending]} + WHEN (#{running})+(#{pending})>0 THEN '#{STATUSES[:running]} + WHEN (#{manual})>0 THEN #{STATUSES[:manual]} + WHEN (#{created})>0 THEN #{STATUSES[:running]} + ELSE #{STATUSES[:failed]} END)" end end def up - execute <<-SQL.strip_heredoc - SQL + Stage.all.in_batches(of: 10000) do |relation| + status_sql = Build + .where('ci_builds.commit_id = ci_stages.pipeline_id') + .where('ci_builds.stage = ci_stages.name') + .status_sql + + execute <<-SQL.strip_heredoc + UPDATE ci_stages SET status = #{status_sql} + WHERE id = (#{relation.select(:id).to_sql}) + SQL + end end def down -- cgit v1.2.1 From d60ce6e9f44eba769a6ad595014ae96095169dd2 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 4 Jul 2017 13:43:26 +0200 Subject: Implement initial working stages statuses migration --- .../20170630111158_migrate_stages_statuses.rb | 46 ++++++++++------------ spec/migrations/migrate_stages_statuses_spec.rb | 21 +++++----- 2 files changed, 30 insertions(+), 37 deletions(-) diff --git a/db/post_migrate/20170630111158_migrate_stages_statuses.rb b/db/post_migrate/20170630111158_migrate_stages_statuses.rb index 8c6de84adf5..62542ed0001 100644 --- a/db/post_migrate/20170630111158_migrate_stages_statuses.rb +++ b/db/post_migrate/20170630111158_migrate_stages_statuses.rb @@ -48,34 +48,31 @@ class MigrateStagesStatuses < ActiveRecord::Migration canceled = scope_relevant.canceled.select('count(*)').to_sql warnings = scope_warnings.select('count(*) > 0').to_sql - "(CASE - WHEN (#{builds})=(#{skipped}) AND (#{warnings}) THEN #{STATUSES[:success]} - WHEN (#{builds})=(#{skipped}) THEN #{STATUSES[:skipped]} - WHEN (#{builds})=(#{success}) THEN #{STATUSES[:success]} - WHEN (#{builds})=(#{created}) THEN #{STATUSES[:created]} - WHEN (#{builds})=(#{success})+(#{skipped}) THEN #{STATUSES[:success]} - WHEN (#{builds})=(#{success})+(#{skipped})+(#{canceled}) THEN #{STATUSES[:canceled]} - WHEN (#{builds})=(#{created})+(#{skipped})+(#{pending}) THEN #{STATUSES[:pending]} - WHEN (#{running})+(#{pending})>0 THEN '#{STATUSES[:running]} - WHEN (#{manual})>0 THEN #{STATUSES[:manual]} - WHEN (#{created})>0 THEN #{STATUSES[:running]} - ELSE #{STATUSES[:failed]} - END)" + <<-SQL.strip_heredoc + (CASE + WHEN (#{builds}) = (#{skipped}) AND (#{warnings}) THEN #{STATUSES[:success]} + WHEN (#{builds}) = (#{skipped}) THEN #{STATUSES[:skipped]} + WHEN (#{builds}) = (#{success}) THEN #{STATUSES[:success]} + WHEN (#{builds}) = (#{created}) THEN #{STATUSES[:created]} + WHEN (#{builds}) = (#{success}) + (#{skipped}) THEN #{STATUSES[:success]} + WHEN (#{builds}) = (#{success}) + (#{skipped}) + (#{canceled}) THEN #{STATUSES[:canceled]} + WHEN (#{builds}) = (#{created}) + (#{skipped}) + (#{pending}) THEN #{STATUSES[:pending]} + WHEN (#{running}) + (#{pending}) > 0 THEN #{STATUSES[:running]} + WHEN (#{manual}) > 0 THEN #{STATUSES[:manual]} + WHEN (#{created}) > 0 THEN #{STATUSES[:running]} + ELSE #{STATUSES[:failed]} + END) + SQL end end def up - Stage.all.in_batches(of: 10000) do |relation| - status_sql = Build - .where('ci_builds.commit_id = ci_stages.pipeline_id') - .where('ci_builds.stage = ci_stages.name') - .status_sql + status_sql = Build + .where('ci_builds.commit_id = ci_stages.pipeline_id') + .where('ci_builds.stage = ci_stages.name') + .status_sql - execute <<-SQL.strip_heredoc - UPDATE ci_stages SET status = #{status_sql} - WHERE id = (#{relation.select(:id).to_sql}) - SQL - end + update_column_in_batches(:ci_stages, :status, Arel.sql("(#{status_sql})")) end def down @@ -83,7 +80,4 @@ class MigrateStagesStatuses < ActiveRecord::Migration UPDATE ci_stages SET status = null SQL end - - private - end diff --git a/spec/migrations/migrate_stages_statuses_spec.rb b/spec/migrations/migrate_stages_statuses_spec.rb index dc54f4acbf4..95fa2977b31 100644 --- a/spec/migrations/migrate_stages_statuses_spec.rb +++ b/spec/migrations/migrate_stages_statuses_spec.rb @@ -15,36 +15,35 @@ describe MigrateStagesStatuses, :migration do projects.create!(id: 1, name: 'gitlab1', path: 'gitlab1') projects.create!(id: 2, name: 'gitlab2', path: 'gitlab2') - pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') - pipelines.create!(id: 2, project_id: 456, ref: 'feature', sha: '21a3deb') + pipelines.create!(id: 1, project_id: 1, ref: 'master', sha: 'adf43c3a') + pipelines.create!(id: 2, project_id: 2, ref: 'feature', sha: '21a3deb') create_job(project: 1, pipeline: 1, stage: 'test', status: 'success') create_job(project: 1, pipeline: 1, stage: 'test', status: 'running') create_job(project: 1, pipeline: 1, stage: 'build', status: 'success') create_job(project: 1, pipeline: 1, stage: 'build', status: 'failed') create_job(project: 2, pipeline: 2, stage: 'test', status: 'success') - create_job(project: 2, pipeline: 2, stage: 'test', status: 'succcss') + create_job(project: 2, pipeline: 2, stage: 'test', status: 'success') - stages.create!(id: 1, pipeline_id: 1, project_id: 1, status: nil) - stages.create!(id: 2, pipeline_id: 1, project_id: 1, status: nil) - stages.create!(id: 3, pipeline_id: 2, project_id: 2, status: nil) + stages.create!(id: 1, pipeline_id: 1, project_id: 1, name: 'test', status: nil) + stages.create!(id: 2, pipeline_id: 1, project_id: 1, name: 'build', status: nil) + stages.create!(id: 3, pipeline_id: 2, project_id: 2, name: 'test', status: nil) end - pending 'correctly migrates stages statuses' do + it 'correctly migrates stages statuses' do expect(stages.where(status: nil).count).to eq 3 migrate! expect(stages.where(status: nil)).to be_empty - expect(stages.all.order(:id, :asc).pluck(:stage)) - .to eq %w[running success failed] + expect(stages.all.order('id ASC').pluck(:status)) + .to eq [STATUSES[:running], STATUSES[:failed], STATUSES[:success]] end def create_job(project:, pipeline:, stage:, status:) stage_idx = STAGES[stage.to_sym] - status_id = STATUSES[status.to_sym] jobs.create!(project_id: project, commit_id: pipeline, - stage_idx: stage_idx, stage: stage, status: status_id) + stage_idx: stage_idx, stage: stage, status: status) end end -- cgit v1.2.1 From f9228f6bf46f1d1caa4c62b80b8bd6ec883d33ae Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 4 Jul 2017 14:07:17 +0200 Subject: Add a test for stage status migration with retried jobs --- spec/migrations/migrate_stages_statuses_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/migrations/migrate_stages_statuses_spec.rb b/spec/migrations/migrate_stages_statuses_spec.rb index 95fa2977b31..81bc38fea10 100644 --- a/spec/migrations/migrate_stages_statuses_spec.rb +++ b/spec/migrations/migrate_stages_statuses_spec.rb @@ -24,6 +24,7 @@ describe MigrateStagesStatuses, :migration do create_job(project: 1, pipeline: 1, stage: 'build', status: 'failed') create_job(project: 2, pipeline: 2, stage: 'test', status: 'success') create_job(project: 2, pipeline: 2, stage: 'test', status: 'success') + create_job(project: 2, pipeline: 2, stage: 'test', status: 'failed', retried: true) stages.create!(id: 1, pipeline_id: 1, project_id: 1, name: 'test', status: nil) stages.create!(id: 2, pipeline_id: 1, project_id: 1, name: 'build', status: nil) @@ -40,10 +41,9 @@ describe MigrateStagesStatuses, :migration do .to eq [STATUSES[:running], STATUSES[:failed], STATUSES[:success]] end - def create_job(project:, pipeline:, stage:, status:) - stage_idx = STAGES[stage.to_sym] - + def create_job(project:, pipeline:, stage:, status:, **opts) jobs.create!(project_id: project, commit_id: pipeline, - stage_idx: stage_idx, stage: stage, status: status) + stage_idx: STAGES[stage.to_sym], stage: stage, + status: status, **opts) end end -- cgit v1.2.1 From a17c90b2a7331a7427813684b04095b55c4b3cc1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 4 Jul 2017 15:31:15 +0200 Subject: Use enumerated status in persisted stage class --- app/models/ci/stage.rb | 3 +++ app/models/concerns/has_status.rb | 10 ++++++++++ spec/factories/ci/stages.rb | 6 ++++++ spec/migrations/migrate_stages_statuses_spec.rb | 5 +++-- spec/models/ci/stage_spec.rb | 21 +++++++++++++++++++++ 5 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 spec/models/ci/stage_spec.rb diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index 59570924c8d..0c7f8c7f485 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -1,6 +1,9 @@ module Ci class Stage < ActiveRecord::Base extend Ci::Model + include HasStatus + + enumerated_status! belongs_to :project belongs_to :pipeline diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb index 32af5566135..235196cae13 100644 --- a/app/models/concerns/has_status.rb +++ b/app/models/concerns/has_status.rb @@ -8,6 +8,8 @@ module HasStatus ACTIVE_STATUSES = %w[pending running].freeze COMPLETED_STATUSES = %w[success failed canceled skipped].freeze ORDERED_STATUSES = %w[failed pending running manual canceled success skipped created].freeze + STATUSES_ENUM = { created: 0, pending: 1, running: 2, success: 3, + failed: 4, canceled: 5, skipped: 6, manual: 7 } class_methods do def status_sql @@ -54,6 +56,14 @@ module HasStatus def all_state_names state_machines.values.flat_map(&:states).flat_map { |s| s.map(&:name) } end + + private + + def enumerated_status! + @status_strategy = :enumerator + + enum status: HasStatus::STATUSES_ENUM + end end included do diff --git a/spec/factories/ci/stages.rb b/spec/factories/ci/stages.rb index d3c8bf9d54f..ee8ac85c92e 100644 --- a/spec/factories/ci/stages.rb +++ b/spec/factories/ci/stages.rb @@ -15,4 +15,10 @@ FactoryGirl.define do warnings: warnings) end end + + factory :ci_stage_entity, class: Ci::Stage do + project factory: :empty_project + pipeline factory: :ci_empty_pipeline + status 'pending' + end end diff --git a/spec/migrations/migrate_stages_statuses_spec.rb b/spec/migrations/migrate_stages_statuses_spec.rb index 81bc38fea10..478ddad262a 100644 --- a/spec/migrations/migrate_stages_statuses_spec.rb +++ b/spec/migrations/migrate_stages_statuses_spec.rb @@ -9,7 +9,6 @@ describe MigrateStagesStatuses, :migration do STATUSES = { created: 0, pending: 1, running: 2, success: 3, failed: 4, canceled: 5, skipped: 6, manual: 7 } - STAGES = { test: 1, build: 2, deploy: 3} before do projects.create!(id: 1, name: 'gitlab1', path: 'gitlab1') @@ -42,8 +41,10 @@ describe MigrateStagesStatuses, :migration do end def create_job(project:, pipeline:, stage:, status:, **opts) + stages = { test: 1, build: 2, deploy: 3} + jobs.create!(project_id: project, commit_id: pipeline, - stage_idx: STAGES[stage.to_sym], stage: stage, + stage_idx: stages[stage.to_sym], stage: stage, status: status, **opts) end end diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb new file mode 100644 index 00000000000..911c468ff1a --- /dev/null +++ b/spec/models/ci/stage_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +describe Ci::Stage, :models do + describe '#status' do + context 'when stage is pending' do + let(:stage) { create(:ci_stage_entity, status: 'pending') } + + it 'has a correct status value' do + expect(stage.status).to eq 'pending' + end + end + + context 'when stage is success' do + let(:stage) { create(:ci_stage_entity, status: 'success') } + + it 'has a correct status value' do + expect(stage.status).to eq 'success' + end + end + end +end -- cgit v1.2.1 From 22d8460b5d9926d7608d23aeb58e20d9035efa92 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 4 Jul 2017 15:50:03 +0200 Subject: Add some validations to persisted stage model --- app/models/ci/stage.rb | 5 +++++ app/models/concerns/has_status.rb | 12 ++++-------- spec/factories/ci/stages.rb | 2 ++ 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index 0c7f8c7f485..da1c3753924 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -1,6 +1,7 @@ module Ci class Stage < ActiveRecord::Base extend Ci::Model + include Importable include HasStatus enumerated_status! @@ -10,5 +11,9 @@ module Ci has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id has_many :builds, foreign_key: :commit_id + + validates :project, presence: true, unless: :importing? + validates :pipeline, presence: true, unless: :importing? + validates :name, presence: true, unless: :importing? end end diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb index 235196cae13..8ea5a007f76 100644 --- a/app/models/concerns/has_status.rb +++ b/app/models/concerns/has_status.rb @@ -12,6 +12,10 @@ module HasStatus failed: 4, canceled: 5, skipped: 6, manual: 7 } class_methods do + def enumerated_status! + enum status: HasStatus::STATUSES_ENUM + end + def status_sql scope_relevant = respond_to?(:exclude_ignored) ? exclude_ignored : all scope_warnings = respond_to?(:failed_but_allowed) ? failed_but_allowed : none @@ -56,14 +60,6 @@ module HasStatus def all_state_names state_machines.values.flat_map(&:states).flat_map { |s| s.map(&:name) } end - - private - - def enumerated_status! - @status_strategy = :enumerator - - enum status: HasStatus::STATUSES_ENUM - end end included do diff --git a/spec/factories/ci/stages.rb b/spec/factories/ci/stages.rb index ee8ac85c92e..bace932cf99 100644 --- a/spec/factories/ci/stages.rb +++ b/spec/factories/ci/stages.rb @@ -19,6 +19,8 @@ FactoryGirl.define do factory :ci_stage_entity, class: Ci::Stage do project factory: :empty_project pipeline factory: :ci_empty_pipeline + + name 'test' status 'pending' end end -- cgit v1.2.1 From b3ee172b4ee6fd22ebf3705edf6762a9dd777cdc Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 4 Jul 2017 16:41:33 +0200 Subject: Add a workaround for a tmp job -> stage relation We still didn't migrate `ci_builds.stage_id`, so we can't use a belongs_to association. We also have `ci_builds.stage` string attribute, that we need to phase out in favor of `ci_stages.name`. --- app/models/commit_status.rb | 9 +++++++++ spec/models/commit_status_spec.rb | 18 +++++++++++++++--- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 07cec63b939..afdc75f75fb 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -38,6 +38,15 @@ class CommitStatus < ActiveRecord::Base scope :retried_ordered, -> { retried.ordered.includes(project: :namespace) } scope :after_stage, -> (index) { where('stage_idx > ?', index) } + ## + # TODO, we will change this to `belongs_to :stage` when we phase out + # `ci_builds.stage` attribute and migrate `ci_builds.stage_id` reference in + # one of upcoming releases. + # + def stage_entity + Ci::Stage.find_by(pipeline: pipeline, name: stage) + end + state_machine :status do event :enqueue do transition [:created, :skipped, :manual] => :pending diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index 1e074c7ad26..c0cbf0b2f95 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -7,10 +7,10 @@ describe CommitStatus, :models do create(:ci_pipeline, project: project, sha: project.commit.id) end - let(:commit_status) { create_status } + let(:commit_status) { create_status(stage: 'test') } - def create_status(args = {}) - create(:commit_status, args.merge(pipeline: pipeline)) + def create_status(**opts) + create(:commit_status, pipeline: pipeline, **opts) end it { is_expected.to belong_to(:pipeline) } @@ -408,6 +408,18 @@ describe CommitStatus, :models do end end + describe '#stage_entity' do + let!(:stage) do + create(:ci_stage_entity, pipeline: commit_status.pipeline, + name: commit_status.stage) + end + + it 'has a correct association with persisted stage' do + expect(commit_status.stage_entity).to eq stage + end + end + + describe '#locking_enabled?' do before do commit_status.lock_version = 100 -- cgit v1.2.1 From 93d217bda639b94c129afd71343e429f935a4ada Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 4 Jul 2017 16:58:35 +0200 Subject: Migrate only old stages without status that is set --- db/post_migrate/20170630111158_migrate_stages_statuses.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/db/post_migrate/20170630111158_migrate_stages_statuses.rb b/db/post_migrate/20170630111158_migrate_stages_statuses.rb index 62542ed0001..9dac91960ff 100644 --- a/db/post_migrate/20170630111158_migrate_stages_statuses.rb +++ b/db/post_migrate/20170630111158_migrate_stages_statuses.rb @@ -72,7 +72,9 @@ class MigrateStagesStatuses < ActiveRecord::Migration .where('ci_builds.stage = ci_stages.name') .status_sql - update_column_in_batches(:ci_stages, :status, Arel.sql("(#{status_sql})")) + update_column_in_batches(:ci_stages, :status, Arel.sql("(#{status_sql})")) do |table, query| + query.where(table[:status].eq(nil)) + end end def down -- cgit v1.2.1 From c5f1e1a70bd79b36fe8cfda75b7366dd8ee90d66 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 5 Jul 2017 09:11:15 +0200 Subject: Disable statement timeout in stages statuses migration --- db/post_migrate/20170630111158_migrate_stages_statuses.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/db/post_migrate/20170630111158_migrate_stages_statuses.rb b/db/post_migrate/20170630111158_migrate_stages_statuses.rb index 9dac91960ff..c0a5294720d 100644 --- a/db/post_migrate/20170630111158_migrate_stages_statuses.rb +++ b/db/post_migrate/20170630111158_migrate_stages_statuses.rb @@ -8,10 +8,6 @@ class MigrateStagesStatuses < ActiveRecord::Migration STATUSES = { created: 0, pending: 1, running: 2, success: 3, failed: 4, canceled: 5, skipped: 6, manual: 7 } - class Stage < ActiveRecord::Base - self.table_name = 'ci_stages' - end - class Build < ActiveRecord::Base self.table_name = 'ci_builds' @@ -67,6 +63,8 @@ class MigrateStagesStatuses < ActiveRecord::Migration end def up + disable_statement_timeout + status_sql = Build .where('ci_builds.commit_id = ci_stages.pipeline_id') .where('ci_builds.stage = ci_stages.name') @@ -78,6 +76,8 @@ class MigrateStagesStatuses < ActiveRecord::Migration end def down + disable_statement_timeout + execute <<-SQL.strip_heredoc UPDATE ci_stages SET status = null SQL -- cgit v1.2.1 From 6c477d5b9496829eb5cb56ef32a0dd813be7dc16 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 5 Jul 2017 10:54:48 +0200 Subject: Move stages status migration to the background worker --- .../20170630111158_migrate_stages_statuses.rb | 71 +++----------------- .../background_migration/migrate_stage_status.rb | 76 ++++++++++++++++++++++ ...igrate_stage_id_reference_in_background_spec.rb | 13 ---- spec/migrations/migrate_stages_statuses_spec.rb | 28 ++++++-- spec/support/background_migrations_matchers.rb | 13 ++++ 5 files changed, 122 insertions(+), 79 deletions(-) create mode 100644 lib/gitlab/background_migration/migrate_stage_status.rb create mode 100644 spec/support/background_migrations_matchers.rb diff --git a/db/post_migrate/20170630111158_migrate_stages_statuses.rb b/db/post_migrate/20170630111158_migrate_stages_statuses.rb index c0a5294720d..2bc067e5d90 100644 --- a/db/post_migrate/20170630111158_migrate_stages_statuses.rb +++ b/db/post_migrate/20170630111158_migrate_stages_statuses.rb @@ -5,73 +5,22 @@ class MigrateStagesStatuses < ActiveRecord::Migration disable_ddl_transaction! - STATUSES = { created: 0, pending: 1, running: 2, success: 3, - failed: 4, canceled: 5, skipped: 6, manual: 7 } + BATCH_SIZE = 10000 + MIGRATION = 'MigrateStageStatus'.freeze - class Build < ActiveRecord::Base - self.table_name = 'ci_builds' - - scope :latest, -> { where(retried: [false, nil]) } - scope :created, -> { where(status: 'created') } - scope :running, -> { where(status: 'running') } - scope :pending, -> { where(status: 'pending') } - scope :success, -> { where(status: 'success') } - scope :failed, -> { where(status: 'failed') } - scope :canceled, -> { where(status: 'canceled') } - scope :skipped, -> { where(status: 'skipped') } - scope :manual, -> { where(status: 'manual') } - - scope :failed_but_allowed, -> do - where(allow_failure: true, status: [:failed, :canceled]) - end - - scope :exclude_ignored, -> do - where("allow_failure = ? OR status IN (?)", - false, %w[created pending running success skipped]) - end - - def self.status_sql - scope_relevant = latest.exclude_ignored - scope_warnings = latest.failed_but_allowed - - builds = scope_relevant.select('count(*)').to_sql - created = scope_relevant.created.select('count(*)').to_sql - success = scope_relevant.success.select('count(*)').to_sql - manual = scope_relevant.manual.select('count(*)').to_sql - pending = scope_relevant.pending.select('count(*)').to_sql - running = scope_relevant.running.select('count(*)').to_sql - skipped = scope_relevant.skipped.select('count(*)').to_sql - canceled = scope_relevant.canceled.select('count(*)').to_sql - warnings = scope_warnings.select('count(*) > 0').to_sql - - <<-SQL.strip_heredoc - (CASE - WHEN (#{builds}) = (#{skipped}) AND (#{warnings}) THEN #{STATUSES[:success]} - WHEN (#{builds}) = (#{skipped}) THEN #{STATUSES[:skipped]} - WHEN (#{builds}) = (#{success}) THEN #{STATUSES[:success]} - WHEN (#{builds}) = (#{created}) THEN #{STATUSES[:created]} - WHEN (#{builds}) = (#{success}) + (#{skipped}) THEN #{STATUSES[:success]} - WHEN (#{builds}) = (#{success}) + (#{skipped}) + (#{canceled}) THEN #{STATUSES[:canceled]} - WHEN (#{builds}) = (#{created}) + (#{skipped}) + (#{pending}) THEN #{STATUSES[:pending]} - WHEN (#{running}) + (#{pending}) > 0 THEN #{STATUSES[:running]} - WHEN (#{manual}) > 0 THEN #{STATUSES[:manual]} - WHEN (#{created}) > 0 THEN #{STATUSES[:running]} - ELSE #{STATUSES[:failed]} - END) - SQL - end + class Stage < ActiveRecord::Base + self.table_name = 'ci_stages' end def up - disable_statement_timeout + index = 1 - status_sql = Build - .where('ci_builds.commit_id = ci_stages.pipeline_id') - .where('ci_builds.stage = ci_stages.name') - .status_sql + Stage.where(status: nil).in_batches(of: BATCH_SIZE) do |relation| + jobs = relation.pluck(:id).map { |id| [MIGRATION, [id]] } + schedule = index * 5.minutes + index += 1 - update_column_in_batches(:ci_stages, :status, Arel.sql("(#{status_sql})")) do |table, query| - query.where(table[:status].eq(nil)) + BackgroundMigrationWorker.perform_bulk_in(schedule, jobs) end end diff --git a/lib/gitlab/background_migration/migrate_stage_status.rb b/lib/gitlab/background_migration/migrate_stage_status.rb new file mode 100644 index 00000000000..e4fdc723b13 --- /dev/null +++ b/lib/gitlab/background_migration/migrate_stage_status.rb @@ -0,0 +1,76 @@ +module Gitlab + module BackgroundMigration + class MigrateStageStatus + STATUSES = { created: 0, pending: 1, running: 2, success: 3, + failed: 4, canceled: 5, skipped: 6, manual: 7 } + + class Build < ActiveRecord::Base + self.table_name = 'ci_builds' + + scope :latest, -> { where(retried: [false, nil]) } + scope :created, -> { where(status: 'created') } + scope :running, -> { where(status: 'running') } + scope :pending, -> { where(status: 'pending') } + scope :success, -> { where(status: 'success') } + scope :failed, -> { where(status: 'failed') } + scope :canceled, -> { where(status: 'canceled') } + scope :skipped, -> { where(status: 'skipped') } + scope :manual, -> { where(status: 'manual') } + + scope :failed_but_allowed, -> do + where(allow_failure: true, status: [:failed, :canceled]) + end + + scope :exclude_ignored, -> do + where("allow_failure = ? OR status IN (?)", + false, %w[created pending running success skipped]) + end + + def self.status_sql + scope_relevant = latest.exclude_ignored + scope_warnings = latest.failed_but_allowed + + builds = scope_relevant.select('count(*)').to_sql + created = scope_relevant.created.select('count(*)').to_sql + success = scope_relevant.success.select('count(*)').to_sql + manual = scope_relevant.manual.select('count(*)').to_sql + pending = scope_relevant.pending.select('count(*)').to_sql + running = scope_relevant.running.select('count(*)').to_sql + skipped = scope_relevant.skipped.select('count(*)').to_sql + canceled = scope_relevant.canceled.select('count(*)').to_sql + warnings = scope_warnings.select('count(*) > 0').to_sql + + <<-SQL.strip_heredoc + (CASE + WHEN (#{builds}) = (#{skipped}) AND (#{warnings}) THEN #{STATUSES[:success]} + WHEN (#{builds}) = (#{skipped}) THEN #{STATUSES[:skipped]} + WHEN (#{builds}) = (#{success}) THEN #{STATUSES[:success]} + WHEN (#{builds}) = (#{created}) THEN #{STATUSES[:created]} + WHEN (#{builds}) = (#{success}) + (#{skipped}) THEN #{STATUSES[:success]} + WHEN (#{builds}) = (#{success}) + (#{skipped}) + (#{canceled}) THEN #{STATUSES[:canceled]} + WHEN (#{builds}) = (#{created}) + (#{skipped}) + (#{pending}) THEN #{STATUSES[:pending]} + WHEN (#{running}) + (#{pending}) > 0 THEN #{STATUSES[:running]} + WHEN (#{manual}) > 0 THEN #{STATUSES[:manual]} + WHEN (#{created}) > 0 THEN #{STATUSES[:running]} + ELSE #{STATUSES[:failed]} + END) + SQL + end + end + + def perform(id) + status_sql = Build + .where('ci_builds.commit_id = ci_stages.pipeline_id') + .where('ci_builds.stage = ci_stages.name') + .status_sql + + sql = <<-SQL + UPDATE ci_stages SET status = (#{status_sql}) + WHERE ci_stages.id = #{id.to_i} + SQL + + ActiveRecord::Base.connection.execute(sql) + end + end + end +end diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index a32a7fceb68..ff137cc7d47 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -2,19 +2,6 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20170628080858_migrate_stage_id_reference_in_background') describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do - matcher :be_scheduled_migration do |delay, *expected| - match do |migration| - BackgroundMigrationWorker.jobs.any? do |job| - job['args'] == [migration, expected] && - job['at'].to_i == (delay.to_i + Time.now.to_i) - end - end - - failure_message do |migration| - "Migration `#{migration}` with args `#{expected.inspect}` not scheduled!" - end - end - let(:jobs) { table(:ci_builds) } let(:stages) { table(:ci_stages) } let(:pipelines) { table(:ci_pipelines) } diff --git a/spec/migrations/migrate_stages_statuses_spec.rb b/spec/migrations/migrate_stages_statuses_spec.rb index 478ddad262a..8463583cef3 100644 --- a/spec/migrations/migrate_stages_statuses_spec.rb +++ b/spec/migrations/migrate_stages_statuses_spec.rb @@ -11,6 +11,8 @@ describe MigrateStagesStatuses, :migration do failed: 4, canceled: 5, skipped: 6, manual: 7 } before do + stub_const("#{described_class.name}::BATCH_SIZE", 2) + projects.create!(id: 1, name: 'gitlab1', path: 'gitlab1') projects.create!(id: 2, name: 'gitlab2', path: 'gitlab2') @@ -31,15 +33,31 @@ describe MigrateStagesStatuses, :migration do end it 'correctly migrates stages statuses' do - expect(stages.where(status: nil).count).to eq 3 + Sidekiq::Testing.inline! do + expect(stages.where(status: nil).count).to eq 3 - migrate! + migrate! - expect(stages.where(status: nil)).to be_empty - expect(stages.all.order('id ASC').pluck(:status)) - .to eq [STATUSES[:running], STATUSES[:failed], STATUSES[:success]] + expect(stages.where(status: nil)).to be_empty + expect(stages.all.order('id ASC').pluck(:status)) + .to eq [STATUSES[:running], STATUSES[:failed], STATUSES[:success]] + end end + it 'correctly schedules background migrations' do + Sidekiq::Testing.fake! do + Timecop.freeze do + migrate! + + expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 1) + expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 2) + expect(described_class::MIGRATION).to be_scheduled_migration(10.minutes, 3) + expect(BackgroundMigrationWorker.jobs.size).to eq 3 + end + end + end + + def create_job(project:, pipeline:, stage:, status:, **opts) stages = { test: 1, build: 2, deploy: 3} diff --git a/spec/support/background_migrations_matchers.rb b/spec/support/background_migrations_matchers.rb new file mode 100644 index 00000000000..423c0e4cefc --- /dev/null +++ b/spec/support/background_migrations_matchers.rb @@ -0,0 +1,13 @@ +RSpec::Matchers.define :be_scheduled_migration do |delay, *expected| + match do |migration| + BackgroundMigrationWorker.jobs.any? do |job| + job['args'] == [migration, expected] && + job['at'].to_i == (delay.to_i + Time.now.to_i) + end + end + + failure_message do |migration| + "Migration `#{migration}` with args `#{expected.inspect}` " \ + 'not scheduled in expected time!' + end +end -- cgit v1.2.1 From 7082530d555ad98fede2823d2123622abaf1c3a3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 10 Jul 2017 15:42:19 +0200 Subject: Schedule stages statuses bg migrations in batches --- db/post_migrate/20170630111158_migrate_stages_statuses.rb | 15 ++++++++------- lib/gitlab/background_migration/migrate_stage_status.rb | 5 +++-- spec/migrations/migrate_stages_statuses_spec.rb | 8 +++++--- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/db/post_migrate/20170630111158_migrate_stages_statuses.rb b/db/post_migrate/20170630111158_migrate_stages_statuses.rb index 2bc067e5d90..1641e550480 100644 --- a/db/post_migrate/20170630111158_migrate_stages_statuses.rb +++ b/db/post_migrate/20170630111158_migrate_stages_statuses.rb @@ -6,21 +6,22 @@ class MigrateStagesStatuses < ActiveRecord::Migration disable_ddl_transaction! BATCH_SIZE = 10000 + RANGE_SIZE = 1000 MIGRATION = 'MigrateStageStatus'.freeze class Stage < ActiveRecord::Base self.table_name = 'ci_stages' + include ::EachBatch end def up - index = 1 + Stage.where(status: nil).each_batch(of: BATCH_SIZE) do |relation, index| + relation.each_batch(of: RANGE_SIZE) do |batch| + range = relation.pluck('MIN(id)', 'MAX(id)').first + schedule = index * 5.minutes - Stage.where(status: nil).in_batches(of: BATCH_SIZE) do |relation| - jobs = relation.pluck(:id).map { |id| [MIGRATION, [id]] } - schedule = index * 5.minutes - index += 1 - - BackgroundMigrationWorker.perform_bulk_in(schedule, jobs) + BackgroundMigrationWorker.perform_in(schedule, MIGRATION, range) + end end end diff --git a/lib/gitlab/background_migration/migrate_stage_status.rb b/lib/gitlab/background_migration/migrate_stage_status.rb index e4fdc723b13..3c9744d1607 100644 --- a/lib/gitlab/background_migration/migrate_stage_status.rb +++ b/lib/gitlab/background_migration/migrate_stage_status.rb @@ -58,7 +58,7 @@ module Gitlab end end - def perform(id) + def perform(start_id, stop_id) status_sql = Build .where('ci_builds.commit_id = ci_stages.pipeline_id') .where('ci_builds.stage = ci_stages.name') @@ -66,7 +66,8 @@ module Gitlab sql = <<-SQL UPDATE ci_stages SET status = (#{status_sql}) - WHERE ci_stages.id = #{id.to_i} + WHERE ci_stages.status IS NULL + AND ci_stages.id BETWEEN #{start_id.to_i} AND #{stop_id.to_i} SQL ActiveRecord::Base.connection.execute(sql) diff --git a/spec/migrations/migrate_stages_statuses_spec.rb b/spec/migrations/migrate_stages_statuses_spec.rb index 8463583cef3..1769b1e256b 100644 --- a/spec/migrations/migrate_stages_statuses_spec.rb +++ b/spec/migrations/migrate_stages_statuses_spec.rb @@ -12,6 +12,7 @@ describe MigrateStagesStatuses, :migration do before do stub_const("#{described_class.name}::BATCH_SIZE", 2) + stub_const("#{described_class.name}::RANGE_SIZE", 1) projects.create!(id: 1, name: 'gitlab1', path: 'gitlab1') projects.create!(id: 2, name: 'gitlab2', path: 'gitlab2') @@ -49,9 +50,10 @@ describe MigrateStagesStatuses, :migration do Timecop.freeze do migrate! - expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 1) - expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 2) - expect(described_class::MIGRATION).to be_scheduled_migration(10.minutes, 3) + puts BackgroundMigrationWorker.jobs.inspect + expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 1, 1) + expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 2, 2) + expect(described_class::MIGRATION).to be_scheduled_migration(10.minutes, 3, 3) expect(BackgroundMigrationWorker.jobs.size).to eq 3 end end -- cgit v1.2.1 From 6e9924a2245004c6b6adb34028880a46fd5471df Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 11 Jul 2017 11:56:15 +0200 Subject: Add a new stage status column to safe attributes --- spec/lib/gitlab/import_export/safe_model_attributes.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 4ef3db3721f..62163c54c6d 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -219,6 +219,7 @@ Ci::Pipeline: Ci::Stage: - id - name +- status - project_id - pipeline_id - created_at -- cgit v1.2.1 From 1d087e073660130c81bf0917a6fa395886b6e2dc Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 11 Jul 2017 12:01:35 +0200 Subject: Freeze mutable constants in stages migration code --- app/models/concerns/has_status.rb | 2 +- lib/gitlab/background_migration/migrate_stage_status.rb | 2 +- spec/migrations/migrate_stages_statuses_spec.rb | 5 ++--- spec/models/commit_status_spec.rb | 1 - 4 files changed, 4 insertions(+), 6 deletions(-) diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb index 8ea5a007f76..a94226bb735 100644 --- a/app/models/concerns/has_status.rb +++ b/app/models/concerns/has_status.rb @@ -9,7 +9,7 @@ module HasStatus COMPLETED_STATUSES = %w[success failed canceled skipped].freeze ORDERED_STATUSES = %w[failed pending running manual canceled success skipped created].freeze STATUSES_ENUM = { created: 0, pending: 1, running: 2, success: 3, - failed: 4, canceled: 5, skipped: 6, manual: 7 } + failed: 4, canceled: 5, skipped: 6, manual: 7 }.freeze class_methods do def enumerated_status! diff --git a/lib/gitlab/background_migration/migrate_stage_status.rb b/lib/gitlab/background_migration/migrate_stage_status.rb index 3c9744d1607..b1ff0900709 100644 --- a/lib/gitlab/background_migration/migrate_stage_status.rb +++ b/lib/gitlab/background_migration/migrate_stage_status.rb @@ -2,7 +2,7 @@ module Gitlab module BackgroundMigration class MigrateStageStatus STATUSES = { created: 0, pending: 1, running: 2, success: 3, - failed: 4, canceled: 5, skipped: 6, manual: 7 } + failed: 4, canceled: 5, skipped: 6, manual: 7 }.freeze class Build < ActiveRecord::Base self.table_name = 'ci_builds' diff --git a/spec/migrations/migrate_stages_statuses_spec.rb b/spec/migrations/migrate_stages_statuses_spec.rb index 1769b1e256b..7a424bdaedb 100644 --- a/spec/migrations/migrate_stages_statuses_spec.rb +++ b/spec/migrations/migrate_stages_statuses_spec.rb @@ -8,7 +8,7 @@ describe MigrateStagesStatuses, :migration do let(:projects) { table(:projects) } STATUSES = { created: 0, pending: 1, running: 2, success: 3, - failed: 4, canceled: 5, skipped: 6, manual: 7 } + failed: 4, canceled: 5, skipped: 6, manual: 7 }.freeze before do stub_const("#{described_class.name}::BATCH_SIZE", 2) @@ -59,9 +59,8 @@ describe MigrateStagesStatuses, :migration do end end - def create_job(project:, pipeline:, stage:, status:, **opts) - stages = { test: 1, build: 2, deploy: 3} + stages = { test: 1, build: 2, deploy: 3 } jobs.create!(project_id: project, commit_id: pipeline, stage_idx: stages[stage.to_sym], stage: stage, diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index c0cbf0b2f95..c7651ce9b46 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -419,7 +419,6 @@ describe CommitStatus, :models do end end - describe '#locking_enabled?' do before do commit_status.lock_version = 100 -- cgit v1.2.1 From 65b3c220090d8fbfcfb2c4ba3b4d70d3c30fd7e3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 11 Jul 2017 12:28:04 +0200 Subject: Fix pipeline stages statuses migration specs --- spec/migrations/migrate_stages_statuses_spec.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/spec/migrations/migrate_stages_statuses_spec.rb b/spec/migrations/migrate_stages_statuses_spec.rb index 7a424bdaedb..ace1efa44ba 100644 --- a/spec/migrations/migrate_stages_statuses_spec.rb +++ b/spec/migrations/migrate_stages_statuses_spec.rb @@ -12,7 +12,7 @@ describe MigrateStagesStatuses, :migration do before do stub_const("#{described_class.name}::BATCH_SIZE", 2) - stub_const("#{described_class.name}::RANGE_SIZE", 1) + stub_const("#{described_class.name}::RANGE_SIZE", 2) projects.create!(id: 1, name: 'gitlab1', path: 'gitlab1') projects.create!(id: 2, name: 'gitlab2', path: 'gitlab2') @@ -50,11 +50,9 @@ describe MigrateStagesStatuses, :migration do Timecop.freeze do migrate! - puts BackgroundMigrationWorker.jobs.inspect - expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 1, 1) - expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 2, 2) + expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 1, 2) expect(described_class::MIGRATION).to be_scheduled_migration(10.minutes, 3, 3) - expect(BackgroundMigrationWorker.jobs.size).to eq 3 + expect(BackgroundMigrationWorker.jobs.size).to eq 2 end end end -- cgit v1.2.1 From bb67b4749b5b4c62d4235c90dc0320967f850cdd Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 11 Jul 2017 14:31:04 +0200 Subject: Update version number of stages statuses migration --- .../20170630105320_add_status_to_ci_stages.rb | 9 ------ .../20170711145320_add_status_to_ci_stages.rb | 9 ++++++ .../20170630111158_migrate_stages_statuses.rb | 35 ---------------------- .../20170711145558_migrate_stages_statuses.rb | 35 ++++++++++++++++++++++ db/schema.rb | 2 +- spec/migrations/migrate_stages_statuses_spec.rb | 2 +- 6 files changed, 46 insertions(+), 46 deletions(-) delete mode 100644 db/migrate/20170630105320_add_status_to_ci_stages.rb create mode 100644 db/migrate/20170711145320_add_status_to_ci_stages.rb delete mode 100644 db/post_migrate/20170630111158_migrate_stages_statuses.rb create mode 100644 db/post_migrate/20170711145558_migrate_stages_statuses.rb diff --git a/db/migrate/20170630105320_add_status_to_ci_stages.rb b/db/migrate/20170630105320_add_status_to_ci_stages.rb deleted file mode 100644 index d497a61a959..00000000000 --- a/db/migrate/20170630105320_add_status_to_ci_stages.rb +++ /dev/null @@ -1,9 +0,0 @@ -class AddStatusToCiStages < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - def change - add_column :ci_stages, :status, :integer - end -end diff --git a/db/migrate/20170711145320_add_status_to_ci_stages.rb b/db/migrate/20170711145320_add_status_to_ci_stages.rb new file mode 100644 index 00000000000..d497a61a959 --- /dev/null +++ b/db/migrate/20170711145320_add_status_to_ci_stages.rb @@ -0,0 +1,9 @@ +class AddStatusToCiStages < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :ci_stages, :status, :integer + end +end diff --git a/db/post_migrate/20170630111158_migrate_stages_statuses.rb b/db/post_migrate/20170630111158_migrate_stages_statuses.rb deleted file mode 100644 index 1641e550480..00000000000 --- a/db/post_migrate/20170630111158_migrate_stages_statuses.rb +++ /dev/null @@ -1,35 +0,0 @@ -class MigrateStagesStatuses < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - disable_ddl_transaction! - - BATCH_SIZE = 10000 - RANGE_SIZE = 1000 - MIGRATION = 'MigrateStageStatus'.freeze - - class Stage < ActiveRecord::Base - self.table_name = 'ci_stages' - include ::EachBatch - end - - def up - Stage.where(status: nil).each_batch(of: BATCH_SIZE) do |relation, index| - relation.each_batch(of: RANGE_SIZE) do |batch| - range = relation.pluck('MIN(id)', 'MAX(id)').first - schedule = index * 5.minutes - - BackgroundMigrationWorker.perform_in(schedule, MIGRATION, range) - end - end - end - - def down - disable_statement_timeout - - execute <<-SQL.strip_heredoc - UPDATE ci_stages SET status = null - SQL - end -end diff --git a/db/post_migrate/20170711145558_migrate_stages_statuses.rb b/db/post_migrate/20170711145558_migrate_stages_statuses.rb new file mode 100644 index 00000000000..1641e550480 --- /dev/null +++ b/db/post_migrate/20170711145558_migrate_stages_statuses.rb @@ -0,0 +1,35 @@ +class MigrateStagesStatuses < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + BATCH_SIZE = 10000 + RANGE_SIZE = 1000 + MIGRATION = 'MigrateStageStatus'.freeze + + class Stage < ActiveRecord::Base + self.table_name = 'ci_stages' + include ::EachBatch + end + + def up + Stage.where(status: nil).each_batch(of: BATCH_SIZE) do |relation, index| + relation.each_batch(of: RANGE_SIZE) do |batch| + range = relation.pluck('MIN(id)', 'MAX(id)').first + schedule = index * 5.minutes + + BackgroundMigrationWorker.perform_in(schedule, MIGRATION, range) + end + end + end + + def down + disable_statement_timeout + + execute <<-SQL.strip_heredoc + UPDATE ci_stages SET status = null + SQL + end +end diff --git a/db/schema.rb b/db/schema.rb index e0c9e5efb33..3ef311f48d8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170707184244) do +ActiveRecord::Schema.define(version: 20170711145558) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/spec/migrations/migrate_stages_statuses_spec.rb b/spec/migrations/migrate_stages_statuses_spec.rb index ace1efa44ba..4102d57e368 100644 --- a/spec/migrations/migrate_stages_statuses_spec.rb +++ b/spec/migrations/migrate_stages_statuses_spec.rb @@ -1,5 +1,5 @@ require 'spec_helper' -require Rails.root.join('db', 'post_migrate', '20170630111158_migrate_stages_statuses.rb') +require Rails.root.join('db', 'post_migrate', '20170711145558_migrate_stages_statuses.rb') describe MigrateStagesStatuses, :migration do let(:jobs) { table(:ci_builds) } -- cgit v1.2.1 From c7a7ef044cf79dcd5ffd25b9fb325cd0abd612b2 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 20 Jul 2017 12:56:27 +0200 Subject: Use a new stage_id reference to a persisted stage --- app/models/commit_status.rb | 4 +--- spec/models/commit_status_spec.rb | 7 ++++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index afdc75f75fb..efb5cbd9d41 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -43,9 +43,7 @@ class CommitStatus < ActiveRecord::Base # `ci_builds.stage` attribute and migrate `ci_builds.stage_id` reference in # one of upcoming releases. # - def stage_entity - Ci::Stage.find_by(pipeline: pipeline, name: stage) - end + belongs_to :stage_entity, foreign_key: :stage_id, class_name: 'Ci::Stage' state_machine :status do event :enqueue do diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index c7651ce9b46..f54e1131813 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -409,9 +409,10 @@ describe CommitStatus, :models do end describe '#stage_entity' do - let!(:stage) do - create(:ci_stage_entity, pipeline: commit_status.pipeline, - name: commit_status.stage) + let(:stage) { create(:ci_stage_entity) } + + let(:commit_status) do + create(:commit_status, stage_id: stage.id) end it 'has a correct association with persisted stage' do -- cgit v1.2.1 From 0605cdd7590b12bad073bf41f3e793274e931a80 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 20 Jul 2017 13:05:26 +0200 Subject: Implement proper associations with a persisted stage --- app/models/ci/stage.rb | 4 ++-- spec/models/ci/stage_spec.rb | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index da1c3753924..ca89caf4782 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -9,8 +9,8 @@ module Ci belongs_to :project belongs_to :pipeline - has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id - has_many :builds, foreign_key: :commit_id + has_many :commit_statuses, foreign_key: :stage_id + has_many :builds, foreign_key: :stage_id validates :project, presence: true, unless: :importing? validates :pipeline, presence: true, unless: :importing? diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb index 911c468ff1a..49573175266 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -1,6 +1,27 @@ require 'spec_helper' describe Ci::Stage, :models do + describe 'associations' do + let(:stage) { create(:ci_stage_entity) } + + before do + create(:ci_build, stage_id: stage.id) + create(:commit_status, stage_id: stage.id) + end + + describe '#commit_statuses' do + it 'returns all commit statuses' do + expect(stage.commit_statuses.count).to be 2 + end + end + + describe '#builds' do + it 'returns only builds' do + expect(stage.builds).to be_one + end + end + end + describe '#status' do context 'when stage is pending' do let(:stage) { create(:ci_stage_entity, status: 'pending') } -- cgit v1.2.1 From 470661e1a70bd3c8415387e9068823536d1fb1bc Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 20 Jul 2017 13:06:45 +0200 Subject: Change a method name which enumerates CI/CD statuses --- app/models/ci/stage.rb | 2 +- app/models/concerns/has_status.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index ca89caf4782..c163d047127 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -4,7 +4,7 @@ module Ci include Importable include HasStatus - enumerated_status! + enumerate_status! belongs_to :project belongs_to :pipeline diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb index a94226bb735..758d71b7f4c 100644 --- a/app/models/concerns/has_status.rb +++ b/app/models/concerns/has_status.rb @@ -12,7 +12,7 @@ module HasStatus failed: 4, canceled: 5, skipped: 6, manual: 7 }.freeze class_methods do - def enumerated_status! + def enumerate_status! enum status: HasStatus::STATUSES_ENUM end -- cgit v1.2.1 From e389507650769304bd61f7a82431cc6c07feb364 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 20 Jul 2017 13:17:48 +0200 Subject: Add optimistic locking column to ci_stages table --- db/migrate/20170720111708_add_lock_version_to_ci_stages.rb | 9 +++++++++ db/schema.rb | 3 ++- 2 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20170720111708_add_lock_version_to_ci_stages.rb diff --git a/db/migrate/20170720111708_add_lock_version_to_ci_stages.rb b/db/migrate/20170720111708_add_lock_version_to_ci_stages.rb new file mode 100644 index 00000000000..e1c4f033286 --- /dev/null +++ b/db/migrate/20170720111708_add_lock_version_to_ci_stages.rb @@ -0,0 +1,9 @@ +class AddLockVersionToCiStages < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :ci_stages, :lock_version, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 862b2e21f4d..567ba4d061b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170717150329) do +ActiveRecord::Schema.define(version: 20170720111708) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -366,6 +366,7 @@ ActiveRecord::Schema.define(version: 20170717150329) do t.datetime "updated_at" t.string "name" t.integer "status" + t.integer "lock_version" end add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name", using: :btree -- cgit v1.2.1 From bbdc35717c1ba08630f5b2ae59a333a81941b181 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 20 Jul 2017 13:30:05 +0200 Subject: Implement method that updates a stage status --- app/models/ci/stage.rb | 47 ++++++++++++++++++++++++++++++++++++++++++++ spec/models/ci/stage_spec.rb | 22 +++++++++++++++++++-- 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index c163d047127..066903ddc5b 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -3,6 +3,7 @@ module Ci extend Ci::Model include Importable include HasStatus + include Gitlab::OptimisticLocking enumerate_status! @@ -15,5 +16,51 @@ module Ci validates :project, presence: true, unless: :importing? validates :pipeline, presence: true, unless: :importing? validates :name, presence: true, unless: :importing? + + state_machine :status, initial: :created do + event :enqueue do + transition created: :pending + transition [:success, :failed, :canceled, :skipped] => :running + end + + event :run do + transition any - [:running] => :running + end + + event :skip do + transition any - [:skipped] => :skipped + end + + event :drop do + transition any - [:failed] => :failed + end + + event :succeed do + transition any - [:success] => :success + end + + event :cancel do + transition any - [:canceled] => :canceled + end + + event :block do + transition any - [:manual] => :manual + end + end + + def update! + retry_optimistic_lock(self) do + case commit_statuses.latest.status + when 'pending' then enqueue + when 'running' then run + when 'success' then succeed + when 'failed' then drop + when 'canceled' then cancel + when 'manual' then block + when 'skipped' then skip + else skip + end + end + end end end diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb index 49573175266..e829ccb048e 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -1,9 +1,9 @@ require 'spec_helper' describe Ci::Stage, :models do - describe 'associations' do - let(:stage) { create(:ci_stage_entity) } + let(:stage) { create(:ci_stage_entity) } + describe 'associations' do before do create(:ci_build, stage_id: stage.id) create(:commit_status, stage_id: stage.id) @@ -39,4 +39,22 @@ describe Ci::Stage, :models do end end end + + describe 'update!' do + context 'when stage objects needs to be updated' do + before do + create(:ci_build, :success, stage_id: stage.id) + create(:ci_build, :running, stage_id: stage.id) + end + + it 'updates stage status correctly' do + expect { stage.update! } + .to change { stage.reload.status } + .to 'running' + end + end + + context 'when stage object is locked' do + end + end end -- cgit v1.2.1 From 5505795ed2e65da20a896b67422a075515552a35 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 20 Jul 2017 13:30:58 +0200 Subject: Add simple asynchronous stage update worker --- app/workers/stage_update_worker.rb | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 app/workers/stage_update_worker.rb diff --git a/app/workers/stage_update_worker.rb b/app/workers/stage_update_worker.rb new file mode 100644 index 00000000000..1d6f23305d6 --- /dev/null +++ b/app/workers/stage_update_worker.rb @@ -0,0 +1,8 @@ +class StageUpdateWorker + include Sidekiq::Worker + include PipelineQueue + + def perform(stage_id) + Ci::Stage.find_by(id: stage_id)&.update! + end +end -- cgit v1.2.1 From d3814ad69876c69cdab574e2958368f2ab648171 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 24 Jul 2017 11:22:01 +0200 Subject: Adds some specs for stage optimistic locking --- .../gitlab/import_export/safe_model_attributes.yml | 1 + spec/models/ci/stage_spec.rb | 19 +++++++++++++++++++ spec/services/ci/retry_build_service_spec.rb | 2 +- 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 62163c54c6d..7bbeea06f04 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -220,6 +220,7 @@ Ci::Stage: - id - name - status +- lock_version - project_id - pipeline_id - created_at diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb index e829ccb048e..3dc00017a2c 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -54,7 +54,26 @@ describe Ci::Stage, :models do end end + context 'when stage is skipped' do + it 'updates status to skipped' do + expect { stage.update! } + .to change { stage.reload.status } + .to 'skipped' + end + end + context 'when stage object is locked' do + before do + create(:ci_build, :failed, stage_id: stage.id) + end + + it 'retries a lock to update a stage status' do + stage.lock_version = 100 + + stage.update! + + expect(stage.reload).to be_failed + end end end end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index ef9927c5969..a0e83deeb54 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -22,7 +22,7 @@ describe Ci::RetryBuildService, :services do %i[type lock_version target_url base_tags commit_id deployments erased_by_id last_deployment project_id runner_id tag_taggings taggings tags trigger_request_id - user_id auto_canceled_by_id retried].freeze + user_id auto_canceled_by_id retried stage_entity].freeze shared_examples 'build duplication' do let(:stage) do -- cgit v1.2.1 From 8657d5dd8af6c365b41d7c2997e6e5c9e18e8241 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 24 Jul 2017 11:33:01 +0200 Subject: Do not implement CI/CD job to stage association yet --- app/models/commit_status.rb | 15 ++++----------- spec/models/commit_status_spec.rb | 12 ------------ spec/services/ci/retry_build_service_spec.rb | 2 +- 3 files changed, 5 insertions(+), 24 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index efb5cbd9d41..d7418a07177 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -38,22 +38,15 @@ class CommitStatus < ActiveRecord::Base scope :retried_ordered, -> { retried.ordered.includes(project: :namespace) } scope :after_stage, -> (index) { where('stage_idx > ?', index) } - ## - # TODO, we will change this to `belongs_to :stage` when we phase out - # `ci_builds.stage` attribute and migrate `ci_builds.stage_id` reference in - # one of upcoming releases. - # - belongs_to :stage_entity, foreign_key: :stage_id, class_name: 'Ci::Stage' - state_machine :status do - event :enqueue do - transition [:created, :skipped, :manual] => :pending - end - event :process do transition [:skipped, :manual] => :created end + event :enqueue do + transition [:created, :skipped, :manual] => :pending + end + event :run do transition pending: :running end diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index f54e1131813..f173e2c142a 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -408,18 +408,6 @@ describe CommitStatus, :models do end end - describe '#stage_entity' do - let(:stage) { create(:ci_stage_entity) } - - let(:commit_status) do - create(:commit_status, stage_id: stage.id) - end - - it 'has a correct association with persisted stage' do - expect(commit_status.stage_entity).to eq stage - end - end - describe '#locking_enabled?' do before do commit_status.lock_version = 100 diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index a0e83deeb54..ef9927c5969 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -22,7 +22,7 @@ describe Ci::RetryBuildService, :services do %i[type lock_version target_url base_tags commit_id deployments erased_by_id last_deployment project_id runner_id tag_taggings taggings tags trigger_request_id - user_id auto_canceled_by_id retried stage_entity].freeze + user_id auto_canceled_by_id retried].freeze shared_examples 'build duplication' do let(:stage) do -- cgit v1.2.1 From 865de49b0832dc1e2fa74034a25d186980b6a361 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 24 Jul 2017 11:34:01 +0200 Subject: Update related stage status when job status is changed --- app/models/ci/stage.rb | 2 ++ app/models/commit_status.rb | 1 + 2 files changed, 3 insertions(+) diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index 066903ddc5b..78caa70e52e 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -17,6 +17,8 @@ module Ci validates :pipeline, presence: true, unless: :importing? validates :name, presence: true, unless: :importing? + ## TODO, should we extract these events to `Ci::Eventable`? + # state_machine :status, initial: :created do event :enqueue do transition created: :pending diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index d7418a07177..842c6e5cb50 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -91,6 +91,7 @@ class CommitStatus < ActiveRecord::Base end end + StageUpdateWorker.perform_async(commit_status.stage_id) ExpireJobCacheWorker.perform_async(commit_status.id) end end -- cgit v1.2.1 From c14bd53d7483b57d47159c52dcb1b3456885f153 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 24 Jul 2017 14:13:29 +0200 Subject: Fix import/export for CI/CD stage commit statuses --- spec/lib/gitlab/import_export/all_models.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 977174a5fd2..28df089928f 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -115,7 +115,7 @@ pipelines: stages: - project - pipeline -- statuses +- commit_statuses - builds statuses: - project -- cgit v1.2.1 From f4e01b597c7cb40797d0b690451322bc79d8dfe0 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 26 Jul 2017 14:48:32 +0200 Subject: Move enum specific code from a concern to CI stage --- app/models/ci/stage.rb | 2 +- app/models/concerns/has_status.rb | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index 78caa70e52e..b1cca06abaa 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -5,7 +5,7 @@ module Ci include HasStatus include Gitlab::OptimisticLocking - enumerate_status! + enum status: HasStatus::STATUSES_ENUM belongs_to :project belongs_to :pipeline diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb index 758d71b7f4c..3803e18a96e 100644 --- a/app/models/concerns/has_status.rb +++ b/app/models/concerns/has_status.rb @@ -12,10 +12,6 @@ module HasStatus failed: 4, canceled: 5, skipped: 6, manual: 7 }.freeze class_methods do - def enumerate_status! - enum status: HasStatus::STATUSES_ENUM - end - def status_sql scope_relevant = respond_to?(:exclude_ignored) ? exclude_ignored : all scope_warnings = respond_to?(:failed_but_allowed) ? failed_but_allowed : none -- cgit v1.2.1 From 7d6538f2e2a6f1b0808a77e347a9083295b17c8c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 26 Jul 2017 14:50:48 +0200 Subject: Rename method responsible for updating stage status --- app/models/ci/stage.rb | 2 +- app/workers/stage_update_worker.rb | 4 +++- spec/models/ci/stage_spec.rb | 8 ++++---- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index b1cca06abaa..cd2f1dd3509 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -50,7 +50,7 @@ module Ci end end - def update! + def update_status retry_optimistic_lock(self) do case commit_statuses.latest.status when 'pending' then enqueue diff --git a/app/workers/stage_update_worker.rb b/app/workers/stage_update_worker.rb index 1d6f23305d6..eef0b11e70b 100644 --- a/app/workers/stage_update_worker.rb +++ b/app/workers/stage_update_worker.rb @@ -3,6 +3,8 @@ class StageUpdateWorker include PipelineQueue def perform(stage_id) - Ci::Stage.find_by(id: stage_id)&.update! + Ci::Stage.find_by(id: stage_id).try do |stage| + stage.update_status + end end end diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb index 3dc00017a2c..d5c66598451 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -40,7 +40,7 @@ describe Ci::Stage, :models do end end - describe 'update!' do + describe 'update_status' do context 'when stage objects needs to be updated' do before do create(:ci_build, :success, stage_id: stage.id) @@ -48,7 +48,7 @@ describe Ci::Stage, :models do end it 'updates stage status correctly' do - expect { stage.update! } + expect { stage.update_status } .to change { stage.reload.status } .to 'running' end @@ -56,7 +56,7 @@ describe Ci::Stage, :models do context 'when stage is skipped' do it 'updates status to skipped' do - expect { stage.update! } + expect { stage.update_status } .to change { stage.reload.status } .to 'skipped' end @@ -70,7 +70,7 @@ describe Ci::Stage, :models do it 'retries a lock to update a stage status' do stage.lock_version = 100 - stage.update! + stage.update_status expect(stage.reload).to be_failed end -- cgit v1.2.1 From cec2cc3ffa3077346c81625f6b1f48c113e19e93 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 31 Jul 2017 12:07:05 +0200 Subject: Add specs for stage update worker --- spec/workers/stage_update_worker_spec.rb | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 spec/workers/stage_update_worker_spec.rb diff --git a/spec/workers/stage_update_worker_spec.rb b/spec/workers/stage_update_worker_spec.rb new file mode 100644 index 00000000000..7bc76c79464 --- /dev/null +++ b/spec/workers/stage_update_worker_spec.rb @@ -0,0 +1,22 @@ +require 'spec_helper' + +describe StageUpdateWorker do + describe '#perform' do + context 'when stage exists' do + let(:stage) { create(:ci_stage_entity) } + + it 'updates stage status' do + expect_any_instance_of(Ci::Stage).to receive(:update_status) + + described_class.new.perform(stage.id) + end + end + + context 'when stage does not exist' do + it 'does not raise exception' do + expect { described_class.new.perform(123) } + .not_to raise_error + end + end + end +end -- cgit v1.2.1 From 13a15e7009e292919109ea911640a627dbd0e327 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 31 Jul 2017 12:23:04 +0200 Subject: Use update_column_in_batches helper in stages migration --- app/models/ci/stage.rb | 2 -- db/post_migrate/20170711145558_migrate_stages_statuses.rb | 4 +--- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index cd2f1dd3509..7819bc3cd2c 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -17,8 +17,6 @@ module Ci validates :pipeline, presence: true, unless: :importing? validates :name, presence: true, unless: :importing? - ## TODO, should we extract these events to `Ci::Eventable`? - # state_machine :status, initial: :created do event :enqueue do transition created: :pending diff --git a/db/post_migrate/20170711145558_migrate_stages_statuses.rb b/db/post_migrate/20170711145558_migrate_stages_statuses.rb index 1641e550480..5a24fb1307f 100644 --- a/db/post_migrate/20170711145558_migrate_stages_statuses.rb +++ b/db/post_migrate/20170711145558_migrate_stages_statuses.rb @@ -28,8 +28,6 @@ class MigrateStagesStatuses < ActiveRecord::Migration def down disable_statement_timeout - execute <<-SQL.strip_heredoc - UPDATE ci_stages SET status = null - SQL + update_column_in_batches(:ci_stages, :status, nil) end end -- cgit v1.2.1 From 1066d8ba77ba242851c906aa523bd7548dad1d15 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 16 Aug 2017 13:30:49 +0200 Subject: Use usual method to retrieve CI/CD stage statuses --- app/models/ci/stage.rb | 4 ++-- spec/factories/ci/stages.rb | 2 +- spec/lib/gitlab/import_export/all_models.yml | 2 +- spec/models/ci/stage_spec.rb | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index 7819bc3cd2c..4ee972fa68d 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -10,7 +10,7 @@ module Ci belongs_to :project belongs_to :pipeline - has_many :commit_statuses, foreign_key: :stage_id + has_many :statuses, class_name: 'CommitStatus', foreign_key: :stage_id has_many :builds, foreign_key: :stage_id validates :project, presence: true, unless: :importing? @@ -50,7 +50,7 @@ module Ci def update_status retry_optimistic_lock(self) do - case commit_statuses.latest.status + case statuses.latest.status when 'pending' then enqueue when 'running' then run when 'success' then succeed diff --git a/spec/factories/ci/stages.rb b/spec/factories/ci/stages.rb index bace932cf99..b2ded945738 100644 --- a/spec/factories/ci/stages.rb +++ b/spec/factories/ci/stages.rb @@ -17,7 +17,7 @@ FactoryGirl.define do end factory :ci_stage_entity, class: Ci::Stage do - project factory: :empty_project + project factory: :project pipeline factory: :ci_empty_pipeline name 'test' diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index f323777b13a..8da02b0cf00 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -119,7 +119,7 @@ pipeline_variables: stages: - project - pipeline -- commit_statuses +- statuses - builds statuses: - project diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb index d5c66598451..74c9d6145e2 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -9,9 +9,9 @@ describe Ci::Stage, :models do create(:commit_status, stage_id: stage.id) end - describe '#commit_statuses' do + describe '#statuses' do it 'returns all commit statuses' do - expect(stage.commit_statuses.count).to be 2 + expect(stage.statuses.count).to be 2 end end -- cgit v1.2.1 From 3a1103fd9173e8cb7a70c871d6a54a846f6eee4a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 17 Aug 2017 12:27:02 +0200 Subject: Add specs for stage status background migration class --- .../migrate_stage_status_spec.rb | 80 ++++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 spec/lib/gitlab/background_migration/migrate_stage_status_spec.rb diff --git a/spec/lib/gitlab/background_migration/migrate_stage_status_spec.rb b/spec/lib/gitlab/background_migration/migrate_stage_status_spec.rb new file mode 100644 index 00000000000..878158910be --- /dev/null +++ b/spec/lib/gitlab/background_migration/migrate_stage_status_spec.rb @@ -0,0 +1,80 @@ +require 'spec_helper' + +describe Gitlab::BackgroundMigration::MigrateStageStatus, :migration, schema: 20170711145320 do + let(:projects) { table(:projects) } + let(:pipelines) { table(:ci_pipelines) } + let(:stages) { table(:ci_stages) } + let(:jobs) { table(:ci_builds) } + + STATUSES = { created: 0, pending: 1, running: 2, success: 3, + failed: 4, canceled: 5, skipped: 6, manual: 7 }.freeze + + before do + projects.create!(id: 1, name: 'gitlab1', path: 'gitlab1') + pipelines.create!(id: 1, project_id: 1, ref: 'master', sha: 'adf43c3a') + stages.create!(id: 1, pipeline_id: 1, project_id: 1, name: 'test', status: nil) + stages.create!(id: 2, pipeline_id: 1, project_id: 1, name: 'deploy', status: nil) + end + + context 'when stage status is known' do + before do + create_job(project: 1, pipeline: 1, stage: 'test', status: 'success') + create_job(project: 1, pipeline: 1, stage: 'test', status: 'running') + create_job(project: 1, pipeline: 1, stage: 'deploy', status: 'failed') + end + + it 'sets a correct stage status' do + described_class.new.perform(1, 2) + + expect(stages.first.status).to eq STATUSES[:running] + expect(stages.second.status).to eq STATUSES[:failed] + end + end + + context 'when stage status is not known' do + it 'sets a skipped stage status' do + described_class.new.perform(1, 2) + + expect(stages.first.status).to eq STATUSES[:skipped] + expect(stages.second.status).to eq STATUSES[:skipped] + end + end + + context 'when stage status includes status of a retried job' do + before do + create_job(project: 1, pipeline: 1, stage: 'test', status: 'canceled') + create_job(project: 1, pipeline: 1, stage: 'deploy', status: 'failed', retried: true) + create_job(project: 1, pipeline: 1, stage: 'deploy', status: 'success') + end + + it 'sets a correct stage status' do + described_class.new.perform(1, 2) + + expect(stages.first.status).to eq STATUSES[:canceled] + expect(stages.second.status).to eq STATUSES[:success] + end + end + + context 'when some job in the stage is blocked / manual' do + before do + create_job(project: 1, pipeline: 1, stage: 'test', status: 'failed') + create_job(project: 1, pipeline: 1, stage: 'test', status: 'manual') + create_job(project: 1, pipeline: 1, stage: 'deploy', status: 'success', when: 'manual') + end + + it 'sets a correct stage status' do + described_class.new.perform(1, 2) + + expect(stages.first.status).to eq STATUSES[:manual] + expect(stages.second.status).to eq STATUSES[:success] + end + end + + def create_job(project:, pipeline:, stage:, status:, **opts) + stages = { test: 1, build: 2, deploy: 3 } + + jobs.create!(project_id: project, commit_id: pipeline, + stage_idx: stages[stage.to_sym], stage: stage, + status: status, **opts) + end +end -- cgit v1.2.1