Merge branch 'persist-expanded-environment-name-in-build-metadata' into 'master'

Persist expanded environment name in ci build metadata

Closes #193816 and #25155

See merge request gitlab-org/gitlab!22374

app/models/ci/build.rb

@@ -59,15 +59,11 @@ module Ci
     ##
     # Since Gitlab 11.5, deployments records started being created right after
     # `ci_builds` creation. We can look up a relevant `environment` through
-    # `deployment` relation today. This is much more efficient than expanding
-    # environment name with variables.
+    # `deployment` relation today.
     # (See more https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/22380)
     #
-    # However, we have to still expand environment name if it's a stop action,
-    # because `deployment` persists information for start action only.
-    #
-    # We will follow up this by persisting expanded name in build metadata or
-    # persisting stop action in database.
+    # Since Gitlab 12.9, we started persisting the expanded environment name to
+    # avoid repeated variables expansion in `action: stop` builds as well.
     def persisted_environment
       return unless has_environment?
 
@@ -465,7 +461,14 @@ module Ci
       return unless has_environment?
 
       strong_memoize(:expanded_environment_name) do
-        ExpandVariables.expand(environment, -> { simple_variables })
+        # We're using a persisted expanded environment name in order to avoid
+        # variable expansion per request.
+        if Feature.enabled?(:ci_persisted_expanded_environment_name, project, default_enabled: true) &&
+          metadata&.expanded_environment_name.present?
+          metadata.expanded_environment_name
+        else
+          ExpandVariables.expand(environment, -> { simple_variables })
+        end
       end
     end
 

app/models/concerns/ci/metadatable.rb

@@ -14,6 +14,8 @@ module Ci
                          inverse_of: :build,
                          autosave: true
 
+      accepts_nested_attributes_for :metadata
+
       delegate :timeout, to: :metadata, prefix: true, allow_nil: true
       delegate :interruptible, to: :metadata, prefix: false, allow_nil: true
       delegate :has_exposed_artifacts?, to: :metadata, prefix: false, allow_nil: true

app/services/ci/retry_build_service.rb

@@ -52,7 +52,7 @@ module Ci
 
     def create_build!(attributes)
       build = project.builds.new(attributes)
-      build.deployment = ::Gitlab::Ci::Pipeline::Seed::Deployment.new(build).to_resource
+      build.assign_attributes(::Gitlab::Ci::Pipeline::Seed::Build.environment_attributes_for(build))
       build.retried = false
       build.save!
       build

changelogs/unreleased/persist-expanded-environment-name-in-build-metadata.yml

+---
+title: Persist expanded environment name in ci build metadata
+merge_request: 22374
+author:
+type: performance

db/migrate/20200102140148_add_expanded_environment_name_to_ci_build_metadata.rb

+# frozen_string_literal: true
+
+class AddExpandedEnvironmentNameToCiBuildMetadata < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def up
+    add_column :ci_builds_metadata, :expanded_environment_name, :string, limit: 255
+  end
+
+  def down
+    remove_column :ci_builds_metadata, :expanded_environment_name
+  end
+end

db/schema.rb

@@ -722,6 +722,7 @@ ActiveRecord::Schema.define(version: 2020_02_27_165129) do
     t.jsonb "config_variables"
     t.boolean "has_exposed_artifacts"
     t.string "environment_auto_stop_in", limit: 255
+    t.string "expanded_environment_name", limit: 255
     t.index ["build_id"], name: "index_ci_builds_metadata_on_build_id", unique: true
     t.index ["build_id"], name: "index_ci_builds_metadata_on_build_id_and_has_exposed_artifacts", where: "(has_exposed_artifacts IS TRUE)"
     t.index ["build_id"], name: "index_ci_builds_metadata_on_build_id_and_interruptible", where: "(interruptible = true)"

lib/gitlab/ci/pipeline/seed/build.rb

@@ -7,6 +7,8 @@ module Gitlab
         class Build < Seed::Base
           include Gitlab::Utils::StrongMemoize
 
+          EnvironmentCreationFailure = Class.new(StandardError)
+
           delegate :dig, to: :@seed_attributes
 
           # When the `ci_dag_limit_needs` is enabled it uses the lower limit
@@ -77,14 +79,39 @@ module Gitlab
               if bridge?
                 ::Ci::Bridge.new(attributes)
               else
-                ::Ci::Build.new(attributes).tap do |job|
-                  job.deployment = Seed::Deployment.new(job).to_resource
-                  job.resource_group = Seed::Build::ResourceGroup.new(job, @resource_group_key).to_resource
+                ::Ci::Build.new(attributes).tap do |build|
+                  build.assign_attributes(self.class.environment_attributes_for(build))
+                  build.resource_group = Seed::Build::ResourceGroup.new(build, @resource_group_key).to_resource
                 end
               end
             end
           end
 
+          def self.environment_attributes_for(build)
+            return {} unless build.has_environment?
+
+            environment = Seed::Environment.new(build).to_resource
+
+            # If there is a validation error on environment creation, such as
+            # the name contains invalid character, the build falls back to a
+            # non-environment job.
+            unless environment.persisted?
+              Gitlab::ErrorTracking.track_exception(
+                EnvironmentCreationFailure.new,
+                project_id: build.project_id,
+                reason: environment.errors.full_messages.to_sentence)
+
+              return { environment: nil }
+            end
+
+            {
+              deployment: Seed::Deployment.new(build, environment).to_resource,
+              metadata_attributes: {
+                expanded_environment_name: environment.name
+              }
+            }
+          end
+
           private
 
           def all_of_only?

lib/gitlab/ci/pipeline/seed/deployment.rb

@@ -7,9 +7,9 @@ module Gitlab
         class Deployment < Seed::Base
           attr_reader :job, :environment
 
-          def initialize(job)
+          def initialize(job, environment)
             @job = job
-            @environment = Seed::Environment.new(@job)
+            @environment = environment
           end
 
           def to_resource
@@ -17,7 +17,6 @@ module Gitlab
             return unless job.starts_environment?
 
             deployment = ::Deployment.new(attributes)
-            deployment.environment = environment.to_resource
 
             # If there is a validation error on environment creation, such as
             # the name contains invalid character, the job will fall back to a
@@ -45,6 +44,7 @@ module Gitlab
           def attributes
             {
               project: job.project,
+              environment: environment,
               user: job.user,
               ref: job.ref,
               tag: job.tag,

lib/gitlab/ci/pipeline/seed/environment.rb

@@ -12,25 +12,15 @@ module Gitlab
           end
 
           def to_resource
-            find_environment || ::Environment.create(attributes)
+            job.project.environments
+              .safe_find_or_create_by(name: expanded_environment_name)
           end
 
           private
 
-          def find_environment
-            job.project.environments.find_by_name(expanded_environment_name)
-          end
-
           def expanded_environment_name
             job.expanded_environment_name
           end
-
-          def attributes
-            {
-              project: job.project,
-              name: expanded_environment_name
-            }
-          end
         end
       end
     end

spec/factories/ci/builds.rb

@@ -230,8 +230,9 @@ FactoryBot.define do
         # Build deployment/environment relations if environment name is set
         # to the job. If `build.deployment` has already been set, it doesn't
         # build a new instance.
+        environment = Gitlab::Ci::Pipeline::Seed::Environment.new(build).to_resource
         build.deployment =
-          Gitlab::Ci::Pipeline::Seed::Deployment.new(build).to_resource
+          Gitlab::Ci::Pipeline::Seed::Deployment.new(build, environment).to_resource
       end
     end
 

spec/lib/gitlab/ci/pipeline/seed/build_spec.rb

@@ -214,24 +214,98 @@ describe Gitlab::Ci::Pipeline::Seed::Build do
       it { is_expected.to be_a(::Ci::Build) }
       it { is_expected.to be_valid }
 
-      context 'when job has environment name' do
-        let(:attributes) { { name: 'rspec', ref: 'master', environment: 'production' } }
-
+      shared_examples_for 'deployment job' do
         it 'returns a job with deployment' do
           expect(subject.deployment).not_to be_nil
           expect(subject.deployment.deployable).to eq(subject)
-          expect(subject.deployment.environment.name).to eq('production')
+          expect(subject.deployment.environment.name).to eq(expected_environment_name)
         end
+      end
+
+      shared_examples_for 'non-deployment job' do
+        it 'returns a job without deployment' do
+          expect(subject.deployment).to be_nil
+        end
+      end
+
+      shared_examples_for 'ensures environment existence' do
+        it 'has environment' do
+          expect(subject).to be_has_environment
+          expect(subject.environment).to eq(environment_name)
+          expect(subject.metadata.expanded_environment_name).to eq(expected_environment_name)
+          expect(Environment.exists?(name: expected_environment_name)).to eq(true)
+        end
+      end
+
+      shared_examples_for 'ensures environment inexistence' do
+        it 'does not have environment' do
+          expect(subject).not_to be_has_environment
+          expect(subject.environment).to be_nil
+          expect(subject.metadata.expanded_environment_name).to be_nil
+          expect(Environment.exists?(name: expected_environment_name)).to eq(false)
+        end
+      end
+
+      context 'when job deploys to production' do
+        let(:environment_name) { 'production' }
+        let(:expected_environment_name) { 'production' }
+        let(:attributes) { { name: 'deploy', ref: 'master', environment: 'production' } }
+
+        it_behaves_like 'deployment job'
+        it_behaves_like 'ensures environment existence'
 
         context 'when the environment name is invalid' do
-          let(:attributes) { { name: 'rspec', ref: 'master', environment: '!!!' } }
+          let(:attributes) { { name: 'deploy', ref: 'master', environment: '!!!' } }
 
-          it 'returns a job without deployment' do
-            expect(subject.deployment).to be_nil
+          it_behaves_like 'non-deployment job'
+          it_behaves_like 'ensures environment inexistence'
+
+          it 'tracks an exception' do
+            expect(Gitlab::ErrorTracking).to receive(:track_exception)
+              .with(an_instance_of(described_class::EnvironmentCreationFailure),
+                    project_id: project.id,
+                    reason: %q{Name can contain only letters, digits, '-', '_', '/', '$', '{', '}', '.', and spaces, but it cannot start or end with '/'})
+              .once
+
+            subject
           end
         end
       end
 
+      context 'when job starts a review app' do
+        let(:environment_name) { 'review/$CI_COMMIT_REF_NAME' }
+        let(:expected_environment_name) { "review/#{pipeline.ref}" }
+
+        let(:attributes) do
+          {
+            name: 'deploy', ref: 'master', environment: environment_name,
+            options: { environment: { name: environment_name } }
+          }
+        end
+
+        it_behaves_like 'deployment job'
+        it_behaves_like 'ensures environment existence'
+      end
+
+      context 'when job stops a review app' do
+        let(:environment_name) { 'review/$CI_COMMIT_REF_NAME' }
+        let(:expected_environment_name) { "review/#{pipeline.ref}" }
+
+        let(:attributes) do
+          {
+            name: 'deploy', ref: 'master', environment: environment_name,
+            options: { environment: { name: environment_name, action: 'stop' } }
+          }
+        end
+
+        it 'returns a job without deployment' do
+          expect(subject.deployment).to be_nil
+        end
+
+        it_behaves_like 'non-deployment job'
+        it_behaves_like 'ensures environment existence'
+      end
+
       context 'when job belongs to a resource group' do
         let(:attributes) { { name: 'rspec', ref: 'master', resource_group_key: 'iOS' } }
 

spec/lib/gitlab/ci/pipeline/seed/deployment_spec.rb

@@ -10,7 +10,8 @@ describe Gitlab::Ci::Pipeline::Seed::Deployment do
   end
 
   let(:job) { build(:ci_build, project: project, pipeline: pipeline) }
-  let(:seed) { described_class.new(job) }
+  let(:environment) { Gitlab::Ci::Pipeline::Seed::Environment.new(job).to_resource }
+  let(:seed) { described_class.new(job, environment) }
   let(:attributes) { {} }
 
   before do
@@ -82,5 +83,13 @@ describe Gitlab::Ci::Pipeline::Seed::Deployment do
         is_expected.to be_nil
       end
     end
+
+    context 'when job does not have environment attribute' do
+      let(:attributes) { { name: 'test' } }
+
+      it 'returns nothing' do
+        is_expected.to be_nil
+      end
+    end
   end
 end

spec/lib/gitlab/ci/pipeline/seed/environment_spec.rb

@@ -15,29 +15,68 @@ describe Gitlab::Ci::Pipeline::Seed::Environment do
   describe '#to_resource' do
     subject { seed.to_resource }
 
-    context 'when job has environment attribute' do
-      let(:attributes) do
-        {
-          environment: 'production',
-          options: { environment: { name: 'production' } }
-        }
-      end
-
+    shared_examples_for 'returning a correct environment' do
       it 'returns a persisted environment object' do
+        expect { subject }.to change { Environment.count }.by(1)
+
         expect(subject).to be_a(Environment)
         expect(subject).to be_persisted
         expect(subject.project).to eq(project)
-        expect(subject.name).to eq('production')
+        expect(subject.name).to eq(expected_environment_name)
       end
 
       context 'when environment has already existed' do
-        let!(:environment) { create(:environment, project: project, name: 'production') }
+        let!(:environment) { create(:environment, project: project, name: expected_environment_name) }
 
         it 'returns the existing environment object' do
+          expect { subject }.not_to change { Environment.count }
+
           expect(subject).to be_persisted
           expect(subject).to eq(environment)
         end
       end
     end
+
+    context 'when job has environment attribute' do
+      let(:environment_name) { 'production' }
+      let(:expected_environment_name) { 'production' }
+
+      let(:attributes) do
+        {
+          environment: environment_name,
+          options: { environment: { name: environment_name } }
+        }
+      end
+
+      it_behaves_like 'returning a correct environment'
+    end
+
+    context 'when job starts a review app' do
+      let(:environment_name) { 'review/$CI_COMMIT_REF_NAME' }
+      let(:expected_environment_name) { "review/#{job.ref}" }
+
+      let(:attributes) do
+        {
+          environment: environment_name,
+          options: { environment: { name: environment_name } }
+        }
+      end
+
+      it_behaves_like 'returning a correct environment'
+    end
+
+    context 'when job stops a review app' do
+      let(:environment_name) { 'review/$CI_COMMIT_REF_NAME' }
+      let(:expected_environment_name) { "review/#{job.ref}" }
+
+      let(:attributes) do
+        {
+          environment: environment_name,
+          options: { environment: { name: environment_name, action: 'stop' } }
+        }
+      end
+
+      it_behaves_like 'returning a correct environment'
+    end
   end
 end

spec/models/ci/build_spec.rb

@@ -1293,7 +1293,35 @@ describe Ci::Build do
                  environment: 'review/$APP_HOST')
         end
 
-        it { is_expected.to eq('review/host') }
+        it 'returns an expanded environment name with a list of variables' do
+          expect(build).to receive(:simple_variables).once.and_call_original
+
+          is_expected.to eq('review/host')
+        end
+
+        context 'when build metadata has already persisted the expanded environment name' do
+          before do
+            build.metadata.expanded_environment_name = 'review/host'
+          end
+
+          it 'returns a persisted expanded environment name without a list of variables' do
+            expect(build).not_to receive(:simple_variables)
+
+            is_expected.to eq('review/host')
+          end
+
+          context 'when ci_persisted_expanded_environment_name feature flag is disabled' do
+            before do
+              stub_feature_flags(ci_persisted_expanded_environment_name: false)
+            end
+
+            it 'returns an expanded environment name with a list of variables' do
+              expect(build).to receive(:simple_variables).once.and_call_original
+
+              is_expected.to eq('review/host')
+            end
+          end
+        end
       end
 
       context 'when using persisted variables' do

spec/services/ci/retry_build_service_spec.rb

@@ -238,6 +238,10 @@ describe Ci::RetryBuildService do
         it 'creates a new deployment' do
           expect { new_build }.to change { Deployment.count }.by(1)
         end
+
+        it 'persists expanded environment name' do
+          expect(new_build.metadata.expanded_environment_name).to eq('production')
+        end
       end
 
       context 'when scheduling_type of build is nil' do