From d886be0725bc6c8af1a977fef78089c1bba8adda Mon Sep 17 00:00:00 2001 From: Bala Kumar Date: Tue, 12 Apr 2022 13:29:10 +0530 Subject: [PATCH] Run all deployment jobs for the common pipeline with same environment Changelog: changed --- .../projects/environments_controller.rb | 6 +- app/models/environment.rb | 53 ++++- app/policies/environment_policy.rb | 6 +- app/serializers/environment_entity.rb | 2 +- app/services/environments/stop_service.rb | 2 +- app/workers/environments/auto_stop_worker.rb | 6 +- .../environment_multiple_stop_actions.yml | 8 + lib/api/environments.rb | 2 +- .../projects/environments_controller_spec.rb | 28 ++- spec/models/environment_spec.rb | 206 ++++++++++++++++-- .../environment_serializer_shared_examples.rb | 12 +- 11 files changed, 281 insertions(+), 50 deletions(-) create mode 100644 config/feature_flags/development/environment_multiple_stop_actions.yml diff --git a/app/controllers/projects/environments_controller.rb b/app/controllers/projects/environments_controller.rb index eabc048e341fb0..8e81e75ad13f95 100644 --- a/app/controllers/projects/environments_controller.rb +++ b/app/controllers/projects/environments_controller.rb @@ -104,11 +104,11 @@ def update def stop return render_404 unless @environment.available? - stop_action = @environment.stop_with_action!(current_user) + stop_actions = @environment.stop_with_actions!(current_user) action_or_env_url = - if stop_action - polymorphic_url([project, stop_action]) + if stop_actions&.count == 1 + polymorphic_url([project, stop_actions.first]) else project_environment_url(project, @environment) end diff --git a/app/models/environment.rb b/app/models/environment.rb index 54323c8bbd834a..ff232288ea8239 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -59,7 +59,7 @@ class Environment < ApplicationRecord allow_nil: true, addressable_url: true - delegate :stop_action, :manual_actions, to: :last_deployment, allow_nil: true + delegate :manual_actions, to: :last_deployment, allow_nil: true delegate :auto_rollback_enabled?, to: :project scope :available, -> { with_state(:available) } @@ -185,6 +185,23 @@ def last_deployable last_deployment&.deployable end + def last_deployment_pipeline + last_deployable&.pipeline + end + + # This method returns the deployment records of the last deployment pipeline, that successfully executed to this environment. + # e.g. + # A pipeline contains + # - deploy job A => production environment + # - deploy job B => production environment + # In this case, `last_deployment_group` returns both deployments, whereas `last_deployable` returns only B. + def last_deployment_group + return Deployment.none unless last_deployment_pipeline + + successful_deployments.where( + deployable_id: last_deployment_pipeline.latest_builds.pluck(:id)) + end + # NOTE: Below assocation overrides is a workaround for issue https://gitlab.com/gitlab-org/gitlab/-/issues/339908 # It helps to avoid cross joins with the CI database. # Caveat: It also overrides and losses the default AR caching mechanism. @@ -255,8 +272,8 @@ def formatted_external_url external_url.gsub(%r{\A.*?://}, '') end - def stop_action_available? - available? && stop_action.present? + def stop_actions_available? + available? && stop_actions.present? end def cancel_deployment_jobs! @@ -269,18 +286,34 @@ def cancel_deployment_jobs! end end - def stop_with_action!(current_user) + def stop_with_actions!(current_user) return unless available? stop! - return unless stop_action + actions = [] - Gitlab::OptimisticLocking.retry_lock( - stop_action, - name: 'environment_stop_with_action' - ) do |build| - build&.play(current_user) + stop_actions.each do |stop_action| + Gitlab::OptimisticLocking.retry_lock( + stop_action, + name: 'environment_stop_with_actions' + ) do |build| + actions << build.play(current_user) + end + end + + actions + end + + def stop_actions + strong_memoize(:stop_actions) do + if ::Feature.enabled?(:environment_multiple_stop_actions, project, default_enabled: :yaml) + # Fix N+1 queries it brings to the serializer. + # Tracked in https://gitlab.com/gitlab-org/gitlab/-/issues/358780 + last_deployment_group.map(&:stop_action).compact + else + [last_deployment&.stop_action].compact + end end end diff --git a/app/policies/environment_policy.rb b/app/policies/environment_policy.rb index e9e3517b3da7c7..72db6d31764905 100644 --- a/app/policies/environment_policy.rb +++ b/app/policies/environment_policy.rb @@ -4,12 +4,12 @@ class EnvironmentPolicy < BasePolicy delegate { @subject.project } condition(:stop_with_deployment_allowed) do - @subject.stop_action_available? && - can?(:create_deployment) && can?(:update_build, @subject.stop_action) + @subject.stop_actions_available? && + can?(:create_deployment) && can?(:update_build, @subject.stop_actions.last) end condition(:stop_with_update_allowed) do - !@subject.stop_action_available? && can?(:update_environment, @subject) + !@subject.stop_actions_available? && can?(:update_environment, @subject) end condition(:stopped) do diff --git a/app/serializers/environment_entity.rb b/app/serializers/environment_entity.rb index d484f60ed8fda6..634be365a9d448 100644 --- a/app/serializers/environment_entity.rb +++ b/app/serializers/environment_entity.rb @@ -18,7 +18,7 @@ class EnvironmentEntity < Grape::Entity expose :environment_type expose :name_without_type expose :last_deployment, using: DeploymentEntity - expose :stop_action_available?, as: :has_stop_action + expose :stop_actions_available?, as: :has_stop_action expose :rollout_status, if: -> (*) { can_read_deploy_board? }, using: RolloutStatusEntity expose :tier diff --git a/app/services/environments/stop_service.rb b/app/services/environments/stop_service.rb index 8e66155f400c07..9bc58ecb7a28c7 100644 --- a/app/services/environments/stop_service.rb +++ b/app/services/environments/stop_service.rb @@ -7,7 +7,7 @@ class StopService < BaseService def execute(environment) return unless can?(current_user, :stop_environment, environment) - environment.stop_with_action!(current_user) + environment.stop_with_actions!(current_user) end def execute_for_branch(branch_name) diff --git a/app/workers/environments/auto_stop_worker.rb b/app/workers/environments/auto_stop_worker.rb index 672a4f4121edd7..aee6e9775509ae 100644 --- a/app/workers/environments/auto_stop_worker.rb +++ b/app/workers/environments/auto_stop_worker.rb @@ -10,8 +10,10 @@ class AutoStopWorker def perform(environment_id, params = {}) Environment.find_by_id(environment_id).try do |environment| - user = environment.stop_action&.user - environment.stop_with_action!(user) + stop_actions = environment.stop_actions + + user = stop_actions.last&.user + environment.stop_with_actions!(user) end end end diff --git a/config/feature_flags/development/environment_multiple_stop_actions.yml b/config/feature_flags/development/environment_multiple_stop_actions.yml new file mode 100644 index 00000000000000..514d5e8cf521c4 --- /dev/null +++ b/config/feature_flags/development/environment_multiple_stop_actions.yml @@ -0,0 +1,8 @@ +--- +name: environment_multiple_stop_actions +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/84922 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/358911 +milestone: '14.10' +type: development +group: group::release +default_enabled: false diff --git a/lib/api/environments.rb b/lib/api/environments.rb index c032b80e39b7fa..19b48c1e3cfd1f 100644 --- a/lib/api/environments.rb +++ b/lib/api/environments.rb @@ -131,7 +131,7 @@ class Environments < ::API::Base environment = user_project.environments.find(params[:environment_id]) authorize! :stop_environment, environment - environment.stop_with_action!(current_user) + environment.stop_with_actions!(current_user) status 200 present environment, with: Entities::Environment, current_user: current_user diff --git a/spec/controllers/projects/environments_controller_spec.rb b/spec/controllers/projects/environments_controller_spec.rb index fdfc21887a60a5..f4cad5790a348e 100644 --- a/spec/controllers/projects/environments_controller_spec.rb +++ b/spec/controllers/projects/environments_controller_spec.rb @@ -254,38 +254,54 @@ end describe 'PATCH #stop' do + subject { patch :stop, params: environment_params(format: :json) } + context 'when env not available' do it 'returns 404' do allow_any_instance_of(Environment).to receive(:available?) { false } - patch :stop, params: environment_params(format: :json) + subject expect(response).to have_gitlab_http_status(:not_found) end end context 'when stop action' do - it 'returns action url' do + it 'returns action url for single stop action' do action = create(:ci_build, :manual) allow_any_instance_of(Environment) - .to receive_messages(available?: true, stop_with_action!: action) + .to receive_messages(available?: true, stop_with_actions!: [action]) - patch :stop, params: environment_params(format: :json) + subject expect(response).to have_gitlab_http_status(:ok) expect(json_response).to eq( { 'redirect_url' => project_job_url(project, action) }) end + + it 'returns environment url for multiple stop actions' do + actions = create_list(:ci_build, 2, :manual) + + allow_any_instance_of(Environment) + .to receive_messages(available?: true, stop_with_actions!: actions) + + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to eq( + { 'redirect_url' => + project_environment_url(project, environment) }) + end end context 'when no stop action' do it 'returns env url' do allow_any_instance_of(Environment) - .to receive_messages(available?: true, stop_with_action!: nil) + .to receive_messages(available?: true, stop_with_actions!: nil) - patch :stop, params: environment_params(format: :json) + subject expect(response).to have_gitlab_http_status(:ok) expect(json_response).to eq( diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index b20c91c53c1048..e34ae7cf684936 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -23,7 +23,6 @@ it { is_expected.to have_one(:upcoming_deployment) } it { is_expected.to have_one(:latest_opened_most_severe_alert) } - it { is_expected.to delegate_method(:stop_action).to(:last_deployment) } it { is_expected.to delegate_method(:manual_actions).to(:last_deployment) } it { is_expected.to validate_presence_of(:name) } @@ -459,8 +458,8 @@ end end - describe '#stop_action_available?' do - subject { environment.stop_action_available? } + describe '#stop_actions_available?' do + subject { environment.stop_actions_available? } context 'when no other actions' do it { is_expected.to be_falsey } @@ -499,10 +498,10 @@ end end - describe '#stop_with_action!' do + describe '#stop_with_actions!' do let(:user) { create(:user) } - subject { environment.stop_with_action!(user) } + subject { environment.stop_with_actions!(user) } before do expect(environment).to receive(:available?).and_call_original @@ -515,9 +514,10 @@ end it do - subject + actions = subject expect(environment).to be_stopped + expect(actions).to match_array([]) end end @@ -536,18 +536,18 @@ context 'when matching action is defined' do let(:pipeline) { create(:ci_pipeline, project: project) } - let(:build) { create(:ci_build, pipeline: pipeline) } + let(:build_a) { create(:ci_build, pipeline: pipeline) } - let!(:deployment) do + before do create(:deployment, :success, - environment: environment, - deployable: build, - on_stop: 'close_app') + environment: environment, + deployable: build_a, + on_stop: 'close_app_a') end context 'when user is not allowed to stop environment' do let!(:close_action) do - create(:ci_build, :manual, pipeline: pipeline, name: 'close_app') + create(:ci_build, :manual, pipeline: pipeline, name: 'close_app_a') end it 'raises an exception' do @@ -565,36 +565,39 @@ context 'when action did not yet finish' do let!(:close_action) do - create(:ci_build, :manual, pipeline: pipeline, name: 'close_app') + create(:ci_build, :manual, pipeline: pipeline, name: 'close_app_a') end it 'returns the same action' do - expect(subject).to eq(close_action) - expect(subject.user).to eq(user) + action = subject.first + expect(action).to eq(close_action) + expect(action.user).to eq(user) end end context 'if action did finish' do let!(:close_action) do create(:ci_build, :manual, :success, - pipeline: pipeline, name: 'close_app') + pipeline: pipeline, name: 'close_app_a') end it 'returns a new action of the same type' do - expect(subject).to be_persisted - expect(subject.name).to eq(close_action.name) - expect(subject.user).to eq(user) + action = subject.first + + expect(action).to be_persisted + expect(action.name).to eq(close_action.name) + expect(action.user).to eq(user) end end context 'close action does not raise ActiveRecord::StaleObjectError' do let!(:close_action) do - create(:ci_build, :manual, pipeline: pipeline, name: 'close_app') + create(:ci_build, :manual, pipeline: pipeline, name: 'close_app_a') end before do # preload the build - environment.stop_action + environment.stop_actions # Update record as the other process. This makes `environment.stop_action` stale. close_action.drop! @@ -613,6 +616,147 @@ end end end + + context 'when there are more then one stop action for the environment' do + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:build_a) { create(:ci_build, pipeline: pipeline) } + let(:build_b) { create(:ci_build, pipeline: pipeline) } + + let!(:close_actions) do + [ + create(:ci_build, :manual, pipeline: pipeline, name: 'close_app_a'), + create(:ci_build, :manual, pipeline: pipeline, name: 'close_app_b') + ] + end + + before do + project.add_developer(user) + + create(:deployment, :success, + environment: environment, + deployable: build_a, + finished_at: 5.minutes.ago, + on_stop: 'close_app_a') + + create(:deployment, :success, + environment: environment, + deployable: build_b, + finished_at: 1.second.ago, + on_stop: 'close_app_b') + end + + it 'returns the same actions' do + actions = subject + + expect(actions.count).to eq(close_actions.count) + expect(actions.pluck(:id)).to match_array(close_actions.pluck(:id)) + expect(actions.pluck(:user)).to match_array(close_actions.pluck(:user)) + end + + context 'when there are failed deployment jobs' do + before do + create(:ci_build, pipeline: pipeline, name: 'close_app_c') + + create(:deployment, :failed, + environment: environment, + deployable: create(:ci_build, pipeline: pipeline), + on_stop: 'close_app_c') + end + + it 'returns only stop actions from successful deployment jobs' do + actions = subject + + expect(actions).to match_array(close_actions) + expect(actions.count).to eq(environment.successful_deployments.count) + end + end + + context 'when the feature is disabled' do + before do + stub_feature_flags(environment_multiple_stop_actions: false) + end + + it 'returns the last deployment job stop action' do + stop_actions = subject + + expect(stop_actions.first).to eq(close_actions[1]) + expect(stop_actions.count).to eq(1) + end + end + end + end + + describe '#stop_actions' do + subject { environment.stop_actions } + + context 'when there are no deployments and builds' do + it 'returns empty array' do + is_expected.to match_array([]) + end + end + + context 'when there are multiple deployments with actions' do + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:ci_build_a) { create(:ci_build, project: project, pipeline: pipeline) } + let(:ci_build_b) { create(:ci_build, project: project, pipeline: pipeline) } + let!(:ci_build_c) { create(:ci_build, :manual, project: project, pipeline: pipeline, name: 'close_app_a') } + let!(:ci_build_d) { create(:ci_build, :manual, project: project, pipeline: pipeline, name: 'close_app_b') } + + let!(:deployment_a) do + create(:deployment, + :success, project: project, environment: environment, deployable: ci_build_a, on_stop: 'close_app_a') + end + + let!(:deployment_b) do + create(:deployment, + :success, project: project, environment: environment, deployable: ci_build_b, on_stop: 'close_app_b') + end + + before do + # Create failed deployment without stop_action. + build = create(:ci_build, project: project, pipeline: pipeline) + create(:deployment, :failed, project: project, environment: environment, deployable: build) + end + + it 'returns only the stop actions' do + expect(subject.pluck(:id)).to contain_exactly(ci_build_c.id, ci_build_d.id) + end + end + end + + describe '#last_deployment_group' do + subject { environment.last_deployment_group } + + context 'when there are no deployments and builds' do + it do + is_expected.to eq(Deployment.none) + end + end + + context 'when there are deployments for multiple pipelines' do + let(:pipeline_a) { create(:ci_pipeline, project: project) } + let(:pipeline_b) { create(:ci_pipeline, project: project) } + let(:ci_build_a) { create(:ci_build, project: project, pipeline: pipeline_a) } + let(:ci_build_b) { create(:ci_build, project: project, pipeline: pipeline_b) } + let(:ci_build_c) { create(:ci_build, project: project, pipeline: pipeline_a) } + let(:ci_build_d) { create(:ci_build, project: project, pipeline: pipeline_a) } + + # Successful deployments for pipeline_a + let!(:deployment_a) { create(:deployment, :success, project: project, environment: environment, deployable: ci_build_a) } + let!(:deployment_b) { create(:deployment, :success, project: project, environment: environment, deployable: ci_build_c) } + + before do + # Failed deployment for pipeline_a + create(:deployment, :failed, project: project, environment: environment, deployable: ci_build_d) + + # Failed deployment for pipeline_b + create(:deployment, :failed, project: project, environment: environment, deployable: ci_build_b) + end + + it 'returns the successful deployment jobs for the last deployment pipeline' do + expect(subject.pluck(:id)).to contain_exactly(deployment_a.id, deployment_b.id) + end + end end describe 'recently_updated_on_branch?' do @@ -772,6 +916,26 @@ end end + describe '#last_deployment_pipeline' do + subject { environment.last_deployment_pipeline } + + let(:pipeline_a) { create(:ci_pipeline, project: project) } + let(:pipeline_b) { create(:ci_pipeline, project: project) } + let(:ci_build_a) { create(:ci_build, project: project, pipeline: pipeline_a) } + let(:ci_build_b) { create(:ci_build, project: project, pipeline: pipeline_b) } + + before do + create(:deployment, :success, project: project, environment: environment, deployable: ci_build_a) + create(:deployment, :failed, project: project, environment: environment, deployable: ci_build_b) + end + + it 'does not join across databases' do + with_cross_joins_prevented do + expect(subject.id).to eq(pipeline_a.id) + end + end + end + describe '#last_visible_deployment' do subject { environment.last_visible_deployment } diff --git a/spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb b/spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb index 8f63192e7095ef..fcd52cdf7fa483 100644 --- a/spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb +++ b/spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb @@ -8,7 +8,11 @@ create_environment_with_associations(project) create_environment_with_associations(project) - expect { serialize(grouping: true) }.not_to exceed_query_limit(control.count) + # Fix N+1 queries introduced by multi stop_actions for environment. + # Tracked in https://gitlab.com/gitlab-org/gitlab/-/issues/358780 + relax_count = 14 + + expect { serialize(grouping: true) }.not_to exceed_query_limit(control.count + relax_count) end it 'avoids N+1 database queries without grouping', :request_store do @@ -19,7 +23,11 @@ create_environment_with_associations(project) create_environment_with_associations(project) - expect { serialize(grouping: false) }.not_to exceed_query_limit(control.count) + # Fix N+1 queries introduced by multi stop_actions for environment. + # Tracked in https://gitlab.com/gitlab-org/gitlab/-/issues/358780 + relax_count = 14 + + expect { serialize(grouping: false) }.not_to exceed_query_limit(control.count + relax_count) end it 'does not preload for environments that does not exist in the page', :request_store do -- GitLab