From d0e6ea648ad30c70576e2a2c6ce3dfc390287b0e Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Tue, 20 Dec 2022 15:33:09 +0200 Subject: [PATCH 1/6] Bump default partition_id value for CI tables to 101 This is the least disturbing way to test that our cascading value strategy works Changelog: other --- ...efault_partition_id_value_for_ci_tables.rb | 50 +++++++++++++++++++ db/schema_migrations/20221220131020 | 1 + 2 files changed, 51 insertions(+) create mode 100644 db/post_migrate/20221220131020_bump_default_partition_id_value_for_ci_tables.rb create mode 100644 db/schema_migrations/20221220131020 diff --git a/db/post_migrate/20221220131020_bump_default_partition_id_value_for_ci_tables.rb b/db/post_migrate/20221220131020_bump_default_partition_id_value_for_ci_tables.rb new file mode 100644 index 00000000000000..889ef86767138e --- /dev/null +++ b/db/post_migrate/20221220131020_bump_default_partition_id_value_for_ci_tables.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +class BumpDefaultPartitionIdValueForCiTables < Gitlab::Database::Migration[2.1] + disable_ddl_transaction! + + TABLES = { + ci_build_needs: [:partition_id], + ci_build_pending_states: [:partition_id], + ci_build_report_results: [:partition_id], + ci_build_trace_chunks: [:partition_id], + ci_build_trace_metadata: [:partition_id], + ci_builds: [:partition_id], + ci_builds_runner_session: [:partition_id], + ci_job_artifacts: [:partition_id], + ci_job_variables: [:partition_id], + ci_pending_builds: [:partition_id], + ci_pipeline_variables: [:partition_id], + ci_pipelines: [:partition_id], + ci_resources: [:partition_id], + ci_running_builds: [:partition_id], + ci_sources_pipelines: [:partition_id, :source_partition_id], + ci_stages: [:partition_id], + ci_unit_test_failures: [:partition_id], + p_ci_builds_metadata: [:partition_id] + } + + def up + return unless Gitlab.com? + + TABLES.each do |table_name, columns| + with_lock_retries do + columns.each do |column_name| # rubocop:disable Migration/WithLockRetriesDisallowedMethod + change_column_default(table_name, column_name, from: 100, to: 101) + end + end + end + end + + def down + return unless Gitlab.com? + + TABLES.each do |table_name, columns| + with_lock_retries do + columns.each do |column_name| # rubocop:disable Migration/WithLockRetriesDisallowedMethod + change_column_default(table_name, column_name, from: 101, to: 100) + end + end + end + end +end diff --git a/db/schema_migrations/20221220131020 b/db/schema_migrations/20221220131020 new file mode 100644 index 00000000000000..36c041b1a33027 --- /dev/null +++ b/db/schema_migrations/20221220131020 @@ -0,0 +1 @@ +8adf517eb859b5c945f70fbdeb911d398cf0a25c75b39b5991280390b70d1adf \ No newline at end of file -- GitLab From 541e8a5b46b867d13af94cae4ad3cf498456d50e Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Tue, 20 Dec 2022 16:03:09 +0200 Subject: [PATCH 2/6] Remove ci_resources from the list --- ...221220131020_bump_default_partition_id_value_for_ci_tables.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/db/post_migrate/20221220131020_bump_default_partition_id_value_for_ci_tables.rb b/db/post_migrate/20221220131020_bump_default_partition_id_value_for_ci_tables.rb index 889ef86767138e..22103c1d75e749 100644 --- a/db/post_migrate/20221220131020_bump_default_partition_id_value_for_ci_tables.rb +++ b/db/post_migrate/20221220131020_bump_default_partition_id_value_for_ci_tables.rb @@ -16,7 +16,6 @@ class BumpDefaultPartitionIdValueForCiTables < Gitlab::Database::Migration[2.1] ci_pending_builds: [:partition_id], ci_pipeline_variables: [:partition_id], ci_pipelines: [:partition_id], - ci_resources: [:partition_id], ci_running_builds: [:partition_id], ci_sources_pipelines: [:partition_id, :source_partition_id], ci_stages: [:partition_id], -- GitLab From 4f786603e371a97c63f95f50f037233222a1a20b Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Thu, 22 Dec 2022 12:27:17 +0200 Subject: [PATCH 3/6] Skip tables where the default was already set --- ...efault_partition_id_value_for_ci_tables.rb | 27 ++++--- ...t_partition_id_value_for_ci_tables_spec.rb | 71 +++++++++++++++++++ 2 files changed, 88 insertions(+), 10 deletions(-) create mode 100644 spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb diff --git a/db/post_migrate/20221220131020_bump_default_partition_id_value_for_ci_tables.rb b/db/post_migrate/20221220131020_bump_default_partition_id_value_for_ci_tables.rb index 22103c1d75e749..3d06f02a6d67f4 100644 --- a/db/post_migrate/20221220131020_bump_default_partition_id_value_for_ci_tables.rb +++ b/db/post_migrate/20221220131020_bump_default_partition_id_value_for_ci_tables.rb @@ -24,26 +24,33 @@ class BumpDefaultPartitionIdValueForCiTables < Gitlab::Database::Migration[2.1] } def up - return unless Gitlab.com? - - TABLES.each do |table_name, columns| - with_lock_retries do - columns.each do |column_name| # rubocop:disable Migration/WithLockRetriesDisallowedMethod - change_column_default(table_name, column_name, from: 100, to: 101) - end - end - end + change_partitions_default_value(from: 100, to: 101) end def down + change_partitions_default_value(from: 101, to: 100) + end + + private + + def change_partitions_default_value(from:, to:) return unless Gitlab.com? TABLES.each do |table_name, columns| + next if columns.all? { |column_name| default_value_for(table_name, column_name) == to } + with_lock_retries do columns.each do |column_name| # rubocop:disable Migration/WithLockRetriesDisallowedMethod - change_column_default(table_name, column_name, from: 101, to: 100) + change_column_default(table_name, column_name, from: from, to: to) end end end end + + def default_value_for(table_name, column_name) + connection + .columns(table_name) + .find { |column| column.name == column_name.to_s } + .default&.to_i + end end diff --git a/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb b/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb new file mode 100644 index 00000000000000..d118702bec1e93 --- /dev/null +++ b/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require_migration! + +RSpec.describe BumpDefaultPartitionIdValueForCiTables, feature_category: :continuous_integration_scaling do + context 'when on sass' do + before do + allow(Gitlab).to receive(:com?).and_return(true) + end + + it 'changes default values' do + reversible_migration do |migration| + migration.before -> { + expect(default_values).to match_array([100]) + } + + migration.after -> { + expect(default_values).to match_array([101]) + } + end + end + + it 'skips tables with default already set' do + active_record_base.connection.execute(<<~SQL) + ALTER TABLE ci_builds ALTER COLUMN partition_id SET DEFAULT 101 + SQL + + recorder = ActiveRecord::QueryRecorder.new { migrate! } + + expect(recorder.log.any?(/ALTER TABLE "ci_builds" ALTER COLUMN "partition_id" SET DEFAULT 101/)) + .to be_falsey + + expect(default_values).to match_array([101]) + + schema_migrate_down! + expect(default_values).to match_array([100]) + end + end + + context 'when self-managed' do + before do + allow(Gitlab).to receive(:com?).and_return(false) + end + + it 'does not change default values' do + reversible_migration do |migration| + migration.before -> { + expect(default_values).to match_array([100]) + } + + migration.after -> { + expect(default_values).to match_array([100]) + } + end + end + end + + def default_values + values = described_class::TABLES.flat_map do |table_name, columns| + active_record_base + .connection + .columns(table_name) + .select { |column| columns.include?(column.name.to_sym) } + .map { |column| column.default&.to_i } + end + + values.uniq + end +end -- GitLab From 16f7b65a5e9107f675cd119330bb5b8ba48c37db Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Thu, 22 Dec 2022 18:07:04 +0200 Subject: [PATCH 4/6] Attempt to fix tests by manual rollback --- ...t_partition_id_value_for_ci_tables_spec.rb | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb b/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb index d118702bec1e93..a6fbd61a799987 100644 --- a/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb +++ b/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb @@ -22,20 +22,27 @@ end end - it 'skips tables with default already set' do - active_record_base.connection.execute(<<~SQL) - ALTER TABLE ci_builds ALTER COLUMN partition_id SET DEFAULT 101 - SQL + context 'with tables already changed' do + before do + active_record_base.connection.execute(<<~SQL) + ALTER TABLE ci_builds ALTER COLUMN partition_id SET DEFAULT 101 + SQL + end - recorder = ActiveRecord::QueryRecorder.new { migrate! } + after do + schema_migrate_down! + end - expect(recorder.log.any?(/ALTER TABLE "ci_builds" ALTER COLUMN "partition_id" SET DEFAULT 101/)) - .to be_falsey + let(:alter_query) do + /ALTER TABLE "ci_builds" ALTER COLUMN "partition_id" SET DEFAULT 101/ + end - expect(default_values).to match_array([101]) + it 'skips updating already changed tables' do + recorder = ActiveRecord::QueryRecorder.new { migrate! } - schema_migrate_down! - expect(default_values).to match_array([100]) + expect(recorder.log.any?(alter_query)).to be_falsey + expect(default_values).to match_array([101]) + end end end -- GitLab From 3e961e058ee3196a90b4d7168aa39e5cfacc536e Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Tue, 3 Jan 2023 11:14:40 +0200 Subject: [PATCH 5/6] Add migration tag to reverse migrations --- ...131020_bump_default_partition_id_value_for_ci_tables_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb b/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb index a6fbd61a799987..c2d40d48afbbdb 100644 --- a/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb +++ b/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb @@ -4,7 +4,7 @@ require_migration! -RSpec.describe BumpDefaultPartitionIdValueForCiTables, feature_category: :continuous_integration_scaling do +RSpec.describe BumpDefaultPartitionIdValueForCiTables, :migration, feature_category: :continuous_integration_scaling do context 'when on sass' do before do allow(Gitlab).to receive(:com?).and_return(true) -- GitLab From 91850345b0a6e54a404b25b1dc9a3a72ef112d77 Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Tue, 3 Jan 2023 14:50:53 +0200 Subject: [PATCH 6/6] Change initial expectations to not include the new value --- ...20_bump_default_partition_id_value_for_ci_tables_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb b/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb index c2d40d48afbbdb..1a2300eb4af64c 100644 --- a/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb +++ b/spec/migrations/20221220131020_bump_default_partition_id_value_for_ci_tables_spec.rb @@ -13,7 +13,7 @@ it 'changes default values' do reversible_migration do |migration| migration.before -> { - expect(default_values).to match_array([100]) + expect(default_values).not_to include(101) } migration.after -> { @@ -54,11 +54,11 @@ it 'does not change default values' do reversible_migration do |migration| migration.before -> { - expect(default_values).to match_array([100]) + expect(default_values).not_to include(101) } migration.after -> { - expect(default_values).to match_array([100]) + expect(default_values).not_to include(101) } end end -- GitLab