diff --git a/app/models/namespace.rb b/app/models/namespace.rb index cc60a64bc9c581ddcb3bb428f60b8bc3bf21f1cb..238556f0cf03ddd972189a4dc8bf29afe77aca10 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -12,6 +12,7 @@ class Namespace < ApplicationRecord include Gitlab::Utils::StrongMemoize include Namespaces::Traversal::Recursive include Namespaces::Traversal::Linear + include Namespaces::Traversal::Cached include EachBatch include BlocksUnsafeSerialization include Ci::NamespaceSettings diff --git a/app/models/namespaces/descendants.rb b/app/models/namespaces/descendants.rb index 99abda2dd6a2a01b8d70930d22f521217d247515..8444cea9848d29d7f92a9576d6ec75def2c55416 100644 --- a/app/models/namespaces/descendants.rb +++ b/app/models/namespaces/descendants.rb @@ -7,5 +7,24 @@ class Descendants < ApplicationRecord belongs_to :namespace validates :namespace_id, uniqueness: true + + def self.expire_for(namespace_ids) + # Union: + # - Look up all parent ids including the given ids via traversal_ids + # - Include the given ids to handle the case when the namespaces records are already deleted + sql = <<~SQL + WITH namespace_ids AS MATERIALIZED ( + ( + SELECT ids.id + FROM namespaces, UNNEST(traversal_ids) ids(id) + WHERE namespaces.id IN (?) + ) UNION + (SELECT UNNEST(ARRAY[?]) AS id) + ) + UPDATE namespace_descendants SET outdated_at = ? FROM namespace_ids WHERE namespace_descendants.namespace_id = namespace_ids.id + SQL + + connection.execute(sanitize_sql_array([sql, namespace_ids, namespace_ids, Time.current])) + end end end diff --git a/app/models/namespaces/traversal/cached.rb b/app/models/namespaces/traversal/cached.rb new file mode 100644 index 0000000000000000000000000000000000000000..55eaaa4667ef17261073d45fe9f67d18b11ee460 --- /dev/null +++ b/app/models/namespaces/traversal/cached.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module Namespaces + module Traversal + module Cached + extend ActiveSupport::Concern + extend Gitlab::Utils::Override + + included do + after_destroy :invalidate_descendants_cache + end + + private + + override :sync_traversal_ids + def sync_traversal_ids + super + return if is_a?(Namespaces::UserNamespace) + return unless Feature.enabled?(:namespace_descendants_cache_expiration, self, type: :gitlab_com_derisk) + + ids = [id] + ids.concat((saved_changes[:parent_id] - [parent_id]).compact) if saved_changes[:parent_id] + Namespaces::Descendants.expire_for(ids) + end + + def invalidate_descendants_cache + return if is_a?(Namespaces::UserNamespace) + return unless Feature.enabled?(:namespace_descendants_cache_expiration, self, type: :gitlab_com_derisk) + + Namespaces::Descendants.expire_for([parent_id, id].compact) + end + end + end +end diff --git a/config/feature_flags/gitlab_com_derisk/namespace_descendants_cache_expiration.yml b/config/feature_flags/gitlab_com_derisk/namespace_descendants_cache_expiration.yml new file mode 100644 index 0000000000000000000000000000000000000000..d374316e271ac9fb0064a2cba4629cc808411caf --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/namespace_descendants_cache_expiration.yml @@ -0,0 +1,9 @@ +--- +name: namespace_descendants_cache_expiration +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/433482 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141588 +rollout_issue_url: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/17388 +milestone: '16.8' +group: group::optimize +type: gitlab_com_derisk +default_enabled: false diff --git a/spec/models/namespaces/descendants_spec.rb b/spec/models/namespaces/descendants_spec.rb index 26d3619ea392603aadee26852ab876fa4d0842c3..6c153c3307b3bab1a90f9e8cdee07fbae0e2388c 100644 --- a/spec/models/namespaces/descendants_spec.rb +++ b/spec/models/namespaces/descendants_spec.rb @@ -40,4 +40,29 @@ ) end end + + describe '.expire_for' do + it 'sets the outdated_at column for the given namespace ids' do + freeze_time do + expire_time = Time.current + + group1 = create(:group).tap do |g| + create(:namespace_descendants, namespace: g).reload.update!(outdated_at: nil) + end + group2 = create(:group, parent: group1).tap { |g| create(:namespace_descendants, namespace: g) } + group3 = create(:group, parent: group1) + + group4 = create(:group).tap do |g| + create(:namespace_descendants, namespace: g).reload.update!(outdated_at: nil) + end + + described_class.expire_for([group1.id, group2.id, group3.id]) + + expect(group1.namespace_descendants.outdated_at).to eq(expire_time) + expect(group2.namespace_descendants.outdated_at).to eq(expire_time) + expect(group3.namespace_descendants).to be_nil + expect(group4.namespace_descendants.outdated_at).to be_nil + end + end + end end diff --git a/spec/models/namespaces/traversal/cached_spec.rb b/spec/models/namespaces/traversal/cached_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..8263e28bb985399e2955cc64e22a33cf493cc8a9 --- /dev/null +++ b/spec/models/namespaces/traversal/cached_spec.rb @@ -0,0 +1,104 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Namespaces::Traversal::Cached, feature_category: :database do + let_it_be_with_refind(:old_parent) { create(:group) } + let_it_be_with_refind(:new_parent) { create(:group) } + let_it_be_with_refind(:group) { create(:group, parent: old_parent) } + let_it_be_with_refind(:subgroup) { create(:group, parent: group) } + + context 'when the namespace_descendants_cache_expiration feature flag is off' do + let!(:cache) { create(:namespace_descendants, namespace: group) } + + before do + stub_feature_flags(namespace_descendants_cache_expiration: false) + end + + it 'does not invalidate the cache' do + expect { group.update!(parent: new_parent) }.not_to change { cache.reload.outdated_at } + end + + context 'when the group is deleted' do + it 'invalidates the cache' do + expect { group.destroy! }.not_to change { cache.reload.outdated_at } + end + end + end + + context 'when no cached records are present' do + it 'does nothing' do + group.parent = new_parent + + expect { group.save! }.not_to change { Namespaces::Descendants.all.to_a } + end + end + + context 'when the namespace record is UserNamespace' do + it 'does nothing' do + # we won't use the optimization for UserNamespace + namespace = create(:user_namespace) + cache = create(:namespace_descendants, namespace: namespace) + + expect { namespace.destroy! }.not_to change { cache.reload.outdated_at } + end + end + + context 'when cached record is present' do + let!(:cache) { create(:namespace_descendants, namespace: group) } + + it 'invalidates the cache' do + expect { group.update!(parent: new_parent) }.to change { cache.reload.outdated_at }.from(nil) + end + + it 'does not invalidate the cache of subgroups' do + subgroup_cache = create(:namespace_descendants, namespace: subgroup) + + expect { group.update!(parent: new_parent) }.not_to change { subgroup_cache.reload.outdated_at } + end + + context 'when a new subgroup is added' do + it 'invalidates the cache' do + expect { create(:group, parent: group) }.to change { cache.reload.outdated_at } + end + end + + context 'when a new project is added' do + it 'invalidates the cache' do + expect { create(:project, group: group) }.to change { cache.reload.outdated_at } + end + end + end + + context 'when parent group has cached record' do + it 'invalidates the parent cache' do + old_parent_cache = create(:namespace_descendants, namespace: old_parent) + new_parent_cache = create(:namespace_descendants, namespace: new_parent) + + group.update!(parent: new_parent) + + expect(old_parent_cache.reload.outdated_at).not_to be_nil + expect(new_parent_cache.reload.outdated_at).not_to be_nil + end + end + + context 'when group is destroyed' do + it 'invalidates the cache' do + cache = create(:namespace_descendants, namespace: group) + + expect { group.destroy! }.to change { cache.reload.outdated_at }.from(nil) + end + + context 'when parent group has cached record' do + it 'invalidates the parent cache' do + old_parent_cache = create(:namespace_descendants, namespace: old_parent) + new_parent_cache = create(:namespace_descendants, namespace: new_parent) + + group.destroy! + + expect(old_parent_cache.reload.outdated_at).not_to be_nil + expect(new_parent_cache.reload.outdated_at).to be_nil # no change + end + end + end +end