Merge branch 'feature/gb/decouple-group-runners-query-namespaces' into 'master'

Denormalize namespace traversal ids to match builds for group runners

See merge request gitlab-org/gitlab!70162

app/models/ci/pending_build.rb

@@ -26,11 +26,13 @@ module Ci
       private
 
       def args_from_build(build)
+        project = build.project
+
         args = {
           build: build,
-          project: build.project,
+          project: project,
           protected: build.protected?,
-          namespace: build.project.namespace
+          namespace: project.namespace
         }
 
         if Feature.enabled?(:ci_pending_builds_maintain_tags_data, type: :development, default_enabled: :yaml)
@@ -38,29 +40,26 @@ module Ci
         end
 
         if Feature.enabled?(:ci_pending_builds_maintain_shared_runners_data, type: :development, default_enabled: :yaml)
-          args.store(:instance_runners_enabled, shareable?(build))
+          args.store(:instance_runners_enabled, shared_runners_enabled?(project))
         end
 
-        args
+        if Feature.enabled?(:ci_pending_builds_maintain_namespace_traversal_ids, type: :development, default_enabled: :yaml)
+          args.store(:namespace_traversal_ids, project.namespace.traversal_ids) if group_runners_enabled?(project)
         end
 
-      def shareable?(build)
-        shared_runner_enabled?(build) &&
-          builds_access_level?(build) &&
-          project_not_removed?(build)
+        args
       end
 
-      def shared_runner_enabled?(build)
-        build.project.shared_runners.exists?
+      def shared_runners_enabled?(project)
+        builds_enabled?(project) && project.shared_runners_enabled?
       end
 
-      def project_not_removed?(build)
-        !build.project.pending_delete?
+      def group_runners_enabled?(project)
+        builds_enabled?(project) && project.group_runners_enabled?
       end
 
-      def builds_access_level?(build)
-        build.project.project_feature.builds_access_level.nil? ||
-          build.project.project_feature.builds_access_level > 0
+      def builds_enabled?(project)
+        project.builds_enabled? && !project.pending_delete?
       end
     end
   end

app/models/ci/runner.rb

@@ -387,6 +387,12 @@ module Ci
       read_attribute(:contacted_at)
     end
 
+    def namespace_ids
+      strong_memoize(:namespace_ids) do
+        runner_namespaces.pluck(:namespace_id).compact
+      end
+    end
+
     private
 
     def cleanup_runner_queue

app/services/ci/queue/build_queue_service.rb

@@ -24,6 +24,9 @@ module Ci
 
       # rubocop:disable CodeReuse/ActiveRecord
       def builds_for_group_runner
+        if strategy.use_denormalized_namespace_traversal_ids?
+          strategy.builds_for_group_runner
+        else
           # Workaround for weird Rails bug, that makes `runner.groups.to_sql` to return `runner_id = NULL`
           groups = ::Group.joins(:runner_namespaces).merge(runner.runner_namespaces)
 
@@ -40,6 +43,7 @@ module Ci
 
           order(relation)
         end
+      end
 
       def builds_for_project_runner
         relation = new_builds

app/services/ci/queue/builds_table_strategy.rb

@@ -31,6 +31,10 @@ module Ci
         end
       end
 
+      def builds_for_group_runner
+        raise NotImplementedError
+      end
+
       def builds_matching_tag_ids(relation, ids)
         # pick builds that does not have other tags than runner's one
         relation.matches_tag_ids(ids)
@@ -61,6 +65,10 @@ module Ci
         false
       end
 
+      def use_denormalized_namespace_traversal_ids?
+        false
+      end
+
       private
 
       def running_builds_for_shared_runners

app/services/ci/queue/pending_builds_strategy.rb

@@ -16,6 +16,12 @@ module Ci
         builds_ordered_for_shared_runners(shared_builds)
       end
 
+      def builds_for_group_runner
+        return new_builds.none if runner.namespace_ids.empty?
+
+        new_builds.where('ci_pending_builds.namespace_traversal_ids && ARRAY[?]::int[]', runner.namespace_ids)
+      end
+
       def builds_matching_tag_ids(relation, ids)
         if ::Feature.enabled?(:ci_queueing_denormalize_tags_information, runner, default_enabled: :yaml)
           relation.where('tag_ids <@ ARRAY[?]::int[]', runner.tags_ids)
@@ -52,6 +58,10 @@ module Ci
         ::Feature.enabled?(:ci_queueing_denormalize_ci_minutes_information, runner, type: :development, default_enabled: :yaml)
       end
 
+      def use_denormalized_namespace_traversal_ids?
+        ::Feature.enabled?(:ci_queueing_denormalize_namespace_traversal_ids, runner, type: :development, default_enabled: :yaml)
+      end
+
       private
 
       def builds_available_for_shared_runners

config/feature_flags/development/ci_pending_builds_maintain_namespace_traversal_ids.yml

+---
+name: ci_pending_builds_maintain_namespace_traversal_ids
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/70162
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/340930
+milestone: '14.3'
+type: development
+group: group::pipeline execution
+default_enabled: false

config/feature_flags/development/ci_queueing_denormalize_namespace_traversal_ids.yml

+---
+name: ci_queueing_denormalize_namespace_traversal_ids
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/70162
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/340930
+milestone: '14.3'
+type: development
+group: group::pipeline execution
+default_enabled: false

db/migrate/20210913122457_add_namespace_traversal_ids_to_ci_pending_builds.rb

+# frozen_string_literal: true
+
+class AddNamespaceTraversalIdsToCiPendingBuilds < Gitlab::Database::Migration[1.0]
+  enable_lock_retries!
+
+  def change
+    add_column :ci_pending_builds, :namespace_traversal_ids, :integer, array: true, default: []
+  end
+end

db/post_migrate/20210914094840_add_gin_index_on_pending_builds_namespace_traversal_ids.rb

+# frozen_string_literal: true
+
+class AddGinIndexOnPendingBuildsNamespaceTraversalIds < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_gin_ci_pending_builds_on_namespace_traversal_ids'
+
+  def up
+    add_concurrent_index :ci_pending_builds, :namespace_traversal_ids, name: INDEX_NAME, using: :gin
+  end
+
+  def down
+    remove_concurrent_index_by_name :ci_pending_builds, INDEX_NAME
+  end
+end

db/schema_migrations/20210913122457

+031fb794e4fa3d54098596defd9de6fb1e507e01bd8f02462c0079e7e59c3416
\ No newline at end of file

db/schema_migrations/20210914094840

+994de4d18202a420a7184cabe787f6e3f872900147df0cd848731483e6d87566
\ No newline at end of file

db/structure.sql

@@ -11629,7 +11629,8 @@ CREATE TABLE ci_pending_builds (
     instance_runners_enabled boolean DEFAULT false NOT NULL,
     namespace_id bigint,
     minutes_exceeded boolean DEFAULT false NOT NULL,
-    tag_ids integer[] DEFAULT '{}'::integer[]
+    tag_ids integer[] DEFAULT '{}'::integer[],
+    namespace_traversal_ids integer[] DEFAULT '{}'::integer[]
 );
 
 CREATE SEQUENCE ci_pending_builds_id_seq
@@ -25151,6 +25152,8 @@ CREATE INDEX index_geo_reset_checksum_events_on_project_id ON geo_reset_checksum
 
 CREATE INDEX index_geo_upload_deleted_events_on_upload_id ON geo_upload_deleted_events USING btree (upload_id);
 
+CREATE INDEX index_gin_ci_pending_builds_on_namespace_traversal_ids ON ci_pending_builds USING gin (namespace_traversal_ids);
+
 CREATE INDEX index_gitlab_subscription_histories_on_gitlab_subscription_id ON gitlab_subscription_histories USING btree (gitlab_subscription_id);
 
 CREATE INDEX index_gitlab_subscriptions_on_end_date_and_namespace_id ON gitlab_subscriptions USING btree (end_date, namespace_id);

spec/models/ci/pending_build_spec.rb

@@ -58,7 +58,11 @@ RSpec.describe Ci::PendingBuild do
       end
     end
 
-    context 'when project does not have shared runner' do
+    context 'when project does not have shared runners enabled' do
+      before do
+        project.shared_runners_enabled = false
+      end
+
       it 'sets instance_runners_enabled to false' do
         described_class.upsert_from_build!(build)
 
@@ -69,6 +73,10 @@ RSpec.describe Ci::PendingBuild do
     context 'when project has shared runner' do
       let_it_be(:runner) { create(:ci_runner, :instance) }
 
+      before do
+        project.shared_runners_enabled = true
+      end
+
       context 'when ci_pending_builds_maintain_shared_runners_data is enabled' do
         it 'sets instance_runners_enabled to true' do
           described_class.upsert_from_build!(build)
@@ -139,5 +147,39 @@ RSpec.describe Ci::PendingBuild do
         end
       end
     end
+
+    context 'when a build project is nested in a subgroup' do
+      let(:group) { create(:group, :with_hierarchy, depth: 2, children: 1) }
+      let(:project) { create(:project, namespace: group.descendants.first) }
+      let(:pipeline) { create(:ci_pipeline, project: project) }
+      let(:build) { create(:ci_build, :created, pipeline: pipeline) }
+
+      subject { described_class.last }
+
+      context 'when build can be picked by a group runner' do
+        before do
+          project.group_runners_enabled = true
+        end
+
+        it 'denormalizes namespace traversal ids' do
+          described_class.upsert_from_build!(build)
+
+          expect(subject.namespace_traversal_ids).not_to be_empty
+          expect(subject.namespace_traversal_ids).to eq [group.id, project.namespace.id]
+        end
+      end
+
+      context 'when build can not be picked by a group runner' do
+        before do
+          project.group_runners_enabled = false
+        end
+
+        it 'creates an empty namespace traversal ids array' do
+          described_class.upsert_from_build!(build)
+
+          expect(subject.namespace_traversal_ids).to be_empty
+        end
+      end
+    end
   end
 end

spec/models/ci/runner_spec.rb

@@ -531,6 +531,10 @@ RSpec.describe Ci::Runner do
         it 'can handle builds' do
           expect(runner.can_pick?(build)).to be_truthy
         end
+
+        it 'knows namespace id it is assigned to' do
+          expect(runner.namespace_ids).to eq [group.id]
+        end
       end
     end
 

spec/services/ci/register_job_service_spec.rb

@@ -219,6 +219,8 @@ module Ci
           before do
             project.update!(shared_runners_enabled: true, group_runners_enabled: true)
             project.project_feature.update_attribute(:builds_access_level, ProjectFeature::DISABLED)
+
+            pending_job.reload.create_queuing_entry!
           end
 
           context 'and uses shared runner' do
@@ -304,6 +306,8 @@ module Ci
         context 'disallow group runners' do
           before do
             project.update!(group_runners_enabled: false)
+
+            pending_job.reload.create_queuing_entry!
           end
 
           context 'group runner' do
@@ -755,6 +759,14 @@ module Ci
 
           include_examples 'handles runner assignment'
         end
+
+        context 'with ci_queueing_denormalize_namespace_traversal_ids disabled' do
+          before do
+            stub_feature_flags(ci_queueing_denormalize_namespace_traversal_ids: false)
+          end
+
+          include_examples 'handles runner assignment'
+        end
       end
 
       context 'when not using pending builds table' do