Merge branch 'add-annotations-to-all-queues-yml' into 'master'

Add annotations to all_queues.yml

Closes gitlab-com/gl-infra/scalability#26

See merge request gitlab-org/gitlab!23771

ee/spec/lib/gitlab/sidekiq_config_spec.rb

@@ -50,31 +50,33 @@ describe Gitlab::SidekiqConfig do
   end
 
   describe '.all_queues_yml_outdated?' do
-    before do
-      workers = [
+    let(:workers) do
+      [
         LdapGroupSyncWorker,
         RepositoryUpdateMirrorWorker
       ].map { |worker| described_class::Worker.new(worker, ee: true) }
+    end
 
+    before do
       allow(described_class).to receive(:workers).and_return(workers)
 
-      allow(File).to receive(:read)
+      allow(YAML).to receive(:load_file)
                        .with(described_class::FOSS_QUEUE_CONFIG_PATH)
-                       .and_return(YAML.dump([]))
+                       .and_return([])
     end
 
     it 'returns true if the YAML file does not match the application code' do
-      allow(File).to receive(:read)
+      allow(YAML).to receive(:load_file)
                        .with(described_class::EE_QUEUE_CONFIG_PATH)
-                       .and_return(YAML.dump(['ldap_group_sync']))
+                       .and_return([workers.first.to_yaml])
 
       expect(described_class.all_queues_yml_outdated?).to be(true)
     end
 
     it 'returns false if the YAML file matches the application code' do
-      allow(File).to receive(:read)
+      allow(YAML).to receive(:load_file)
                        .with(described_class::EE_QUEUE_CONFIG_PATH)
-                       .and_return(YAML.dump(%w[ldap_group_sync repository_update_mirror]))
+                       .and_return(workers.map(&:to_yaml))
 
       expect(described_class.all_queues_yml_outdated?).to be(false)
     end

lib/gitlab/sidekiq_config.rb

@@ -13,21 +13,10 @@ module Gitlab
       (EE_QUEUE_CONFIG_PATH if Gitlab.ee?)
     ].compact.freeze
 
-    # For queues that don't have explicit workers - default and mailers
-    DummyWorker = Struct.new(:queue, :weight) do
-      def queue_namespace
-        nil
-      end
-
-      def get_weight
-        weight
-      end
-    end
-
     DEFAULT_WORKERS = [
-      Gitlab::SidekiqConfig::Worker.new(DummyWorker.new('default', 1), ee: false),
-      Gitlab::SidekiqConfig::Worker.new(DummyWorker.new('mailers', 2), ee: false)
-    ].freeze
+      DummyWorker.new('default', weight: 1),
+      DummyWorker.new('mailers', weight: 2)
+    ].map { |worker| Gitlab::SidekiqConfig::Worker.new(worker, ee: false) }.freeze
 
     class << self
       include Gitlab::SidekiqConfig::CliMethods
@@ -66,12 +55,13 @@ module Gitlab
         workers.partition(&:ee?).reverse.map(&:sort)
       end
 
+      # YAML.load_file is OK here as we control the file contents
       def all_queues_yml_outdated?
         foss_workers, ee_workers = workers_for_all_queues_yml
 
-        return true if foss_workers != YAML.safe_load(File.read(FOSS_QUEUE_CONFIG_PATH))
+        return true if foss_workers != YAML.load_file(FOSS_QUEUE_CONFIG_PATH)
 
-        Gitlab.ee? && ee_workers != YAML.safe_load(File.read(EE_QUEUE_CONFIG_PATH))
+        Gitlab.ee? && ee_workers != YAML.load_file(EE_QUEUE_CONFIG_PATH)
       end
 
       def queues_for_sidekiq_queues_yml
@@ -89,9 +79,9 @@ module Gitlab
          remaining_queues.map(&:queue_and_weight)).sort
       end
 
+      # YAML.load_file is OK here as we control the file contents
       def sidekiq_queues_yml_outdated?
-        # YAML.load is OK here as we control the file contents
-        config_queues = YAML.load(File.read(SIDEKIQ_QUEUES_PATH))[:queues] # rubocop:disable Security/YAMLLoad
+        config_queues = YAML.load_file(SIDEKIQ_QUEUES_PATH)[:queues]
 
         queues_for_sidekiq_queues_yml != config_queues
       end

lib/gitlab/sidekiq_config/cli_methods.rb

@@ -23,8 +23,10 @@ module Gitlab
 
         @worker_queues[rails_path] ||= QUEUE_CONFIG_PATHS.flat_map do |path|
           full_path = File.join(rails_path, path)
+          queues = File.exist?(full_path) ? YAML.load_file(full_path) : []
 
-          File.exist?(full_path) ? YAML.load_file(full_path) : []
+          # https://gitlab.com/gitlab-org/gitlab/issues/199230
+          queues.map { |queue| queue.is_a?(Hash) ? queue[:name] : queue }
         end
       end
 
@@ -37,6 +39,12 @@ module Gitlab
           [queue, *queues_set.grep(/\A#{queue}:/)]
         end
       end
+
+      def clear_memoization!
+        if instance_variable_defined?('@worker_queues')
+          remove_instance_variable('@worker_queues')
+        end
+      end
       # rubocop:enable Gitlab/ModuleWithInstanceVariables
     end
   end

lib/gitlab/sidekiq_config/dummy_worker.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module SidekiqConfig
+    # For queues that don't have explicit workers - default and mailers
+    class DummyWorker
+      attr_accessor :queue
+
+      ATTRIBUTE_METHODS = {
+        feature_category: :get_feature_category,
+        has_external_dependencies: :worker_has_external_dependencies?,
+        latency_sensitive: :latency_sensitive_worker?,
+        resource_boundary: :get_worker_resource_boundary,
+        weight: :get_weight
+      }.freeze
+
+      def initialize(queue, attributes = {})
+        @queue = queue
+        @attributes = attributes
+      end
+
+      def queue_namespace
+        nil
+      end
+
+      ATTRIBUTE_METHODS.each do |attribute, meth|
+        define_method meth do
+          @attributes[attribute]
+        end
+      end
+    end
+  end
+end

lib/gitlab/sidekiq_config/worker.rb

@@ -41,11 +41,18 @@ module Gitlab
 
       # YAML representation
       def encode_with(coder)
-        coder.represent_scalar(nil, to_yaml)
+        coder.represent_map(nil, to_yaml)
       end
 
       def to_yaml
-        queue
+        {
+          name: queue,
+          feature_category: get_feature_category,
+          has_external_dependencies: worker_has_external_dependencies?,
+          latency_sensitive: latency_sensitive_worker?,
+          resource_boundary: get_worker_resource_boundary,
+          weight: get_weight
+        }
       end
 
       def namespace_and_weight

spec/lib/gitlab/sidekiq_config/cli_methods_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+describe Gitlab::SidekiqConfig::CliMethods do
+  let(:dummy_root) { '/tmp/' }
+
+  describe '.worker_queues' do
+    def expand_path(path)
+      File.join(dummy_root, path)
+    end
+
+    def stub_exists(exists: true)
+      ['app/workers/all_queues.yml', 'ee/app/workers/all_queues.yml'].each do |path|
+        allow(File).to receive(:exist?).with(expand_path(path)).and_return(exists)
+      end
+    end
+
+    def stub_contents(foss_queues, ee_queues)
+      allow(YAML).to receive(:load_file)
+                       .with(expand_path('app/workers/all_queues.yml'))
+                       .and_return(foss_queues)
+
+      allow(YAML).to receive(:load_file)
+                       .with(expand_path('ee/app/workers/all_queues.yml'))
+                       .and_return(ee_queues)
+    end
+
+    before do
+      described_class.clear_memoization!
+    end
+
+    context 'when the file exists' do
+      before do
+        stub_exists(exists: true)
+      end
+
+      shared_examples 'valid file contents' do
+        it 'memoizes the result' do
+          result = described_class.worker_queues(dummy_root)
+
+          stub_exists(exists: false)
+
+          expect(described_class.worker_queues(dummy_root)).to eq(result)
+        end
+
+        it 'flattens and joins the contents' do
+          expected_queues = %w[queue_a queue_b]
+          expected_queues = expected_queues.first(1) unless Gitlab.ee?
+
+          expect(described_class.worker_queues(dummy_root))
+            .to match_array(expected_queues)
+        end
+      end
+
+      context 'when the file contains an array of strings' do
+        before do
+          stub_contents(['queue_a'], ['queue_b'])
+        end
+
+        include_examples 'valid file contents'
+      end
+
+      context 'when the file contains an array of hashes' do
+        before do
+          stub_contents([{ name: 'queue_a' }], [{ name: 'queue_b' }])
+        end
+
+        include_examples 'valid file contents'
+      end
+    end
+
+    context 'when the file does not exist' do
+      before do
+        stub_exists(exists: false)
+      end
+
+      it 'returns an empty array' do
+        expect(described_class.worker_queues(dummy_root)).to be_empty
+      end
+    end
+  end
+
+  describe '.expand_queues' do
+    let(:all_queues) do
+      ['cronjob:stuck_import_jobs', 'cronjob:stuck_merge_jobs', 'post_receive']
+    end
+
+    it 'defaults the value of the second argument to .worker_queues' do
+      allow(described_class).to receive(:worker_queues).and_return([])
+
+      expect(described_class.expand_queues(['cronjob']))
+        .to contain_exactly('cronjob')
+
+      allow(described_class).to receive(:worker_queues).and_return(all_queues)
+
+      expect(described_class.expand_queues(['cronjob']))
+        .to contain_exactly('cronjob', 'cronjob:stuck_import_jobs', 'cronjob:stuck_merge_jobs')
+    end
+
+    it 'expands queue namespaces to concrete queue names' do
+      expect(described_class.expand_queues(['cronjob'], all_queues))
+        .to contain_exactly('cronjob', 'cronjob:stuck_import_jobs', 'cronjob:stuck_merge_jobs')
+    end
+
+    it 'lets concrete queue names pass through' do
+      expect(described_class.expand_queues(['post_receive'], all_queues))
+        .to contain_exactly('post_receive')
+    end
+
+    it 'lets unknown queues pass through' do
+      expect(described_class.expand_queues(['unknown'], all_queues))
+        .to contain_exactly('unknown')
+    end
+  end
+end

spec/lib/gitlab/sidekiq_config/worker_spec.rb

@@ -3,9 +3,17 @@
 require 'fast_spec_helper'
 
 describe Gitlab::SidekiqConfig::Worker do
-  def create_worker(queue:, weight: 0)
+  def create_worker(queue:, **attributes)
     namespace = queue.include?(':') && queue.split(':').first
-    inner_worker = double(queue: queue, queue_namespace: namespace, get_weight: weight)
+    inner_worker = double(
+      queue: queue,
+      queue_namespace: namespace,
+      get_feature_category: attributes[:feature_category],
+      get_weight: attributes[:weight],
+      get_worker_resource_boundary: attributes[:resource_boundary],
+      latency_sensitive_worker?: attributes[:latency_sensitive],
+      worker_has_external_dependencies?: attributes[:has_external_dependencies]
+    )
 
     described_class.new(inner_worker, ee: false)
   end
@@ -75,13 +83,32 @@ describe Gitlab::SidekiqConfig::Worker do
   end
 
   describe 'YAML encoding' do
-    it 'encodes the worker in YAML as a string of the queue' do
-      worker_a = create_worker(queue: 'a')
-      worker_b = create_worker(queue: 'b')
+    it 'encodes the worker in YAML as a hash of the queue' do
+      attributes_a = {
+        feature_category: :source_code_management,
+        has_external_dependencies: false,
+        latency_sensitive: false,
+        resource_boundary: :memory,
+        weight: 2
+      }
+
+      attributes_b = {
+        feature_category: :not_owned,
+        has_external_dependencies: true,
+        latency_sensitive: true,
+        resource_boundary: :unknown,
+        weight: 1
+      }
+
+      worker_a = create_worker(queue: 'a', **attributes_a)
+      worker_b = create_worker(queue: 'b', **attributes_b)
+
+      expect(YAML.dump(worker_a))
+        .to eq(YAML.dump(attributes_a.reverse_merge(name: 'a')))
 
-      expect(YAML.dump(worker_a)).to eq(YAML.dump('a'))
       expect(YAML.dump([worker_a, worker_b]))
-        .to eq(YAML.dump(%w[a b]))
+        .to eq(YAML.dump([attributes_a.reverse_merge(name: 'a'),
+                          attributes_b.reverse_merge(name: 'b')]))
     end
   end
 

spec/lib/gitlab/sidekiq_config_spec.rb

@@ -24,27 +24,6 @@ describe Gitlab::SidekiqConfig do
     end
   end
 
-  describe '.expand_queues' do
-    it 'expands queue namespaces to concrete queue names' do
-      queues = described_class.expand_queues(%w[cronjob])
-
-      expect(queues).to include('cronjob:stuck_import_jobs')
-      expect(queues).to include('cronjob:stuck_merge_jobs')
-    end
-
-    it 'lets concrete queue names pass through' do
-      queues = described_class.expand_queues(%w[post_receive])
-
-      expect(queues).to include('post_receive')
-    end
-
-    it 'lets unknown queues pass through' do
-      queues = described_class.expand_queues(%w[unknown])
-
-      expect(queues).to include('unknown')
-    end
-  end
-
   describe '.workers_for_all_queues_yml' do
     it 'returns a tuple with FOSS workers first' do
       expect(described_class.workers_for_all_queues_yml.first)
@@ -53,29 +32,31 @@ describe Gitlab::SidekiqConfig do
   end
 
   describe '.all_queues_yml_outdated?' do
-    before do
-      workers = [
-        PostReceive,
+    let(:workers) do
+      [
         MergeWorker,
+        PostReceive,
         ProcessCommitWorker
       ].map { |worker| described_class::Worker.new(worker, ee: false) }
+    end
 
+    before do
       allow(described_class).to receive(:workers).and_return(workers)
       allow(Gitlab).to receive(:ee?).and_return(false)
     end
 
-    it 'returns true if the YAML file does not match the application code' do
-      allow(File).to receive(:read)
+    it 'returns true if the YAML file does not matcph the application code' do
+      allow(YAML).to receive(:load_file)
                        .with(described_class::FOSS_QUEUE_CONFIG_PATH)
-                       .and_return(YAML.dump(%w[post_receive merge]))
+                       .and_return(workers.first(2).map(&:to_yaml))
 
       expect(described_class.all_queues_yml_outdated?).to be(true)
     end
 
     it 'returns false if the YAML file matches the application code' do
-      allow(File).to receive(:read)
+      allow(YAML).to receive(:load_file)
                        .with(described_class::FOSS_QUEUE_CONFIG_PATH)
-                       .and_return(YAML.dump(%w[merge post_receive process_commit]))
+                       .and_return(workers.map(&:to_yaml))
 
       expect(described_class.all_queues_yml_outdated?).to be(false)
     end
@@ -125,17 +106,17 @@ describe Gitlab::SidekiqConfig do
     end
 
     it 'returns true if the YAML file does not match the application code' do
-      allow(File).to receive(:read)
+      allow(YAML).to receive(:load_file)
                        .with(described_class::SIDEKIQ_QUEUES_PATH)
-                       .and_return(YAML.dump(queues: expected_queues.reverse))
+                       .and_return(queues: expected_queues.reverse)
 
       expect(described_class.sidekiq_queues_yml_outdated?).to be(true)
     end
 
     it 'returns false if the YAML file matches the application code' do
-      allow(File).to receive(:read)
+      allow(YAML).to receive(:load_file)
                        .with(described_class::SIDEKIQ_QUEUES_PATH)
-                       .and_return(YAML.dump(queues: expected_queues))
+                       .and_return(queues: expected_queues)
 
       expect(described_class.sidekiq_queues_yml_outdated?).to be(false)
     end