Merge branch 'jv-redis-trace-chunks-attempt-2' into 'master'

Add Gitlab::Redis::TraceChunks instance, attempt 2

See merge request gitlab-org/gitlab!63403

.gitignore

@@ -42,6 +42,7 @@ eslint-report.html
 /config/redis.cache.yml
 /config/redis.queues.yml
 /config/redis.shared_state.yml
+/config/redis.trace_chunks.yml
 /config/unicorn.rb
 /config/puma.rb
 /config/puma_actioncable.rb

app/controllers/admin/dashboard_controller.rb

@@ -15,7 +15,7 @@ class Admin::DashboardController < Admin::ApplicationController
     @groups = Group.order_id_desc.with_route.limit(10)
     @notices = Gitlab::ConfigChecker::PumaRuggedChecker.check
     @notices += Gitlab::ConfigChecker::ExternalDatabaseChecker.check
-    @redis_versions = [Gitlab::Redis::Queues, Gitlab::Redis::SharedState, Gitlab::Redis::Cache].map(&:version).uniq
+    @redis_versions = [Gitlab::Redis::Queues, Gitlab::Redis::SharedState, Gitlab::Redis::Cache, Gitlab::Redis::TraceChunks].map(&:version).uniq
   end
   # rubocop: enable CodeReuse/ActiveRecord
 

app/controllers/health_controller.rb

@@ -15,6 +15,7 @@ class HealthController < ActionController::Base
     Gitlab::HealthChecks::Redis::CacheCheck,
     Gitlab::HealthChecks::Redis::QueuesCheck,
     Gitlab::HealthChecks::Redis::SharedStateCheck,
+    Gitlab::HealthChecks::Redis::TraceChunksCheck,
     Gitlab::HealthChecks::GitalyCheck
   ].freeze
 

config/README.md

@@ -147,3 +147,34 @@ searched):
 3. the configuration file pointed to by the
 `GITLAB_REDIS_CONFIG_FILE` environment variable
 4. the configuration file `resque.yml`
+
+## redis.trace_chunks.yml
+
+If configured, `redis.trace_chunks.yml` overrides the
+`resque.yml` settings to configure the Redis database instance
+used for clients of `::Gitlab::Redis::TraceChunks` which stores CI trace chunks.
+
+Settings here can be overridden by the environment variable
+`GITLAB_REDIS_TRACE_CHUNKS_CONFIG_FILE` which provides
+an alternate location for configuration settings.
+
+The order of precedence for the URL used to connect to the Redis instance
+used for `trace_chunks` is:
+1. URL from a configuration file pointed to by the
+`GITLAB_REDIS_TRACE_CHUNKS_CONFIG_FILE` environment variable
+2. URL from `redis.trace_chunks.yml`
+3. URL from a configuration file pointed to by the
+`GITLAB_REDIS_CONFIG_FILE` environment variable
+4. URL from `resque.yml`
+5. `redis://localhost:6383`
+
+The order of precedence for all other configuration settings for `trace_chunks`
+are selected from only the first of the following files found (if a setting
+is not provided in an earlier file, the remainder of the files are not
+searched):
+1. the configuration file pointed to by the
+`GITLAB_REDIS_TRACE_CHUNKS_CONFIG_FILE` environment variable
+2. the configuration file `redis.trace_chunks.yml`
+3. the configuration file pointed to by the
+`GITLAB_REDIS_CONFIG_FILE` environment variable
+4. the configuration file `resque.yml`

config/initializers/7_redis.rb

@@ -8,3 +8,4 @@
 Gitlab::Redis::Cache.with { nil }
 Gitlab::Redis::Queues.with { nil }
 Gitlab::Redis::SharedState.with { nil }
+Gitlab::Redis::TraceChunks.with { nil }

config/redis.trace_chunks.yml.example

+# If you change this file in a merge request, please also create
+# a merge request on https://gitlab.com/gitlab-org/omnibus-gitlab/merge_requests
+#
+development:
+  url: redis://localhost:6379/13
+  #
+  # url: redis://localhost:6382
+  # sentinels:
+  #   -
+  #     host: localhost
+  #     port: 26382 # point to sentinel, not to redis port
+  #   -
+  #     host: replica2
+  #     port: 26382 # point to sentinel, not to redis port
+test:
+  url: redis://localhost:6379/13
+  #
+  # url: redis://localhost:6382
+production:
+  # Redis (single instance)
+  url: unix:/var/run/redis/redis.trace_chunks.sock
+  ##
+  # Redis + Sentinel (for HA)
+  #
+  # Please read instructions carefully before using it as you may lose data:
+  # http://redis.io/topics/sentinel
+  #
+  # You must specify a list of a few sentinels that will handle client connection
+  # please read here for more information: https://docs.gitlab.com/ee/administration/redis/index.html
+  ##
+  # url: redis://master:6382
+  # sentinels:
+  #   -
+  #     host: replica1
+  #     port: 26382 # point to sentinel, not to redis port
+  #   -
+  #     host: replica2
+  #     port: 26382 # point to sentinel, not to redis port

lib/gitlab/health_checks/redis/redis_check.rb

@@ -20,7 +20,8 @@ module Gitlab
           def check
             ::Gitlab::HealthChecks::Redis::CacheCheck.check_up &&
               ::Gitlab::HealthChecks::Redis::QueuesCheck.check_up &&
-              ::Gitlab::HealthChecks::Redis::SharedStateCheck.check_up
+              ::Gitlab::HealthChecks::Redis::SharedStateCheck.check_up &&
+              ::Gitlab::HealthChecks::Redis::TraceChunksCheck.check_up
           end
         end
       end

lib/gitlab/health_checks/redis/trace_chunks_check.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module HealthChecks
+    module Redis
+      class TraceChunksCheck
+        extend SimpleAbstractCheck
+
+        class << self
+          def check_up
+            check
+          end
+
+          private
+
+          def metric_prefix
+            'redis_trace_chunks_ping'
+          end
+
+          def successful?(result)
+            result == 'PONG'
+          end
+
+          # rubocop: disable CodeReuse/ActiveRecord
+          def check
+            catch_timeout 10.seconds do
+              Gitlab::Redis::TraceChunks.with(&:ping)
+            end
+          end
+          # rubocop: enable CodeReuse/ActiveRecord
+        end
+      end
+    end
+  end
+end

lib/gitlab/instrumentation/redis.rb

@@ -8,8 +8,9 @@ module Gitlab
       Cache = Class.new(RedisBase).enable_redis_cluster_validation
       Queues = Class.new(RedisBase)
       SharedState = Class.new(RedisBase).enable_redis_cluster_validation
+      TraceChunks = Class.new(RedisBase).enable_redis_cluster_validation
 
-      STORAGES = [ActionCable, Cache, Queues, SharedState].freeze
+      STORAGES = [ActionCable, Cache, Queues, SharedState, TraceChunks].freeze
 
       # Milliseconds represented in seconds (from 1 millisecond to 2 seconds).
       QUERY_TIME_BUCKETS = [0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2].freeze

lib/gitlab/redis/cache.rb

@@ -8,7 +8,9 @@ module Gitlab
       private
 
       def raw_config_hash
-        super || { url: 'redis://localhost:6380' }
+        config = super
+        config[:url] = 'redis://localhost:6380' if config[:url].blank?
+        config
       end
     end
   end

lib/gitlab/redis/queues.rb

@@ -2,6 +2,7 @@
 
 # We need this require for MailRoom
 require_relative 'wrapper' unless defined?(::Gitlab::Redis::Wrapper)
+require 'active_support/core_ext/object/blank'
 
 module Gitlab
   module Redis
@@ -12,7 +13,9 @@ module Gitlab
       private
 
       def raw_config_hash
-        super || { url: 'redis://localhost:6381' }
+        config = super
+        config[:url] = 'redis://localhost:6381' if config[:url].blank?
+        config
       end
     end
   end

lib/gitlab/redis/shared_state.rb

@@ -11,7 +11,9 @@ module Gitlab
       private
 
       def raw_config_hash
-        super || { url: 'redis://localhost:6382' }
+        config = super
+        config[:url] = 'redis://localhost:6382' if config[:url].blank?
+        config
       end
     end
   end

lib/gitlab/redis/trace_chunks.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Redis
+    class TraceChunks < ::Gitlab::Redis::Wrapper
+      # The data we store on TraceChunks used to be stored on SharedState.
+      def self.config_fallback
+        SharedState
+      end
+    end
+  end
+end

lib/gitlab/redis/wrapper.rb

@@ -64,8 +64,19 @@ module Gitlab
 
         def config_file_name
           [
+            # Instance specific config sources:
             ENV["GITLAB_REDIS_#{store_name.underscore.upcase}_CONFIG_FILE"],
             config_file_path("redis.#{store_name.underscore}.yml"),
+
+            # The current Redis instance may have been split off from another one
+            # (e.g. TraceChunks was split off from SharedState). There are
+            # installations out there where the lowest priority config source
+            # (resque.yml) contains bogus values. In those cases, config_file_name
+            # should resolve to the instance we originated from (the
+            # "config_fallback") rather than resque.yml.
+            config_fallback&.config_file_name,
+
+            # Global config sources:
             ENV['GITLAB_REDIS_CONFIG_FILE'],
             config_file_path('resque.yml')
           ].compact.first
@@ -75,6 +86,10 @@ module Gitlab
           name.demodulize
         end
 
+        def config_fallback
+          nil
+        end
+
         def instrumentation_class
           "::Gitlab::Instrumentation::Redis::#{store_name}".constantize
         end
@@ -133,6 +148,8 @@ module Gitlab
 
         if config_data
           config_data.is_a?(String) ? { url: config_data } : config_data.deep_symbolize_keys
+        else
+          { url: '' }
         end
       end
 

scripts/prepare_build.sh

@@ -42,6 +42,9 @@ sed -i 's|url:.*$|url: redis://redis:6379/11|g' config/redis.queues.yml
 cp config/redis.shared_state.yml.example config/redis.shared_state.yml
 sed -i 's|url:.*$|url: redis://redis:6379/12|g' config/redis.shared_state.yml
 
+cp config/redis.trace_chunks.yml.example config/redis.trace_chunks.yml
+sed -i 's|url:.*$|url: redis://redis:6379/13|g' config/redis.trace_chunks.yml
+
 if [ "$SETUP_DB" != "false" ]; then
   setup_db
 elif getent hosts postgres; then

spec/lib/gitlab/health_checks/redis/trace_chunks_check_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_relative '../simple_check_shared'
+
+RSpec.describe Gitlab::HealthChecks::Redis::TraceChunksCheck do
+  include_examples 'simple_check', 'redis_trace_chunks_ping', 'RedisTraceChunks', 'PONG'
+end

spec/lib/gitlab/instrumentation/redis_spec.rb

@@ -68,7 +68,8 @@ RSpec.describe Gitlab::Instrumentation::Redis do
         .to contain_exactly(details_row.merge(storage: 'ActionCable'),
                             details_row.merge(storage: 'Cache'),
                             details_row.merge(storage: 'Queues'),
-                            details_row.merge(storage: 'SharedState'))
+                            details_row.merge(storage: 'SharedState'),
+                            details_row.merge(storage: 'TraceChunks'))
     end
   end
 end

spec/lib/gitlab/redis/trace_chunks_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Redis::TraceChunks do
+  let(:instance_specific_config_file) { "config/redis.trace_chunks.yml" }
+  let(:environment_config_file_name) { "GITLAB_REDIS_TRACE_CHUNKS_CONFIG_FILE" }
+  let(:shared_state_config_file) { nil }
+
+  before do
+    allow(Gitlab::Redis::SharedState).to receive(:config_file_name).and_return(shared_state_config_file)
+  end
+
+  include_examples "redis_shared_examples"
+
+  describe '.config_file_name' do
+    subject { described_class.config_file_name }
+
+    let(:rails_root) { Dir.mktmpdir('redis_shared_examples') }
+
+    before do
+      # Undo top-level stub of config_file_name because we are testing that method now.
+      allow(described_class).to receive(:config_file_name).and_call_original
+
+      allow(described_class).to receive(:rails_root).and_return(rails_root)
+      FileUtils.mkdir_p(File.join(rails_root, 'config'))
+    end
+
+    after do
+      FileUtils.rm_rf(rails_root)
+    end
+
+    context 'when there is only a resque.yml' do
+      before do
+        FileUtils.touch(File.join(rails_root, 'config/resque.yml'))
+      end
+
+      it { expect(subject).to eq("#{rails_root}/config/resque.yml") }
+
+      context 'and there is a global env override' do
+        before do
+          stub_env('GITLAB_REDIS_CONFIG_FILE', 'global override')
+        end
+
+        it { expect(subject).to eq('global override') }
+
+        context 'and SharedState has a different config file' do
+          let(:shared_state_config_file) { 'shared state config file' }
+
+          it { expect(subject).to eq('shared state config file') }
+        end
+      end
+    end
+  end
+end

spec/support/redis/redis_shared_examples.rb

@@ -16,6 +16,7 @@ RSpec.shared_examples "redis_shared_examples" do
   let(:sentinel_port) { 26379 }
   let(:config_with_environment_variable_inside) { "spec/fixtures/config/redis_config_with_env.yml"}
   let(:config_env_variable_url) {"TEST_GITLAB_REDIS_URL"}
+  let(:rails_root) { Dir.mktmpdir('redis_shared_examples') }
 
   before do
     allow(described_class).to receive(:config_file_name).and_return(Rails.root.join(config_file_name).to_s)
@@ -29,8 +30,6 @@ RSpec.shared_examples "redis_shared_examples" do
   describe '.config_file_name' do
     subject { described_class.config_file_name }
 
-    let(:rails_root) { Dir.mktmpdir('redis_shared_examples') }
-
     before do
       # Undo top-level stub of config_file_name because we are testing that method now.
       allow(described_class).to receive(:config_file_name).and_call_original
@@ -237,6 +236,23 @@ RSpec.shared_examples "redis_shared_examples" do
         described_class.with { |_redis_shared_example| true }
       end
     end
+
+    context 'when there is no config at all' do
+      before do
+        # Undo top-level stub of config_file_name because we are testing that method now.
+        allow(described_class).to receive(:config_file_name).and_call_original
+
+        allow(described_class).to receive(:rails_root).and_return(rails_root)
+      end
+
+      after do
+        FileUtils.rm_rf(rails_root)
+      end
+
+      it 'can run an empty block' do
+        expect { described_class.with { nil } }.not_to raise_error
+      end
+    end
   end
 
   describe '#sentinels' do