Merge branch 'ruby-composite-status' into 'master'

Calculate Composite Status in Ruby instead of using SQL

See merge request gitlab-org/gitlab!16808

app/models/ci/group.rb

@@ -9,6 +9,7 @@ module Ci
   #
   class Group
     include StaticModel
+    include Gitlab::Utils::StrongMemoize
 
     attr_reader :stage, :name, :jobs
 
@@ -21,7 +22,17 @@ module Ci
     end
 
     def status
-      @status ||= commit_statuses.status
+      strong_memoize(:status) do
+        if Feature.enabled?(:ci_composite_status, default_enabled: false)
+          Gitlab::Ci::Status::Composite
+            .new(@jobs)
+            .status
+        else
+          CommitStatus
+            .where(id: @jobs)
+            .legacy_status
+        end
+      end
     end
 
     def detailed_status(current_user)
@@ -40,11 +51,5 @@ module Ci
           self.new(stage, name: group_name, jobs: grouped_statuses)
         end
     end
-
-    private
-
-    def commit_statuses
-      @commit_statuses ||= CommitStatus.where(id: jobs.map(&:id))
-    end
   end
 end

app/models/ci/legacy_stage.rb

@@ -14,7 +14,8 @@ module Ci
       @pipeline = pipeline
       @name = name
       @status = status
-      @warnings = warnings
+      # support ints and booleans
+      @has_warnings = ActiveRecord::Type::Boolean.new.cast(warnings)
     end
 
     def groups
@@ -30,7 +31,7 @@ module Ci
     end
 
     def status
-      @status ||= statuses.latest.status
+      @status ||= statuses.latest.slow_composite_status
     end
 
     def detailed_status(current_user)
@@ -52,11 +53,12 @@ module Ci
     end
 
     def has_warnings?
-      if @warnings.is_a?(Integer)
-        @warnings > 0
-      else
-        statuses.latest.failed_but_allowed.any?
+      # lazilly calculate the warnings
+      if @has_warnings.nil?
+        @has_warnings = statuses.latest.failed_but_allowed.any?
       end
+
+      @has_warnings
     end
 
     def manual_playable?

app/models/ci/pipeline.rb

@@ -386,13 +386,12 @@ module Ci
       end
     end
 
-    def legacy_stages
+    def legacy_stages_using_sql
       # TODO, this needs refactoring, see gitlab-foss#26481.
-
       stages_query = statuses
         .group('stage').select(:stage).order('max(stage_idx)')
 
-      status_sql = statuses.latest.where('stage=sg.stage').status_sql
+      status_sql = statuses.latest.where('stage=sg.stage').legacy_status_sql
 
       warnings_sql = statuses.latest.select('COUNT(*)')
         .where('stage=sg.stage').failed_but_allowed.to_sql
@@ -405,6 +404,30 @@ module Ci
       end
     end
 
+    def legacy_stages_using_composite_status
+      stages = statuses.latest
+        .order(:stage_idx, :stage)
+        .group_by(&:stage)
+
+      stages.map do |stage_name, jobs|
+        composite_status = Gitlab::Ci::Status::Composite
+          .new(jobs)
+
+        Ci::LegacyStage.new(self,
+          name: stage_name,
+          status: composite_status.status,
+          warnings: composite_status.warnings?)
+      end
+    end
+
+    def legacy_stages
+      if Feature.enabled?(:ci_composite_status, default_enabled: false)
+        legacy_stages_using_composite_status
+      else
+        legacy_stages_using_sql
+      end
+    end
+
     def valid_commit_sha
       if self.sha == Gitlab::Git::BLANK_SHA
         self.errors.add(:sha, " cant be 00000000 (branch removal)")
@@ -635,7 +658,8 @@ module Ci
 
     def update_status
       retry_optimistic_lock(self) do
-        case latest_builds_status.to_s
+        new_status = latest_builds_status.to_s
+        case new_status
         when 'created' then nil
         when 'preparing' then prepare
         when 'pending' then enqueue
@@ -648,7 +672,7 @@ module Ci
         when 'scheduled' then delay
         else
           raise HasStatus::UnknownStatusError,
-                "Unknown status `#{latest_builds_status}`"
+                "Unknown status `#{new_status}`"
         end
       end
     end
@@ -907,7 +931,7 @@ module Ci
     def latest_builds_status
       return 'failed' unless yaml_errors.blank?
 
-      statuses.latest.status || 'skipped'
+      statuses.latest.slow_composite_status || 'skipped'
     end
 
     def keep_around_commits

app/models/ci/stage.rb

@@ -78,7 +78,8 @@ module Ci
 
     def update_status
       retry_optimistic_lock(self) do
-        case statuses.latest.status
+        new_status = latest_stage_status.to_s
+        case new_status
         when 'created' then nil
         when 'preparing' then prepare
         when 'pending' then enqueue
@@ -91,7 +92,7 @@ module Ci
         when 'skipped', nil then skip
         else
           raise HasStatus::UnknownStatusError,
-                "Unknown status `#{statuses.latest.status}`"
+                "Unknown status `#{new_status}`"
         end
       end
     end
@@ -124,5 +125,9 @@ module Ci
     def manual_playable?
       blocked? || skipped?
     end
+
+    def latest_stage_status
+      statuses.latest.slow_composite_status || 'skipped'
+    end
   end
 end

app/models/commit_status.rb

@@ -48,6 +48,10 @@ class CommitStatus < ApplicationRecord
   scope :processables, -> { where(type: %w[Ci::Build Ci::Bridge]) }
   scope :for_ids, -> (ids) { where(id: ids) }
 
+  scope :with_preloads, -> do
+    preload(:project, :user)
+  end
+
   scope :with_needs, -> (names = nil) do
     needs = Ci::BuildNeed.scoped_build.select(1)
     needs = needs.where(name: names) if names
@@ -161,11 +165,11 @@ class CommitStatus < ApplicationRecord
   end
 
   def self.status_for_prior_stages(index)
-    before_stage(index).latest.status || 'success'
+    before_stage(index).latest.slow_composite_status || 'success'
   end
 
   def self.status_for_names(names)
-    where(name: names).latest.status || 'success'
+    where(name: names).latest.slow_composite_status || 'success'
   end
 
   def locking_enabled?

app/models/concerns/has_status.rb

@@ -10,6 +10,8 @@ module HasStatus
   ACTIVE_STATUSES = %w[preparing pending running].freeze
   COMPLETED_STATUSES = %w[success failed canceled skipped].freeze
   ORDERED_STATUSES = %w[failed preparing pending running manual scheduled canceled success skipped created].freeze
+  PASSED_WITH_WARNINGS_STATUSES = %w[failed canceled].to_set.freeze
+  EXCLUDE_IGNORED_STATUSES = %w[manual failed canceled].to_set.freeze
   STATUSES_ENUM = { created: 0, pending: 1, running: 2, success: 3,
                     failed: 4, canceled: 5, skipped: 6, manual: 7,
                     scheduled: 8, preparing: 9 }.freeze
@@ -17,7 +19,7 @@ module HasStatus
   UnknownStatusError = Class.new(StandardError)
 
   class_methods do
-    def status_sql
+    def legacy_status_sql
       scope_relevant = respond_to?(:exclude_ignored) ? exclude_ignored : all
       scope_warnings = respond_to?(:failed_but_allowed) ? failed_but_allowed : none
 
@@ -53,8 +55,22 @@ module HasStatus
       )
     end
 
-    def status
-      all.pluck(status_sql).first
+    def legacy_status
+      all.pluck(legacy_status_sql).first
+    end
+
+    # This method should not be used.
+    # This method performs expensive calculation of status:
+    # 1. By plucking all related objects,
+    # 2. Or executes expensive SQL query
+    def slow_composite_status
+      if Feature.enabled?(:ci_composite_status, default_enabled: false)
+        Gitlab::Ci::Status::Composite
+          .new(all, with_allow_failure: columns_hash.key?('allow_failure'))
+          .status
+      else
+        legacy_status
+      end
     end
 
     def started_at

app/services/ci/process_pipeline_service.rb

@@ -2,6 +2,8 @@
 
 module Ci
   class ProcessPipelineService < BaseService
+    include Gitlab::Utils::StrongMemoize
+
     attr_reader :pipeline
 
     def execute(pipeline, trigger_build_ids = nil)
@@ -33,9 +35,9 @@ module Ci
 
       return unless HasStatus::COMPLETED_STATUSES.include?(current_status)
 
-      created_processables_in_stage_without_needs(index).select do |build|
+      created_processables_in_stage_without_needs(index).find_each.select do |build|
         process_build(build, current_status)
-      end
+      end.any?
     end
 
     def process_builds_with_needs(trigger_build_ids)
@@ -92,6 +94,7 @@ module Ci
 
     def created_processables_in_stage_without_needs(index)
       created_processables_without_needs
+        .with_preloads
         .for_stage(index)
     end
 

lib/gitlab/ci/status/composite.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Status
+      class Composite
+        include Gitlab::Utils::StrongMemoize
+
+        # This class accepts an array of arrays/hashes/or objects
+        def initialize(all_statuses, with_allow_failure: true)
+          unless all_statuses.respond_to?(:pluck)
+            raise ArgumentError, "all_statuses needs to respond to `.pluck`"
+          end
+
+          @status_set = Set.new
+          @status_key = 0
+          @allow_failure_key = 1 if with_allow_failure
+
+          consume_all_statuses(all_statuses)
+        end
+
+        # The status calculation is order dependent,
+        # 1. In some cases we assume that that status is exact
+        #    if the we only have given statues,
+        # 2. In other cases we assume that status is of that type
+        #    based on what statuses are no longer valid based on the
+        #    data set that we have
+        def status
+          return if none?
+
+          strong_memoize(:status) do
+            if only_of?(:skipped, :ignored)
+              'skipped'
+            elsif only_of?(:success, :skipped, :success_with_warnings, :ignored)
+              'success'
+            elsif only_of?(:created, :success_with_warnings, :ignored)
+              'created'
+            elsif only_of?(:preparing, :success_with_warnings, :ignored)
+              'preparing'
+            elsif only_of?(:canceled, :success, :skipped, :success_with_warnings, :ignored)
+              'canceled'
+            elsif only_of?(:pending, :created, :skipped, :success_with_warnings, :ignored)
+              'pending'
+            elsif any_of?(:running, :pending)
+              'running'
+            elsif any_of?(:manual)
+              'manual'
+            elsif any_of?(:scheduled)
+              'scheduled'
+            elsif any_of?(:preparing)
+              'preparing'
+            elsif any_of?(:created)
+              'running'
+            else
+              'failed'
+            end
+          end
+        end
+
+        def warnings?
+          @status_set.include?(:success_with_warnings)
+        end
+
+        private
+
+        def none?
+          @status_set.empty?
+        end
+
+        def any_of?(*names)
+          names.any? { |name| @status_set.include?(name) }
+        end
+
+        def only_of?(*names)
+          matching = names.count { |name| @status_set.include?(name) }
+          matching > 0 &&
+            matching == @status_set.size
+        end
+
+        def consume_all_statuses(all_statuses)
+          columns = []
+          columns[@status_key] = :status
+          columns[@allow_failure_key] = :allow_failure if @allow_failure_key
+
+          all_statuses
+            .pluck(*columns) # rubocop: disable CodeReuse/ActiveRecord
+            .each(&method(:consume_status))
+        end
+
+        def consume_status(description)
+          # convert `"status"` into `["status"]`
+          description = Array(description)
+
+          status =
+            if success_with_warnings?(description)
+              :success_with_warnings
+            elsif ignored_status?(description)
+              :ignored
+            else
+              description[@status_key].to_sym
+            end
+
+          @status_set.add(status)
+        end
+
+        def success_with_warnings?(status)
+          @allow_failure_key &&
+            status[@allow_failure_key] &&
+            HasStatus::PASSED_WITH_WARNINGS_STATUSES.include?(status[@status_key])
+        end
+
+        def ignored_status?(status)
+          @allow_failure_key &&
+            status[@allow_failure_key] &&
+            HasStatus::EXCLUDE_IGNORED_STATUSES.include?(status[@status_key])
+        end
+      end
+    end
+  end
+end

spec/features/projects/jobs/user_browses_job_spec.rb

@@ -104,20 +104,20 @@ describe 'User browses a job', :js do
     it 'displays the failure reason' do
       wait_for_all_requests
       within('.builds-container') do
-        build_link = first('.build-job > a')
-        expect(build_link['data-original-title']).to eq('test - failed - (unknown failure)')
+        expect(page).to have_selector(
+          ".build-job > a[data-original-title='test - failed - (unknown failure)']")
       end
     end
   end
 
   context 'when a failed job has been retried' do
-    let!(:build) { create(:ci_build, :failed, :retried, :trace_artifact, pipeline: pipeline) }
+    let!(:build_retried) { create(:ci_build, :failed, :retried, :trace_artifact, pipeline: pipeline) }
 
     it 'displays the failure reason and retried label' do
       wait_for_all_requests
       within('.builds-container') do
-        build_link = first('.build-job > a')
-        expect(build_link['data-original-title']).to eq('test - failed - (unknown failure) (retried)')
+        expect(page).to have_selector(
+          ".build-job > a[data-original-title='test - failed - (unknown failure) (retried)']")
       end
     end
   end

spec/lib/gitlab/ci/status/composite_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Ci::Status::Composite do
+  set(:pipeline) { create(:ci_pipeline) }
+
+  before(:all) do
+    @statuses = HasStatus::STATUSES_ENUM.map do |status, idx|
+      [status, create(:ci_build, pipeline: pipeline, status: status, importing: true)]
+    end.to_h
+
+    @statuses_with_allow_failure = HasStatus::STATUSES_ENUM.map do |status, idx|
+      [status, create(:ci_build, pipeline: pipeline, status: status, allow_failure: true, importing: true)]
+    end.to_h
+  end
+
+  describe '#status' do
+    shared_examples 'compares composite with SQL status' do
+      it 'returns exactly the same result' do
+        builds = Ci::Build.where(id: all_statuses)
+
+        expect(composite_status.status).to eq(builds.legacy_status)
+        expect(composite_status.warnings?).to eq(builds.failed_but_allowed.any?)
+      end
+    end
+
+    shared_examples 'validate all combinations' do |perms|
+      HasStatus::STATUSES_ENUM.keys.combination(perms).each do |statuses|
+        context "with #{statuses.join(",")}" do
+          it_behaves_like 'compares composite with SQL status' do
+            let(:all_statuses) do
+              statuses.map { |status| @statuses[status] }
+            end
+
+            let(:composite_status) do
+              described_class.new(all_statuses)
+            end
+          end
+
+          HasStatus::STATUSES_ENUM.each do |allow_failure_status, _|
+            context "and allow_failure #{allow_failure_status}" do
+              it_behaves_like 'compares composite with SQL status' do
+                let(:all_statuses) do
+                  statuses.map { |status| @statuses[status] } +
+                    [@statuses_with_allow_failure[allow_failure_status]]
+                end
+
+                let(:composite_status) do
+                  described_class.new(all_statuses)
+                end
+              end
+            end
+          end
+        end
+      end
+    end
+
+    it_behaves_like 'validate all combinations', 0
+    it_behaves_like 'validate all combinations', 1
+    it_behaves_like 'validate all combinations', 2
+  end
+end

spec/models/ci/group_spec.rb

@@ -22,6 +22,32 @@ describe Ci::Group do
     end
   end
 
+  describe '#status' do
+    let(:jobs) do
+      [create(:ci_build, :failed)]
+    end
+
+    context 'when ci_composite_status is enabled' do
+      before do
+        stub_feature_flags(ci_composite_status: true)
+      end
+
+      it 'returns a failed status' do
+        expect(subject.status).to eq('failed')
+      end
+    end
+
+    context 'when ci_composite_status is disabled' do
+      before do
+        stub_feature_flags(ci_composite_status: false)
+      end
+
+      it 'returns a failed status' do
+        expect(subject.status).to eq('failed')
+      end
+    end
+  end
+
   describe '#detailed_status' do
     context 'when there is only one item in the group' do
       it 'calls the status from the object itself' do

spec/models/ci/legacy_stage_spec.rb

@@ -216,7 +216,7 @@ describe Ci::LegacyStage do
     context 'when stage has warnings' do
       context 'when using memoized warnings flag' do
         context 'when there are warnings' do
-          let(:stage) { build(:ci_stage, warnings: 2) }
+          let(:stage) { build(:ci_stage, warnings: true) }
 
           it 'returns true using memoized value' do
             expect(stage).not_to receive(:statuses)
@@ -225,22 +225,13 @@ describe Ci::LegacyStage do
         end
 
         context 'when there are no warnings' do
-          let(:stage) { build(:ci_stage, warnings: 0) }
+          let(:stage) { build(:ci_stage, warnings: false) }
 
           it 'returns false using memoized value' do
             expect(stage).not_to receive(:statuses)
             expect(stage).not_to have_warnings
           end
         end
-
-        context 'when number of warnings is not a valid value' do
-          let(:stage) { build(:ci_stage, warnings: true) }
-
-          it 'calculates statuses using database queries' do
-            expect(stage).to receive(:statuses).and_call_original
-            expect(stage).not_to have_warnings
-          end
-        end
       end
 
       context 'when calculating warnings from statuses' do

spec/models/ci/pipeline_spec.rb

@@ -1136,59 +1136,71 @@ describe Ci::Pipeline, :mailer do
       end
 
       describe '#legacy_stages' do
+        using RSpec::Parameterized::TableSyntax
+
         subject { pipeline.legacy_stages }
 
-        context 'stages list' do
-          it 'returns ordered list of stages' do
-            expect(subject.map(&:name)).to eq(%w[build test deploy])
-          end
+        where(:ci_composite_status) do
+          [false, true]
         end
 
-        context 'stages with statuses' do
-          let(:statuses) do
-            subject.map { |stage| [stage.name, stage.status] }
+        with_them do
+          before do
+            stub_feature_flags(ci_composite_status: ci_composite_status)
           end
 
-          it 'returns list of stages with correct statuses' do
-            expect(statuses).to eq([%w(build failed),
-                                    %w(test success),
-                                    %w(deploy running)])
+          context 'stages list' do
+            it 'returns ordered list of stages' do
+              expect(subject.map(&:name)).to eq(%w[build test deploy])
+            end
           end
 
-          context 'when commit status is retried' do
-            before do
-              create(:commit_status, pipeline: pipeline,
-                                     stage: 'build',
-                                     name: 'mac',
-                                     stage_idx: 0,
-                                     status: 'success')
-
-              pipeline.process!
+          context 'stages with statuses' do
+            let(:statuses) do
+              subject.map { |stage| [stage.name, stage.status] }
             end
 
-            it 'ignores the previous state' do
-              expect(statuses).to eq([%w(build success),
+            it 'returns list of stages with correct statuses' do
+              expect(statuses).to eq([%w(build failed),
                                       %w(test success),
                                       %w(deploy running)])
             end
-          end
-        end
 
-        context 'when there is a stage with warnings' do
-          before do
-            create(:commit_status, pipeline: pipeline,
-                                   stage: 'deploy',
-                                   name: 'prod:2',
-                                   stage_idx: 2,
-                                   status: 'failed',
-                                   allow_failure: true)
+            context 'when commit status is retried' do
+              before do
+                create(:commit_status, pipeline: pipeline,
+                                      stage: 'build',
+                                      name: 'mac',
+                                      stage_idx: 0,
+                                      status: 'success')
+
+                pipeline.process!
+              end
+
+              it 'ignores the previous state' do
+                expect(statuses).to eq([%w(build success),
+                                        %w(test success),
+                                        %w(deploy running)])
+              end
+            end
           end
 
-          it 'populates stage with correct number of warnings' do
-            deploy_stage = pipeline.legacy_stages.third
+          context 'when there is a stage with warnings' do
+            before do
+              create(:commit_status, pipeline: pipeline,
+                                    stage: 'deploy',
+                                    name: 'prod:2',
+                                    stage_idx: 2,
+                                    status: 'failed',
+                                    allow_failure: true)
+            end
 
-            expect(deploy_stage).not_to receive(:statuses)
-            expect(deploy_stage).to have_warnings
+            it 'populates stage with correct number of warnings' do
+              deploy_stage = pipeline.legacy_stages.third
+
+              expect(deploy_stage).not_to receive(:statuses)
+              expect(deploy_stage).to have_warnings
+            end
           end
         end
       end
@@ -2326,36 +2338,38 @@ describe Ci::Pipeline, :mailer do
   describe '#update_status' do
     context 'when pipeline is empty' do
       it 'updates does not change pipeline status' do
-        expect(pipeline.statuses.latest.status).to be_nil
+        expect(pipeline.statuses.latest.slow_composite_status).to be_nil
 
         expect { pipeline.update_status }
-          .to change { pipeline.reload.status }.to 'skipped'
+          .to change { pipeline.reload.status }
+          .from('created')
+          .to('skipped')
       end
     end
 
     context 'when updating status to pending' do
       before do
-        allow(pipeline)
-          .to receive_message_chain(:statuses, :latest, :status)
-          .and_return(:running)
+        create(:ci_build, pipeline: pipeline, status: :running)
       end
 
       it 'updates pipeline status to running' do
         expect { pipeline.update_status }
-          .to change { pipeline.reload.status }.to 'running'
+          .to change { pipeline.reload.status }
+          .from('created')
+          .to('running')
       end
     end
 
     context 'when updating status to scheduled' do
       before do
-        allow(pipeline)
-          .to receive_message_chain(:statuses, :latest, :status)
-          .and_return(:scheduled)
+        create(:ci_build, pipeline: pipeline, status: :scheduled)
       end
 
       it 'updates pipeline status to scheduled' do
         expect { pipeline.update_status }
-          .to change { pipeline.reload.status }.to 'scheduled'
+          .to change { pipeline.reload.status }
+          .from('created')
+          .to('scheduled')
       end
     end
 

spec/models/ci/stage_spec.rb

@@ -130,7 +130,7 @@ describe Ci::Stage, :models do
     context 'when statuses status was not recognized' do
       before do
         allow(stage)
-          .to receive_message_chain(:statuses, :latest, :status)
+          .to receive(:latest_stage_status)
           .and_return(:unknown)
       end
 

spec/models/commit_status_spec.rb

@@ -321,7 +321,7 @@ describe CommitStatus do
       end
 
       it 'returns a correct compound status' do
-        expect(described_class.all.status).to eq 'running'
+        expect(described_class.all.slow_composite_status).to eq 'running'
       end
     end
 
@@ -331,7 +331,7 @@ describe CommitStatus do
       end
 
       it 'returns status that indicates success' do
-        expect(described_class.all.status).to eq 'success'
+        expect(described_class.all.slow_composite_status).to eq 'success'
       end
     end
 
@@ -342,7 +342,7 @@ describe CommitStatus do
       end
 
       it 'returns status according to the scope' do
-        expect(described_class.latest.status).to eq 'success'
+        expect(described_class.latest.slow_composite_status).to eq 'success'
       end
     end
   end

spec/models/concerns/has_status_spec.rb

@@ -3,12 +3,15 @@
 require 'spec_helper'
 
 describe HasStatus do
-  describe '.status' do
-    subject { CommitStatus.status }
+  describe '.slow_composite_status' do
+    using RSpec::Parameterized::TableSyntax
+
+    subject { CommitStatus.slow_composite_status }
 
     shared_examples 'build status summary' do
       context 'all successful' do
         let!(:statuses) { Array.new(2) { create(type, status: :success) } }
+
         it { is_expected.to eq 'success' }
       end
 
@@ -165,16 +168,26 @@ describe HasStatus do
       end
     end
 
-    context 'ci build statuses' do
-      let(:type) { :ci_build }
-
-      it_behaves_like 'build status summary'
+    where(:ci_composite_status) do
+      [false, true]
     end
 
-    context 'generic commit statuses' do
-      let(:type) { :generic_commit_status }
+    with_them do
+      before do
+        stub_feature_flags(ci_composite_status: ci_composite_status)
+      end
+
+      context 'ci build statuses' do
+        let(:type) { :ci_build }
 
-      it_behaves_like 'build status summary'
+        it_behaves_like 'build status summary'
+      end
+
+      context 'generic commit statuses' do
+        let(:type) { :generic_commit_status }
+
+        it_behaves_like 'build status summary'
+      end
     end
   end
 
@@ -372,8 +385,8 @@ describe HasStatus do
     end
   end
 
-  describe '.status_sql' do
-    subject { Ci::Build.status_sql }
+  describe '.legacy_status_sql' do
+    subject { Ci::Build.legacy_status_sql }
 
     it 'returns SQL' do
       puts subject