Optimise trace handling code to use streaming instead of full read

6 files changed, 48 insertions, 160 deletions

diff --git a/app/assets/javascripts/build.js b/app/assets/javascripts/build.js
index fe54ecffdfe..7cb022c848a 100644
--- a/app/assets/javascripts/build.js
+++ b/app/assets/javascripts/build.js
@@ -84,10 +84,10 @@ window.Build = (function() {
     var removeRefreshStatuses = ['success', 'failed', 'canceled', 'skipped'];
 
     return $.ajax({
-      url: this.buildUrl,
+      url: this.pageUrl + "/trace.json",
       dataType: 'json',
       success: function(buildData) {
-        $('.js-build-output').html(buildData.trace_html);
+        $('.js-build-output').html(buildData.html);
         gl.utils.setCiStatusFavicon(`${this.pageUrl}/status.json`);
         if (window.location.hash === DOWN_BUILD_TRACE) {
           $("html,body").scrollTop(this.$buildTrace.height());
diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb
index 3f3c90a49ab..add66ce9f84 100644
--- a/app/controllers/projects/builds_controller.rb
+++ b/app/controllers/projects/builds_controller.rb
@@ -31,25 +31,25 @@ class Projects::BuildsController < Projects::ApplicationController
     @builds = @project.pipelines.find_by_sha(@build.sha).builds.order('id DESC')
     @builds = @builds.where("id not in (?)", @build.id)
     @pipeline = @build.pipeline
-
-    respond_to do |format|
-      format.html
-      format.json do
-        render json: {
-          id: @build.id,
-          status: @build.status,
-          trace_html: @build.trace_html
-        }
-      end
-    end
   end
 
   def trace
-    respond_to do |format|
-      format.json do
-        state = params[:state].presence
-        render json: @build.trace_with_state(state: state).
-          merge!(id: @build.id, status: @build.status)
+    build.trace.read do |stream|
+      respond_to do |format|
+        format.json do
+          result = {
+            id: @build.id, status: @build.status, complete: @build.complete?
+          }
+
+          if stream.valid?
+            stream.limit
+            state = params[:state].presence
+            trace = stream.html_with_state(state)
+            result.merge!(trace.to_h)
+          end
+
+          render json: result
+        end
       end
     end
   end
@@ -86,10 +86,12 @@ class Projects::BuildsController < Projects::ApplicationController
   end
 
   def raw
-    if @build.has_trace_file?
-      send_file @build.trace_file_path, type: 'text/plain; charset=utf-8', disposition: 'inline'
-    else
-      render_404
+    build.trace.read do |stream|
+      if stream.file?
+        send_file stream.path, type: 'text/plain; charset=utf-8', disposition: 'inline'
+      else
+        render_404
+      end
     end
   end
 
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 8431c5f228c..70470414bc4 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -171,19 +171,6 @@ module Ci
       latest_builds.where('stage_idx < ?', stage_idx)
     end
 
-    def trace_html(**args)
-      trace_with_state(**args)[:html] || ''
-    end
-
-    def trace_with_state(state: nil, last_lines: nil)
-      trace_ansi = trace(last_lines: last_lines)
-      if trace_ansi.present?
-        Ci::Ansi2html.convert(trace_ansi, state)
-      else
-        {}
-      end
-    end
-
     def timeout
       project.build_timeout
     end
@@ -244,136 +231,35 @@ module Ci
     end
 
     def update_coverage
-      coverage = extract_coverage(trace, coverage_regex)
+      coverage = trace.extract_coverage(coverage_regex)
       update_attributes(coverage: coverage) if coverage.present?
     end
 
-    def extract_coverage(text, regex)
-      return unless regex
-
-      matches = text.scan(Regexp.new(regex)).last
-      matches = matches.last if matches.is_a?(Array)
-      coverage = matches.gsub(/\d+(\.\d+)?/).first
-
-      if coverage.present?
-        coverage.to_f
-      end
-    rescue
-      # if bad regex or something goes wrong we dont want to interrupt transition
-      # so we just silentrly ignore error for now
-    end
-
-    def has_trace_file?
-      File.exist?(path_to_trace) || has_old_trace_file?
+    def trace
+      Gitlab::Ci::Trace.new(self)
     end
 
     def has_trace?
-      raw_trace.present?
+      trace.exist?
     end
 
-    def raw_trace(last_lines: nil)
-      if File.exist?(trace_file_path)
-        Gitlab::Ci::TraceReader.new(trace_file_path).
-          read(last_lines: last_lines)
-      else
-        # backward compatibility
-        read_attribute :trace
-      end
+    def trace=(data)
+      raise NotImplementedError
     end
 
-    ##
-    # Deprecated
-    #
-    # This is a hotfix for CI build data integrity, see #4246
-    def has_old_trace_file?
-      project.ci_id && File.exist?(old_path_to_trace)
-    end
-
-    def trace(last_lines: nil)
-      hide_secrets(raw_trace(last_lines: last_lines))
+    def old_trace
+      read_attribute(:trace)
     end
 
-    def trace_length
-      if raw_trace
-        raw_trace.bytesize
-      else
-        0
-      end
-    end
-
-    def trace=(trace)
-      recreate_trace_dir
-      trace = hide_secrets(trace)
-      File.write(path_to_trace, trace)
-    end
-
-    def recreate_trace_dir
-      unless Dir.exist?(dir_to_trace)
-        FileUtils.mkdir_p(dir_to_trace)
-      end
-    end
-    private :recreate_trace_dir
-
-    def append_trace(trace_part, offset)
-      recreate_trace_dir
-      touch if needs_touch?
-
-      trace_part = hide_secrets(trace_part)
-
-      File.truncate(path_to_trace, offset) if File.exist?(path_to_trace)
-      File.open(path_to_trace, 'ab') do |f|
-        f.write(trace_part)
-      end
+    def erase_old_trace!
+      write_attribute(:trace, nil)
+      save
     end
 
     def needs_touch?
       Time.now - updated_at > 15.minutes.to_i
     end
 
-    def trace_file_path
-      if has_old_trace_file?
-        old_path_to_trace
-      else
-        path_to_trace
-      end
-    end
-
-    def dir_to_trace
-      File.join(
-        Settings.gitlab_ci.builds_path,
-        created_at.utc.strftime("%Y_%m"),
-        project.id.to_s
-      )
-    end
-
-    def path_to_trace
-      "#{dir_to_trace}/#{id}.log"
-    end
-
-    ##
-    # Deprecated
-    #
-    # This is a hotfix for CI build data integrity, see #4246
-    # Should be removed in 8.4, after CI files migration has been done.
-    #
-    def old_dir_to_trace
-      File.join(
-        Settings.gitlab_ci.builds_path,
-        created_at.utc.strftime("%Y_%m"),
-        project.ci_id.to_s
-      )
-    end
-
-    ##
-    # Deprecated
-    #
-    # This is a hotfix for CI build data integrity, see #4246
-    # Should be removed in 8.4, after CI files migration has been done.
-    #
-    def old_path_to_trace
-      "#{old_dir_to_trace}/#{id}.log"
-    end
-
     ##
     # Deprecated
     #
@@ -555,6 +441,15 @@ module Ci
       options[:dependencies]&.empty?
     end
 
+    def hide_secrets(trace)
+      return unless trace
+
+      trace = trace.dup
+      Ci::MaskSecret.mask!(trace, project.runners_token) if project
+      Ci::MaskSecret.mask!(trace, token)
+      trace
+    end
+
     private
 
     def update_artifacts_size
@@ -566,7 +461,7 @@ module Ci
     end
 
     def erase_trace!
-      self.trace = nil
+      trace.erase!
     end
 
     def update_erased!(user = nil)
@@ -628,15 +523,6 @@ module Ci
       pipeline.config_processor.build_attributes(name)
     end
 
-    def hide_secrets(trace)
-      return unless trace
-
-      trace = trace.dup
-      Ci::MaskSecret.mask!(trace, project.runners_token) if project
-      Ci::MaskSecret.mask!(trace, token)
-      trace
-    end
-
     def update_project_statistics
       return unless project
 
diff --git a/app/views/notify/pipeline_failed_email.html.haml b/app/views/notify/pipeline_failed_email.html.haml
index 4beb6fcee5d..a83faa839df 100644
--- a/app/views/notify/pipeline_failed_email.html.haml
+++ b/app/views/notify/pipeline_failed_email.html.haml
@@ -137,6 +137,6 @@
             - if build.has_trace?
               %td{ colspan: "2", style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;padding:0 0 15px;" }
                 %pre{ style: "font-family:Monaco,'Lucida Console','Courier New',Courier,monospace;background-color:#fafafa;border-radius:3px;overflow:hidden;white-space:pre-wrap;word-break:break-all;font-size:13px;line-height:1.4;padding:12px;color:#333333;margin:0;" }
-                  = build.trace_html(last_lines: 10).html_safe
+                  = build.trace.html(last_lines: 10).html_safe
             - else
               %td{ colspan: "2" }
diff --git a/app/views/notify/pipeline_failed_email.text.erb b/app/views/notify/pipeline_failed_email.text.erb
index c1a4ea40cf5..294238eee51 100644
--- a/app/views/notify/pipeline_failed_email.text.erb
+++ b/app/views/notify/pipeline_failed_email.text.erb
@@ -35,7 +35,7 @@ had <%= failed.size %> failed <%= 'build'.pluralize(failed.size) %>.
 Stage: <%= build.stage %>
 Name: <%= build.name %>
 <% if build.has_trace? -%>
-Trace: <%= build.trace_with_state(last_lines: 10)[:text] %>
+Trace: <%= build.trace.raw(last_lines: 10) %>
 <% end -%>
 
 <% end -%>
diff --git a/app/views/projects/builds/_sidebar.html.haml b/app/views/projects/builds/_sidebar.html.haml
index 6f45d5b0689..f4a66398c85 100644
--- a/app/views/projects/builds/_sidebar.html.haml
+++ b/app/views/projects/builds/_sidebar.html.haml
@@ -68,7 +68,7 @@
         - elsif @build.runner
           \##{@build.runner.id}
       .btn-group.btn-group-justified{ role: :group }
-        - if @build.has_trace_file?
+        - if @build.has_trace?
           = link_to 'Raw', raw_namespace_project_build_path(@project.namespace, @project, @build), class: 'btn btn-sm btn-default'
         - if @build.active?
           = link_to "Cancel", cancel_namespace_project_build_path(@project.namespace, @project, @build), class: 'btn btn-sm btn-default', method: :post