Merge branch '64079-aggregation-schedule-should-keep-lease-until-timeout' into 'master'

Implements `lease_release?` on `NamespaceAggregation`

Closes #64079

See merge request gitlab-org/gitlab-ce!30305

3 files changed, 63 insertions, 15 deletions

diff --git a/app/models/namespace/aggregation_schedule.rb b/app/models/namespace/aggregation_schedule.rb
index 355593597c6..0bef352cf24 100644
--- a/app/models/namespace/aggregation_schedule.rb
+++ b/app/models/namespace/aggregation_schedule.rb
@@ -44,4 +44,12 @@ class Namespace::AggregationSchedule < ApplicationRecord
   def lease_key
     "namespace:namespaces_root_statistics:#{namespace_id}"
   end
+
+  # Used by ExclusiveLeaseGuard
+  # Overriding value as we never release the lease
+  # before the timeout in order to prevent multiple
+  # RootStatisticsWorker to start in a short span of time
+  def lease_release?
+    false
+  end
 end
diff --git a/spec/models/namespace/aggregation_schedule_spec.rb b/spec/models/namespace/aggregation_schedule_spec.rb
index 8ed0248e1b2..0f1283717e0 100644
--- a/spec/models/namespace/aggregation_schedule_spec.rb
+++ b/spec/models/namespace/aggregation_schedule_spec.rb
@@ -53,10 +53,39 @@ RSpec.describe Namespace::AggregationSchedule, :clean_gitlab_redis_shared_state,
 
         expect(Namespaces::RootStatisticsWorker)
           .to receive(:perform_in).once
-          .with(described_class::DEFAULT_LEASE_TIMEOUT, aggregation_schedule.namespace_id )
+          .with(described_class::DEFAULT_LEASE_TIMEOUT, aggregation_schedule.namespace_id)
 
         aggregation_schedule.save!
       end
+
+      it 'does not release the lease' do
+        stub_exclusive_lease(lease_key, timeout: described_class::DEFAULT_LEASE_TIMEOUT)
+
+        aggregation_schedule.save!
+
+        exclusive_lease = aggregation_schedule.exclusive_lease
+        expect(exclusive_lease.exists?).to be_truthy
+      end
+
+      it 'only executes the workers once' do
+        # Avoid automatic deletion of Namespace::AggregationSchedule
+        # for testing purposes.
+        expect(Namespaces::RootStatisticsWorker)
+          .to receive(:perform_async).once
+          .and_return(nil)
+
+        expect(Namespaces::RootStatisticsWorker)
+          .to receive(:perform_in).once
+          .with(described_class::DEFAULT_LEASE_TIMEOUT, aggregation_schedule.namespace_id)
+          .and_return(nil)
+
+        # Scheduling workers for the first time
+        aggregation_schedule.schedule_root_storage_statistics
+
+        # Executing again, this time workers should not be scheduled
+        # due to the lease not been released.
+        aggregation_schedule.schedule_root_storage_statistics
+      end
     end
 
     context 'with a personalized lease timeout' do
diff --git a/spec/workers/namespaces/schedule_aggregation_worker_spec.rb b/spec/workers/namespaces/schedule_aggregation_worker_spec.rb
index 7432ca12f2a..d4a49a3f53a 100644
--- a/spec/workers/namespaces/schedule_aggregation_worker_spec.rb
+++ b/spec/workers/namespaces/schedule_aggregation_worker_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Namespaces::ScheduleAggregationWorker, '#perform' do
+describe Namespaces::ScheduleAggregationWorker, '#perform', :clean_gitlab_redis_shared_state do
   let(:group) { create(:group) }
 
   subject(:worker) { described_class.new }
@@ -10,6 +10,8 @@ describe Namespaces::ScheduleAggregationWorker, '#perform' do
   context 'when group is the root ancestor' do
     context 'when aggregation schedule exists' do
       it 'does not create a new one' do
+        stub_aggregation_schedule_statistics
+
         Namespace::AggregationSchedule.safe_find_or_create_by!(namespace_id: group.id)
 
         expect do
@@ -18,26 +20,25 @@ describe Namespaces::ScheduleAggregationWorker, '#perform' do
       end
     end
 
-    context 'when update_statistics_namespace is off' do
-      it 'does not create a new one' do
-        stub_feature_flags(update_statistics_namespace: false, namespace: group)
+    context 'when aggregation schedule does not exist' do
+      it 'creates one' do
+        stub_aggregation_schedule_statistics
 
         expect do
           worker.perform(group.id)
-        end.not_to change(Namespace::AggregationSchedule, :count)
+        end.to change(Namespace::AggregationSchedule, :count).by(1)
+
+        expect(group.aggregation_schedule).to be_present
       end
     end
 
-    context 'when aggregation schedule does not exist' do
-      it 'creates one' do
-        allow_any_instance_of(Namespace::AggregationSchedule)
-          .to receive(:schedule_root_storage_statistics).and_return(nil)
+    context 'when update_statistics_namespace is off' do
+      it 'does not create a new one' do
+        stub_feature_flags(update_statistics_namespace: false, namespace: group)
 
         expect do
           worker.perform(group.id)
-        end.to change(Namespace::AggregationSchedule, :count).by(1)
-
-        expect(group.aggregation_schedule).to be_present
+        end.not_to change(Namespace::AggregationSchedule, :count)
       end
     end
   end
@@ -47,8 +48,7 @@ describe Namespaces::ScheduleAggregationWorker, '#perform' do
     let(:group) { create(:group, parent: parent_group) }
 
     it 'creates an aggregation schedule for the root' do
-      allow_any_instance_of(Namespace::AggregationSchedule)
-        .to receive(:schedule_root_storage_statistics).and_return(nil)
+      stub_aggregation_schedule_statistics
 
       worker.perform(group.id)
 
@@ -63,4 +63,15 @@ describe Namespaces::ScheduleAggregationWorker, '#perform' do
       worker.perform(12345)
     end
   end
+
+  def stub_aggregation_schedule_statistics
+    # Namespace::Aggregations are deleted by
+    # Namespace::AggregationSchedule::schedule_root_storage_statistics,
+    # which is executed async. Stubing the service so instances are not deleted
+    # while still running the specs.
+    expect_next_instance_of(Namespace::AggregationSchedule) do |aggregation_schedule|
+      expect(aggregation_schedule)
+        .to receive(:schedule_root_storage_statistics)
+    end
+  end
 end