Merge branch 'optimise-owned-runners' into 'master'

Optimise `ci_namespace_mirrors_for_group_members` to search shortest prefix

See merge request gitlab-org/gitlab!83521

app/models/ci/namespace_mirror.rb

@@ -11,6 +11,8 @@ module Ci
     end
 
     scope :contains_any_of_namespaces, -> (ids) do
+      return none if ids.empty?
+
       where('traversal_ids && ARRAY[?]::int[]', ids)
     end
 

app/models/namespaces/traversal/linear.rb

@@ -49,6 +49,33 @@ module Namespaces
         before_commit :sync_traversal_ids, on: [:create], if: -> { sync_traversal_ids? }
       end
 
+      class_methods do
+        # This method looks into a list of namespaces trying to optimise a returned traversal_ids
+        # into a list of shortest prefixes, due to fact that the shortest prefixes include all childrens.
+        # Example:
+        # INPUT: [[4909902], [4909902,51065789], [4909902,51065793], [7135830], [15599674, 1], [15599674, 1, 3], [15599674, 2]]
+        # RESULT: [[4909902], [7135830], [15599674, 1], [15599674, 2]]
+        def shortest_traversal_ids_prefixes
+          raise ArgumentError, 'Feature not supported since the `:use_traversal_ids` is disabled' unless use_traversal_ids?
+
+          prefixes = []
+
+          # The array needs to be sorted (O(nlogn)) to ensure shortest elements are always first
+          # This allows to do O(n) search of shortest prefixes
+          all_traversal_ids = all.order('namespaces.traversal_ids').pluck('namespaces.traversal_ids')
+          last_prefix = [nil]
+
+          all_traversal_ids.each do |traversal_ids|
+            next if last_prefix == traversal_ids[0..(last_prefix.count - 1)]
+
+            last_prefix = traversal_ids
+            prefixes << traversal_ids
+          end
+
+          prefixes
+        end
+      end
+
       def sync_traversal_ids?
         Feature.enabled?(:sync_traversal_ids, root_ancestor, default_enabled: :yaml)
       end

app/models/user.rb

@@ -2244,9 +2244,23 @@ class User < ApplicationRecord
   end
 
   def ci_namespace_mirrors_for_group_members(level)
-    Ci::NamespaceMirror.contains_any_of_namespaces(
-      group_members.where('access_level >= ?', level).pluck(:source_id)
-    )
+    search_members = group_members.where('access_level >= ?', level)
+
+    # This reduces searched prefixes to only shortest ones
+    # to avoid querying descendants since they are already covered
+    # by ancestor namespaces. If the FF is not available fallback to
+    # inefficient search: https://gitlab.com/gitlab-org/gitlab/-/issues/336436
+    namespace_ids =
+      if Feature.enabled?(:use_traversal_ids, default_enabled: :yaml)
+        Group.joins(:all_group_members)
+          .merge(search_members)
+          .shortest_traversal_ids_prefixes
+          .map(&:last)
+      else
+        search_members.pluck(:source_id)
+      end
+
+    Ci::NamespaceMirror.contains_any_of_namespaces(namespace_ids)
   end
 end
 

spec/models/group_spec.rb

@@ -535,6 +535,10 @@ RSpec.describe Group do
       describe '#ancestors_upto' do
         it { expect(group.ancestors_upto.to_sql).not_to include "WITH ORDINALITY" }
       end
+
+      describe '.shortest_traversal_ids_prefixes' do
+        it { expect { described_class.shortest_traversal_ids_prefixes }.to raise_error /Feature not supported since the `:use_traversal_ids` is disabled/ }
+      end
     end
 
     context 'linear' do
@@ -576,6 +580,90 @@ RSpec.describe Group do
         it { expect(group.ancestors_upto.to_sql).to include "WITH ORDINALITY" }
       end
 
+      describe '.shortest_traversal_ids_prefixes' do
+        subject { filter.shortest_traversal_ids_prefixes }
+
+        context 'for many top-level namespaces' do
+          let!(:top_level_groups) { create_list(:group, 4) }
+
+          context 'when querying all groups' do
+            let(:filter) { described_class.id_in(top_level_groups) }
+
+            it "returns all traversal_ids" do
+              is_expected.to contain_exactly(
+                *top_level_groups.map { |group| [group.id] }
+              )
+            end
+          end
+
+          context 'when querying selected groups' do
+            let(:filter) { described_class.id_in(top_level_groups.first) }
+
+            it "returns only a selected traversal_ids" do
+              is_expected.to contain_exactly([top_level_groups.first.id])
+            end
+          end
+        end
+
+        context 'for namespace hierarchy' do
+          let!(:group_a) { create(:group) }
+          let!(:group_a_sub_1) { create(:group, parent: group_a) }
+          let!(:group_a_sub_2) { create(:group, parent: group_a) }
+          let!(:group_b) { create(:group) }
+          let!(:group_b_sub_1) { create(:group, parent: group_b) }
+          let!(:group_c) { create(:group) }
+
+          context 'when querying all groups' do
+            let(:filter) { described_class.id_in([group_a, group_a_sub_1, group_a_sub_2, group_b, group_b_sub_1, group_c]) }
+
+            it 'returns only shortest prefixes of top-level groups' do
+              is_expected.to contain_exactly(
+                [group_a.id],
+                [group_b.id],
+                [group_c.id]
+              )
+            end
+          end
+
+          context 'when sub-group is reparented' do
+            let(:filter) { described_class.id_in([group_b_sub_1, group_c]) }
+
+            before do
+              group_b_sub_1.update!(parent: group_c)
+            end
+
+            it 'returns a proper shortest prefix of a new group' do
+              is_expected.to contain_exactly(
+                [group_c.id]
+              )
+            end
+          end
+
+          context 'when querying sub-groups' do
+            let(:filter) { described_class.id_in([group_a_sub_1, group_b_sub_1, group_c]) }
+
+            it 'returns sub-groups as they are shortest prefixes' do
+              is_expected.to contain_exactly(
+                [group_a.id, group_a_sub_1.id],
+                [group_b.id, group_b_sub_1.id],
+                [group_c.id]
+              )
+            end
+          end
+
+          context 'when querying group and sub-group of this group' do
+            let(:filter) { described_class.id_in([group_a, group_a_sub_1, group_c]) }
+
+            it 'returns parent groups as this contains all sub-groups' do
+              is_expected.to contain_exactly(
+                [group_a.id],
+                [group_c.id]
+              )
+            end
+          end
+        end
+      end
+
       context 'when project namespace exists in the group' do
         let!(:project) { create(:project, group: group) }
         let!(:project_namespace) { project.project_namespace }

spec/models/user_spec.rb

@@ -4251,16 +4251,26 @@ RSpec.describe User do
     end
   end
 
-  it_behaves_like '#ci_owned_runners'
+  describe '#ci_owned_runners' do
+    it_behaves_like '#ci_owned_runners'
 
-  context 'when FF ci_owned_runners_cross_joins_fix is disabled' do
-    before do
-      skip_if_multiple_databases_are_setup
+    context 'when FF use_traversal_ids is disabled fallbacks to inefficient implementation' do
+      before do
+        stub_feature_flags(use_traversal_ids: false)
+      end
 
-      stub_feature_flags(ci_owned_runners_cross_joins_fix: false)
+      it_behaves_like '#ci_owned_runners'
     end
 
-    it_behaves_like '#ci_owned_runners'
+    context 'when FF ci_owned_runners_cross_joins_fix is disabled' do
+      before do
+        skip_if_multiple_databases_are_setup
+
+        stub_feature_flags(ci_owned_runners_cross_joins_fix: false)
+      end
+
+      it_behaves_like '#ci_owned_runners'
+    end
   end
 
   describe '#projects_with_reporter_access_limited_to' do