From bb64b20a1f7dd831e4c200343b531f9d048c02a8 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 5 Mar 2018 17:35:06 +0100 Subject: [PATCH] Refactorize Ci::Build and Ci::BuildMetadata models --- app/models/ci/build.rb | 14 +++++++------- app/models/ci/build_metadata.rb | 16 +++++++++++----- ...0301010859_create_ci_builds_metadata_table.rb | 7 +++++-- ...dd_build_foreign_key_to_ci_builds_metadata.rb | 15 --------------- db/schema.rb | 7 +++---- spec/models/ci/build_spec.rb | 10 +++++++--- 6 files changed, 33 insertions(+), 36 deletions(-) delete mode 100644 db/migrate/20180301011323_add_build_foreign_key_to_ci_builds_metadata.rb 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) -- GitLab