Merge branch 'revert-90404a77' into 'master'

Revert "Merge branch...

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183389



Merged-by: Marius Bobin <mbobin@gitlab.com>
Approved-by: Pedro Pombeiro <noreply@pedro.pombei.ro>
Approved-by: Marius Bobin <mbobin@gitlab.com>
Reviewed-by: Pedro Pombeiro <noreply@pedro.pombei.ro>
Co-authored-by: Pedro Pombeiro <noreply@pedro.pombei.ro>
Co-authored-by: Peter Leitzen <pleitzen@gitlab.com>

app/models/ci/runner.rb

@@ -18,8 +18,6 @@ class Runner < Ci::ApplicationRecord
 
     extend ::Gitlab::Utils::Override
 
-    self.primary_key = :id
-
     add_authentication_token_field :token,
       encrypted: :optional,
       expires_at: :compute_token_expiration,
@@ -670,7 +668,7 @@ def no_allowed_plan_ids
     def partition_id_prefix_in_16_bit_encode
       # Prefix with t1 / t2 / t3 (`t` as in runner type, to allow us to easily detect how a token got prefixed).
       # This is needed in order to ensure that tokens have unique values across partitions
-      # in the new ci_runners partitioned table.
+      # in the new ci_runners_e59bb2812d partitioned table.
       "t#{self.class.runner_types[runner_type].to_s(16)}_"
     end
 

config/gitlab_loose_foreign_keys.yml

@@ -46,9 +46,6 @@ ci_instance_runner_monthly_usages:
   - table: projects
     column: project_id
     on_delete: async_nullify
-  - table: ci_runners
-    column: runner_id
-    on_delete: async_nullify
 ci_job_token_authorizations:
   - table: projects
     column: origin_project_id
@@ -156,13 +153,14 @@ ci_runners:
   - table: users
     column: creator_id
     on_delete: async_nullify
+ci_runners_e59bb2812d:
+  - table: users
+    column: creator_id
+    on_delete: async_nullify
 ci_running_builds:
   - table: projects
     column: project_id
     on_delete: async_delete
-  - table: ci_runners
-    column: runner_id
-    on_delete: async_nullify
 ci_secure_files:
   - table: projects
     column: project_id

db/docs/ci_runners.yml

@@ -6,9 +6,9 @@ feature_categories:
 - runner
 - fleet_visibility
 - hosted_runners
-description: Routing table for CI runners
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166308
-milestone: '17.6'
+description: Registered CI runners
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/commit/046b28312704f3131e72dcd2dbdacc5264d4aa62
+milestone: '8.0'
 gitlab_schema: gitlab_ci
-exempt_from_sharding: true
+sharding_key_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/442395
 table_size: small

db/docs/ci_runners_archived.yml

----
-table_name: ci_runners_archived
-classes: []
-feature_categories:
-- runner
-- fleet_visibility
-- hosted_runners
-description: Registered CI runners (from before table partitioning)
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/commit/046b28312704f3131e72dcd2dbdacc5264d4aa62
-milestone: '8.0'
-gitlab_schema: gitlab_ci
-sharding_key_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/442395
-table_size: small

db/docs/deleted_tables/ci_runners_e59bb2812d.yml → db/docs/ci_runners_e59bb2812d.yml

@@ -12,5 +12,3 @@ milestone: '17.6'
 gitlab_schema: gitlab_ci
 exempt_from_sharding: true
 table_size: small
-removed_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/182549
-removed_in_milestone: '17.10'

db/docs/group_type_ci_runners.yml

----
-table_name: group_type_ci_runners
-classes:
-- Ci::Runner
-feature_categories:
-- runner
-- fleet_visibility
-description: Registered CI group runners
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166308
-milestone: '17.6'
-gitlab_schema: gitlab_ci
-sharding_key:
-  sharding_key_id: namespaces
-table_size: small

db/docs/deleted_tables/group_type_ci_runners_e59bb2812d.yml → db/docs/group_type_ci_runners_e59bb2812d.yml

@@ -12,5 +12,3 @@ gitlab_schema: gitlab_ci
 sharding_key:
   sharding_key_id: namespaces
 table_size: small
-removed_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/182549
-removed_in_milestone: '17.10'

db/docs/instance_type_ci_runners.yml

----
-table_name: instance_type_ci_runners
-classes:
-- Ci::Runner
-feature_categories:
-- runner
-- fleet_visibility
-- hosted_runners
-description: Registered CI instance runners
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166308
-milestone: '17.6'
-gitlab_schema: gitlab_ci
-exempt_from_sharding: true
-table_size: small

db/docs/deleted_tables/instance_type_ci_runners_e59bb2812d.yml → db/docs/instance_type_ci_runners_e59bb2812d.yml

@@ -12,5 +12,3 @@ milestone: '17.6'
 gitlab_schema: gitlab_ci
 exempt_from_sharding: true
 table_size: small
-removed_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/182549
-removed_in_milestone: '17.10'

db/docs/project_type_ci_runners.yml

----
-table_name: project_type_ci_runners
-classes:
-- Ci::Runner
-feature_categories:
-- runner
-- fleet_visibility
-description: Registered CI project runners
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166308
-milestone: '17.6'
-gitlab_schema: gitlab_ci
-sharding_key:
-  sharding_key_id: projects
-table_size: small

db/docs/deleted_tables/project_type_ci_runners_e59bb2812d.yml → db/docs/project_type_ci_runners_e59bb2812d.yml

@@ -12,5 +12,3 @@ gitlab_schema: gitlab_ci
 sharding_key:
   sharding_key_id: projects
 table_size: small
-removed_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/182549
-removed_in_milestone: '17.10'

db/post_migrate/20250301214729_replace_ci_runners_with_partitioned_table.rb

 # frozen_string_literal: true
 
 class ReplaceCiRunnersWithPartitionedTable < Gitlab::Database::Migration[2.2]
-  include Gitlab::Database::PartitioningMigrationHelpers
-
   milestone '17.10'
 
   def up
-    replace_with_partitioned_table 'ci_runners'
+    # no-op -- reverted due to failing check constraint causing broken master
   end
 
   def down
-    rollback_replace_with_partitioned_table 'ci_runners'
+    # no-op
   end
 end

db/post_migrate/20250301214800_switch_record_change_tracking_to_partitioned_ci_runners_table.rb

 # frozen_string_literal: true
 
 class SwitchRecordChangeTrackingToPartitionedCiRunnersTable < Gitlab::Database::Migration[2.2]
-  include Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers
-
-  disable_ddl_transaction!
-
   milestone '17.10'
 
-  PARTITIONED_TABLE = :ci_runners
-  TABLES = %i[instance_type_ci_runners group_type_ci_runners project_type_ci_runners ci_runners].freeze
-
   def up
-    # Remove current LFK trigger that is currently assigned to ci_runners_archived, due to the call to
-    # replace_with_partitioned_table:
-    # CREATE TRIGGER ci_runners_loose_fk_trigger AFTER DELETE ON ci_runners_archived REFERENCING OLD TABLE AS old_table
-    #   FOR EACH STATEMENT EXECUTE FUNCTION insert_into_loose_foreign_keys_deleted_records();
-    drop_trigger(:ci_runners_archived, :ci_runners_loose_fk_trigger, if_exists: true)
-
-    TABLES.each do |table|
-      with_lock_retries do
-        untrack_record_deletions(table)
-
-        # Reattaching the new trigger function to the existing partitioned tables
-        # but with an overridden table name
-        track_record_deletions_override_table_name(table, PARTITIONED_TABLE)
-      end
-    end
+    # no-op -- reverted due to failing check constraint causing broken master
   end
 
   def down
-    TABLES.each do |table|
-      with_lock_retries do
-        untrack_record_deletions(table)
-      end
-    end
-
-    # Mimic track_record_deletions but in a way to restore the previous state
-    # (i.e. trigger on ci_runners_archived but still named ci_runners_loose_fk_trigger)
-    execute(<<~SQL.squish)
-      CREATE TRIGGER #{record_deletion_trigger_name(:ci_runners)}
-      AFTER DELETE ON ci_runners_archived REFERENCING OLD TABLE AS old_table
-      FOR EACH STATEMENT
-      EXECUTE FUNCTION #{INSERT_FUNCTION_NAME}();
-    SQL
+    # no-op
   end
 end

ee/app/models/ee/ci/runner.rb

@@ -30,7 +30,7 @@ module Runner
 
         # NOTE: This scope is meant to be used with scopes that leverage the most_active_runners method
         scope :order_most_active_desc, -> do
-          group(:id, :runner_type).reorder('COUNT(limited_builds.runner_id) DESC NULLS LAST', arel_table['id'].desc)
+          group(:id).reorder('COUNT(limited_builds.runner_id) DESC NULLS LAST', arel_table['id'].desc)
         end
 
         def self.any_shared_runners_with_enabled_cost_factor?(project)

lib/api/ci/helpers/runner.rb

@@ -16,6 +16,11 @@ def authenticate_runner!(ensure_runner_manager: true, update_contacted_at: true)
           track_runner_authentication
           forbidden! unless current_runner
 
+          # TODO: Remove in https://gitlab.com/gitlab-org/gitlab/-/issues/504963 (when ci_runners is swapped)
+          # This is because the new table will have check constraints for these scenarios, and therefore
+          # any orphaned runners will be missing
+          forbidden!('Runner is orphaned') if current_runner.sharding_key_id.nil? && !current_runner.instance_type?
+
           current_runner.heartbeat if update_contacted_at
           return unless ensure_runner_manager
 

spec/db/schema_spec.rb

@@ -120,10 +120,10 @@
       ci_runner_taggings: %w[runner_id sharding_key_id], # The sharding_key_id value is meant to populate the partitioned table, no other usage. The runner_id FK exists at the partition level
       ci_runner_taggings_instance_type: %w[sharding_key_id], # This field is always NULL in this partition
       ci_runners: %w[sharding_key_id], # This value is meant to populate the partitioned table, no other usage
-      ci_runners_archived: %w[sharding_key_id creator_id], # This field is only used in the partitions, and has the appropriate FKs. We don't need the LFK for creator_id since that is already mirrored from ci_runners
-      instance_type_ci_runners: %w[creator_id sharding_key_id], # No need for LFKs on partition, already handled on ci_runners routing table.
-      group_type_ci_runners: %w[creator_id sharding_key_id], # No need for LFKs on partition, already handled on ci_runners routing table.
-      project_type_ci_runners: %w[creator_id sharding_key_id], # No need for LFKs on partition, already handled on ci_runners routing table.
+      ci_runners_e59bb2812d: %w[sharding_key_id], # This field is only used in the partitions, and has the appropriate FKs
+      instance_type_ci_runners_e59bb2812d: %w[creator_id sharding_key_id], # No need for LFKs on partition, already handled on ci_runners_e59bb2812d routing table.
+      group_type_ci_runners_e59bb2812d: %w[creator_id sharding_key_id], # No need for LFKs on partition, already handled on ci_runners_e59bb2812d routing table.
+      project_type_ci_runners_e59bb2812d: %w[creator_id sharding_key_id], # No need for LFKs on partition, already handled on ci_runners_e59bb2812d routing table.
       ci_runner_machines: %w[sharding_key_id], # This value is meant to populate the partitioned table, no other usage
       ci_runner_machines_687967fa8a: %w[runner_id sharding_key_id], # The sharding_key_id field is only used in the partitions, and has the appropriate FKs. The runner_id field will be removed with https://gitlab.com/gitlab-org/gitlab/-/issues/503749.
       instance_type_ci_runner_machines_687967fa8a: %w[sharding_key_id], # This field is always NULL in this partition.
@@ -270,13 +270,13 @@
       approval_merge_request_rules: 17,
       ci_builds: 27,
       ci_pipelines: 24,
-      ci_runners: 16,
-      ci_runners_archived: 17,
+      ci_runners: 17,
+      ci_runners_e59bb2812d: 16,
       deployments: 18,
       epics: 19,
       events: 16,
-      group_type_ci_runners: 17,
-      instance_type_ci_runners: 17,
+      group_type_ci_runners_e59bb2812d: 17,
+      instance_type_ci_runners_e59bb2812d: 17,
       issues: 39,
       members: 21,
       merge_requests: 33,
@@ -285,7 +285,7 @@
       p_ci_pipelines: 24,
       packages_package_files: 16,
       packages_packages: 27,
-      project_type_ci_runners: 17,
+      project_type_ci_runners_e59bb2812d: 17,
       projects: 55,
       sbom_occurrences: 25,
       users: 32,

spec/lib/api/ci/helpers/runner_spec.rb

@@ -106,6 +106,33 @@
       it 'does not update the contacted_at field' do
         expect(current_runner_manager.contacted_at).to eq 1.hour.ago
       end
+
+      # TODO Remove when https://gitlab.com/gitlab-org/gitlab/-/issues/503749 is merged
+      context 'with nil sharding_key_id' do
+        let!(:existing_runner_manager) do
+          Ci::ApplicationRecord.connection.execute <<~SQL
+            ALTER TABLE ci_runner_machines DISABLE TRIGGER ALL;
+
+            INSERT INTO ci_runner_machines
+               (created_at, updated_at, contacted_at, runner_id, runner_type, system_xid, sharding_key_id)
+              VALUES(NOW(), NOW(), '#{1.hour.ago}', #{runner.id}, 2, 'bar', NULL);
+
+            ALTER TABLE ci_runner_machines ENABLE TRIGGER ALL;
+          SQL
+
+          Ci::RunnerManager.for_runner(runner).with_system_xid('bar').first
+        end
+
+        it 'reuses existing runner manager', :aggregate_failures do
+          expect { current_runner_manager }.not_to raise_error
+
+          expect(current_runner_manager).not_to be_nil
+          expect(current_runner_manager).to eq existing_runner_manager
+          expect(current_runner_manager.reload.contacted_at).to eq 1.hour.ago
+          expect(current_runner_manager.runner_type).to eq runner.runner_type
+          expect(current_runner_manager.sharding_key_id).to be_nil
+        end
+      end
     end
 
     context 'when runner manager cannot be found' do

spec/lib/gitlab/background_migration/copy_runner_taggings_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::BackgroundMigration::CopyRunnerTaggings, feature_category: :runner, migration: :gitlab_ci do
+RSpec.describe Gitlab::BackgroundMigration::CopyRunnerTaggings, feature_category: :runner do
   let(:runners_table) { table(:ci_runners, database: :ci, primary_key: :id) }
   let(:runner_taggings_table) { table(:ci_runner_taggings, database: :ci, primary_key: :id) }
   let(:taggings_table) { table(:taggings, database: :ci) }

spec/lib/gitlab/background_migration/delete_orphaned_partitioned_ci_runner_machine_records_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::DeleteOrphanedPartitionedCiRunnerMachineRecords,
+  feature_category: :fleet_visibility, migration: :gitlab_ci do
+  let(:connection) { Ci::ApplicationRecord.connection }
+  let(:runners) { table(:ci_runners_e59bb2812d, database: :ci, primary_key: :id) }
+  let(:runner_managers) { table(:ci_runner_machines_687967fa8a, database: :ci, primary_key: :id) }
+  let(:orphaned_group_runner_manager) { runner_managers.find(3) }
+  let(:orphaned_project_runner_manager) { runner_managers.find(5) }
+
+  before do
+    # Allow creating legacy runner managers (created when FK was not present) that are not connected to a runner
+    connection.transaction do
+      connection.execute(<<~SQL)
+        ALTER TABLE group_type_ci_runner_machines_687967fa8a DISABLE TRIGGER ALL;
+        ALTER TABLE project_type_ci_runner_machines_687967fa8a DISABLE TRIGGER ALL;
+      SQL
+
+      create_runner_and_runner_manager(id: 1, runner_type: 1, system_xid: 'system1')
+      create_runner_and_runner_manager(id: 2, runner_type: 2, sharding_key_id: 89, system_xid: 'system2')
+      runner_managers.create!(
+        id: 3, runner_id: non_existing_record_id, runner_type: 2, sharding_key_id: 100, system_xid: 'system2'
+      )
+      create_runner_and_runner_manager(id: 4, runner_type: 3, sharding_key_id: 10, system_xid: 'system2')
+      runner_managers.create!(
+        id: 5, runner_id: non_existing_record_id, runner_type: 3, sharding_key_id: 100, system_xid: 'system3'
+      )
+
+      connection.execute(<<~SQL)
+        ALTER TABLE group_type_ci_runner_machines_687967fa8a ENABLE TRIGGER ALL;
+        ALTER TABLE project_type_ci_runner_machines_687967fa8a ENABLE TRIGGER ALL;
+      SQL
+    end
+  end
+
+  describe '#perform' do
+    subject(:migration) do
+      described_class.new(
+        start_id: runner_managers.minimum(:runner_id),
+        end_id: runner_managers.maximum(:runner_id),
+        batch_table: :ci_runner_machines_687967fa8a,
+        batch_column: :runner_id,
+        sub_batch_size: 2,
+        pause_ms: 0,
+        connection: connection
+      )
+    end
+
+    it 'deletes from ci_runner_machines_687967fa8a where runner_id has no related', :aggregate_failures do
+      instance_runner_manager = runner_managers.where(runner_type: 1).first
+
+      expect { migration.perform }.to change { runner_managers.count }.from(5).to(3)
+
+      expect(instance_runner_manager.reload).to be_persisted
+      expect { orphaned_group_runner_manager.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      expect { orphaned_project_runner_manager.reload }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
+
+  private
+
+  def create_runner_and_runner_manager(**attrs)
+    runner = runners.create!(**attrs.except(:system_xid))
+    runner_managers.create!(runner_id: runner.id, **attrs)
+  end
+end