13 files changed, 262 insertions, 32 deletions

diff --git a/app/workers/gitlab_usage_ping_worker.rb b/app/workers/gitlab_usage_ping_worker.rb
index b75e724ca98..a5e22f88a3b 100644
--- a/app/workers/gitlab_usage_ping_worker.rb
+++ b/app/workers/gitlab_usage_ping_worker.rb
@@ -6,10 +6,16 @@ class GitlabUsagePingWorker
   include ApplicationWorker
   include CronjobQueue
 
+  # Retry for up to approximately three hours then give up.
+  sidekiq_options retry: 10, dead: false
+
   def perform
     # Multiple Sidekiq workers could run this. We should only do this at most once a day.
     return unless try_obtain_lease
 
+    # Splay the request over a minute to avoid thundering herd problems.
+    sleep(rand(0.0..60.0).round(3))
+
     SubmitUsagePingService.new.execute
   end
 
diff --git a/changelogs/unreleased/bjk-usage_ping.yml b/changelogs/unreleased/bjk-usage_ping.yml
new file mode 100644
index 00000000000..dee6c1ad291
--- /dev/null
+++ b/changelogs/unreleased/bjk-usage_ping.yml
@@ -0,0 +1,5 @@
+---
+title: Update usage ping cron behavior
+merge_request: 30842
+author:
+type: performance
diff --git a/changelogs/unreleased/sh-add-rugged-logs.yml b/changelogs/unreleased/sh-add-rugged-logs.yml
new file mode 100644
index 00000000000..1f464dd92ff
--- /dev/null
+++ b/changelogs/unreleased/sh-add-rugged-logs.yml
@@ -0,0 +1,5 @@
+---
+title: Add Rugged calls and duration to API and Rails logs
+merge_request: 30871
+author:
+type: other
diff --git a/config/initializers/lograge.rb b/config/initializers/lograge.rb
index fbec28186eb..3d84b4e44ce 100644
--- a/config/initializers/lograge.rb
+++ b/config/initializers/lograge.rb
@@ -34,6 +34,13 @@ unless Sidekiq.server?
         payload[:gitaly_duration] = Gitlab::GitalyClient.query_time_ms
       end
 
+      rugged_calls = Gitlab::RuggedInstrumentation.query_count
+
+      if rugged_calls > 0
+        payload[:rugged_calls] = rugged_calls
+        payload[:rugged_duration_ms] = Gitlab::RuggedInstrumentation.query_time_ms
+      end
+
       payload[:response] = event.payload[:response] if event.payload[:response]
       payload[Labkit::Correlation::CorrelationId::LOG_KEY] = Labkit::Correlation::CorrelationId.current_id
 
diff --git a/config/settings.rb b/config/settings.rb
index da459afcce2..8756c120645 100644
--- a/config/settings.rb
+++ b/config/settings.rb
@@ -1,4 +1,5 @@
 require 'settingslogic'
+require 'digest/md5'
 
 # We can not use `Rails.root` here, as this file might be loaded without the
 # full Rails environment being loaded. We can not use `require_relative` either,
@@ -170,14 +171,17 @@ class Settings < Settingslogic
       URI.parse(url_without_path).host
     end
 
-    # Runs every minute in a random ten-minute period on Sundays, to balance the
-    # load on the server receiving these pings. The usage ping is safe to run
-    # multiple times because of a 24 hour exclusive lock.
+    # Runs at a random time of day on a consistent day of the week based on
+    # the instance UUID. This is to balance the load on the service receiving
+    # these pings. The sidekiq job handles temporary http failures.
     def cron_for_usage_ping
       hour = rand(24)
-      minute = rand(6)
+      minute = rand(60)
+      # Set a default UUID for the case when the UUID hasn't been initialized.
+      uuid = Gitlab::CurrentSettings.uuid || 'uuid-not-set'
+      day_of_week = Digest::MD5.hexdigest(uuid).to_i(16) % 7
 
-      "#{minute}0-#{minute}9 #{hour} * * 0"
+      "#{minute} #{hour} * * #{day_of_week}"
     end
   end
 end
diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb
index 0c5f33e1b2a..2ae807697bc 100644
--- a/lib/gitlab/database/migration_helpers.rb
+++ b/lib/gitlab/database/migration_helpers.rb
@@ -6,31 +6,45 @@ module Gitlab
       BACKGROUND_MIGRATION_BATCH_SIZE = 1000 # Number of rows to process per job
       BACKGROUND_MIGRATION_JOB_BUFFER_SIZE = 1000 # Number of jobs to bulk queue at a time
 
+      PERMITTED_TIMESTAMP_COLUMNS = %i[created_at updated_at deleted_at].to_set.freeze
+      DEFAULT_TIMESTAMP_COLUMNS = %i[created_at updated_at].freeze
+
       # Adds `created_at` and `updated_at` columns with timezone information.
       #
       # This method is an improved version of Rails' built-in method `add_timestamps`.
       #
+      # By default, adds `created_at` and `updated_at` columns, but these can be specified as:
+      #
+      #   add_timestamps_with_timezone(:my_table, columns: [:created_at, :deleted_at])
+      #
+      # This allows you to create just the timestamps you need, saving space.
+      #
       # Available options are:
-      # default - The default value for the column.
-      # null - When set to `true` the column will allow NULL values.
+      #  :default - The default value for the column.
+      #  :null - When set to `true` the column will allow NULL values.
       #        The default is to not allow NULL values.
+      #  :columns - the column names to create. Must be one
+      #             of `Gitlab::Database::MigrationHelpers::PERMITTED_TIMESTAMP_COLUMNS`.
+      #             Default value: `DEFAULT_TIMESTAMP_COLUMNS`
+      #
+      # All options are optional.
       def add_timestamps_with_timezone(table_name, options = {})
         options[:null] = false if options[:null].nil?
+        columns = options.fetch(:columns, DEFAULT_TIMESTAMP_COLUMNS)
+        default_value = options[:default]
 
-        [:created_at, :updated_at].each do |column_name|
-          if options[:default] && transaction_open?
-            raise '`add_timestamps_with_timezone` with default value cannot be run inside a transaction. ' \
-              'You can disable transactions by calling `disable_ddl_transaction!` ' \
-              'in the body of your migration class'
-          end
+        validate_not_in_transaction!(:add_timestamps_with_timezone, 'with default value') if default_value
+
+        columns.each do |column_name|
+          validate_timestamp_column_name!(column_name)
 
           # If default value is presented, use `add_column_with_default` method instead.
-          if options[:default]
+          if default_value
             add_column_with_default(
               table_name,
               column_name,
               :datetime_with_timezone,
-              default: options[:default],
+              default: default_value,
               allow_null: options[:null]
             )
           else
@@ -39,6 +53,21 @@ module Gitlab
         end
       end
 
+      # To be used in the `#down` method of migrations that
+      # use `#add_timestamps_with_timezone`.
+      #
+      # Available options are:
+      #  :columns - the column names to remove. Must be one
+      #             Default value: `DEFAULT_TIMESTAMP_COLUMNS`
+      #
+      # All options are optional.
+      def remove_timestamps(table_name, options = {})
+        columns = options.fetch(:columns, DEFAULT_TIMESTAMP_COLUMNS)
+        columns.each do |column_name|
+          remove_column(table_name, column_name)
+        end
+      end
+
       # Creates a new index, concurrently when supported
       #
       # On PostgreSQL this method creates an index concurrently, on MySQL this
@@ -1098,6 +1127,28 @@ into similar problems in the future (e.g. when new tables are created).
       def mysql_compatible_index_length
         Gitlab::Database.mysql? ? 20 : nil
       end
+
+      private
+
+      def validate_timestamp_column_name!(column_name)
+        return if PERMITTED_TIMESTAMP_COLUMNS.member?(column_name)
+
+        raise <<~MESSAGE
+          Illegal timestamp column name! Got #{column_name}.
+          Must be one of: #{PERMITTED_TIMESTAMP_COLUMNS.to_a}
+        MESSAGE
+      end
+
+      def validate_not_in_transaction!(method_name, modifier = nil)
+        return unless transaction_open?
+
+        raise <<~ERROR
+          #{["`#{method_name}`", modifier].compact.join(' ')} cannot be run inside a transaction.
+
+          You can disable transactions by calling `disable_ddl_transaction!` in the body of
+          your migration class
+        ERROR
+      end
     end
   end
 end
diff --git a/lib/gitlab/git/rugged_impl/use_rugged.rb b/lib/gitlab/git/rugged_impl/use_rugged.rb
index 902fa3c7822..badf943e39c 100644
--- a/lib/gitlab/git/rugged_impl/use_rugged.rb
+++ b/lib/gitlab/git/rugged_impl/use_rugged.rb
@@ -13,7 +13,18 @@ module Gitlab
 
         def wrap_rugged_call(&block)
           Gitlab::GitalyClient::StorageSettings.allow_disk_access do
-            yield
+            start = Gitlab::Metrics::System.monotonic_time
+
+            result = yield
+
+            duration = Gitlab::Metrics::System.monotonic_time - start
+
+            if Gitlab::RuggedInstrumentation.active?
+              Gitlab::RuggedInstrumentation.increment_query_count
+              Gitlab::RuggedInstrumentation.query_time += duration
+            end
+
+            result
           end
         end
       end
diff --git a/lib/gitlab/grape_logging/loggers/perf_logger.rb b/lib/gitlab/grape_logging/loggers/perf_logger.rb
index 18ea3a8d2f3..7e86b35a215 100644
--- a/lib/gitlab/grape_logging/loggers/perf_logger.rb
+++ b/lib/gitlab/grape_logging/loggers/perf_logger.rb
@@ -6,11 +6,30 @@ module Gitlab
     module Loggers
       class PerfLogger < ::GrapeLogging::Loggers::Base
         def parameters(_, _)
+          gitaly_data.merge(rugged_data)
+        end
+
+        def gitaly_data
+          gitaly_calls = Gitlab::GitalyClient.get_request_count
+
+          return {} if gitaly_calls.zero?
+
           {
             gitaly_calls: Gitlab::GitalyClient.get_request_count,
             gitaly_duration: Gitlab::GitalyClient.query_time_ms
           }
         end
+
+        def rugged_data
+          rugged_calls = Gitlab::RuggedInstrumentation.query_count
+
+          return {} if rugged_calls.zero?
+
+          {
+            rugged_calls: rugged_calls,
+            rugged_duration_ms: Gitlab::RuggedInstrumentation.query_time_ms
+          }
+        end
       end
     end
   end
diff --git a/lib/gitlab/rugged_instrumentation.rb b/lib/gitlab/rugged_instrumentation.rb
new file mode 100644
index 00000000000..70c06e8b308
--- /dev/null
+++ b/lib/gitlab/rugged_instrumentation.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module RuggedInstrumentation
+    def self.query_time
+      SafeRequestStore[:rugged_query_time] ||= 0
+    end
+
+    def self.query_time=(duration)
+      SafeRequestStore[:rugged_query_time] = duration
+    end
+
+    def self.query_time_ms
+      (self.query_time * 1000).round(2)
+    end
+
+    def self.query_count
+      SafeRequestStore[:rugged_call_count] ||= 0
+    end
+
+    def self.increment_query_count
+      SafeRequestStore[:rugged_call_count] ||= 0
+      SafeRequestStore[:rugged_call_count] += 1
+    end
+
+    def self.active?
+      Gitlab::SafeRequestStore.active?
+    end
+  end
+end
diff --git a/scripts/lint-rugged b/scripts/lint-rugged
index 9466c62a415..e1605b4501b 100755
--- a/scripts/lint-rugged
+++ b/scripts/lint-rugged
@@ -10,7 +10,12 @@ ALLOWED = [
   # Reverted Rugged calls due to Gitaly atop NFS performance
   # See https://docs.gitlab.com/ee/development/gitaly.html#legacy-rugged-code.
   'lib/gitlab/git/rugged_impl/',
-  'lib/gitlab/gitaly_client/storage_settings.rb'
+  'lib/gitlab/gitaly_client/storage_settings.rb',
+
+  # Needed for logging
+  'config/initializers/lograge.rb',
+  'lib/gitlab/grape_logging/loggers/perf_logger.rb',
+  'lib/gitlab/rugged_instrumentation.rb'
 ].freeze
 
 rugged_lines = IO.popen(%w[git grep -i -n rugged -- app config lib], &:read).lines
diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb
index 7409572288c..dd0033bbc14 100644
--- a/spec/lib/gitlab/database/migration_helpers_spec.rb
+++ b/spec/lib/gitlab/database/migration_helpers_spec.rb
@@ -9,9 +9,27 @@ describe Gitlab::Database::MigrationHelpers do
     allow(model).to receive(:puts)
   end
 
+  describe '#remove_timestamps' do
+    it 'can remove the default timestamps' do
+      Gitlab::Database::MigrationHelpers::DEFAULT_TIMESTAMP_COLUMNS.each do |column_name|
+        expect(model).to receive(:remove_column).with(:foo, column_name)
+      end
+
+      model.remove_timestamps(:foo)
+    end
+
+    it 'can remove custom timestamps' do
+      expect(model).to receive(:remove_column).with(:foo, :bar)
+
+      model.remove_timestamps(:foo, columns: [:bar])
+    end
+  end
+
   describe '#add_timestamps_with_timezone' do
+    let(:in_transaction) { false }
+
     before do
-      allow(model).to receive(:transaction_open?).and_return(false)
+      allow(model).to receive(:transaction_open?).and_return(in_transaction)
     end
 
     context 'using PostgreSQL' do
@@ -21,11 +39,64 @@ describe Gitlab::Database::MigrationHelpers do
       end
 
       it 'adds "created_at" and "updated_at" fields with the "datetime_with_timezone" data type' do
-        expect(model).to receive(:add_column).with(:foo, :created_at, :datetime_with_timezone, { null: false })
-        expect(model).to receive(:add_column).with(:foo, :updated_at, :datetime_with_timezone, { null: false })
+        Gitlab::Database::MigrationHelpers::DEFAULT_TIMESTAMP_COLUMNS.each do |column_name|
+          expect(model).to receive(:add_column).with(:foo, column_name, :datetime_with_timezone, { null: false })
+        end
 
         model.add_timestamps_with_timezone(:foo)
       end
+
+      it 'can disable the NOT NULL constraint' do
+        Gitlab::Database::MigrationHelpers::DEFAULT_TIMESTAMP_COLUMNS.each do |column_name|
+          expect(model).to receive(:add_column).with(:foo, column_name, :datetime_with_timezone, { null: true })
+        end
+
+        model.add_timestamps_with_timezone(:foo, null: true)
+      end
+
+      it 'can add just one column' do
+        expect(model).to receive(:add_column).with(:foo, :created_at, :datetime_with_timezone, anything)
+        expect(model).not_to receive(:add_column).with(:foo, :updated_at, :datetime_with_timezone, anything)
+
+        model.add_timestamps_with_timezone(:foo, columns: [:created_at])
+      end
+
+      it 'can add choice of acceptable columns' do
+        expect(model).to receive(:add_column).with(:foo, :created_at, :datetime_with_timezone, anything)
+        expect(model).to receive(:add_column).with(:foo, :deleted_at, :datetime_with_timezone, anything)
+        expect(model).not_to receive(:add_column).with(:foo, :updated_at, :datetime_with_timezone, anything)
+
+        model.add_timestamps_with_timezone(:foo, columns: [:created_at, :deleted_at])
+      end
+
+      it 'cannot add unacceptable column names' do
+        expect do
+          model.add_timestamps_with_timezone(:foo, columns: [:bar])
+        end.to raise_error %r/Illegal timestamp column name/
+      end
+
+      context 'in a transaction' do
+        let(:in_transaction) { true }
+
+        before do
+          allow(model).to receive(:add_column).with(any_args).and_call_original
+          allow(model).to receive(:add_column)
+            .with(:foo, anything, :datetime_with_timezone, anything)
+            .and_return(nil)
+        end
+
+        it 'cannot add a default value' do
+          expect do
+            model.add_timestamps_with_timezone(:foo, default: :i_cause_an_error)
+          end.to raise_error %r/add_timestamps_with_timezone/
+        end
+
+        it 'can add columns without defaults' do
+          expect do
+            model.add_timestamps_with_timezone(:foo)
+          end.not_to raise_error
+        end
+      end
     end
 
     context 'using MySQL' do
diff --git a/spec/lib/gitlab/rugged_instrumentation_spec.rb b/spec/lib/gitlab/rugged_instrumentation_spec.rb
new file mode 100644
index 00000000000..4dcc8ae514a
--- /dev/null
+++ b/spec/lib/gitlab/rugged_instrumentation_spec.rb
@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::RuggedInstrumentation, :request_store do
+  subject { described_class }
+
+  describe '.query_time' do
+    it 'increments query times' do
+      subject.query_time += 0.451
+      subject.query_time += 0.322
+
+      expect(subject.query_time).to be_within(0.001).of(0.773)
+      expect(subject.query_time_ms).to eq(773.0)
+    end
+  end
+
+  context '.increment_query_count' do
+    it 'tracks query counts' do
+      expect(subject.query_count).to eq(0)
+
+      2.times { subject.increment_query_count }
+
+      expect(subject.query_count).to eq(2)
+    end
+  end
+end
diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb
index 893b10ea752..16938da50cc 100644
--- a/spec/support/helpers/test_env.rb
+++ b/spec/support/helpers/test_env.rb
@@ -141,14 +141,6 @@ module TestEnv
     FileUtils.mkdir_p(artifacts_path)
   end
 
-  def clean_gitlab_test_path
-    Dir[TMP_TEST_PATH].each do |entry|
-      unless test_dirs.include?(File.basename(entry))
-        FileUtils.rm_rf(entry)
-      end
-    end
-  end
-
   def setup_gitlab_shell
     component_timed_setup('GitLab Shell',
       install_dir: Gitlab.config.gitlab_shell.path,
@@ -368,10 +360,7 @@ module TestEnv
     # Try to reset without fetching to avoid using the network.
     unless reset.call
       raise 'Could not fetch test seed repository.' unless system(*%W(#{Gitlab.config.git.bin_path} -C #{repo_path} fetch origin))
-
-      # Before we used Git clone's --mirror option, bare repos could end up
-      # with missing refs, clearing them and retrying should fix the issue.
-      clean_gitlab_test_path && init unless reset.call
+      raise "Could not update test seed repository, please delete #{repo_path} and try again" unless reset.call
     end
   end