From c229f25fb9e4bf4ecd98b081ccc9067f0dc52e50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Tue, 10 May 2022 14:56:13 -0300 Subject: [PATCH 1/6] Debug batch update Changelog: other --- app/models/namespace.rb | 2 +- ...20510175036_test_batch_migration_for_cr.rb | 57 +++++++++++++++++++ db/schema_migrations/20220510175036 | 1 + 3 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 db/post_migrate/20220510175036_test_batch_migration_for_cr.rb create mode 100644 db/schema_migrations/20220510175036 diff --git a/app/models/namespace.rb b/app/models/namespace.rb index af29850971f9bf..2d32a7430605f1 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -531,7 +531,7 @@ def storage_enforcement_date def certificate_based_clusters_enabled? ::Gitlab::SafeRequestStore.fetch("certificate_based_clusters:ns:#{self.id}") do - Feature.enabled?(:certificate_based_clusters, self, type: :ops) + Feature.enabled?(:certificate_based_clusters, self.root_ancestor, type: :ops) end end diff --git a/db/post_migrate/20220510175036_test_batch_migration_for_cr.rb b/db/post_migrate/20220510175036_test_batch_migration_for_cr.rb new file mode 100644 index 00000000000000..a5dd076924c5d0 --- /dev/null +++ b/db/post_migrate/20220510175036_test_batch_migration_for_cr.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class TestBatchMigrationForCr < Gitlab::Database::Migration[2.0] + # When using the methods "add_concurrent_index" or "remove_concurrent_index" + # you must disable the use of transactions + # as these methods can not run in an existing transaction. + # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure + # that either of them is the _only_ method called in the migration, + # any other changes should go in a separate migration. + # This ensures that upon failure _only_ the index creation or removing fails + # and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + disable_ddl_transaction! + # + # Configure the `gitlab_schema` to perform data manipulation (DML). + # Visit: https://docs.gitlab.com/ee/development/database/migrations_for_multiple_databases.html + restrict_gitlab_migration gitlab_schema: :gitlab_main + + def up + root_ancestor_set = Set[] + + Group.where(id: + Group.joins(:cluster_groups) + .select('DISTINCT(traversal_ids[1])') + ).find_each(batch_size: 1000) do |namespace| + root_ancestor_set.add(namespace) + end + + Namespace.where(id: + Namespace.joins(projects: :cluster_project) + .select('DISTINCT(traversal_ids[1])') + ).find_each(batch_size: 1000) do |namespace| + root_ancestor_set.add(namespace) + end + + # Group.joins(:cluster_groups).distinct.find_each(batch_size: 1000) do |namespace| + # root_ancestor_set.add(namespace.root_ancestor) + # end + # + # Namespace.joins(projects: :cluster_project).distinct.find_each(batch_size: 1000) do |namespace| + # root_ancestor_set.add(namespace.root_ancestor) + # end + + root_ancestor_set.each do |root_ancestor| + Feature.enable(:certificate_based_clusters, root_ancestor) + end + end + + def down + # no op + end +end diff --git a/db/schema_migrations/20220510175036 b/db/schema_migrations/20220510175036 new file mode 100644 index 00000000000000..3bbeb6a64c0db3 --- /dev/null +++ b/db/schema_migrations/20220510175036 @@ -0,0 +1 @@ +42bc24fe7c786a20e5a32f8bd024062c27a3d7142e99279c213d17867543b5e5 \ No newline at end of file -- GitLab From beeb662dc91d45fa6a8c4c31327a4983bee8a3be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Wed, 18 May 2022 23:30:21 -0300 Subject: [PATCH 2/6] Create ClusterEnabledGrant model and populates table This new table is going to be used to temporary permit a set of users to use the certificate-based-clusters feature, which allowing us to remove for the majority of other users. On our first attemp we tried to do this with FF only on the namespace leve. But the spectrum of users that need this feature enabled for longer is too big. So big that toggling the FF for all these actors is something that our FF framework wasn't ready to handle. So checking the enabled users directly from a table will be much simpler, faster and easy on memory consumption. Changelog: other --- app/models/clusters/cluster_enabled_grant.rb | 9 +++ app/models/namespace.rb | 14 +++- db/docs/cluster_enabled_grants.yml | 9 +++ ...519013213_create_cluster_enabled_grants.rb | 12 +++ ...20510175036_test_batch_migration_for_cr.rb | 81 ++++++++++--------- db/schema_migrations/20220519013213 | 1 + db/structure.sql | 25 ++++++ lib/gitlab/database/gitlab_schemas.yml | 1 + .../clusters/cluster_enabled_grant.rb | 7 ++ .../clusters/cluster_enabled_grant_spec.rb | 7 ++ spec/models/namespace_spec.rb | 43 +++++----- 11 files changed, 149 insertions(+), 60 deletions(-) create mode 100644 app/models/clusters/cluster_enabled_grant.rb create mode 100644 db/docs/cluster_enabled_grants.yml create mode 100644 db/migrate/20220519013213_create_cluster_enabled_grants.rb create mode 100644 db/schema_migrations/20220519013213 create mode 100644 spec/factories/clusters/cluster_enabled_grant.rb create mode 100644 spec/models/clusters/cluster_enabled_grant_spec.rb diff --git a/app/models/clusters/cluster_enabled_grant.rb b/app/models/clusters/cluster_enabled_grant.rb new file mode 100644 index 00000000000000..4dca6a78759b94 --- /dev/null +++ b/app/models/clusters/cluster_enabled_grant.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module Clusters + class ClusterEnabledGrant < ApplicationRecord + self.table_name = 'cluster_enabled_grants' + + belongs_to :namespace + end +end diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 2d32a7430605f1..6fc08006344abd 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -73,6 +73,8 @@ class Namespace < ApplicationRecord has_one :ci_namespace_mirror, class_name: 'Ci::NamespaceMirror' has_many :sync_events, class_name: 'Namespaces::SyncEvent' + has_one :cluster_enabled_grant, inverse_of: :namespace, class_name: 'Clusters::ClusterEnabledGrant' + validates :owner, presence: true, if: ->(n) { n.owner_required? } validates :name, presence: true, @@ -530,13 +532,19 @@ def storage_enforcement_date end def certificate_based_clusters_enabled? - ::Gitlab::SafeRequestStore.fetch("certificate_based_clusters:ns:#{self.id}") do - Feature.enabled?(:certificate_based_clusters, self.root_ancestor, type: :ops) - end + cluster_enabled_granted? || certificate_based_clusters_enabled_ff? end private + def cluster_enabled_granted? + root_ancestor.cluster_enabled_grant.present? + end + + def certificate_based_clusters_enabled_ff? + Feature.enabled?(:certificate_based_clusters, type: :ops) + end + def expire_child_caches Namespace.where(id: descendants).each_batch do |namespaces| namespaces.touch_all diff --git a/db/docs/cluster_enabled_grants.yml b/db/docs/cluster_enabled_grants.yml new file mode 100644 index 00000000000000..7a8faba26d69ed --- /dev/null +++ b/db/docs/cluster_enabled_grants.yml @@ -0,0 +1,9 @@ +--- +table_name: cluster_enabled_grants +classes: +- Clusters::ClusterEnabledGrant +feature_categories: +- kubernetes_management +description: Persists information about namespaces which got an extended life for certificate based clusters +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/87149 +milestone: '15.1' diff --git a/db/migrate/20220519013213_create_cluster_enabled_grants.rb b/db/migrate/20220519013213_create_cluster_enabled_grants.rb new file mode 100644 index 00000000000000..45c18ecca45112 --- /dev/null +++ b/db/migrate/20220519013213_create_cluster_enabled_grants.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class CreateClusterEnabledGrants < Gitlab::Database::Migration[2.0] + enable_lock_retries! + + def change + create_table :cluster_enabled_grants do |t| + t.references :namespace, index: { unique: true }, null: false, foreign_key: { on_delete: :cascade } + t.datetime_with_timezone :created_at, null: false + end + end +end diff --git a/db/post_migrate/20220510175036_test_batch_migration_for_cr.rb b/db/post_migrate/20220510175036_test_batch_migration_for_cr.rb index a5dd076924c5d0..b107c7313f9acd 100644 --- a/db/post_migrate/20220510175036_test_batch_migration_for_cr.rb +++ b/db/post_migrate/20220510175036_test_batch_migration_for_cr.rb @@ -4,54 +4,57 @@ # for more information on how to write migrations for GitLab. class TestBatchMigrationForCr < Gitlab::Database::Migration[2.0] - # When using the methods "add_concurrent_index" or "remove_concurrent_index" - # you must disable the use of transactions - # as these methods can not run in an existing transaction. - # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure - # that either of them is the _only_ method called in the migration, - # any other changes should go in a separate migration. - # This ensures that upon failure _only_ the index creation or removing fails - # and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - disable_ddl_transaction! - # - # Configure the `gitlab_schema` to perform data manipulation (DML). - # Visit: https://docs.gitlab.com/ee/development/database/migrations_for_multiple_databases.html restrict_gitlab_migration gitlab_schema: :gitlab_main - def up - root_ancestor_set = Set[] + disable_ddl_transaction! - Group.where(id: - Group.joins(:cluster_groups) - .select('DISTINCT(traversal_ids[1])') - ).find_each(batch_size: 1000) do |namespace| - root_ancestor_set.add(namespace) - end + class ClusterEnabledGrant < MigrationRecord + self.table_name = 'cluster_enabled_grants' + end - Namespace.where(id: - Namespace.joins(projects: :cluster_project) - .select('DISTINCT(traversal_ids[1])') - ).find_each(batch_size: 1000) do |namespace| - root_ancestor_set.add(namespace) + def up + Group + .select(:id) + .where(id: + Group.joins(:cluster_groups) + .select('DISTINCT(traversal_ids[1])') + ) + .each_batch(of: 1000) do |batch| + values = batch.map do |namespace| + "(#{namespace.id}, NOW())" + end.join(',') + + bulk_insert_query = <<-SQL + INSERT INTO cluster_enabled_grants (namespace_id) + VALUES #{values} + ON CONFLICT (namespace_id) DO NOTHING; + SQL + + connection.execute(bulk_insert_query) end - # Group.joins(:cluster_groups).distinct.find_each(batch_size: 1000) do |namespace| - # root_ancestor_set.add(namespace.root_ancestor) - # end - # - # Namespace.joins(projects: :cluster_project).distinct.find_each(batch_size: 1000) do |namespace| - # root_ancestor_set.add(namespace.root_ancestor) - # end - - root_ancestor_set.each do |root_ancestor| - Feature.enable(:certificate_based_clusters, root_ancestor) + Namespace + .select(:id) + .where(id: + Namespace.joins(projects: :cluster_project) + .select('DISTINCT(traversal_ids[1])') + ) + .each_batch(of: 1000) do |batch| + values = batch.map do |namespace| + "(#{namespace.id}, NOW())" + end.join(',') + + bulk_insert_query = <<-SQL + INSERT INTO cluster_enabled_grants (namespace_id, created_at) + VALUES #{values} + ON CONFLICT (namespace_id) DO NOTHING; + SQL + + connection.execute(bulk_insert_query) end end def down - # no op + ClusterEnabledGrant.delete_all end end diff --git a/db/schema_migrations/20220519013213 b/db/schema_migrations/20220519013213 new file mode 100644 index 00000000000000..c3575b668e4e0b --- /dev/null +++ b/db/schema_migrations/20220519013213 @@ -0,0 +1 @@ +d8ae65034a7768c238a65c4c36d709364dee65652da93c368774e3828b0edb41 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index db235d6d2d6994..26897ba471f34e 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -13341,6 +13341,21 @@ CREATE SEQUENCE cluster_agents_id_seq ALTER SEQUENCE cluster_agents_id_seq OWNED BY cluster_agents.id; +CREATE TABLE cluster_enabled_grants ( + id bigint NOT NULL, + namespace_id bigint NOT NULL, + created_at timestamp with time zone NOT NULL +); + +CREATE SEQUENCE cluster_enabled_grants_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE cluster_enabled_grants_id_seq OWNED BY cluster_enabled_grants.id; + CREATE TABLE cluster_groups ( id integer NOT NULL, cluster_id integer NOT NULL, @@ -22607,6 +22622,8 @@ ALTER TABLE ONLY cluster_agent_tokens ALTER COLUMN id SET DEFAULT nextval('clust ALTER TABLE ONLY cluster_agents ALTER COLUMN id SET DEFAULT nextval('cluster_agents_id_seq'::regclass); +ALTER TABLE ONLY cluster_enabled_grants ALTER COLUMN id SET DEFAULT nextval('cluster_enabled_grants_id_seq'::regclass); + ALTER TABLE ONLY cluster_groups ALTER COLUMN id SET DEFAULT nextval('cluster_groups_id_seq'::regclass); ALTER TABLE ONLY cluster_platforms_kubernetes ALTER COLUMN id SET DEFAULT nextval('cluster_platforms_kubernetes_id_seq'::regclass); @@ -24331,6 +24348,9 @@ ALTER TABLE ONLY cluster_agent_tokens ALTER TABLE ONLY cluster_agents ADD CONSTRAINT cluster_agents_pkey PRIMARY KEY (id); +ALTER TABLE ONLY cluster_enabled_grants + ADD CONSTRAINT cluster_enabled_grants_pkey PRIMARY KEY (id); + ALTER TABLE ONLY cluster_groups ADD CONSTRAINT cluster_groups_pkey PRIMARY KEY (id); @@ -27392,6 +27412,8 @@ CREATE INDEX index_cluster_agents_on_created_by_user_id ON cluster_agents USING CREATE UNIQUE INDEX index_cluster_agents_on_project_id_and_name ON cluster_agents USING btree (project_id, name); +CREATE UNIQUE INDEX index_cluster_enabled_grants_on_namespace_id ON cluster_enabled_grants USING btree (namespace_id); + CREATE UNIQUE INDEX index_cluster_groups_on_cluster_id_and_group_id ON cluster_groups USING btree (cluster_id, group_id); CREATE INDEX index_cluster_groups_on_group_id ON cluster_groups USING btree (group_id); @@ -32843,6 +32865,9 @@ ALTER TABLE ONLY approval_merge_request_rules_users ALTER TABLE ONLY required_code_owners_sections ADD CONSTRAINT fk_rails_817708cf2d FOREIGN KEY (protected_branch_id) REFERENCES protected_branches(id) ON DELETE CASCADE; +ALTER TABLE ONLY cluster_enabled_grants + ADD CONSTRAINT fk_rails_8336ce35af FOREIGN KEY (namespace_id) REFERENCES namespaces(id) ON DELETE CASCADE; + ALTER TABLE ONLY dast_site_profiles ADD CONSTRAINT fk_rails_83e309d69e FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; diff --git a/lib/gitlab/database/gitlab_schemas.yml b/lib/gitlab/database/gitlab_schemas.yml index d779da6e597f73..7ba6249b5983b3 100644 --- a/lib/gitlab/database/gitlab_schemas.yml +++ b/lib/gitlab/database/gitlab_schemas.yml @@ -121,6 +121,7 @@ ci_unit_tests: :gitlab_ci ci_variables: :gitlab_ci cluster_agents: :gitlab_main cluster_agent_tokens: :gitlab_main +cluster_enabled_grants: :gitlab_main cluster_groups: :gitlab_main cluster_platforms_kubernetes: :gitlab_main cluster_projects: :gitlab_main diff --git a/spec/factories/clusters/cluster_enabled_grant.rb b/spec/factories/clusters/cluster_enabled_grant.rb new file mode 100644 index 00000000000000..f995bc876f3e16 --- /dev/null +++ b/spec/factories/clusters/cluster_enabled_grant.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :cluster_enabled_grant, class: 'Clusters::ClusterEnabledGrant' do + namespace + end +end diff --git a/spec/models/clusters/cluster_enabled_grant_spec.rb b/spec/models/clusters/cluster_enabled_grant_spec.rb new file mode 100644 index 00000000000000..1418d854b41c16 --- /dev/null +++ b/spec/models/clusters/cluster_enabled_grant_spec.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Clusters::ClusterEnabledGrant do + it { is_expected.to belong_to :namespace } +end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 4373d9a0b24ee4..02d2c9a88affe5 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -31,6 +31,7 @@ it { is_expected.to have_many :pending_builds } it { is_expected.to have_one :namespace_route } it { is_expected.to have_many :namespace_members } + it { is_expected.to have_one :cluster_enabled_grant } it do is_expected.to have_one(:ci_cd_settings).class_name('NamespaceCiCdSetting').inverse_of(:namespace).autosave(true) @@ -2251,27 +2252,23 @@ def project_rugged(project) end describe '#certificate_based_clusters_enabled?' do - it 'does not call Feature.enabled? twice with request_store', :request_store do - expect(Feature).to receive(:enabled?).once - - namespace.certificate_based_clusters_enabled? - namespace.certificate_based_clusters_enabled? - end - - it 'call Feature.enabled? twice without request_store' do - expect(Feature).to receive(:enabled?).twice - - namespace.certificate_based_clusters_enabled? - namespace.certificate_based_clusters_enabled? - end - context 'with ff disabled' do before do stub_feature_flags(certificate_based_clusters: false) end - it 'is truthy' do - expect(namespace.certificate_based_clusters_enabled?).to be_falsy + context 'with a cluster_enabled_grant' do + it 'is truthy' do + create(:cluster_enabled_grant, namespace: namespace) + + expect(namespace.certificate_based_clusters_enabled?).to be_truthy + end + end + + context 'without a cluster_enabled_grant' do + it 'is falsy' do + expect(namespace.certificate_based_clusters_enabled?).to be_falsy + end end end @@ -2280,8 +2277,18 @@ def project_rugged(project) stub_feature_flags(certificate_based_clusters: true) end - it 'is truthy' do - expect(namespace.certificate_based_clusters_enabled?).to be_truthy + context 'with a cluster_enabled_grant' do + it 'is truthy' do + create(:cluster_enabled_grant, namespace: namespace) + + expect(namespace.certificate_based_clusters_enabled?).to be_truthy + end + end + + context 'without a cluster_enabled_grant' do + it 'is truthy' do + expect(namespace.certificate_based_clusters_enabled?).to be_truthy + end end end end -- GitLab From a1fdcaea72c51e0094bbf9c238ac12e409763d45 Mon Sep 17 00:00:00 2001 From: Tiger Date: Thu, 19 May 2022 14:53:48 +1000 Subject: [PATCH 3/6] Regenerate data migration --- ..._for_cr.rb => 20220519045133_test_batch_migration_for_cr.rb} | 2 +- db/schema_migrations/20220510175036 | 1 - db/schema_migrations/20220519045133 | 1 + 3 files changed, 2 insertions(+), 2 deletions(-) rename db/post_migrate/{20220510175036_test_batch_migration_for_cr.rb => 20220519045133_test_batch_migration_for_cr.rb} (95%) delete mode 100644 db/schema_migrations/20220510175036 create mode 100644 db/schema_migrations/20220519045133 diff --git a/db/post_migrate/20220510175036_test_batch_migration_for_cr.rb b/db/post_migrate/20220519045133_test_batch_migration_for_cr.rb similarity index 95% rename from db/post_migrate/20220510175036_test_batch_migration_for_cr.rb rename to db/post_migrate/20220519045133_test_batch_migration_for_cr.rb index b107c7313f9acd..85ab35a8adc9d3 100644 --- a/db/post_migrate/20220510175036_test_batch_migration_for_cr.rb +++ b/db/post_migrate/20220519045133_test_batch_migration_for_cr.rb @@ -25,7 +25,7 @@ def up end.join(',') bulk_insert_query = <<-SQL - INSERT INTO cluster_enabled_grants (namespace_id) + INSERT INTO cluster_enabled_grants (namespace_id, created_at) VALUES #{values} ON CONFLICT (namespace_id) DO NOTHING; SQL diff --git a/db/schema_migrations/20220510175036 b/db/schema_migrations/20220510175036 deleted file mode 100644 index 3bbeb6a64c0db3..00000000000000 --- a/db/schema_migrations/20220510175036 +++ /dev/null @@ -1 +0,0 @@ -42bc24fe7c786a20e5a32f8bd024062c27a3d7142e99279c213d17867543b5e5 \ No newline at end of file diff --git a/db/schema_migrations/20220519045133 b/db/schema_migrations/20220519045133 new file mode 100644 index 00000000000000..099a74f8b34894 --- /dev/null +++ b/db/schema_migrations/20220519045133 @@ -0,0 +1 @@ +99fd05c3102300c115edf09a54feddfd9721bf63ae09063e6dc9d568be6d8f1f \ No newline at end of file -- GitLab From 19c33eb1b96c21c966b85b23ef734b46e7170c2a Mon Sep 17 00:00:00 2001 From: Tiger Date: Thu, 19 May 2022 15:36:15 +1000 Subject: [PATCH 4/6] Process cluster groups/projects in smaller batches --- ...20519045133_test_batch_migration_for_cr.rb | 56 +++++++------------ 1 file changed, 21 insertions(+), 35 deletions(-) diff --git a/db/post_migrate/20220519045133_test_batch_migration_for_cr.rb b/db/post_migrate/20220519045133_test_batch_migration_for_cr.rb index 85ab35a8adc9d3..eeea85cb1a90d4 100644 --- a/db/post_migrate/20220519045133_test_batch_migration_for_cr.rb +++ b/db/post_migrate/20220519045133_test_batch_migration_for_cr.rb @@ -8,53 +8,39 @@ class TestBatchMigrationForCr < Gitlab::Database::Migration[2.0] disable_ddl_transaction! - class ClusterEnabledGrant < MigrationRecord - self.table_name = 'cluster_enabled_grants' - end - def up - Group - .select(:id) - .where(id: - Group.joins(:cluster_groups) - .select('DISTINCT(traversal_ids[1])') - ) - .each_batch(of: 1000) do |batch| - values = batch.map do |namespace| - "(#{namespace.id}, NOW())" - end.join(',') - - bulk_insert_query = <<-SQL + define_batchable_model('cluster_groups').each_batch do |batch| + min, max = batch.pick('MIN(id), MAX(id)') + + bulk_insert = <<-SQL INSERT INTO cluster_enabled_grants (namespace_id, created_at) - VALUES #{values} - ON CONFLICT (namespace_id) DO NOTHING; + SELECT DISTINCT(traversal_ids[1]), NOW() + FROM cluster_groups + INNER JOIN namespaces ON cluster_groups.group_id = namespaces.id + WHERE cluster_groups.id BETWEEN #{min} AND #{max} + ON CONFLICT (namespace_id) DO NOTHING SQL - connection.execute(bulk_insert_query) + connection.execute(bulk_insert) end - Namespace - .select(:id) - .where(id: - Namespace.joins(projects: :cluster_project) - .select('DISTINCT(traversal_ids[1])') - ) - .each_batch(of: 1000) do |batch| - values = batch.map do |namespace| - "(#{namespace.id}, NOW())" - end.join(',') - - bulk_insert_query = <<-SQL + define_batchable_model('cluster_projects').each_batch do |batch| + min, max = batch.pick('MIN(id), MAX(id)') + + bulk_insert = <<-SQL INSERT INTO cluster_enabled_grants (namespace_id, created_at) - VALUES #{values} - ON CONFLICT (namespace_id) DO NOTHING; + SELECT DISTINCT(traversal_ids[1]), NOW() + FROM cluster_projects + INNER JOIN projects ON cluster_projects.project_id = projects.id + INNER JOIN namespaces on projects.namespace_id = namespaces.id + WHERE cluster_projects.id BETWEEN #{min} AND #{max} + ON CONFLICT (namespace_id) DO NOTHING SQL - connection.execute(bulk_insert_query) + connection.execute(bulk_insert) end end def down - ClusterEnabledGrant.delete_all end end -- GitLab From 33f93f14f6a63174365e7945b88dffd95ca86d32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Thu, 19 May 2022 09:32:18 -0300 Subject: [PATCH 5/6] Renames migration and adds testing The former migraiton name was not meaningfull, as the migration was firstly created with testing purposes. --- ...133_bulk_insert_cluster_enabled_grants.rb} | 8 +- ...bulk_insert_cluster_enabled_grants_spec.rb | 85 +++++++++++++++++++ 2 files changed, 89 insertions(+), 4 deletions(-) rename db/post_migrate/{20220519045133_test_batch_migration_for_cr.rb => 20220519045133_bulk_insert_cluster_enabled_grants.rb} (86%) create mode 100644 spec/migrations/bulk_insert_cluster_enabled_grants_spec.rb diff --git a/db/post_migrate/20220519045133_test_batch_migration_for_cr.rb b/db/post_migrate/20220519045133_bulk_insert_cluster_enabled_grants.rb similarity index 86% rename from db/post_migrate/20220519045133_test_batch_migration_for_cr.rb rename to db/post_migrate/20220519045133_bulk_insert_cluster_enabled_grants.rb index eeea85cb1a90d4..6c1d90586738aa 100644 --- a/db/post_migrate/20220519045133_test_batch_migration_for_cr.rb +++ b/db/post_migrate/20220519045133_bulk_insert_cluster_enabled_grants.rb @@ -1,14 +1,13 @@ # frozen_string_literal: true -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class TestBatchMigrationForCr < Gitlab::Database::Migration[2.0] +class BulkInsertClusterEnabledGrants < Gitlab::Database::Migration[2.0] restrict_gitlab_migration gitlab_schema: :gitlab_main disable_ddl_transaction! def up + return unless Gitlab.dev_or_test_env? || Gitlab.com? + define_batchable_model('cluster_groups').each_batch do |batch| min, max = batch.pick('MIN(id), MAX(id)') @@ -42,5 +41,6 @@ def up end def down + # no-op end end diff --git a/spec/migrations/bulk_insert_cluster_enabled_grants_spec.rb b/spec/migrations/bulk_insert_cluster_enabled_grants_spec.rb new file mode 100644 index 00000000000000..a359a78ab45b55 --- /dev/null +++ b/spec/migrations/bulk_insert_cluster_enabled_grants_spec.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe BulkInsertClusterEnabledGrants, :migration do + let(:migration) { described_class.new } + + let(:cluster_enabled_grants) { table(:cluster_enabled_grants) } + let(:namespaces) { table(:namespaces) } + let(:cluster_projects) { table(:cluster_projects) } + let(:cluster_groups) { table(:cluster_groups) } + let(:clusters) { table(:clusters) } + let(:projects) { table(:projects) } + + context 'with namespaces, cluster_groups and cluster_projects' do + it 'creates unique cluster_enabled_grants for root_namespaces with clusters' do + # Does not create grants for namespaces without clusters + namespaces.create!(id: 1, path: 'eee', name: 'eee', traversal_ids: [1]) # not used + + # Creates unique grant for a root namespace with its own cluster + root_ns_with_own_cluster = namespaces.create!(id: 2, path: 'ddd', name: 'ddd', traversal_ids: [2]) + cluster_root_ns_with_own_cluster = clusters.create!(name: 'cluster_root_ns_with_own_cluster') + cluster_groups.create!( + cluster_id: cluster_root_ns_with_own_cluster.id, + group_id: root_ns_with_own_cluster.id) + + # Creates unique grant for namespaces with multiple sub-group clusters + root_ns_with_sub_group_clusters = namespaces.create!(id: 3, path: 'aaa', name: 'aaa', traversal_ids: [3]) + + subgroup_1 = namespaces.create!( + id: 4, + path: 'bbb', + name: 'bbb', + parent_id: root_ns_with_sub_group_clusters.id, + traversal_ids: [root_ns_with_sub_group_clusters.id, 4]) + cluster_subgroup_1 = clusters.create!(name: 'cluster_subgroup_1') + cluster_groups.create!(cluster_id: cluster_subgroup_1.id, group_id: subgroup_1.id) + + subgroup_2 = namespaces.create!( + id: 5, + path: 'ccc', + name: 'ccc', + parent_id: subgroup_1.id, + traversal_ids: [root_ns_with_sub_group_clusters.id, subgroup_1.id, 5]) + cluster_subgroup_2 = clusters.create!(name: 'cluster_subgroup_2') + cluster_groups.create!(cluster_id: cluster_subgroup_2.id, group_id: subgroup_2.id) + + # Creates unique grant for a root namespace with multiple projects clusters + root_ns_with_project_group_clusters = namespaces.create!(id: 6, path: 'fff', name: 'fff', traversal_ids: [6]) + + project_namespace_1 = namespaces.create!(id: 7, path: 'ggg', name: 'ggg', traversal_ids: [7]) + project_1 = projects.create!( + name: 'project_1', + namespace_id: root_ns_with_project_group_clusters.id, + project_namespace_id: project_namespace_1.id) + cluster_project_1 = clusters.create!(name: 'cluster_project_1') + cluster_projects.create!(cluster_id: cluster_project_1.id, project_id: project_1.id) + + project_namespace_2 = namespaces.create!(id: 8, path: 'hhh', name: 'hhh', traversal_ids: [8]) + project_2 = projects.create!( + name: 'project_2', + namespace_id: root_ns_with_project_group_clusters.id, + project_namespace_id: project_namespace_2.id) + cluster_project_2 = clusters.create!(name: 'cluster_project_2') + cluster_projects.create!(cluster_id: cluster_project_2.id, project_id: project_2.id) + + migrate! + + expected_cluster_enabled_grants = [ + root_ns_with_sub_group_clusters.id, + root_ns_with_own_cluster.id, + root_ns_with_project_group_clusters.id + ] + + expect(cluster_enabled_grants.pluck(:namespace_id)).to match_array(expected_cluster_enabled_grants) + end + end + + context 'without namespaces, cluster_groups or cluster_projects' do + it 'does nothing' do + expect { migrate! }.not_to change { cluster_enabled_grants.count } + end + end +end -- GitLab From 34ceeacd2a1fe7bea06003fa3158102f5f941eb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Thu, 19 May 2022 11:42:35 -0300 Subject: [PATCH 6/6] Stop checking cluster_enabled_grants for self-managed This table should be used as a GitLab.com only table as we don't want self-managed to be able to use it, cause it would interfere with our metrics counting how many self-managed users have opted-in into enabling the certificate_based_clusters FF globally. --- app/models/namespace.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 6fc08006344abd..6234d1fa6825fa 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -538,7 +538,7 @@ def certificate_based_clusters_enabled? private def cluster_enabled_granted? - root_ancestor.cluster_enabled_grant.present? + root_ancestor.cluster_enabled_grant.present? && (Gitlab.com? || Gitlab.dev_or_test_env?) end def certificate_based_clusters_enabled_ff? -- GitLab