Refactorize Ci::Build and Ci::BuildMetadata models

6 files changed, 33 insertions, 36 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index cc29c6150d3..7a12d7a3deb 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -24,7 +24,7 @@ module Ci
     has_one :job_artifacts_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id
     has_one :job_artifacts_trace, -> { where(file_type: Ci::JobArtifact.file_types[:trace]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id
 
-    has_one :metadata, class_name: 'Ci::BuildMetadata'
+    has_one :build_metadata, class_name: 'Ci::BuildMetadata'
 
     # The "environment" field for builds is a String, and is the unexpanded name
     def persisted_environment
@@ -85,10 +85,6 @@ module Ci
     before_save :ensure_token
     before_destroy { unscoped_project }
 
-    before_create do |build|
-      build.metadata = Ci::BuildMetadata.new
-    end
-
     after_create unless: :importing? do |build|
       run_after_commit { BuildHooksWorker.perform_async(build.id) }
     end
@@ -161,10 +157,14 @@ module Ci
       end
 
       before_transition pending: :running do |build|
-        build.metadata.save_timeout_state! unless build.metadata.nil?
+        build.metadata.save_timeout_state!
       end
     end
 
+    def metadata
+      self.build_metadata ||= Ci::BuildMetadata.new
+    end
+
     def detailed_status(current_user)
       Gitlab::Ci::Status::Build::Factory
         .new(self, current_user)
@@ -242,7 +242,7 @@ module Ci
     end
 
     def timeout
-      [project.build_timeout, runner&.maximum_job_timeout].compact.min
+      metadata.used_timeout
     end
 
     def triggered_by?(current_user)
diff --git a/app/models/ci/build_metadata.rb b/app/models/ci/build_metadata.rb
index 7a1315dfcf9..9043bed86bf 100644
--- a/app/models/ci/build_metadata.rb
+++ b/app/models/ci/build_metadata.rb
@@ -1,4 +1,6 @@
 module Ci
+  # The purpose of this class is to store Build related data that can be disposed.
+  # Data that should be persisted forever, should be stored with Ci::Build model.
   class BuildMetadata < ActiveRecord::Base
     extend Gitlab::Ci::Model
     include Presentable
@@ -11,14 +13,18 @@ module Ci
     chronic_duration_attr_reader :used_timeout_human_readable, :used_timeout
 
     enum timeout_source: {
-        unknown_timeout_source: nil,
-        project_timeout_source: 1,
-        runner_timeout_source: 2
+        unknown_timeout_source: 1,
+        project_timeout_source: 2,
+        runner_timeout_source: 3
     }
 
     def save_timeout_state!
-      self.used_timeout = build.timeout
-      self.timeout_source = build.timeout < build.project.build_timeout ? :runner_timeout_source : :project_timeout_source
+      project_timeout = build.project&.build_timeout
+      timeout = [project_timeout, build.runner&.maximum_job_timeout].compact.min
+
+      self.used_timeout = timeout
+      self.timeout_source = timeout < project_timeout ? :runner_timeout_source : :project_timeout_source
+
       save!
     end
   end
diff --git a/db/migrate/20180301010859_create_ci_builds_metadata_table.rb b/db/migrate/20180301010859_create_ci_builds_metadata_table.rb
index 9d5e9c1779b..cd7824d7788 100644
--- a/db/migrate/20180301010859_create_ci_builds_metadata_table.rb
+++ b/db/migrate/20180301010859_create_ci_builds_metadata_table.rb
@@ -4,10 +4,13 @@ class CreateCiBuildsMetadataTable < ActiveRecord::Migration
   DOWNTIME = false
 
   def change
-    create_table :ci_builds_metadata do |t|
+    create_table :ci_builds_metadata, id: false do |t|
       t.integer :build_id, null: false
       t.integer :used_timeout
-      t.integer :timeout_source
+      t.integer :timeout_source, null: false, default: 1
+
+      t.primary_key :build_id
+      t.foreign_key :ci_builds, column: :build_id, on_delete: :cascade
     end
   end
 end
diff --git a/db/migrate/20180301011323_add_build_foreign_key_to_ci_builds_metadata.rb b/db/migrate/20180301011323_add_build_foreign_key_to_ci_builds_metadata.rb
deleted file mode 100644
index feda2d6e9c9..00000000000
--- a/db/migrate/20180301011323_add_build_foreign_key_to_ci_builds_metadata.rb
+++ /dev/null
@@ -1,15 +0,0 @@
-class AddBuildForeignKeyToCiBuildsMetadata < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def up
-    add_concurrent_foreign_key(:ci_builds_metadata, :ci_builds, column: :build_id)
-  end
-
-  def down
-    remove_foreign_key(:ci_builds_metadata, column: :build_id)
-  end
-end
diff --git a/db/schema.rb b/db/schema.rb
index 5463b3f1219..920d71e0110 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -329,10 +329,9 @@ ActiveRecord::Schema.define(version: 20180327101207) do
   add_index "ci_builds", ["updated_at"], name: "index_ci_builds_on_updated_at", using: :btree
   add_index "ci_builds", ["user_id"], name: "index_ci_builds_on_user_id", using: :btree
 
-  create_table "ci_builds_metadata", force: :cascade do |t|
-    t.integer "build_id", null: false
+  create_table "ci_builds_metadata", primary_key: "build_id", force: :cascade do |t|
     t.integer "used_timeout"
-    t.integer "timeout_source"
+    t.integer "timeout_source", default: 1, null: false
   end
 
   create_table "ci_group_variables", force: :cascade do |t|
@@ -2035,7 +2034,7 @@ ActiveRecord::Schema.define(version: 20180327101207) do
   add_foreign_key "ci_builds", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_a2141b1522", on_delete: :nullify
   add_foreign_key "ci_builds", "ci_stages", column: "stage_id", name: "fk_3a9eaa254d", on_delete: :cascade
   add_foreign_key "ci_builds", "projects", name: "fk_befce0568a", on_delete: :cascade
-  add_foreign_key "ci_builds_metadata", "ci_builds", column: "build_id", name: "fk_e20479742e", on_delete: :cascade
+  add_foreign_key "ci_builds_metadata", "ci_builds", column: "build_id", on_delete: :cascade
   add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade
   add_foreign_key "ci_job_artifacts", "ci_builds", column: "job_id", on_delete: :cascade
   add_foreign_key "ci_job_artifacts", "projects", on_delete: :cascade
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 1da84d6caa9..670803a0883 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1275,17 +1275,21 @@ describe Ci::Build do
       set(:project2) { create(:project, :repository, group: group, build_timeout: 1000) }
       set(:pipeline2) { create(:ci_pipeline, project: project2, sha: project2.commit.id, ref: project2.default_branch, status: 'success') }
 
-      let(:build) { create(:ci_build, :manual, pipeline: pipeline2) }
+      let(:build) { create(:ci_build, :pending, pipeline: pipeline2) }
 
       subject { build.timeout }
 
+      before do
+        build.run!
+      end
+
       it 'returns project timeout configuration' do
         is_expected.to eq(project2.build_timeout)
       end
 
       context 'when runner sets timeout to bigger value' do
         let(:runner2) { create(:ci_runner, maximum_job_timeout: 2000) }
-        let(:build) { create(:ci_build, :manual, pipeline: pipeline2, runner: runner2) }
+        let(:build) { create(:ci_build, :pending, pipeline: pipeline2, runner: runner2) }
 
         it 'returns project timeout configuration' do
           is_expected.to eq(project2.build_timeout)
@@ -1294,7 +1298,7 @@ describe Ci::Build do
 
       context 'when runner sets timeout to smaller value' do
         let(:runner2) { create(:ci_runner, maximum_job_timeout: 500) }
-        let(:build) { create(:ci_build, :manual, pipeline: pipeline2, runner: runner2) }
+        let(:build) { create(:ci_build, :pending, pipeline: pipeline2, runner: runner2) }
 
         it 'returns project timeout configuration' do
           is_expected.to eq(runner2.maximum_job_timeout)