Merge branch 'show_pending_warning' into 'master'

Show warning when build is pending and there are not runners

This change is based on: !1530.

This resolves #2967.



See merge request !1573

CHANGELOG

@@ -28,6 +28,7 @@ v 8.1.0 (unreleased)
   - Move CI triggers page to project settings area
   - Move CI project settings page to CE project settings area
   - Fix bug when removed file was not appearing in merge request diff
+  - Show warning when build cannot be served by any of the available CI runners
   - Note the original location of a moved project when notifying users of the move
   - Improve error message when merging fails
   - Add support of multibyte characters in LDAP UID (Roman Petrov)

app/controllers/ci/admin/runners_controller.rb

@@ -6,7 +6,7 @@ module Ci
       @runners = Ci::Runner.order('id DESC')
       @runners = @runners.search(params[:search]) if params[:search].present?
       @runners = @runners.page(params[:page]).per(30)
-      @active_runners_cnt = Ci::Runner.where("contacted_at > ?", 1.minutes.ago).count
+      @active_runners_cnt = Ci::Runner.online.count
     end
 
     def show
@@ -66,7 +66,7 @@ module Ci
     end
 
     def runner_params
-      params.require(:runner).permit(:token, :description, :tag_list, :contacted_at, :active)
+      params.require(:runner).permit(:token, :description, :tag_list, :active)
     end
   end
 end

app/controllers/projects/runners_controller.rb

@@ -60,6 +60,6 @@ class Projects::RunnersController < Projects::ApplicationController
   end
 
   def runner_params
-    params.require(:runner).permit(:description, :tag_list, :contacted_at, :active)
+    params.require(:runner).permit(:description, :tag_list, :active)
   end
 end

app/helpers/runners_helper.rb

@@ -1,20 +1,16 @@
 module RunnersHelper
   def runner_status_icon(runner)
-    unless runner.contacted_at
-      return content_tag :i, nil,
-        class: "fa fa-warning-sign",
-        title: "New runner. Has not connected yet"
+    status = runner.status
+    case status
+    when :not_connected
+      content_tag :i, nil,
+                  class: "fa fa-warning",
+                  title: "New runner. Has not connected yet"
+
+    when :online, :offline, :paused
+      content_tag :i, nil,
+                  class: "fa fa-circle runner-status-#{status}",
+                  title: "Runner is #{status}, last contact was #{time_ago_in_words(runner.contacted_at)} ago"
     end
-
-    status =
-      if runner.active?
-        runner.contacted_at > 3.hour.ago ? :online : :offline
-      else
-        :paused
-      end
-
-    content_tag :i, nil,
-      class: "fa fa-circle runner-status-#{status}",
-      title: "Runner is #{status}, last contact was #{time_ago_in_words(runner.contacted_at)} ago"
   end
 end

app/models/ci/build.rb

@@ -231,6 +231,18 @@ module Ci
       end
     end
 
+    def can_be_served?(runner)
+      (tag_list - runner.tag_list).empty?
+    end
+
+    def any_runners_online?
+      project.any_runners? { |runner| runner.active? && runner.online? && can_be_served?(runner) }
+    end
+
+    def show_warning?
+      pending? && !any_runners_online?
+    end
+
     private
 
     def yaml_variables

app/models/ci/project.rb

@@ -115,12 +115,12 @@ module Ci
       web_url
     end
 
-    def any_runners?
-      if runners.active.any?
+    def any_runners?(&block)
+      if runners.active.any?(&block)
         return true
       end
 
-      shared_runners_enabled && Ci::Runner.shared.active.any?
+      shared_runners_enabled && Ci::Runner.shared.active.any?(&block)
     end
 
     def set_default_values

app/models/ci/runner.rb

@@ -20,6 +20,8 @@
 module Ci
   class Runner < ActiveRecord::Base
     extend Ci::Model
+
+    LAST_CONTACT_TIME = 5.minutes.ago
     
     has_many :builds, class_name: 'Ci::Build'
     has_many :runner_projects, dependent: :destroy, class_name: 'Ci::RunnerProject'
@@ -33,6 +35,7 @@ module Ci
     scope :shared, ->() { where(is_shared: true) }
     scope :active, ->() { where(active: true) }
     scope :paused, ->() { where(active: false) }
+    scope :online, ->() { where('contacted_at > ?', LAST_CONTACT_TIME) }
 
     acts_as_taggable
 
@@ -65,6 +68,20 @@ module Ci
       is_shared
     end
 
+    def online?
+      contacted_at && contacted_at > LAST_CONTACT_TIME
+    end
+
+    def status
+      if contacted_at.nil?
+        :not_connected
+      elsif active?
+        online? ? :online : :offline
+      else
+        :paused
+      end
+    end
+
     def belongs_to_one_project?
       runner_projects.count == 1
     end

app/models/commit_status.rb

@@ -88,4 +88,8 @@ class CommitStatus < ActiveRecord::Base
   def retry_url
     nil
   end
+
+  def show_warning?
+    false
+  end
 end

app/services/ci/register_build_service.rb

@@ -17,7 +17,7 @@ module Ci
       builds = builds.order('created_at ASC')
 
       build = builds.find do |build|
-        (build.tag_list - current_runner.tag_list).empty?
+        build.can_be_served?(current_runner)
       end
 
 

app/views/projects/builds/show.html.haml

@@ -25,7 +25,7 @@
           %a
             Build ##{@build.id}
             ·
-            %i.fa.fa-warning-sign
+            %i.fa.fa-warning
             This build was retried.
 
   .gray-content-block.second-block
@@ -39,6 +39,27 @@
         .pull-right
           = @build.updated_at.stamp('19:00 Aug 27')
 
+  - if @build.show_warning?
+    - unless @build.any_runners_online?
+      .bs-callout.bs-callout-warning
+        %p
+          - if no_runners_for_project?(@build.project)
+            This build is stuck, because the project doesn't have runners assigned.
+          - elsif @build.tags.any?
+            This build is stuck.
+            %br
+            This build is stuck, because you don't have any active runners online with these tags assigned to the project:
+            - @build.tags.each do |tag|
+              %span.label.label-primary
+                = tag
+          - else
+            This build is stuck, because you don't have any active runners online that can run this build.
+
+          %br
+          Go to
+          = link_to namespace_project_runners_path(@build.gl_project.namespace, @build.gl_project) do
+            Runners page
+
   .row.prepend-top-default
     .col-md-9
       .clearfix

app/views/projects/commit_statuses/_commit_status.html.haml

@@ -9,6 +9,9 @@
     - else
       %strong Build ##{commit_status.id}
 
+    - if commit_status.show_warning?
+      %i.fa.fa-warning.text-warning
+
   %td
     = commit_status.ref
 

spec/helpers/runners_helper_spec.rb

@@ -12,7 +12,7 @@ describe RunnersHelper do
   end
 
   it "returns online text" do
-    runner = FactoryGirl.build(:ci_runner, contacted_at: 1.hour.ago, active: true)
+    runner = FactoryGirl.build(:ci_runner, contacted_at: 1.second.ago, active: true)
     expect(runner_status_icon(runner)).to include("Runner is online")
   end
 end

spec/models/build_spec.rb

@@ -299,4 +299,105 @@ describe Ci::Build do
       is_expected.to eq(['rec1', pusher_email])
     end
   end
+
+  describe :can_be_served? do
+    let(:runner) { FactoryGirl.create :ci_specific_runner }
+
+    before { build.project.runners << runner }
+
+    context 'runner without tags' do
+      it 'can handle builds without tags' do
+        expect(build.can_be_served?(runner)).to be_truthy
+      end
+
+      it 'cannot handle build with tags' do
+        build.tag_list = ['aa']
+        expect(build.can_be_served?(runner)).to be_falsey
+      end
+    end
+
+    context 'runner with tags' do
+      before { runner.tag_list = ['bb', 'cc'] }
+
+      it 'can handle builds without tags' do
+        expect(build.can_be_served?(runner)).to be_truthy
+      end
+
+      it 'can handle build with matching tags' do
+        build.tag_list = ['bb']
+        expect(build.can_be_served?(runner)).to be_truthy
+      end
+
+      it 'cannot handle build with not matching tags' do
+        build.tag_list = ['aa']
+        expect(build.can_be_served?(runner)).to be_falsey
+      end
+    end
+  end
+
+  describe :any_runners_online? do
+    subject { build.any_runners_online? }
+
+    context 'when no runners' do
+      it { is_expected.to be_falsey }
+    end
+
+    context 'if there are runner' do
+      let(:runner) { FactoryGirl.create :ci_specific_runner }
+
+      before do
+        build.project.runners << runner
+        runner.update_attributes(contacted_at: 1.second.ago)
+      end
+
+      it { is_expected.to be_truthy }
+
+      it 'that is inactive' do
+        runner.update_attributes(active: false)
+        is_expected.to be_falsey
+      end
+
+      it 'that is not online' do
+        runner.update_attributes(contacted_at: nil)
+        is_expected.to be_falsey
+      end
+
+      it 'that cannot handle build' do
+        expect_any_instance_of(Ci::Build).to receive(:can_be_served?).and_return(false)
+        is_expected.to be_falsey
+      end
+
+    end
+  end
+
+  describe :show_warning? do
+    subject { build.show_warning? }
+
+    %w(pending).each do |state|
+      context "if commit_status.status is #{state}" do
+        before { build.status = state }
+
+        it { is_expected.to be_truthy }
+
+        context "and there are specific runner" do
+          let(:runner) { FactoryGirl.create :ci_specific_runner, contacted_at: 1.second.ago }
+
+          before do
+            build.project.runners << runner
+            runner.save
+          end
+
+          it { is_expected.to be_falsey }
+        end
+      end
+    end
+
+    %w(success failed canceled running).each do |state|
+      context "if commit_status.status is #{state}" do
+        before { build.status = state }
+
+        it { is_expected.to be_falsey }
+      end
+    end
+  end
 end

spec/models/ci/project_spec.rb

@@ -259,5 +259,18 @@ describe Ci::Project do
       FactoryGirl.create(:ci_shared_runner)
       expect(project.any_runners?).to be_falsey
     end
+
+    it "checks the presence of specific runner" do
+      project = FactoryGirl.create(:ci_project)
+      specific_runner = FactoryGirl.create(:ci_specific_runner)
+      project.runners << specific_runner
+      expect(project.any_runners? { |runner| runner == specific_runner }).to be_truthy
+    end
+
+    it "checks the presence of shared runner" do
+      project = FactoryGirl.create(:ci_project, shared_runners_enabled: true)
+      shared_runner = FactoryGirl.create(:ci_shared_runner)
+      expect(project.any_runners? { |runner| runner == shared_runner }).to be_truthy
+    end
   end
 end

spec/models/ci/runner_spec.rb

@@ -48,6 +48,71 @@ describe Ci::Runner do
     it { expect(shared_runner.only_for?(project)).to be_truthy }
   end
 
+  describe :online do
+    subject { Ci::Runner.online }
+
+    before do
+      @runner1 = FactoryGirl.create(:ci_shared_runner, contacted_at: 1.year.ago)
+      @runner2 = FactoryGirl.create(:ci_shared_runner, contacted_at: 1.second.ago)
+    end
+
+    it { is_expected.to eq([@runner2])}
+  end
+
+  describe :online? do
+    let(:runner) { FactoryGirl.create(:ci_shared_runner) }
+
+    subject { runner.online? }
+
+    context 'never contacted' do
+      before { runner.contacted_at = nil }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'contacted long time ago time' do
+      before { runner.contacted_at = 1.year.ago }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'contacted 1s ago' do
+      before { runner.contacted_at = 1.second.ago }
+
+      it { is_expected.to be_truthy }
+    end
+  end
+
+  describe :status do
+    let(:runner) { FactoryGirl.create(:ci_shared_runner, contacted_at: 1.second.ago) }
+
+    subject { runner.status }
+
+    context 'never connected' do
+      before { runner.contacted_at = nil }
+
+      it { is_expected.to eq(:not_connected) }
+    end
+
+    context 'contacted 1s ago' do
+      before { runner.contacted_at = 1.second.ago }
+
+      it { is_expected.to eq(:online) }
+    end
+
+    context 'contacted long time ago' do
+      before { runner.contacted_at = 1.year.ago }
+
+      it { is_expected.to eq(:offline) }
+    end
+
+    context 'inactive' do
+      before { runner.active = false }
+
+      it { is_expected.to eq(:paused) }
+    end
+  end
+
   describe "belongs_to_one_project?" do
     it "returns false if there are two projects runner assigned to" do
       runner = FactoryGirl.create(:ci_specific_runner)