diff options
28 files changed, 147 insertions, 36 deletions
diff --git a/app/controllers/projects/clusters/gcp_controller.rb b/app/controllers/projects/clusters/gcp_controller.rb index b64f7a2a6bd..d3b9d8a9bbc 100644 --- a/app/controllers/projects/clusters/gcp_controller.rb +++ b/app/controllers/projects/clusters/gcp_controller.rb @@ -39,6 +39,7 @@ class Projects::Clusters::GcpController < Projects::ApplicationController params.require(:cluster).permit( :enabled, :name, + :environment_scope, provider_gcp_attributes: [ :gcp_project_id, :zone, diff --git a/app/controllers/projects/clusters/user_controller.rb b/app/controllers/projects/clusters/user_controller.rb index d7678512073..d0db64b2fa9 100644 --- a/app/controllers/projects/clusters/user_controller.rb +++ b/app/controllers/projects/clusters/user_controller.rb @@ -26,6 +26,7 @@ class Projects::Clusters::UserController < Projects::ApplicationController params.require(:cluster).permit( :enabled, :name, + :environment_scope, platform_kubernetes_attributes: [ :namespace, :api_url, diff --git a/app/controllers/projects/clusters_controller.rb b/app/controllers/projects/clusters_controller.rb index 0907daacbc3..05629b89d25 100644 --- a/app/controllers/projects/clusters_controller.rb +++ b/app/controllers/projects/clusters_controller.rb @@ -90,6 +90,7 @@ class Projects::ClustersController < Projects::ApplicationController if cluster.managed? params.require(:cluster).permit( :enabled, + :environment_scope, platform_kubernetes_attributes: [ :namespace ] @@ -98,6 +99,7 @@ class Projects::ClustersController < Projects::ApplicationController params.require(:cluster).permit( :enabled, :name, + :environment_scope, platform_kubernetes_attributes: [ :api_url, :token, diff --git a/app/helpers/clusters_helper.rb b/app/helpers/clusters_helper.rb new file mode 100644 index 00000000000..7e4eb06b99d --- /dev/null +++ b/app/helpers/clusters_helper.rb @@ -0,0 +1,5 @@ +module ClustersHelper + def has_multiple_clusters?(project) + false + end +end diff --git a/app/models/project.rb b/app/models/project.rb index 3440c01b356..f9c640300ff 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -951,7 +951,9 @@ class Project < ActiveRecord::Base def send_move_instructions(old_path_with_namespace) # New project path needs to be committed to the DB or notification will # retrieve stale information - run_after_commit { NotificationService.new.project_was_moved(self, old_path_with_namespace) } + run_after_commit do + NotificationService.new.project_was_moved(self, old_path_with_namespace) + end end def owner @@ -963,15 +965,19 @@ class Project < ActiveRecord::Base end def execute_hooks(data, hooks_scope = :push_hooks) - hooks.public_send(hooks_scope).each do |hook| # rubocop:disable GitlabSecurity/PublicSend - hook.async_execute(data, hooks_scope.to_s) + run_after_commit_or_now do + hooks.public_send(hooks_scope).each do |hook| # rubocop:disable GitlabSecurity/PublicSend + hook.async_execute(data, hooks_scope.to_s) + end end end def execute_services(data, hooks_scope = :push_hooks) # Call only service hooks that are active for this scope - services.public_send(hooks_scope).each do |service| # rubocop:disable GitlabSecurity/PublicSend - service.async_execute(data) + run_after_commit_or_now do + services.public_send(hooks_scope).each do |service| # rubocop:disable GitlabSecurity/PublicSend + service.async_execute(data) + end end end diff --git a/app/services/projects/unlink_fork_service.rb b/app/services/projects/unlink_fork_service.rb index c499f384426..842fe4e09c4 100644 --- a/app/services/projects/unlink_fork_service.rb +++ b/app/services/projects/unlink_fork_service.rb @@ -5,7 +5,7 @@ module Projects if fork_source = @project.fork_source fork_source.lfs_objects.find_each do |lfs_object| - lfs_object.projects << @project + lfs_object.projects << @project unless lfs_object.projects.include?(@project) end refresh_forks_count(fork_source) diff --git a/app/views/projects/clusters/gcp/_form.html.haml b/app/views/projects/clusters/gcp/_form.html.haml index 0f6bae97571..e384b60d8d9 100644 --- a/app/views/projects/clusters/gcp/_form.html.haml +++ b/app/views/projects/clusters/gcp/_form.html.haml @@ -7,6 +7,9 @@ .form-group = field.label :name, s_('ClusterIntegration|Cluster name') = field.text_field :name, class: 'form-control', placeholder: s_('ClusterIntegration|Cluster name') + .form-group + = field.label :environment_scope, s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control', readonly: !has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') = field.fields_for :provider_gcp, @cluster.provider_gcp do |provider_gcp_field| .form-group diff --git a/app/views/projects/clusters/gcp/_show.html.haml b/app/views/projects/clusters/gcp/_show.html.haml index 3fa9f69708a..bde85aed341 100644 --- a/app/views/projects/clusters/gcp/_show.html.haml +++ b/app/views/projects/clusters/gcp/_show.html.haml @@ -8,6 +8,11 @@ = form_for @cluster, url: namespace_project_cluster_path(@project.namespace, @project, @cluster), as: :cluster do |field| = form_errors(@cluster) + + .form-group + = field.label :environment_scope, s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control js-select-on-focus', readonly: !has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') + = field.fields_for :platform_kubernetes, @cluster.platform_kubernetes do |platform_kubernetes_field| .form-group = platform_kubernetes_field.label :api_url, s_('ClusterIntegration|API URL') diff --git a/app/views/projects/clusters/user/_form.html.haml b/app/views/projects/clusters/user/_form.html.haml index 4a9bd5186c6..babfca0c567 100644 --- a/app/views/projects/clusters/user/_form.html.haml +++ b/app/views/projects/clusters/user/_form.html.haml @@ -3,6 +3,9 @@ .form-group = field.label :name, s_('ClusterIntegration|Cluster name') = field.text_field :name, class: 'form-control', placeholder: s_('ClusterIntegration|Cluster name') + .form-group + = field.label :environment_scope, s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control', readonly: !has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') = field.fields_for :platform_kubernetes, @cluster.platform_kubernetes do |platform_kubernetes_field| .form-group diff --git a/app/views/projects/clusters/user/_show.html.haml b/app/views/projects/clusters/user/_show.html.haml index 5931e0b7f17..89595bca007 100644 --- a/app/views/projects/clusters/user/_show.html.haml +++ b/app/views/projects/clusters/user/_show.html.haml @@ -4,6 +4,10 @@ = field.label :name, s_('ClusterIntegration|Cluster name') = field.text_field :name, class: 'form-control', placeholder: s_('ClusterIntegration|Cluster name') + .form-group + = field.label :environment_scope, s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control js-select-on-focus', readonly: !has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') + = field.fields_for :platform_kubernetes, @cluster.platform_kubernetes do |platform_kubernetes_field| .form-group = platform_kubernetes_field.label :api_url, s_('ClusterIntegration|API URL') diff --git a/changelogs/unreleased/bvl-fix-unlinking-with-lfs-objects.yml b/changelogs/unreleased/bvl-fix-unlinking-with-lfs-objects.yml new file mode 100644 index 00000000000..058d686e74c --- /dev/null +++ b/changelogs/unreleased/bvl-fix-unlinking-with-lfs-objects.yml @@ -0,0 +1,6 @@ +--- +title: Don't link LFS objects to a project when unlinking forks when they were already + linked +merge_request: 16006 +author: +type: fixed diff --git a/changelogs/unreleased/dm-issue-move-transaction-error.yml b/changelogs/unreleased/dm-issue-move-transaction-error.yml new file mode 100644 index 00000000000..0892169894a --- /dev/null +++ b/changelogs/unreleased/dm-issue-move-transaction-error.yml @@ -0,0 +1,5 @@ +--- +title: Execute project hooks and services after commit when moving an issue +merge_request: +author: +type: fixed diff --git a/changelogs/unreleased/pawel-reduce_cardinality_of_prometheus_metrics.yml b/changelogs/unreleased/pawel-reduce_cardinality_of_prometheus_metrics.yml new file mode 100644 index 00000000000..0cee0b634d6 --- /dev/null +++ b/changelogs/unreleased/pawel-reduce_cardinality_of_prometheus_metrics.yml @@ -0,0 +1,5 @@ +--- +title: Reduce the number of buckets in gitlab_cache_operation_duration_seconds metric +merge_request: 15881 +author: +type: changed diff --git a/changelogs/unreleased/sh-handle-anonymous-clones-project-moved.yml b/changelogs/unreleased/sh-handle-anonymous-clones-project-moved.yml new file mode 100644 index 00000000000..a0860871152 --- /dev/null +++ b/changelogs/unreleased/sh-handle-anonymous-clones-project-moved.yml @@ -0,0 +1,5 @@ +--- +title: Fix Error 500s with anonymous clones for a project that has moved +merge_request: +author: +type: fixed diff --git a/config/initializers/forbid_sidekiq_in_transactions.rb b/config/initializers/forbid_sidekiq_in_transactions.rb index bedd57ede04..cb611aa21df 100644 --- a/config/initializers/forbid_sidekiq_in_transactions.rb +++ b/config/initializers/forbid_sidekiq_in_transactions.rb @@ -1,5 +1,7 @@ module Sidekiq module Worker + EnqueueFromTransactionError = Class.new(StandardError) + mattr_accessor :skip_transaction_check self.skip_transaction_check = false @@ -12,11 +14,11 @@ module Sidekiq end module ClassMethods - module NoSchedulingFromTransactions + module NoEnqueueingFromTransactions %i(perform_async perform_at perform_in).each do |name| define_method(name) do |*args| if !Sidekiq::Worker.skip_transaction_check && AfterCommitQueue.inside_transaction? - raise <<-MSG.strip_heredoc + raise Sidekiq::Worker::EnqueueFromTransactionError, <<~MSG `#{self}.#{name}` cannot be called inside a transaction as this can lead to race conditions when the worker runs before the transaction is committed and tries to access a model that has not been saved yet. @@ -30,7 +32,7 @@ module Sidekiq end end - prepend NoSchedulingFromTransactions + prepend NoEnqueueingFromTransactions end end end diff --git a/lib/after_commit_queue.rb b/lib/after_commit_queue.rb index db63c5038ae..a4d8507960e 100644 --- a/lib/after_commit_queue.rb +++ b/lib/after_commit_queue.rb @@ -14,7 +14,15 @@ module AfterCommitQueue def run_after_commit_or_now(&block) if AfterCommitQueue.inside_transaction? - run_after_commit(&block) + if ActiveRecord::Base.connection.current_transaction.records.include?(self) + run_after_commit(&block) + else + # If the current transaction does not include this record, we can run + # the block now, even if it queues a Sidekiq job. + Sidekiq::Worker.skipping_transaction_check do + instance_eval(&block) + end + end else instance_eval(&block) end diff --git a/lib/gitlab/checks/project_moved.rb b/lib/gitlab/checks/project_moved.rb index 3a1c0a3455e..dfb2f4d4054 100644 --- a/lib/gitlab/checks/project_moved.rb +++ b/lib/gitlab/checks/project_moved.rb @@ -21,6 +21,10 @@ module Gitlab end def add_redirect_message + # Don't bother with sending a redirect message for anonymous clones + # because they never see it via the `/internal/post_receive` endpoint + return unless user.present? && project.present? + Gitlab::Redis::SharedState.with do |redis| key = self.class.redirect_message_key(user.id, project.id) redis.setex(key, 5.minutes, redirect_message) diff --git a/lib/gitlab/metrics/influx_db.rb b/lib/gitlab/metrics/influx_db.rb index bdf7910b7c7..153e236d018 100644 --- a/lib/gitlab/metrics/influx_db.rb +++ b/lib/gitlab/metrics/influx_db.rb @@ -38,6 +38,7 @@ module Gitlab # This is memoized since this method is called for every instrumented # method. Loading data from an external cache on every method call slows # things down too much. + # in milliseconds @method_call_threshold ||= settings[:method_call_threshold] end diff --git a/lib/gitlab/metrics/method_call.rb b/lib/gitlab/metrics/method_call.rb index 28444c2afcc..c2f9db56824 100644 --- a/lib/gitlab/metrics/method_call.rb +++ b/lib/gitlab/metrics/method_call.rb @@ -35,8 +35,8 @@ module Gitlab @transaction = transaction @name = name @labels = { module: @module_name, method: @method_name } - @real_time = 0 - @cpu_time = 0 + @real_time = 0.0 + @cpu_time = 0.0 @call_count = 0 end @@ -54,7 +54,7 @@ module Gitlab @call_count += 1 if call_measurement_enabled? && above_threshold? - self.class.call_duration_histogram.observe(@transaction.labels.merge(labels), real_time / 1000.0) + self.class.call_duration_histogram.observe(@transaction.labels.merge(labels), real_time) end retval @@ -65,8 +65,8 @@ module Gitlab Metric.new( Instrumentation.series, { - duration: real_time, - cpu_duration: cpu_time, + duration: real_time.in_milliseconds.to_i, + cpu_duration: cpu_time.in_milliseconds.to_i, call_count: call_count }, method: @name @@ -76,7 +76,7 @@ module Gitlab # Returns true if the total runtime of this method exceeds the method call # threshold. def above_threshold? - real_time >= Metrics.method_call_threshold + real_time.in_milliseconds >= Metrics.method_call_threshold end def call_measurement_enabled? diff --git a/lib/gitlab/metrics/samplers/ruby_sampler.rb b/lib/gitlab/metrics/samplers/ruby_sampler.rb index b68800417a2..4e1ea62351f 100644 --- a/lib/gitlab/metrics/samplers/ruby_sampler.rb +++ b/lib/gitlab/metrics/samplers/ruby_sampler.rb @@ -52,7 +52,7 @@ module Gitlab metrics[:memory_usage].set(labels, System.memory_usage) metrics[:file_descriptors].set(labels, System.file_descriptor_count) - metrics[:sampler_duration].observe(labels.merge(worker_label), (System.monotonic_time - start_time) / 1000.0) + metrics[:sampler_duration].observe(labels.merge(worker_label), System.monotonic_time - start_time) ensure GC::Profiler.clear end diff --git a/lib/gitlab/metrics/subscribers/rails_cache.rb b/lib/gitlab/metrics/subscribers/rails_cache.rb index efd3c9daf79..250897a79c2 100644 --- a/lib/gitlab/metrics/subscribers/rails_cache.rb +++ b/lib/gitlab/metrics/subscribers/rails_cache.rb @@ -66,7 +66,7 @@ module Gitlab :gitlab_cache_operation_duration_seconds, 'Cache access time', Transaction::BASE_LABELS.merge({ action: nil }), - [0.001, 0.002, 0.005, 0.01, 0.02, 0.05, 0.1, 0.500, 2.0, 10.0] + [0.001, 0.01, 0.1, 1, 10] ) end diff --git a/lib/gitlab/metrics/system.rb b/lib/gitlab/metrics/system.rb index c2cbd3c16a1..e60e245cf89 100644 --- a/lib/gitlab/metrics/system.rb +++ b/lib/gitlab/metrics/system.rb @@ -35,27 +35,27 @@ module Gitlab if Process.const_defined?(:CLOCK_THREAD_CPUTIME_ID) def self.cpu_time Process - .clock_gettime(Process::CLOCK_THREAD_CPUTIME_ID, :millisecond) + .clock_gettime(Process::CLOCK_THREAD_CPUTIME_ID, :float_second) end else def self.cpu_time Process - .clock_gettime(Process::CLOCK_PROCESS_CPUTIME_ID, :millisecond) + .clock_gettime(Process::CLOCK_PROCESS_CPUTIME_ID, :float_second) end end # Returns the current real time in a given precision. # - # Returns the time as a Fixnum. - def self.real_time(precision = :millisecond) + # Returns the time as a Float for precision = :float_second. + def self.real_time(precision = :float_second) Process.clock_gettime(Process::CLOCK_REALTIME, precision) end - # Returns the current monotonic clock time in a given precision. + # Returns the current monotonic clock time as seconds with microseconds precision. # - # Returns the time as a Fixnum. - def self.monotonic_time(precision = :millisecond) - Process.clock_gettime(Process::CLOCK_MONOTONIC, precision) + # Returns the time as a Float. + def self.monotonic_time + Process.clock_gettime(Process::CLOCK_MONOTONIC, :float_second) end end end diff --git a/lib/gitlab/metrics/transaction.rb b/lib/gitlab/metrics/transaction.rb index ee3afc5ffdb..e7975c023a9 100644 --- a/lib/gitlab/metrics/transaction.rb +++ b/lib/gitlab/metrics/transaction.rb @@ -35,6 +35,10 @@ module Gitlab @finished_at ? (@finished_at - @started_at) : 0.0 end + def duration_milliseconds + duration.in_milliseconds.to_i + end + def allocated_memory @memory_after - @memory_before end @@ -50,7 +54,7 @@ module Gitlab @memory_after = System.memory_usage @finished_at = System.monotonic_time - self.class.metric_transaction_duration_seconds.observe(labels, duration * 1000) + self.class.metric_transaction_duration_seconds.observe(labels, duration) self.class.metric_transaction_allocated_memory_bytes.observe(labels, allocated_memory * 1024.0) Thread.current[THREAD_KEY] = nil @@ -97,7 +101,7 @@ module Gitlab end def track_self - values = { duration: duration, allocated_memory: allocated_memory } + values = { duration: duration_milliseconds, allocated_memory: allocated_memory } @values.each do |name, value| values[name] = value diff --git a/spec/lib/gitlab/checks/project_moved_spec.rb b/spec/lib/gitlab/checks/project_moved_spec.rb index fa1575e2177..f90c2d6aded 100644 --- a/spec/lib/gitlab/checks/project_moved_spec.rb +++ b/spec/lib/gitlab/checks/project_moved_spec.rb @@ -35,6 +35,12 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do project_moved = described_class.new(project, user, 'foo/bar', 'http') expect(project_moved.add_redirect_message).to eq("OK") end + + it 'should handle anonymous clones' do + project_moved = described_class.new(project, nil, 'foo/bar', 'http') + + expect(project_moved.add_redirect_message).to eq(nil) + end end describe '#redirect_message' do diff --git a/spec/lib/gitlab/metrics/method_call_spec.rb b/spec/lib/gitlab/metrics/method_call_spec.rb index 78767d06462..41a9d1d9c90 100644 --- a/spec/lib/gitlab/metrics/method_call_spec.rb +++ b/spec/lib/gitlab/metrics/method_call_spec.rb @@ -96,14 +96,17 @@ describe Gitlab::Metrics::MethodCall do describe '#to_metric' do it 'returns a Metric instance' do + expect(method_call).to receive(:real_time).and_return(4.0001) + expect(method_call).to receive(:cpu_time).and_return(3.0001) + method_call.measure { 'foo' } metric = method_call.to_metric expect(metric).to be_an_instance_of(Gitlab::Metrics::Metric) expect(metric.series).to eq('rails_method_calls') - expect(metric.values[:duration]).to be_a_kind_of(Numeric) - expect(metric.values[:cpu_duration]).to be_a_kind_of(Numeric) + expect(metric.values[:duration]).to eq(4000) + expect(metric.values[:cpu_duration]).to eq(3000) expect(metric.values[:call_count]).to be_an(Integer) expect(metric.tags).to eq({ method: 'Foo#bar' }) @@ -116,13 +119,13 @@ describe Gitlab::Metrics::MethodCall do end it 'returns false when the total call time is not above the threshold' do - expect(method_call).to receive(:real_time).and_return(9) + expect(method_call).to receive(:real_time).and_return(0.009) expect(method_call.above_threshold?).to eq(false) end it 'returns true when the total call time is above the threshold' do - expect(method_call).to receive(:real_time).and_return(9000) + expect(method_call).to receive(:real_time).and_return(9) expect(method_call.above_threshold?).to eq(true) end diff --git a/spec/lib/gitlab/metrics/system_spec.rb b/spec/lib/gitlab/metrics/system_spec.rb index 4d94d8705fb..14afcdf5daa 100644 --- a/spec/lib/gitlab/metrics/system_spec.rb +++ b/spec/lib/gitlab/metrics/system_spec.rb @@ -29,19 +29,19 @@ describe Gitlab::Metrics::System do describe '.cpu_time' do it 'returns a Fixnum' do - expect(described_class.cpu_time).to be_an(Integer) + expect(described_class.cpu_time).to be_an(Float) end end describe '.real_time' do it 'returns a Fixnum' do - expect(described_class.real_time).to be_an(Integer) + expect(described_class.real_time).to be_an(Float) end end describe '.monotonic_time' do - it 'returns a Fixnum' do - expect(described_class.monotonic_time).to be_an(Integer) + it 'returns a Float' do + expect(described_class.monotonic_time).to be_an(Float) end end end diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index f2b35a8fadf..2cad695d7c4 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -289,6 +289,18 @@ describe Issues::MoveService do .to raise_error(StandardError, /Cannot move issue/) end end + + context 'project issue hooks' do + let(:hook) { create(:project_hook, project: old_project, issues_events: true) } + + it 'executes project issue hooks' do + # Ideally, we'd test that `WebHookWorker.jobs.size` increased by 1, + # but since the entire spec run takes place in a transaction, we never + # actually get to the `after_commit` hook that queues these jobs. + expect { move_service.execute(old_issue, new_project) } + .not_to raise_error # Sidekiq::Worker::EnqueueFromTransactionError + end + end end describe 'move permissions' do diff --git a/spec/services/projects/unlink_fork_service_spec.rb b/spec/services/projects/unlink_fork_service_spec.rb index 2bba71fef4f..3ec6139bfa6 100644 --- a/spec/services/projects/unlink_fork_service_spec.rb +++ b/spec/services/projects/unlink_fork_service_spec.rb @@ -62,6 +62,26 @@ describe Projects::UnlinkForkService do expect(source.forks_count).to be_zero end + context 'when the source has LFS objects' do + let(:lfs_object) { create(:lfs_object) } + + before do + lfs_object.projects << project + end + + it 'links the fork to the lfs object before unlinking' do + subject.execute + + expect(lfs_object.projects).to include(forked_project) + end + + it 'does not fail if the lfs objects were already linked' do + lfs_object.projects << forked_project + + expect { subject.execute }.not_to raise_error + end + end + context 'when the original project was deleted' do it 'does not fail when the original project is deleted' do source = forked_project.forked_from_project |