Merge branch 'runner-features' into 'master'

Add `runner_unsupported` CI failure

See merge request gitlab-org/gitlab-ce!20664

11 files changed, 249 insertions, 41 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 35b20bc1e0b..93bbee49c09 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -8,8 +8,6 @@ module Ci
     include Importable
     include Gitlab::Utils::StrongMemoize
 
-    MissingDependenciesError = Class.new(StandardError)
-
     belongs_to :project, inverse_of: :builds
     belongs_to :runner
     belongs_to :trigger_request
@@ -17,6 +15,10 @@ module Ci
 
     has_many :deployments, as: :deployable
 
+    RUNNER_FEATURES = {
+      upload_multiple_artifacts: -> (build) { build.publishes_artifacts_reports? }
+    }.freeze
+
     has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment'
     has_many :trace_sections, class_name: 'Ci::BuildTraceSection'
     has_many :trace_chunks, class_name: 'Ci::BuildTraceChunk', foreign_key: :build_id
@@ -174,10 +176,6 @@ module Ci
         end
       end
 
-      before_transition any => [:running] do |build|
-        build.validates_dependencies! unless Feature.enabled?('ci_disable_validates_dependencies')
-      end
-
       after_transition pending: :running do |build|
         build.ensure_metadata.update_timeout_state
       end
@@ -581,10 +579,10 @@ module Ci
       options[:dependencies]&.empty?
     end
 
-    def validates_dependencies!
-      dependencies.each do |dependency|
-        raise MissingDependenciesError unless dependency.valid_dependency?
-      end
+    def has_valid_build_dependencies?
+      return true if Feature.enabled?('ci_disable_validates_dependencies')
+
+      dependencies.all?(&:valid_dependency?)
     end
 
     def valid_dependency?
@@ -594,6 +592,24 @@ module Ci
       true
     end
 
+    def runner_required_feature_names
+      strong_memoize(:runner_required_feature_names) do
+        RUNNER_FEATURES.select do |feature, method|
+          method.call(self)
+        end.keys
+      end
+    end
+
+    def supported_runner?(features)
+      runner_required_feature_names.all? do |feature_name|
+        features&.dig(feature_name)
+      end
+    end
+
+    def publishes_artifacts_reports?
+      options&.dig(:artifacts, :reports)&.any?
+    end
+
     def hide_secrets(trace)
       return unless trace
 
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 97516079b66..8b1093655b7 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -46,7 +46,8 @@ class CommitStatus < ActiveRecord::Base
     api_failure: 2,
     stuck_or_timeout_failure: 3,
     runner_system_failure: 4,
-    missing_dependency_failure: 5
+    missing_dependency_failure: 5,
+    runner_unsupported: 6
   }
 
   ##
diff --git a/app/presenters/commit_status_presenter.rb b/app/presenters/commit_status_presenter.rb
index 3a9088cfcb8..a08f34e2335 100644
--- a/app/presenters/commit_status_presenter.rb
+++ b/app/presenters/commit_status_presenter.rb
@@ -2,17 +2,19 @@
 
 class CommitStatusPresenter < Gitlab::View::Presenter::Delegated
   CALLOUT_FAILURE_MESSAGES = {
-      unknown_failure: 'There is an unknown failure, please try again',
-      api_failure: 'There has been an API failure, please try again',
-      stuck_or_timeout_failure: 'There has been a timeout failure or the job got stuck. Check your timeout limits or try again',
-      runner_system_failure: 'There has been a runner system failure, please try again',
-      missing_dependency_failure: 'There has been a missing dependency failure'
+    unknown_failure: 'There is an unknown failure, please try again',
+    script_failure: nil,
+    api_failure: 'There has been an API failure, please try again',
+    stuck_or_timeout_failure: 'There has been a timeout failure or the job got stuck. Check your timeout limits or try again',
+    runner_system_failure: 'There has been a runner system failure, please try again',
+    missing_dependency_failure: 'There has been a missing dependency failure',
+    runner_unsupported: 'Your runner is outdated, please upgrade your runner'
   }.freeze
 
   presents :build
 
   def callout_failure_message
-    CALLOUT_FAILURE_MESSAGES[failure_reason.to_sym]
+    CALLOUT_FAILURE_MESSAGES.fetch(failure_reason.to_sym)
   end
 
   def recoverable?
diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb
index f7ccec3a700..11f85627faf 100644
--- a/app/services/ci/register_job_service.rb
+++ b/app/services/ci/register_job_service.rb
@@ -41,16 +41,10 @@ module Ci
         begin
           # In case when 2 runners try to assign the same build, second runner will be declined
           # with StateMachines::InvalidTransition or StaleObjectError when doing run! or save method.
-          begin
-            build.runner_id = runner.id
-            build.runner_session_attributes = params[:session] if params[:session].present?
-
-            build.run!
+          if assign_runner!(build, params)
             register_success(build)
 
             return Result.new(build, true) # rubocop:disable Cop/AvoidReturnFromBlocks
-          rescue Ci::Build::MissingDependenciesError
-            build.drop!(:missing_dependency_failure)
           end
         rescue StateMachines::InvalidTransition, ActiveRecord::StaleObjectError
           # We are looping to find another build that is not conflicting
@@ -72,6 +66,24 @@ module Ci
 
     private
 
+    def assign_runner!(build, params)
+      build.runner_id = runner.id
+      build.runner_session_attributes = params[:session] if params[:session].present?
+
+      unless build.has_valid_build_dependencies?
+        build.drop!(:missing_dependency_failure)
+        return false
+      end
+
+      unless build.supported_runner?(params.dig(:info, :features))
+        build.drop!(:runner_unsupported)
+        return false
+      end
+
+      build.run!
+      true
+    end
+
     def builds_for_shared_runner
       new_builds.
         # don't run projects which have not enabled shared runners and builds
diff --git a/changelogs/unreleased/runner-features.yml b/changelogs/unreleased/runner-features.yml
new file mode 100644
index 00000000000..c5e0fff5a18
--- /dev/null
+++ b/changelogs/unreleased/runner-features.yml
@@ -0,0 +1,5 @@
+---
+title: Verify runner feature set
+merge_request: 20664
+author:
+type: added
diff --git a/lib/api/runner.rb b/lib/api/runner.rb
index c7595493e11..c9931c2d603 100644
--- a/lib/api/runner.rb
+++ b/lib/api/runner.rb
@@ -80,7 +80,15 @@ module API
       params do
         requires :token, type: String, desc: %q(Runner's authentication token)
         optional :last_update, type: String, desc: %q(Runner's queue last_update token)
-        optional :info, type: Hash, desc: %q(Runner's metadata)
+        optional :info, type: Hash, desc: %q(Runner's metadata) do
+          optional :name, type: String, desc: %q(Runner's name)
+          optional :version, type: String, desc: %q(Runner's version)
+          optional :revision, type: String, desc: %q(Runner's revision)
+          optional :platform, type: String, desc: %q(Runner's platform)
+          optional :architecture, type: String, desc: %q(Runner's architecture)
+          optional :executor, type: String, desc: %q(Runner's executor)
+          optional :features, type: Hash, desc: %q(Runner's features)
+        end
         optional :session, type: Hash, desc: %q(Runner's session data) do
           optional :url, type: String, desc: %q(Session's url)
           optional :certificate, type: String, desc: %q(Session's certificate)
diff --git a/lib/gitlab/ci/status/build/failed.rb b/lib/gitlab/ci/status/build/failed.rb
index 155f4fc1343..703f0b9217b 100644
--- a/lib/gitlab/ci/status/build/failed.rb
+++ b/lib/gitlab/ci/status/build/failed.rb
@@ -4,12 +4,13 @@ module Gitlab
       module Build
         class Failed < Status::Extended
           REASONS = {
-            'unknown_failure' => 'unknown failure',
-            'script_failure' => 'script failure',
-            'api_failure' => 'API failure',
-            'stuck_or_timeout_failure' => 'stuck or timeout failure',
-            'runner_system_failure' => 'runner system failure',
-            'missing_dependency_failure' => 'missing dependency failure'
+            unknown_failure: 'unknown failure',
+            script_failure: 'script failure',
+            api_failure: 'API failure',
+            stuck_or_timeout_failure: 'stuck or timeout failure',
+            runner_system_failure: 'runner system failure',
+            missing_dependency_failure: 'missing dependency failure',
+            runner_unsupported: 'unsupported runner'
           }.freeze
 
           def status_tooltip
@@ -31,7 +32,11 @@ module Gitlab
           end
 
           def description
-            "<br> (#{REASONS[subject.failure_reason]})"
+            "<br> (#{failure_reason_message})"
+          end
+
+          def failure_reason_message
+            REASONS.fetch(subject.failure_reason.to_sym)
           end
         end
       end
diff --git a/spec/lib/gitlab/ci/status/build/failed_spec.rb b/spec/lib/gitlab/ci/status/build/failed_spec.rb
index cadb424ea2c..b6676b40fd3 100644
--- a/spec/lib/gitlab/ci/status/build/failed_spec.rb
+++ b/spec/lib/gitlab/ci/status/build/failed_spec.rb
@@ -80,4 +80,31 @@ describe Gitlab::Ci::Status::Build::Failed do
       end
     end
   end
+
+  describe 'covers all failure reasons' do
+    let(:status) { Gitlab::Ci::Status::Failed.new(build, user) }
+    let(:tooltip) { subject.status_tooltip }
+
+    CommitStatus.failure_reasons.keys.each do |failure_reason|
+      context failure_reason do
+        before do
+          build.failure_reason = failure_reason
+        end
+
+        it "is a valid status" do
+          expect { tooltip }.not_to raise_error
+        end
+      end
+    end
+
+    context 'invalid failure message' do
+      before do
+        expect(build).to receive(:failure_reason) { 'invalid failure message' }
+      end
+
+      it "is an invalid status" do
+        expect { tooltip }.to raise_error(/key not found:/)
+      end
+    end
+  end
 end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index e4fa04baae6..6955f7f4cd8 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -2409,18 +2409,18 @@ describe Ci::Build do
     end
   end
 
-  describe 'state transition: any => [:running]' do
+  describe '#has_valid_build_dependencies?' do
     shared_examples 'validation is active' do
       context 'when depended job has not been completed yet' do
         let!(:pre_stage_job) { create(:ci_build, :manual, pipeline: pipeline, name: 'test', stage_idx: 0) }
 
-        it { expect { job.run! }.not_to raise_error }
+        it { expect(job).to have_valid_build_dependencies }
       end
 
       context 'when artifacts of depended job has been expired' do
         let!(:pre_stage_job) { create(:ci_build, :success, :expired, pipeline: pipeline, name: 'test', stage_idx: 0) }
 
-        it { expect { job.run! }.to raise_error(Ci::Build::MissingDependenciesError) }
+        it { expect(job).not_to have_valid_build_dependencies }
       end
 
       context 'when artifacts of depended job has been erased' do
@@ -2430,7 +2430,7 @@ describe Ci::Build do
           pre_stage_job.erase
         end
 
-        it { expect { job.run! }.to raise_error(Ci::Build::MissingDependenciesError) }
+        it { expect(job).not_to have_valid_build_dependencies }
       end
     end
 
@@ -2438,12 +2438,13 @@ describe Ci::Build do
       context 'when depended job has not been completed yet' do
         let!(:pre_stage_job) { create(:ci_build, :manual, pipeline: pipeline, name: 'test', stage_idx: 0) }
 
-        it { expect { job.run! }.not_to raise_error }
+        it { expect(job).to have_valid_build_dependencies }
       end
+
       context 'when artifacts of depended job has been expired' do
         let!(:pre_stage_job) { create(:ci_build, :success, :expired, pipeline: pipeline, name: 'test', stage_idx: 0) }
 
-        it { expect { job.run! }.not_to raise_error }
+        it { expect(job).to have_valid_build_dependencies }
       end
 
       context 'when artifacts of depended job has been erased' do
@@ -2453,7 +2454,7 @@ describe Ci::Build do
           pre_stage_job.erase
         end
 
-        it { expect { job.run! }.not_to raise_error }
+        it { expect(job).to have_valid_build_dependencies }
       end
     end
 
@@ -2469,13 +2470,13 @@ describe Ci::Build do
       context 'when "dependencies" keyword is not defined' do
         let(:options) { {} }
 
-        it { expect { job.run! }.not_to raise_error }
+        it { expect(job).to have_valid_build_dependencies }
       end
 
       context 'when "dependencies" keyword is empty' do
         let(:options) { { dependencies: [] } }
 
-        it { expect { job.run! }.not_to raise_error }
+        it { expect(job).to have_valid_build_dependencies }
       end
 
       context 'when "dependencies" keyword is specified' do
@@ -2812,4 +2813,76 @@ describe Ci::Build do
       end
     end
   end
+
+  describe '#publishes_artifacts_reports?' do
+    let(:build) { create(:ci_build, options: options) }
+
+    subject { build.publishes_artifacts_reports? }
+
+    context 'when artifacts reports are defined' do
+      let(:options) do
+        { artifacts: { reports: { junit: "junit.xml" } } }
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when artifacts reports missing defined' do
+      let(:options) do
+        { artifacts: { paths: ["file.txt"] } }
+      end
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when options are missing' do
+      let(:options) { nil }
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
+  describe '#runner_required_feature_names' do
+    let(:build) { create(:ci_build, options: options) }
+
+    subject { build.runner_required_feature_names }
+
+    context 'when artifacts reports are defined' do
+      let(:options) do
+        { artifacts: { reports: { junit: "junit.xml" } } }
+      end
+
+      it { is_expected.to include(:upload_multiple_artifacts) }
+    end
+  end
+
+  describe '#supported_runner?' do
+    set(:build) { create(:ci_build) }
+
+    subject { build.supported_runner?(runner_features) }
+
+    context 'when feature is required by build' do
+      before do
+        expect(build).to receive(:runner_required_feature_names) do
+          [:upload_multiple_artifacts]
+        end
+      end
+
+      context 'when runner provides given feature' do
+        let(:runner_features) do
+          { upload_multiple_artifacts: true }
+        end
+
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when runner does not provide given feature' do
+        let(:runner_features) do
+          {}
+        end
+
+        it { is_expected.to be_falsey }
+      end
+    end
+  end
 end
diff --git a/spec/presenters/commit_status_presenter_spec.rb b/spec/presenters/commit_status_presenter_spec.rb
index f81ee44e371..2b7742ddbb8 100644
--- a/spec/presenters/commit_status_presenter_spec.rb
+++ b/spec/presenters/commit_status_presenter_spec.rb
@@ -12,4 +12,30 @@ describe CommitStatusPresenter do
   it 'inherits from Gitlab::View::Presenter::Delegated' do
     expect(described_class.superclass).to eq(Gitlab::View::Presenter::Delegated)
   end
+
+  describe 'covers all failure reasons' do
+    let(:message) { presenter.callout_failure_message }
+
+    CommitStatus.failure_reasons.keys.each do |failure_reason|
+      context failure_reason do
+        before do
+          build.failure_reason = failure_reason
+        end
+
+        it "is a valid status" do
+          expect { message }.not_to raise_error
+        end
+      end
+    end
+
+    context 'invalid failure message' do
+      before do
+        expect(build).to receive(:failure_reason) { 'invalid failure message' }
+      end
+
+      it "is an invalid status" do
+        expect { message }.to raise_error(/key not found:/)
+      end
+    end
+  end
 end
diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb
index dbb5e33bbdc..a6565709641 100644
--- a/spec/services/ci/register_job_service_spec.rb
+++ b/spec/services/ci/register_job_service_spec.rb
@@ -351,6 +351,38 @@ module Ci
         end
       end
 
+      context 'runner feature set is verified' do
+        let!(:pending_job) { create(:ci_build, :pending, pipeline: pipeline) }
+
+        before do
+          expect_any_instance_of(Ci::Build).to receive(:runner_required_feature_names) do
+            [:runner_required_feature]
+          end
+        end
+
+        subject { execute(specific_runner, params) }
+
+        context 'when feature is missing by runner' do
+          let(:params) { {} }
+
+          it 'does not pick the build and drops the build' do
+            expect(subject).to be_nil
+            expect(pending_job.reload).to be_failed
+            expect(pending_job).to be_runner_unsupported
+          end
+        end
+
+        context 'when feature is supported by runner' do
+          let(:params) do
+            { info: { features: { runner_required_feature: true } } }
+          end
+
+          it 'does pick job' do
+            expect(subject).not_to be_nil
+          end
+        end
+      end
+
       context 'when "dependencies" keyword is specified' do
         shared_examples 'not pick' do
           it 'does not pick the build and drops the build' do
@@ -403,6 +435,7 @@ module Ci
 
             it { expect(subject).to eq(pending_job) }
           end
+
           context 'when artifacts of depended job has been expired' do
             let!(:pre_stage_job) { create(:ci_build, :success, :expired, pipeline: pipeline, name: 'test', stage_idx: 0) }