Calculate real queueing time to exclude gaps from:

retries and probably also manual actions!

Feedback:

* https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6084#note_14735478
* https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6084#note_14737804

4 files changed, 34 insertions, 38 deletions

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 9799c06c19f..b7d4b492a75 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -260,13 +260,8 @@ module Ci
     def update_duration
       return unless started_at
 
-      calculated_status = %w[success failed running canceled]
-      calculated_builds = builds.latest.where(status: calculated_status)
-      calculator = Gitlab::Ci::PipelineDuration.from_builds(calculated_builds)
-
-      self.duration = calculator.duration
-      self.pending_duration =
-        started_at - created_at + calculator.pending_duration
+      self.duration, self.pending_duration =
+        Gitlab::Ci::PipelineDuration.from_pipeline(self)
     end
 
     def execute_hooks
diff --git a/lib/gitlab/ci/pipeline_duration.rb b/lib/gitlab/ci/pipeline_duration.rb
index e37ba19bca9..baf84954b7e 100644
--- a/lib/gitlab/ci/pipeline_duration.rb
+++ b/lib/gitlab/ci/pipeline_duration.rb
@@ -106,17 +106,27 @@ module Gitlab
         end
       end
 
-      def self.from_builds(builds)
+      def self.from_pipeline(pipeline)
+        status = %w[success failed running canceled]
+        builds = pipeline.builds.latest.where(status: status)
+
+        duration = from_builds(builds, :started_at, :finished_at).duration
+        queued = from_builds(builds, :queued_at, :started_at).duration
+
+        [duration, pipeline.started_at - pipeline.created_at + queued]
+      end
+
+      def self.from_builds(builds, from, to)
         now = Time.now
 
         periods = builds.map do |b|
-          Period.new(b.started_at || now, b.finished_at || now)
+          Period.new(b.public_send(from) || now, b.public_send(to) || now)
         end
 
         new(periods)
       end
 
-      attr_reader :duration, :pending_duration
+      attr_reader :duration
 
       def initialize(periods)
         process(periods.sort_by(&:first))
@@ -125,10 +135,7 @@ module Gitlab
       private
 
       def process(periods)
-        merged = process_periods(periods)
-
-        @duration = process_duration(merged)
-        @pending_duration = process_pending_duration(merged)
+        @duration = process_duration(process_periods(periods))
       end
 
       def process_periods(periods)
@@ -157,13 +164,6 @@ module Gitlab
           result + per.duration
         end
       end
-
-      def process_pending_duration(periods)
-        return 0 if periods.empty?
-
-        total = periods.last.last - periods.first.first
-        total - duration
-      end
     end
   end
 end
diff --git a/spec/lib/gitlab/ci/pipeline_duration_spec.rb b/spec/lib/gitlab/ci/pipeline_duration_spec.rb
index acb690581d6..a160c0a6e39 100644
--- a/spec/lib/gitlab/ci/pipeline_duration_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline_duration_spec.rb
@@ -6,7 +6,6 @@ describe Gitlab::Ci::PipelineDuration do
   shared_examples 'calculating duration' do
     it do
       expect(calculator.duration).to eq(duration)
-      expect(calculator.pending_duration).to eq(pending_duration)
     end
   end
 
@@ -19,7 +18,6 @@ describe Gitlab::Ci::PipelineDuration do
     end
 
     let(:duration) { 4 }
-    let(:pending_duration) { 2 }
 
     it_behaves_like 'calculating duration'
   end
@@ -34,7 +32,6 @@ describe Gitlab::Ci::PipelineDuration do
     end
 
     let(:duration) { 4 }
-    let(:pending_duration) { 0 }
 
     it_behaves_like 'calculating duration'
   end
@@ -48,7 +45,6 @@ describe Gitlab::Ci::PipelineDuration do
     end
 
     let(:duration) { 8 }
-    let(:pending_duration) { 1 }
 
     it_behaves_like 'calculating duration'
   end
@@ -62,7 +58,6 @@ describe Gitlab::Ci::PipelineDuration do
     end
 
     let(:duration) { 4 }
-    let(:pending_duration) { 3 }
 
     it_behaves_like 'calculating duration'
   end
@@ -80,7 +75,6 @@ describe Gitlab::Ci::PipelineDuration do
     end
 
     let(:duration) { 7 }
-    let(:pending_duration) { 2 }
 
     it_behaves_like 'calculating duration'
   end
@@ -95,7 +89,6 @@ describe Gitlab::Ci::PipelineDuration do
     end
 
     let(:duration) { 6 }
-    let(:pending_duration) { 1 }
 
     it_behaves_like 'calculating duration'
   end
@@ -108,7 +101,6 @@ describe Gitlab::Ci::PipelineDuration do
     end
 
     let(:duration) { 4 }
-    let(:pending_duration) { 2 }
 
     it_behaves_like 'calculating duration'
   end
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index a34b9cb4461..2d239caab64 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -124,18 +124,23 @@ describe Ci::Pipeline, models: true do
 
   describe 'state machine' do
     let(:current) { Time.now.change(usec: 0) }
-    let(:build) { create_build('build1', current - 60) }
-    let(:another_build) { create_build('build2', current + 20) }
+    let(:build) { create_build('build1', current, 10) }
+    let(:another_build) { create_build('build2', current + 80, 5) }
 
-    describe '#duration' do
+    describe '#duration and #pending_duration' do
       before do
-        pipeline.run
+        pipeline.update(created_at: current)
 
-        travel_to(current) do
+        travel_to(current + 5) do
+          pipeline.run
+          pipeline.save
+        end
+
+        travel_to(current + 70) do
           build.success
         end
 
-        travel_to(current + 80) do
+        travel_to(current + 145) do
           another_build.drop
         end
 
@@ -143,7 +148,10 @@ describe Ci::Pipeline, models: true do
       end
 
       it 'matches sum of builds duration' do
-        expect(pipeline.reload.duration).to eq(120)
+        pipeline.reload
+
+        expect(pipeline.duration).to eq(70 - 10 + 145 - 85)
+        expect(pipeline.pending_duration).to eq(5 + 10 + 5)
       end
     end
 
@@ -175,11 +183,12 @@ describe Ci::Pipeline, models: true do
       end
     end
 
-    def create_build(name, started_at = current)
+    def create_build(name, queued_at = current, started_from = 0)
       create(:ci_build,
              name: name,
              pipeline: pipeline,
-             started_at: started_at)
+             queued_at: queued_at,
+             started_at: queued_at + started_from)
     end
   end