Set all NULL `lock_version` values to 0 PART 2

Rails silently casts NULL `lock_version` values to 0 while doing
optimistic locking, which causes false stale object exceptions. We had a
monkey patch that would change it to check for [NULL, 0] but want to
avoid monkey-patching if possible, which means we need to clean up our
database values.

This Commit is for CI objects (CI Stages, CI Builds, CI Pipelines)

changelogs/unreleased/28560_cleanup_optimistic_locking_db_pt2.yml

+---
+title: Set NULL `lock_version` values to 0 for CI objects
+merge_request: 25396
+author:
+type: fixed

db/post_migrate/20200128210353_cleanup_optimistic_locking_nulls.rb

@@ -31,7 +31,7 @@ class CleanupOptimisticLockingNulls < ActiveRecord::Migration[5.2]
         'CleanupOptimisticLockingNulls',
         2.minutes,
         batch_size: BATCH_SIZE,
-        other_arguments: [table]
+        other_job_arguments: [table]
       )
     end
   end

db/post_migrate/20200217210353_cleanup_optimistic_locking_nulls_pt2.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class CleanupOptimisticLockingNullsPt2 < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  TABLES = %w(ci_stages ci_builds ci_pipelines).freeze
+  BATCH_SIZE = 10_000
+
+  def declare_class(table)
+    Class.new(ActiveRecord::Base) do
+      include EachBatch
+
+      self.table_name = table
+      self.inheritance_column = :_type_disabled # Disable STI
+    end
+  end
+
+  def up
+    last_table_final_delay = 0
+
+    TABLES.each do |table|
+      add_concurrent_index table.to_sym, :lock_version, where: "lock_version IS NULL"
+
+      last_table_final_delay = queue_background_migration_jobs_by_range_at_intervals(
+        declare_class(table).where(lock_version: nil),
+        'CleanupOptimisticLockingNulls',
+        2.minutes,
+        batch_size: BATCH_SIZE,
+        other_job_arguments: [table],
+        initial_delay: last_table_final_delay
+      )
+    end
+  end
+
+  def down
+    TABLES.each do |table|
+      remove_concurrent_index table.to_sym, :lock_version, where: "lock_version IS NULL"
+    end
+  end
+end

db/structure.sql

@@ -8632,6 +8632,8 @@ CREATE INDEX index_ci_builds_on_commit_id_and_type_and_name_and_ref ON public.ci
 
 CREATE INDEX index_ci_builds_on_commit_id_and_type_and_ref ON public.ci_builds USING btree (commit_id, type, ref);
 
+CREATE INDEX index_ci_builds_on_lock_version ON public.ci_builds USING btree (lock_version) WHERE (lock_version IS NULL);
+
 CREATE INDEX index_ci_builds_on_name_and_security_type_eq_ci_build ON public.ci_builds USING btree (name, id) WHERE (((name)::text = ANY (ARRAY[('container_scanning'::character varying)::text, ('dast'::character varying)::text, ('dependency_scanning'::character varying)::text, ('license_management'::character varying)::text, ('sast'::character varying)::text, ('license_scanning'::character varying)::text])) AND ((type)::text = 'Ci::Build'::text));
 
 CREATE INDEX index_ci_builds_on_name_for_security_reports_values ON public.ci_builds USING btree (name) WHERE ((name)::text = ANY (ARRAY[('container_scanning'::character varying)::text, ('dast'::character varying)::text, ('dependency_scanning'::character varying)::text, ('license_management'::character varying)::text, ('sast'::character varying)::text, ('license_scanning'::character varying)::text]));
@@ -8708,6 +8710,8 @@ CREATE INDEX index_ci_pipelines_on_auto_canceled_by_id ON public.ci_pipelines US
 
 CREATE INDEX index_ci_pipelines_on_external_pull_request_id ON public.ci_pipelines USING btree (external_pull_request_id) WHERE (external_pull_request_id IS NOT NULL);
 
+CREATE INDEX index_ci_pipelines_on_lock_version ON public.ci_pipelines USING btree (lock_version) WHERE (lock_version IS NULL);
+
 CREATE INDEX index_ci_pipelines_on_merge_request_id ON public.ci_pipelines USING btree (merge_request_id) WHERE (merge_request_id IS NOT NULL);
 
 CREATE INDEX index_ci_pipelines_on_pipeline_schedule_id ON public.ci_pipelines USING btree (pipeline_schedule_id);
@@ -8776,6 +8780,8 @@ CREATE INDEX index_ci_sources_projects_on_pipeline_id ON public.ci_sources_proje
 
 CREATE UNIQUE INDEX index_ci_sources_projects_on_source_project_id_and_pipeline_id ON public.ci_sources_projects USING btree (source_project_id, pipeline_id);
 
+CREATE INDEX index_ci_stages_on_lock_version ON public.ci_stages USING btree (lock_version) WHERE (lock_version IS NULL);
+
 CREATE INDEX index_ci_stages_on_pipeline_id ON public.ci_stages USING btree (pipeline_id);
 
 CREATE UNIQUE INDEX index_ci_stages_on_pipeline_id_and_name ON public.ci_stages USING btree (pipeline_id, name);
@@ -12736,6 +12742,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200214214934
 20200215222507
 20200215225103
+20200217210353
 20200217223651
 20200217225719
 20200219105209

lib/gitlab/database/migration_helpers.rb

@@ -1061,6 +1061,8 @@ into similar problems in the future (e.g. when new tables are created).
       # batch_size - The maximum number of rows per job
       # other_arguments - Other arguments to send to the job
       #
+      # *Returns the final migration delay*
+      #
       # Example:
       #
       #     class Route < ActiveRecord::Base
@@ -1077,7 +1079,7 @@ into similar problems in the future (e.g. when new tables are created).
       #         # do something
       #       end
       #     end
-      def queue_background_migration_jobs_by_range_at_intervals(model_class, job_class_name, delay_interval, batch_size: BACKGROUND_MIGRATION_BATCH_SIZE, other_arguments: [])
+      def queue_background_migration_jobs_by_range_at_intervals(model_class, job_class_name, delay_interval, batch_size: BACKGROUND_MIGRATION_BATCH_SIZE, other_job_arguments: [], initial_delay: 0)
         raise "#{model_class} does not have an ID to use for batch ranges" unless model_class.column_names.include?('id')
 
         # To not overload the worker too much we enforce a minimum interval both
@@ -1086,14 +1088,19 @@ into similar problems in the future (e.g. when new tables are created).
           delay_interval = BackgroundMigrationWorker.minimum_interval
         end
 
+        final_delay = nil
+
         model_class.each_batch(of: batch_size) do |relation, index|
           start_id, end_id = relation.pluck(Arel.sql('MIN(id), MAX(id)')).first
 
           # `BackgroundMigrationWorker.bulk_perform_in` schedules all jobs for
           # the same time, which is not helpful in most cases where we wish to
           # spread the work over time.
-          migrate_in(delay_interval * index, job_class_name, [start_id, end_id] + other_arguments)
+          final_delay = initial_delay + delay_interval * index
+          migrate_in(final_delay, job_class_name, [start_id, end_id] + other_job_arguments)
         end
+
+        final_delay
       end
 
       # Fetches indexes on a column by name for postgres.

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -1359,6 +1359,14 @@ describe Gitlab::Database::MigrationHelpers do
         end
       end
 
+      it 'returns the final expected delay' do
+        Sidekiq::Testing.fake! do
+          final_delay = model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.minutes, batch_size: 2)
+
+          expect(final_delay.to_f).to eq(20.minutes.to_f)
+        end
+      end
+
       context 'with batch_size option' do
         it 'queues jobs correctly' do
           Sidekiq::Testing.fake! do
@@ -1383,9 +1391,10 @@ describe Gitlab::Database::MigrationHelpers do
         end
       end
 
-      context 'with other_arguments option' do
+      context 'with other_job_arguments option' do
         it 'queues jobs correctly' do
-          model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.minutes, other_arguments: [1, 2])
+          Sidekiq::Testing.fake! do
+            model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.minutes, other_job_arguments: [1, 2])
 
             expect(BackgroundMigrationWorker.jobs[0]['args']).to eq(['FooJob', [id1, id3, 1, 2]])
             expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(10.minutes.from_now.to_f)
@@ -1393,6 +1402,18 @@ describe Gitlab::Database::MigrationHelpers do
         end
       end
 
+      context 'with initial_delay option' do
+        it 'queues jobs correctly' do
+          Sidekiq::Testing.fake! do
+            model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.minutes, other_job_arguments: [1, 2], initial_delay: 10.minutes)
+
+            expect(BackgroundMigrationWorker.jobs[0]['args']).to eq(['FooJob', [id1, id3, 1, 2]])
+            expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(20.minutes.from_now.to_f)
+          end
+        end
+      end
+    end
+
     context "when the model doesn't have an ID column" do
       it 'raises error (for now)' do
         expect do

spec/migrations/cleanup_optimistic_locking_nulls_pt2_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200217210353_cleanup_optimistic_locking_nulls_pt2')
+
+describe CleanupOptimisticLockingNullsPt2, :migration do
+  TABLES = %w(ci_stages ci_builds ci_pipelines).freeze
+  TABLES.each do |table|
+    let(table.to_sym) { table(table.to_sym) }
+  end
+  let(:tables) { TABLES.map { |t| method(t.to_sym).call } }
+
+  before do
+    # Create necessary rows
+    ci_stages.create!
+    ci_builds.create!
+    ci_pipelines.create!
+
+    # Nullify `lock_version` column for all rows
+    # Needs to be done with a SQL fragment, otherwise Rails will coerce it to 0
+    tables.each do |table|
+      table.update_all('lock_version = NULL')
+    end
+  end
+
+  it 'correctly migrates nullified lock_version column', :sidekiq_might_not_need_inline do
+    tables.each do |table|
+      expect(table.where(lock_version: nil).count).to eq(1)
+    end
+
+    tables.each do |table|
+      expect(table.where(lock_version: 0).count).to eq(0)
+    end
+
+    migrate!
+
+    tables.each do |table|
+      expect(table.where(lock_version: nil).count).to eq(0)
+    end
+
+    tables.each do |table|
+      expect(table.where(lock_version: 0).count).to eq(1)
+    end
+  end
+end