Add latest changes from gitlab-org/gitlab@master

app/models/snippet.rb

@@ -102,6 +102,10 @@ class Snippet < ApplicationRecord
     where(project_id: nil)
   end
 
+  def self.only_project_snippets
+    where.not(project_id: nil)
+  end
+
   def self.only_include_projects_visible_to(current_user = nil)
     levels = Gitlab::VisibilityLevel.levels_for_user(current_user)
 

app/services/users/migrate_to_ghost_user_service.rb

@@ -52,6 +52,7 @@ module Users
       migrate_notes
       migrate_abuse_reports
       migrate_award_emoji
+      migrate_snippets
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
@@ -79,6 +80,11 @@ module Users
     def migrate_award_emoji
       user.award_emoji.update_all(user_id: ghost_user.id)
     end
+
+    def migrate_snippets
+      snippets = user.snippets.only_project_snippets
+      snippets.update_all(author_id: ghost_user.id)
+    end
   end
 end
 

changelogs/unreleased/28560_cleanup_optimistic_locking_db_pt2-second-try.yml

+---
+title: Set NULL `lock_version` values to 0 for CI objects
+merge_request: 30305
+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
+
+class CleanupOptimisticLockingNullsPt2 < ActiveRecord::Migration[5.2]
+  def change
+    # no-op: the MR that contained this migration was reverted
+  end
+end

db/post_migrate/20200427064130_cleanup_optimistic_locking_nulls_pt2_fixed.rb

+# frozen_string_literal: true
+
+class CleanupOptimisticLockingNullsPt2Fixed < ActiveRecord::Migration[6.0]
+  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|
+      # cleanup wrong index created in the previous migration, it might be there on staging
+      remove_concurrent_index table.to_sym, :lock_version, where: "lock_version IS NULL"
+
+      add_concurrent_index table.to_sym, :id, where: "lock_version IS NULL", name: "tmp_index_#{table}_lock_version"
+
+      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, :id, where: "lock_version IS NULL", name: "tmp_index_#{table}_lock_version"
+    end
+  end
+end

db/structure.sql

@@ -10723,6 +10723,12 @@ CREATE INDEX tmp_build_stage_position_index ON public.ci_builds USING btree (sta
 
 CREATE INDEX tmp_idx_on_user_id_where_bio_is_filled ON public.users USING btree (id) WHERE ((COALESCE(bio, ''::character varying))::text IS DISTINCT FROM ''::text);
 
+CREATE INDEX tmp_index_ci_builds_lock_version ON public.ci_builds USING btree (id) WHERE (lock_version IS NULL);
+
+CREATE INDEX tmp_index_ci_pipelines_lock_version ON public.ci_pipelines USING btree (id) WHERE (lock_version IS NULL);
+
+CREATE INDEX tmp_index_ci_stages_lock_version ON public.ci_stages USING btree (id) WHERE (lock_version IS NULL);
+
 CREATE UNIQUE INDEX users_security_dashboard_projects_unique_index ON public.users_security_dashboard_projects USING btree (project_id, user_id);
 
 CREATE UNIQUE INDEX vulnerability_feedback_unique_idx ON public.vulnerability_feedback USING btree (project_id, category, feedback_type, project_fingerprint);
@@ -13257,6 +13263,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200214214934
 20200215222507
 20200215225103
+20200217210353
 20200217223651
 20200217225719
 20200218113721
@@ -13484,5 +13491,6 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200423080607
 20200423081409
 20200424050250
+20200427064130
 \.
 

lib/gitlab/database/migration_helpers.rb

@@ -1063,6 +1063,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
@@ -1079,7 +1081,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
@@ -1088,14 +1090,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

@@ -1365,6 +1365,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
@@ -1389,12 +1397,25 @@ 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
+          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)
+          end
+        end
+      end
+
+      context 'with initial_delay 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], initial_delay: 10.minutes)
 
-          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)
+            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

spec/migrations/cleanup_optimistic_locking_nulls_pt2_fixed_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200427064130_cleanup_optimistic_locking_nulls_pt2_fixed.rb')
+
+describe CleanupOptimisticLockingNullsPt2Fixed, :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

spec/services/users/destroy_service_spec.rb

@@ -42,13 +42,11 @@ describe Users::DestroyService do
 
       it 'calls the bulk snippet destroy service for the user personal snippets' do
         repo1 = create(:personal_snippet, :repository, author: user).snippet_repository
-        repo2 = create(:project_snippet, :repository, author: user).snippet_repository
-        repo3 = create(:project_snippet, :repository, project: project, author: user).snippet_repository
+        repo2 = create(:project_snippet, :repository, project: project, author: user).snippet_repository
 
         aggregate_failures do
           expect(gitlab_shell.repository_exists?(repo1.shard_name, repo1.disk_path + '.git')).to be_truthy
           expect(gitlab_shell.repository_exists?(repo2.shard_name, repo2.disk_path + '.git')).to be_truthy
-          expect(gitlab_shell.repository_exists?(repo3.shard_name, repo3.disk_path + '.git')).to be_truthy
         end
 
         # Call made when destroying user personal projects
@@ -59,17 +57,23 @@ describe Users::DestroyService do
         # project snippets where projects are not user personal
         # ones
         expect(Snippets::BulkDestroyService).to receive(:new)
-          .with(admin, user.snippets).and_call_original
+          .with(admin, user.snippets.only_personal_snippets).and_call_original
 
         service.execute(user)
 
         aggregate_failures do
           expect(gitlab_shell.repository_exists?(repo1.shard_name, repo1.disk_path + '.git')).to be_falsey
           expect(gitlab_shell.repository_exists?(repo2.shard_name, repo2.disk_path + '.git')).to be_falsey
-          expect(gitlab_shell.repository_exists?(repo3.shard_name, repo3.disk_path + '.git')).to be_falsey
         end
       end
 
+      it 'does not delete project snippets that the user is the author of' do
+        repo = create(:project_snippet, :repository, author: user).snippet_repository
+        service.execute(user)
+        expect(gitlab_shell.repository_exists?(repo.shard_name, repo.disk_path + '.git')).to be_truthy
+        expect(User.ghost.snippets).to include(repo.snippet)
+      end
+
       context 'when an error is raised deleting snippets' do
         it 'does not delete user' do
           snippet = create(:personal_snippet, :repository, author: user)

spec/services/users/migrate_to_ghost_user_service_spec.rb

@@ -78,6 +78,12 @@ describe Users::MigrateToGhostUserService do
       end
     end
 
+    context 'snippets' do
+      include_examples "migrating a deleted user's associated records to the ghost user", Snippet do
+        let(:created_record) { create(:snippet, project: project, author: user) }
+      end
+    end
+
     context "when record migration fails with a rollback exception" do
       before do
         expect_any_instance_of(ActiveRecord::Associations::CollectionProxy)