diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2019-10-22 03:06:05 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2019-10-22 03:06:05 +0300 |
commit | 8dfb94309c3e84937189f73a4149d654e20332e9 (patch) | |
tree | 1543a7b74b2e5c683a39fd93b09afc578f758c2b | |
parent | 170f0bdcdef9c9b226abfe0a50d6687c65e8d613 (diff) |
Add latest changes from gitlab-org/gitlab@master
19 files changed, 150 insertions, 5 deletions
@@ -469,3 +469,5 @@ gem 'gitlab-net-dns', '~> 0.9.1' # Countries list gem 'countries', '~> 3.0' + +gem 'retriable', '~> 3.1.2' diff --git a/Gemfile.lock b/Gemfile.lock index 902a8281101..fab1e5347dc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1276,6 +1276,7 @@ DEPENDENCIES redis-rails (~> 5.0.2) request_store (~> 1.3) responders (~> 2.0) + retriable (~> 3.1.2) rouge (~> 3.11.0) rqrcode-rails3 (~> 0.1.7) rspec-parameterized diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue index 339e154affc..57be97855e3 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue @@ -65,9 +65,13 @@ export default { simplePoll(this.checkRebaseStatus); }) .catch(error => { - this.rebasingError = error.merge_error; this.isMakingRequest = false; - Flash(__('Something went wrong. Please try again.')); + + if (error.response && error.response.data && error.response.data.merge_error) { + this.rebasingError = error.response.data.merge_error; + } else { + Flash(__('Something went wrong. Please try again.')); + } }); }, checkRebaseStatus(continuePolling, stopPolling) { diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index e6032323c13..8d388151dbc 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -226,6 +226,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo @merge_request.rebase_async(current_user.id) head :ok + rescue MergeRequest::RebaseLockTimeout => e + render json: { merge_error: e.message }, status: :conflict end def discussions diff --git a/app/helpers/environments_helper.rb b/app/helpers/environments_helper.rb index c642a64ad61..f57d0fa19d4 100644 --- a/app/helpers/environments_helper.rb +++ b/app/helpers/environments_helper.rb @@ -34,6 +34,7 @@ module EnvironmentsHelper "project-path" => project_path(project), "tags-path" => project_tags_path(project), "has-metrics" => "#{environment.has_metrics?}", + "prometheus-status" => "#{environment.prometheus_status}", "external-dashboard-url" => project.metrics_setting_external_dashboard_url } end diff --git a/app/models/environment.rb b/app/models/environment.rb index af0c219d9a0..b426941d8af 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -188,6 +188,10 @@ class Environment < ApplicationRecord prometheus_adapter.query(:environment, self) if has_metrics? end + def prometheus_status + deployment_platform&.cluster&.application_prometheus&.status_name + end + def additional_metrics(*args) return unless has_metrics? diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 6ef84c5f59b..b1c7e778743 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -220,6 +220,10 @@ class MergeRequest < ApplicationRecord alias_attribute :auto_merge_enabled, :merge_when_pipeline_succeeds alias_method :issuing_parent, :target_project + RebaseLockTimeout = Class.new(StandardError) + + REBASE_LOCK_MESSAGE = _("Failed to enqueue the rebase operation, possibly due to a long-lived transaction. Try again later.") + def self.reference_prefix '!' end @@ -409,9 +413,7 @@ class MergeRequest < ApplicationRecord # Set off a rebase asynchronously, atomically updating the `rebase_jid` of # the MR so that the status of the operation can be tracked. def rebase_async(user_id) - transaction do - lock! - + with_rebase_lock do raise ActiveRecord::StaleObjectError if !open? || rebase_in_progress? # Although there is a race between setting rebase_jid here and clearing it @@ -1468,6 +1470,30 @@ class MergeRequest < ApplicationRecord private + def with_rebase_lock + if Feature.enabled?(:merge_request_rebase_nowait_lock, default_enabled: true) + with_retried_nowait_lock { yield } + else + with_lock(true) { yield } + end + end + + # If the merge request is idle in transaction or has a SELECT FOR + # UPDATE, we don't want to block indefinitely or this could cause a + # queue of SELECT FOR UPDATE calls. Instead, try to get the lock for + # 5 s before raising an error to the user. + def with_retried_nowait_lock + # Try at most 0.25 + (1.5 * .25) + (1.5^2 * .25) ... (1.5^5 * .25) = 5.2 s to get the lock + Retriable.retriable(on: ActiveRecord::LockWaitTimeout, tries: 6, base_interval: 0.25) do + with_lock('FOR UPDATE NOWAIT') do + yield + end + end + rescue ActiveRecord::LockWaitTimeout => e + Gitlab::Sentry.track_acceptable_exception(e) + raise RebaseLockTimeout, REBASE_LOCK_MESSAGE + end + def source_project_variables Gitlab::Ci::Variables::Collection.new.tap do |variables| break variables unless source_project diff --git a/changelogs/unreleased/sh-time-limit-merge-rebase-lock.yml b/changelogs/unreleased/sh-time-limit-merge-rebase-lock.yml new file mode 100644 index 00000000000..c96db4d2ea9 --- /dev/null +++ b/changelogs/unreleased/sh-time-limit-merge-rebase-lock.yml @@ -0,0 +1,5 @@ +--- +title: Time limit the database lock when rebasing a merge request +merge_request: 18481 +author: +type: fixed diff --git a/changelogs/unreleased/show-prometheus-is-updating.yml b/changelogs/unreleased/show-prometheus-is-updating.yml new file mode 100644 index 00000000000..a62c0eb26ac --- /dev/null +++ b/changelogs/unreleased/show-prometheus-is-updating.yml @@ -0,0 +1,5 @@ +--- +title: Expose prometheus status to monitor dashboard +merge_request: 18289 +author: +type: fixed diff --git a/doc/user/group/epics/img/epic_view_roadmap_v12.3.png b/doc/user/group/epics/img/epic_view_roadmap_v12.3.png Binary files differindex a17c56c618b..a17c56c618b 100755..100644 --- a/doc/user/group/epics/img/epic_view_roadmap_v12.3.png +++ b/doc/user/group/epics/img/epic_view_roadmap_v12.3.png diff --git a/doc/user/group/epics/img/epic_view_v12.3.png b/doc/user/group/epics/img/epic_view_v12.3.png Binary files differindex 79758cf3d52..79758cf3d52 100755..100644 --- a/doc/user/group/epics/img/epic_view_v12.3.png +++ b/doc/user/group/epics/img/epic_view_v12.3.png diff --git a/doc/user/group/epics/img/epics_list_view_v12.3.png b/doc/user/group/epics/img/epics_list_view_v12.3.png Binary files differindex c6817a503e7..c6817a503e7 100755..100644 --- a/doc/user/group/epics/img/epics_list_view_v12.3.png +++ b/doc/user/group/epics/img/epics_list_view_v12.3.png diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 1436238c5cf..a18380586a7 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -455,6 +455,8 @@ module API status :accepted present rebase_in_progress: merge_request.rebase_in_progress? + rescue ::MergeRequest::RebaseLockTimeout => e + render_api_error!(e.message, 409) end desc 'List issues that will be closed on merge' do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6f66b51951d..d97a149805b 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -6846,6 +6846,9 @@ msgstr "" msgid "Failed to deploy to" msgstr "" +msgid "Failed to enqueue the rebase operation, possibly due to a long-lived transaction. Try again later." +msgstr "" + msgid "Failed to get ref." msgstr "" diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 827b34b8850..74834e86467 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -1409,6 +1409,33 @@ describe Projects::MergeRequestsController do end end + context 'with SELECT FOR UPDATE lock' do + before do + stub_feature_flags(merge_request_rebase_nowait_lock: false) + end + + it 'executes rebase' do + allow_any_instance_of(MergeRequest).to receive(:with_lock).with(true).and_call_original + expect(RebaseWorker).to receive(:perform_async) + + post_rebase + + expect(response.status).to eq(200) + end + end + + context 'with NOWAIT lock' do + it 'returns a 409' do + allow_any_instance_of(MergeRequest).to receive(:with_lock).with('FOR UPDATE NOWAIT').and_raise(ActiveRecord::LockWaitTimeout) + expect(RebaseWorker).not_to receive(:perform_async) + + post_rebase + + expect(response.status).to eq(409) + expect(json_response['merge_error']).to eq(MergeRequest::REBASE_LOCK_MESSAGE) + end + end + context 'with a forked project' do let(:forked_project) { fork_project(project, fork_owner, repository: true) } let(:fork_owner) { create(:user) } diff --git a/spec/helpers/environments_helper_spec.rb b/spec/helpers/environments_helper_spec.rb index 2b8bf9319fc..a50c8e9bf8e 100644 --- a/spec/helpers/environments_helper_spec.rb +++ b/spec/helpers/environments_helper_spec.rb @@ -32,6 +32,7 @@ describe EnvironmentsHelper do 'project-path' => project_path(project), 'tags-path' => project_tags_path(project), 'has-metrics' => "#{environment.has_metrics?}", + 'prometheus-status' => "#{environment.prometheus_status}", 'external-dashboard-url' => nil ) end diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 786f3b832c4..85a8f35393b 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -727,6 +727,51 @@ describe Environment, :use_clean_rails_memory_store_caching do end end + describe '#prometheus_status' do + context 'when a cluster is present' do + context 'when a deployment platform is present' do + let(:cluster) { create(:cluster, :provided_by_user, :project) } + let(:environment) { create(:environment, project: cluster.project) } + + subject { environment.prometheus_status } + + context 'when the prometheus application status is :updating' do + let!(:prometheus) { create(:clusters_applications_prometheus, :updating, cluster: cluster) } + + it { is_expected.to eq(:updating) } + end + + context 'when the prometheus application state is :updated' do + let!(:prometheus) { create(:clusters_applications_prometheus, :updated, cluster: cluster) } + + it { is_expected.to eq(:updated) } + end + + context 'when the prometheus application is not installed' do + it { is_expected.to be_nil } + end + end + + context 'when a deployment platform is not present' do + let(:cluster) { create(:cluster, :project) } + let(:environment) { create(:environment, project: cluster.project) } + + subject { environment.prometheus_status } + + it { is_expected.to be_nil } + end + end + + context 'when a cluster is not present' do + let(:project) { create(:project, :stubbed_repository) } + let(:environment) { create(:environment, project: project) } + + subject { environment.prometheus_status } + + it { is_expected.to be_nil } + end + end + describe '#additional_metrics' do let(:project) { create(:prometheus_project) } let(:metric_params) { [] } diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index b8d09f03fe6..e2a0acf85f6 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -2138,6 +2138,13 @@ describe MergeRequest do expect { execute }.to raise_error(ActiveRecord::StaleObjectError) end + + it "raises ActiveRecord::LockWaitTimeout after 6 tries" do + expect(merge_request).to receive(:with_lock).exactly(6).times.and_raise(ActiveRecord::LockWaitTimeout) + expect(RebaseWorker).not_to receive(:perform_async) + + expect { execute }.to raise_error(MergeRequest::RebaseLockTimeout) + end end describe '#mergeable?' do diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 8179da2f97c..e9bf8e7adc7 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -2120,6 +2120,16 @@ describe API::MergeRequests do expect(response).to have_gitlab_http_status(409) end + + it "returns 409 if rebase can't lock the row" do + allow_any_instance_of(MergeRequest).to receive(:with_lock).and_raise(ActiveRecord::LockWaitTimeout) + expect(RebaseWorker).not_to receive(:perform_async) + + put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/rebase", user) + + expect(response).to have_gitlab_http_status(409) + expect(json_response['message']).to eq(MergeRequest::REBASE_LOCK_MESSAGE) + end end describe 'Time tracking' do |