Add latest changes from gitlab-org/gitlab@master

config/feature_flags/development/ci_pipeline_creation_step_duration_tracking.yml

@@ -0,0 +1,8 @@
+---
+name: ci_pipeline_creation_step_duration_tracking
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68485
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/339486
+milestone: '14.2'
+type: development
+group: group::pipeline execution
+default_enabled: true

db/post_migrate/20210823113259_steal_merge_request_diff_commit_users_migration.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+# See https://docs.gitlab.com/ee/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class StealMergeRequestDiffCommitUsersMigration < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    job = Gitlab::Database::BackgroundMigrationJob
+      .for_migration_class('MigrateMergeRequestDiffCommitUsers')
+      .pending
+      .last
+
+    return unless job
+
+    # We schedule in one hour so we don't end up running the migrations while a
+    # deployment is still wrapping up. Not that that really matters, but it
+    # prevents from too much happening during a deployment window.
+    migrate_in(1.hour, 'StealMigrateMergeRequestDiffCommitUsers', job.arguments)
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema_migrations/20210823113259

@@ -0,0 +1 @@
+06b44a856fc970f52b19ad8eeb38f885182003eff50ef1524ecf30887f4664d9

lib/gitlab/background_migration/migrate_merge_request_diff_commit_users.rb

@@ -78,6 +78,8 @@ module Gitlab
       # rubocop: enable Style/Documentation
 
       def perform(start_id, stop_id)
+        return if already_processed?(start_id, stop_id)
+
         # This Hash maps user names + emails to their corresponding rows in
         # merge_request_diff_commit_users.
         user_mapping = {}
@@ -94,6 +96,13 @@ module Gitlab
         )
       end
 
+      def already_processed?(start_id, stop_id)
+        Database::BackgroundMigrationJob
+          .for_migration_execution('MigrateMergeRequestDiffCommitUsers', [start_id, stop_id])
+          .succeeded
+          .any?
+      end
+
       # Returns the data we'll use to determine what merge_request_diff_commits
       # rows to update, and what data to use for populating their
       # commit_author_id and committer_id columns.

lib/gitlab/background_migration/steal_migrate_merge_request_diff_commit_users.rb

@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # A background migration that finished any pending
+    # MigrateMergeRequestDiffCommitUsers jobs, and schedules new jobs itself.
+    #
+    # This migration exists so we can bypass rescheduling issues (e.g. jobs
+    # getting dropped after too many retries) that may occur when
+    # MigrateMergeRequestDiffCommitUsers jobs take longer than expected.
+    class StealMigrateMergeRequestDiffCommitUsers
+      def perform(start_id, stop_id)
+        MigrateMergeRequestDiffCommitUsers.new.perform(start_id, stop_id)
+        schedule_next_job
+      end
+
+      def schedule_next_job
+        # We process jobs in reverse order, so that (hopefully) we are less
+        # likely to process jobs that the regular background migration job is
+        # also processing.
+        next_job = Database::BackgroundMigrationJob
+          .for_migration_class('MigrateMergeRequestDiffCommitUsers')
+          .pending
+          .last
+
+        return unless next_job
+
+        BackgroundMigrationWorker.perform_in(
+          5.minutes,
+          'StealMigrateMergeRequestDiffCommitUsers',
+          next_job.arguments
+        )
+      end
+    end
+  end
+end

lib/gitlab/ci/pipeline/chain/command.rb

@@ -87,6 +87,13 @@ module Gitlab
             @metrics ||= ::Gitlab::Ci::Pipeline::Metrics
           end
 
+          def observe_step_duration(step_class, duration)
+            if Feature.enabled?(:ci_pipeline_creation_step_duration_tracking, default_enabled: :yaml)
+              metrics.pipeline_creation_step_duration_histogram
+                .observe({ step: step_class.name }, duration.seconds)
+            end
+          end
+
           def observe_creation_duration(duration)
             metrics.pipeline_creation_duration_histogram
               .observe({}, duration.seconds)

lib/gitlab/ci/pipeline/chain/sequence.rb

@@ -14,9 +14,16 @@ module Gitlab
 
           def build!
             @sequence.each do |step_class|
+              step_start = ::Gitlab::Metrics::System.monotonic_time
               step = step_class.new(@pipeline, @command)
 
               step.perform!
+
+              @command.observe_step_duration(
+                step_class,
+                ::Gitlab::Metrics::System.monotonic_time - step_start
+              )
+
               break if step.break?
             end
 

lib/gitlab/ci/pipeline/metrics.rb

@@ -4,6 +4,8 @@ module Gitlab
   module Ci
     module Pipeline
       class Metrics
+        extend Gitlab::Utils::StrongMemoize
+
         def self.pipeline_creation_duration_histogram
           name = :gitlab_ci_pipeline_creation_duration_seconds
           comment = 'Pipeline creation duration'
@@ -13,6 +15,17 @@ module Gitlab
           ::Gitlab::Metrics.histogram(name, comment, labels, buckets)
         end
 
+        def self.pipeline_creation_step_duration_histogram
+          strong_memoize(:pipeline_creation_step_histogram) do
+            name = :gitlab_ci_pipeline_creation_step_duration_seconds
+            comment = 'Duration of each pipeline creation step'
+            labels = { step: nil }
+            buckets = [0.01, 0.05, 0.1, 0.5, 1.0, 2.0, 5.0, 10.0, 15.0, 20.0, 50.0, 240.0]
+
+            ::Gitlab::Metrics.histogram(name, comment, labels, buckets)
+          end
+        end
+
         def self.pipeline_security_orchestration_policy_processing_duration_histogram
           name = :gitlab_ci_pipeline_security_orchestration_policy_processing_duration_seconds
           comment = 'Pipeline security orchestration policy processing duration'

spec/lib/gitlab/background_migration/migrate_merge_request_diff_commit_users_spec.rb

@@ -91,6 +91,18 @@ RSpec.describe Gitlab::BackgroundMigration::MigrateMergeRequestDiffCommitUsers d
   end
 
   describe '#perform' do
+    it 'skips jobs that have already been completed' do
+      Gitlab::Database::BackgroundMigrationJob.create!(
+        class_name: 'MigrateMergeRequestDiffCommitUsers',
+        arguments: [1, 10],
+        status: :succeeded
+      )
+
+      expect(migration).not_to receive(:get_data_to_update)
+
+      migration.perform(1, 10)
+    end
+
     it 'migrates the data in the range' do
       commits.create!(
         merge_request_diff_id: diff.id,

spec/lib/gitlab/background_migration/steal_migrate_merge_request_diff_commit_users_spec.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::StealMigrateMergeRequestDiffCommitUsers do
+  let(:migration) { described_class.new }
+
+  describe '#perform' do
+    it 'processes the background migration' do
+      spy = instance_spy(
+        Gitlab::BackgroundMigration::MigrateMergeRequestDiffCommitUsers
+      )
+
+      allow(Gitlab::BackgroundMigration::MigrateMergeRequestDiffCommitUsers)
+        .to receive(:new)
+        .and_return(spy)
+
+      expect(spy).to receive(:perform).with(1, 4)
+      expect(migration).to receive(:schedule_next_job)
+
+      migration.perform(1, 4)
+    end
+  end
+
+  describe '#schedule_next_job' do
+    it 'schedules the next job in reverse order' do
+      Gitlab::Database::BackgroundMigrationJob.create!(
+        class_name: 'MigrateMergeRequestDiffCommitUsers',
+        arguments: [10, 20]
+      )
+
+      Gitlab::Database::BackgroundMigrationJob.create!(
+        class_name: 'MigrateMergeRequestDiffCommitUsers',
+        arguments: [40, 50]
+      )
+
+      expect(BackgroundMigrationWorker)
+        .to receive(:perform_in)
+        .with(5.minutes, 'StealMigrateMergeRequestDiffCommitUsers', [40, 50])
+
+      migration.schedule_next_job
+    end
+
+    it 'does not schedule any new jobs when there are none' do
+      expect(BackgroundMigrationWorker).not_to receive(:perform_in)
+
+      migration.schedule_next_job
+    end
+  end
+end

spec/lib/gitlab/ci/pipeline/chain/command_spec.rb

@@ -341,4 +341,40 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Command do
       end
     end
   end
+
+  describe '#observe_step_duration' do
+    context 'when ci_pipeline_creation_step_duration_tracking is enabled' do
+      it 'adds the duration to the step duration histogram' do
+        histogram = double(:histogram)
+        duration = 1.hour
+
+        expect(::Gitlab::Ci::Pipeline::Metrics).to receive(:pipeline_creation_step_duration_histogram)
+          .and_return(histogram)
+        expect(histogram).to receive(:observe)
+          .with({ step: 'Gitlab::Ci::Pipeline::Chain::Build' }, duration.seconds)
+
+        described_class.new.observe_step_duration(
+          Gitlab::Ci::Pipeline::Chain::Build,
+          duration
+        )
+      end
+    end
+
+    context 'when ci_pipeline_creation_step_duration_tracking is disabled' do
+      before do
+        stub_feature_flags(ci_pipeline_creation_step_duration_tracking: false)
+      end
+
+      it 'does nothing' do
+        duration = 1.hour
+
+        expect(::Gitlab::Ci::Pipeline::Metrics).not_to receive(:pipeline_creation_step_duration_histogram)
+
+        described_class.new.observe_step_duration(
+          Gitlab::Ci::Pipeline::Chain::Build,
+          duration
+        )
+      end
+    end
+  end
 end

spec/lib/gitlab/ci/pipeline/chain/sequence_spec.rb

@@ -8,8 +8,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Sequence do
 
   let(:pipeline) { build_stubbed(:ci_pipeline) }
   let(:command) { Gitlab::Ci::Pipeline::Chain::Command.new(project: project) }
-  let(:first_step) { spy('first step') }
+  let(:first_step) { spy('first step', name: 'FirstStep') }
-  let(:second_step) { spy('second step') }
+  let(:second_step) { spy('second step', name: 'SecondStep') }
   let(:sequence) { [first_step, second_step] }
   let(:histogram) { spy('prometheus metric') }
 
@@ -61,6 +61,17 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Sequence do
       expect(histogram).to have_received(:observe)
     end
 
+    it 'adds step sequence duration to duration histogram' do
+      expect(command.metrics)
+        .to receive(:pipeline_creation_step_duration_histogram)
+        .twice
+        .and_return(histogram)
+      expect(histogram).to receive(:observe).with({ step: 'FirstStep' }, any_args).ordered
+      expect(histogram).to receive(:observe).with({ step: 'SecondStep' }, any_args).ordered
+
+      subject.build!
+    end
+
     it 'records pipeline size by pipeline source in a histogram' do
       allow(command.metrics)
         .to receive(:pipeline_size_histogram)

spec/lib/gitlab/ci/pipeline/metrics_spec.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Gitlab::Ci::Pipeline::Metrics do
+  describe '.pipeline_creation_step_duration_histogram' do
+    it 'adds the step to the step duration histogram' do
+      described_class.clear_memoization(:pipeline_creation_step_histogram)
+
+      expect(::Gitlab::Metrics).to receive(:histogram)
+        .with(
+          :gitlab_ci_pipeline_creation_step_duration_seconds,
+          'Duration of each pipeline creation step',
+          { step: nil },
+          [0.01, 0.05, 0.1, 0.5, 1.0, 2.0, 5.0, 10.0, 15.0, 20.0, 50.0, 240.0]
+        )
+
+      described_class.pipeline_creation_step_duration_histogram
+    end
+  end
+end

spec/migrations/steal_merge_request_diff_commit_users_migration_spec.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration! 'steal_merge_request_diff_commit_users_migration'
+
+RSpec.describe StealMergeRequestDiffCommitUsersMigration, :migration do
+  let(:migration) { described_class.new }
+
+  describe '#up' do
+    it 'schedules a job if there are pending jobs' do
+      Gitlab::Database::BackgroundMigrationJob.create!(
+        class_name: 'MigrateMergeRequestDiffCommitUsers',
+        arguments: [10, 20]
+      )
+
+      expect(migration)
+        .to receive(:migrate_in)
+        .with(1.hour, 'StealMigrateMergeRequestDiffCommitUsers', [10, 20])
+
+      migration.up
+    end
+
+    it 'does not schedule any jobs when all jobs have been completed' do
+      expect(migration).not_to receive(:migrate_in)
+
+      migration.up
+    end
+  end
+end