Merge branch 'move-resource-group-processable-context' into 'master'

Define resource group at Ci::Processable level

See merge request gitlab-org/gitlab!51880

app/models/ci/build.rb

@@ -20,7 +20,6 @@ module Ci
     belongs_to :runner
     belongs_to :trigger_request
     belongs_to :erased_by, class_name: 'User'
-    belongs_to :resource_group, class_name: 'Ci::ResourceGroup', inverse_of: :builds
     belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :commit_id
 
     RUNNER_FEATURES = {
@@ -38,7 +37,6 @@ module Ci
     DEGRADATION_THRESHOLD_VARIABLE_NAME = 'DEGRADATION_THRESHOLD'
 
     has_one :deployment, as: :deployable, class_name: 'Deployment'
-    has_one :resource, class_name: 'Ci::Resource', inverse_of: :build
     has_one :pending_state, class_name: 'Ci::BuildPendingState', inverse_of: :build
     has_many :trace_sections, class_name: 'Ci::BuildTraceSection'
     has_many :trace_chunks, class_name: 'Ci::BuildTraceChunk', foreign_key: :build_id, inverse_of: :build
@@ -236,21 +234,14 @@ module Ci
 
     state_machine :status do
       event :enqueue do
-        transition [:created, :skipped, :manual, :scheduled] => :waiting_for_resource, if: :requires_resource?
         transition [:created, :skipped, :manual, :scheduled] => :preparing, if: :any_unmet_prerequisites?
       end
 
       event :enqueue_scheduled do
-        transition scheduled: :waiting_for_resource, if: :requires_resource?
         transition scheduled: :preparing, if: :any_unmet_prerequisites?
         transition scheduled: :pending
       end
 
-      event :enqueue_waiting_for_resource do
-        transition waiting_for_resource: :preparing, if: :any_unmet_prerequisites?
-        transition waiting_for_resource: :pending
-      end
-
       event :enqueue_preparing do
         transition preparing: :pending
       end
@@ -279,23 +270,6 @@ module Ci
         build.scheduled_at = build.options_scheduled_at
       end
 
-      before_transition any => :waiting_for_resource do |build|
-        build.waiting_for_resource_at = Time.current
-      end
-
-      before_transition on: :enqueue_waiting_for_resource do |build|
-        next unless build.requires_resource?
-
-        build.resource_group.assign_resource_to(build) # If false is returned, it stops the transition
-      end
-
-      after_transition any => :waiting_for_resource do |build|
-        build.run_after_commit do
-          Ci::ResourceGroups::AssignResourceFromResourceGroupWorker
-            .perform_async(build.resource_group_id)
-        end
-      end
-
       before_transition on: :enqueue_preparing do |build|
         !build.any_unmet_prerequisites? # If false is returned, it stops the transition
       end
@@ -328,16 +302,6 @@ module Ci
         end
       end
 
-      after_transition any => ::Ci::Build.completed_statuses do |build|
-        next unless build.resource_group_id.present?
-        next unless build.resource_group.release_resource_from(build)
-
-        build.run_after_commit do
-          Ci::ResourceGroups::AssignResourceFromResourceGroupWorker
-            .perform_async(build.resource_group_id)
-        end
-      end
-
       after_transition any => [:success, :failed, :canceled] do |build|
         build.run_after_commit do
           build.run_status_commit_hooks!
@@ -467,6 +431,11 @@ module Ci
       pipeline.builds.retried.where(name: self.name).count
     end
 
+    override :all_met_to_become_pending?
+    def all_met_to_become_pending?
+      super && !any_unmet_prerequisites?
+    end
+
     def any_unmet_prerequisites?
       prerequisites.present?
     end
@@ -501,10 +470,6 @@ module Ci
       end
     end
 
-    def requires_resource?
-      self.resource_group_id.present?
-    end
-
     def has_environment?
       environment.present?
     end

app/models/ci/processable.rb

@@ -3,6 +3,11 @@
 module Ci
   class Processable < ::CommitStatus
     include Gitlab::Utils::StrongMemoize
+    extend ::Gitlab::Utils::Override
+
+    has_one :resource, class_name: 'Ci::Resource', foreign_key: 'build_id', inverse_of: :processable
+
+    belongs_to :resource_group, class_name: 'Ci::ResourceGroup', inverse_of: :processables
 
     accepts_nested_attributes_for :needs
 
@@ -20,6 +25,48 @@ module Ci
       where('NOT EXISTS (?)', needs)
     end
 
+    state_machine :status do
+      event :enqueue do
+        transition [:created, :skipped, :manual, :scheduled] => :waiting_for_resource, if: :with_resource_group?
+      end
+
+      event :enqueue_scheduled do
+        transition scheduled: :waiting_for_resource, if: :with_resource_group?
+      end
+
+      event :enqueue_waiting_for_resource do
+        transition waiting_for_resource: :preparing, if: :any_unmet_prerequisites?
+        transition waiting_for_resource: :pending
+      end
+
+      before_transition any => :waiting_for_resource do |processable|
+        processable.waiting_for_resource_at = Time.current
+      end
+
+      before_transition on: :enqueue_waiting_for_resource do |processable|
+        next unless processable.with_resource_group?
+
+        processable.resource_group.assign_resource_to(processable)
+      end
+
+      after_transition any => :waiting_for_resource do |processable|
+        processable.run_after_commit do
+          Ci::ResourceGroups::AssignResourceFromResourceGroupWorker
+            .perform_async(processable.resource_group_id)
+        end
+      end
+
+      after_transition any => ::Ci::Processable.completed_statuses do |processable|
+        next unless processable.with_resource_group?
+        next unless processable.resource_group.release_resource_from(processable)
+
+        processable.run_after_commit do
+          Ci::ResourceGroups::AssignResourceFromResourceGroupWorker
+            .perform_async(processable.resource_group_id)
+        end
+      end
+    end
+
     def self.select_with_aggregated_needs(project)
       aggregated_needs_names = Ci::BuildNeed
         .scoped_build
@@ -77,6 +124,15 @@ module Ci
       raise NotImplementedError
     end
 
+    override :all_met_to_become_pending?
+    def all_met_to_become_pending?
+      super && !with_resource_group?
+    end
+
+    def with_resource_group?
+      self.resource_group_id.present?
+    end
+
     # Overriding scheduling_type enum's method for nil `scheduling_type`s
     def scheduling_type_dag?
       scheduling_type.nil? ? find_legacy_scheduling_type == :dag : super

app/models/ci/resource.rb

@@ -5,9 +5,9 @@ module Ci
     extend Gitlab::Ci::Model
 
     belongs_to :resource_group, class_name: 'Ci::ResourceGroup', inverse_of: :resources
-    belongs_to :build, class_name: 'Ci::Build', inverse_of: :resource
+    belongs_to :processable, class_name: 'Ci::Processable', foreign_key: 'build_id', inverse_of: :resource
 
-    scope :free, -> { where(build: nil) }
-    scope :retained_by, -> (build) { where(build: build) }
+    scope :free, -> { where(processable: nil) }
+    scope :retained_by, -> (processable) { where(processable: processable) }
   end
 end

app/models/ci/resource_group.rb

@@ -7,7 +7,7 @@ module Ci
     belongs_to :project, inverse_of: :resource_groups
 
     has_many :resources, class_name: 'Ci::Resource', inverse_of: :resource_group
-    has_many :builds, class_name: 'Ci::Build', inverse_of: :resource_group
+    has_many :processables, class_name: 'Ci::Processable', inverse_of: :resource_group
 
     validates :key,
       length: { maximum: 255 },
@@ -19,12 +19,12 @@ module Ci
     ##
     # NOTE: This is concurrency-safe method that the subquery in the `UPDATE`
     # works as explicit locking.
-    def assign_resource_to(build)
-      resources.free.limit(1).update_all(build_id: build.id) > 0
+    def assign_resource_to(processable)
+      resources.free.limit(1).update_all(build_id: processable.id) > 0
     end
 
-    def release_resource_from(build)
-      resources.retained_by(build).update_all(build_id: nil) > 0
+    def release_resource_from(processable)
+      resources.retained_by(processable).update_all(build_id: nil) > 0
     end
 
     private

app/models/commit_status.rb

@@ -255,15 +255,7 @@ class CommitStatus < ApplicationRecord
   end
 
   def all_met_to_become_pending?
-    !any_unmet_prerequisites? && !requires_resource?
-  end
-
-  def any_unmet_prerequisites?
-    false
-  end
-
-  def requires_resource?
-    false
+    true
   end
 
   def auto_canceled?

app/services/ci/resource_groups/assign_resource_from_resource_group_service.rb

@@ -7,8 +7,8 @@ module Ci
       def execute(resource_group)
         free_resources = resource_group.resources.free.count
 
-        resource_group.builds.waiting_for_resource.take(free_resources).each do |build|
-          build.enqueue_waiting_for_resource
+        resource_group.processables.waiting_for_resource.take(free_resources).each do |processable|
+          processable.enqueue_waiting_for_resource
         end
       end
       # rubocop: enable CodeReuse/ActiveRecord

lib/gitlab/ci/config/entry/job.rb

@@ -14,7 +14,7 @@ module Gitlab
           ALLOWED_KEYS = %i[tags script type image services start_in artifacts
                             cache dependencies before_script after_script
                             environment coverage retry parallel interruptible timeout
-                            resource_group release secrets].freeze
+                            release secrets].freeze
 
           REQUIRED_BY_NEEDS = %i[stage].freeze
 
@@ -30,7 +30,6 @@ module Gitlab
               }
 
               validates :dependencies, array_of_strings: true
-              validates :resource_group, type: String
               validates :allow_failure, hash_or_boolean: true
             end
 
@@ -124,7 +123,7 @@ module Gitlab
 
           attributes :script, :tags, :when, :dependencies,
                      :needs, :retry, :parallel, :start_in,
-                     :interruptible, :timeout, :resource_group,
+                     :interruptible, :timeout,
                      :release, :allow_failure
 
           def self.matching?(name, config)
@@ -174,7 +173,6 @@ module Gitlab
               ignore: ignored?,
               allow_failure_criteria: allow_failure_criteria,
               needs: needs_defined? ? needs_value : nil,
-              resource_group: resource_group,
               scheduling_type: needs_defined? ? :dag : :stage
             ).compact
           end

lib/gitlab/ci/config/entry/processable.rb

@@ -15,7 +15,7 @@ module Gitlab
           include ::Gitlab::Config::Entry::Inheritable
 
           PROCESSABLE_ALLOWED_KEYS = %i[extends stage only except rules variables
-                                        inherit allow_failure when needs].freeze
+                                        inherit allow_failure when needs resource_group].freeze
 
           included do
             validations do
@@ -32,6 +32,7 @@ module Gitlab
               with_options allow_nil: true do
                 validates :extends, array_of_strings_or_string: true
                 validates :rules, array_of_hashes: true
+                validates :resource_group, type: String
               end
             end
 
@@ -64,7 +65,7 @@ module Gitlab
               inherit: false,
               default: {}
 
-            attributes :extends, :rules
+            attributes :extends, :rules, :resource_group
           end
 
           def compose!(deps = nil)
@@ -125,7 +126,8 @@ module Gitlab
               rules: rules_value,
               variables: root_and_job_variables_value,
               only: only_value,
-              except: except_value }.compact
+              except: except_value,
+              resource_group: resource_group }.compact
           end
 
           def root_and_job_variables_value

lib/gitlab/ci/pipeline/seed/build/resource_group.rb

@@ -8,17 +8,17 @@ module Gitlab
           class ResourceGroup < Seed::Base
             include Gitlab::Utils::StrongMemoize
 
-            attr_reader :build, :resource_group_key
+            attr_reader :processable, :resource_group_key
 
-            def initialize(build, resource_group_key)
-              @build = build
+            def initialize(processable, resource_group_key)
+              @processable = processable
               @resource_group_key = resource_group_key
             end
 
             def to_resource
               return unless resource_group_key.present?
 
-              resource_group = build.project.resource_groups
+              resource_group = processable.project.resource_groups
                 .safe_find_or_create_by(key: expanded_resource_group_key)
 
               resource_group if resource_group.persisted?
@@ -28,7 +28,7 @@ module Gitlab
 
             def expanded_resource_group_key
               strong_memoize(:expanded_resource_group_key) do
-                ExpandVariables.expand(resource_group_key, -> { build.simple_variables })
+                ExpandVariables.expand(resource_group_key, -> { processable.simple_variables })
               end
             end
           end

spec/factories/ci/resource.rb

@@ -5,7 +5,7 @@ FactoryBot.define do
     resource_group factory: :ci_resource_group
 
     trait(:retained) do
-      build factory: :ci_build
+      processable factory: :ci_build
     end
   end
 end

spec/lib/gitlab/ci/config/entry/processable_spec.rb

@@ -73,6 +73,15 @@ RSpec.describe Gitlab::Ci::Config::Entry::Processable do
         end
       end
 
+      context 'when resource_group key is not a string' do
+        let(:config) { { resource_group: 123 } }
+
+        it 'returns error about wrong value type' do
+          expect(entry).not_to be_valid
+          expect(entry.errors).to include "job resource group should be a string"
+        end
+      end
+
       context 'when it uses both "when:" and "rules:"' do
         let(:config) do
           {
@@ -340,6 +349,26 @@ RSpec.describe Gitlab::Ci::Config::Entry::Processable do
       end
     end
 
+    context 'with resource group' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:resource_group, :result) do
+        'iOS'                         | 'iOS'
+        'review/$CI_COMMIT_REF_NAME'  | 'review/$CI_COMMIT_REF_NAME'
+        nil                           | nil
+      end
+
+      with_them do
+        let(:config) { { script: 'ls', resource_group: resource_group }.compact }
+
+        it do
+          entry.compose!(deps)
+
+          expect(entry.resource_group).to eq(result)
+        end
+      end
+    end
+
     context 'with inheritance' do
       context 'of variables' do
         let(:config) do

spec/models/ci/build_spec.rb

@@ -1185,60 +1185,6 @@ RSpec.describe Ci::Build do
     end
   end
 
-  describe 'state transition with resource group' do
-    let(:resource_group) { create(:ci_resource_group, project: project) }
-
-    context 'when build status is created' do
-      let(:build) { create(:ci_build, :created, project: project, resource_group: resource_group) }
-
-      it 'is waiting for resource when build is enqueued' do
-        expect(Ci::ResourceGroups::AssignResourceFromResourceGroupWorker).to receive(:perform_async).with(resource_group.id)
-
-        expect { build.enqueue! }.to change { build.status }.from('created').to('waiting_for_resource')
-
-        expect(build.waiting_for_resource_at).not_to be_nil
-      end
-
-      context 'when build is waiting for resource' do
-        before do
-          build.update_column(:status, 'waiting_for_resource')
-        end
-
-        it 'is enqueued when build requests resource' do
-          expect { build.enqueue_waiting_for_resource! }.to change { build.status }.from('waiting_for_resource').to('pending')
-        end
-
-        it 'releases a resource when build finished' do
-          expect(build.resource_group).to receive(:release_resource_from).with(build).and_call_original
-          expect(Ci::ResourceGroups::AssignResourceFromResourceGroupWorker).to receive(:perform_async).with(build.resource_group_id)
-
-          build.enqueue_waiting_for_resource!
-          build.success!
-        end
-
-        context 'when build has prerequisites' do
-          before do
-            allow(build).to receive(:any_unmet_prerequisites?) { true }
-          end
-
-          it 'is preparing when build is enqueued' do
-            expect { build.enqueue_waiting_for_resource! }.to change { build.status }.from('waiting_for_resource').to('preparing')
-          end
-        end
-
-        context 'when there are no available resources' do
-          before do
-            resource_group.assign_resource_to(create(:ci_build))
-          end
-
-          it 'stays as waiting for resource when build requests resource' do
-            expect { build.enqueue_waiting_for_resource }.not_to change { build.status }
-          end
-        end
-      end
-    end
-  end
-
   describe '#on_stop' do
     subject { build.on_stop }
 

spec/models/ci/pipeline_spec.rb

@@ -2321,7 +2321,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
 
     context 'on waiting for resource' do
       before do
-        allow(build).to receive(:requires_resource?) { true }
+        allow(build).to receive(:with_resource_group?) { true }
         allow(Ci::ResourceGroups::AssignResourceFromResourceGroupWorker).to receive(:perform_async)
 
         build.enqueue

spec/models/ci/processable_spec.rb

@@ -122,4 +122,58 @@ RSpec.describe Ci::Processable do
       it { is_expected.to be_empty }
     end
   end
+
+  describe 'state transition with resource group' do
+    let(:resource_group) { create(:ci_resource_group, project: project) }
+
+    context 'when build status is created' do
+      let(:build) { create(:ci_build, :created, project: project, resource_group: resource_group) }
+
+      it 'is waiting for resource when build is enqueued' do
+        expect(Ci::ResourceGroups::AssignResourceFromResourceGroupWorker).to receive(:perform_async).with(resource_group.id)
+
+        expect { build.enqueue! }.to change { build.status }.from('created').to('waiting_for_resource')
+
+        expect(build.waiting_for_resource_at).not_to be_nil
+      end
+
+      context 'when build is waiting for resource' do
+        before do
+          build.update_column(:status, 'waiting_for_resource')
+        end
+
+        it 'is enqueued when build requests resource' do
+          expect { build.enqueue_waiting_for_resource! }.to change { build.status }.from('waiting_for_resource').to('pending')
+        end
+
+        it 'releases a resource when build finished' do
+          expect(build.resource_group).to receive(:release_resource_from).with(build).and_call_original
+          expect(Ci::ResourceGroups::AssignResourceFromResourceGroupWorker).to receive(:perform_async).with(build.resource_group_id)
+
+          build.enqueue_waiting_for_resource!
+          build.success!
+        end
+
+        context 'when build has prerequisites' do
+          before do
+            allow(build).to receive(:any_unmet_prerequisites?) { true }
+          end
+
+          it 'is preparing when build is enqueued' do
+            expect { build.enqueue_waiting_for_resource! }.to change { build.status }.from('waiting_for_resource').to('preparing')
+          end
+        end
+
+        context 'when there are no available resources' do
+          before do
+            resource_group.assign_resource_to(create(:ci_build))
+          end
+
+          it 'stays as waiting for resource when build requests resource' do
+            expect { build.enqueue_waiting_for_resource }.not_to change { build.status }
+          end
+        end
+      end
+    end
+  end
 end

spec/models/ci/resource_group_spec.rb

@@ -32,12 +32,12 @@ RSpec.describe Ci::ResourceGroup do
     let(:build) { create(:ci_build) }
     let(:resource_group) { create(:ci_resource_group) }
 
-    it 'retains resource for the build' do
-      expect(resource_group.resources.first.build).to be_nil
+    it 'retains resource for the processable' do
+      expect(resource_group.resources.first.processable).to be_nil
 
       is_expected.to eq(true)
 
-      expect(resource_group.resources.first.build).to eq(build)
+      expect(resource_group.resources.first.processable).to eq(build)
     end
 
     context 'when there are no free resources' do
@@ -51,7 +51,7 @@ RSpec.describe Ci::ResourceGroup do
     end
 
     context 'when the build has already retained a resource' do
-      let!(:another_resource) { create(:ci_resource, resource_group: resource_group, build: build) }
+      let!(:another_resource) { create(:ci_resource, resource_group: resource_group, processable: build) }
 
       it 'fails to retain resource' do
         expect { subject }.to raise_error(ActiveRecord::RecordNotUnique)
@@ -71,11 +71,11 @@ RSpec.describe Ci::ResourceGroup do
       end
 
       it 'releases resource from the build' do
-        expect(resource_group.resources.first.build).to eq(build)
+        expect(resource_group.resources.first.processable).to eq(build)
 
         is_expected.to eq(true)
 
-        expect(resource_group.resources.first.build).to be_nil
+        expect(resource_group.resources.first.processable).to be_nil
       end
     end
 

spec/models/ci/resource_spec.rb

@@ -19,7 +19,7 @@ RSpec.describe Ci::Resource do
     subject { described_class.retained_by(build) }
 
     let(:build) { create(:ci_build) }
-    let!(:resource) { create(:ci_resource, build: build) }
+    let!(:resource) { create(:ci_resource, processable: build) }
 
     it 'returns retained resources' do
       is_expected.to eq([resource])

spec/models/commit_status_spec.rb

@@ -725,22 +725,6 @@ RSpec.describe CommitStatus do
     let(:commit_status) { create(:commit_status) }
 
     it { is_expected.to eq(true) }
-
-    context 'when build requires a resource' do
-      before do
-        allow(commit_status).to receive(:requires_resource?) { true }
-      end
-
-      it { is_expected.to eq(false) }
-    end
-
-    context 'when build has a prerequisite' do
-      before do
-        allow(commit_status).to receive(:any_unmet_prerequisites?) { true }
-      end
-
-      it { is_expected.to eq(false) }
-    end
   end
 
   describe '#enqueue' do
@@ -748,7 +732,6 @@ RSpec.describe CommitStatus do
 
     before do
       allow(Time).to receive(:now).and_return(current_time)
-      expect(commit_status.any_unmet_prerequisites?).to eq false
     end
 
     shared_examples 'commit status enqueued' do

spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb

@@ -76,6 +76,31 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do
         }
       end
     end
+
+    context 'with resource group' do
+      let(:config) do
+        <<~YAML
+        instrumentation_test:
+          stage: test
+          resource_group: iOS
+          trigger:
+            include:
+              - local: path/to/child.yml
+        YAML
+      end
+
+      # TODO: This test will be properly implemented in the next MR
+      # for https://gitlab.com/gitlab-org/gitlab/-/issues/39057.
+      it 'creates bridge job but still resource group is no-op', :aggregate_failures do
+        pipeline = create_pipeline!
+
+        test = pipeline.statuses.find_by(name: 'instrumentation_test')
+
+        expect(pipeline).to be_persisted
+        expect(test).to be_a Ci::Bridge
+        expect(project.resource_groups.count).to eq(0)
+      end
+    end
   end
 
   describe 'child pipeline triggers' do

spec/services/ci/create_pipeline_service_spec.rb

@@ -952,9 +952,9 @@ RSpec.describe Ci::CreatePipelineService do
           expect(result).to be_persisted
           expect(deploy_job.resource_group.key).to eq(resource_group_key)
           expect(project.resource_groups.count).to eq(1)
-          expect(resource_group.builds.count).to eq(1)
+          expect(resource_group.processables.count).to eq(1)
           expect(resource_group.resources.count).to eq(1)
-          expect(resource_group.resources.first.build).to eq(nil)
+          expect(resource_group.resources.first.processable).to eq(nil)
         end
 
         context 'when resource group key includes predefined variables' do