From d132f73d42aec530c78680f53bf8a612bac61a3b Mon Sep 17 00:00:00 2001 From: Mayra Cabrera Date: Thu, 4 Jul 2019 15:52:02 +0000 Subject: [PATCH] Implements lease_release on NamespaceAggregation Sets lease_release? to false to prevent the job to be re-executed more often than lease timeout Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/64079 --- app/models/namespace/aggregation_schedule.rb | 8 ++++ .../namespace/aggregation_schedule_spec.rb | 31 ++++++++++++- .../schedule_aggregation_worker_spec.rb | 43 ++++++++++++------- 3 files changed, 65 insertions(+), 17 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,6 +20,18 @@ describe Namespaces::ScheduleAggregationWorker, '#perform' do end end + context 'when aggregation schedule does not exist' do + it 'creates one' do + stub_aggregation_schedule_statistics + + expect do + worker.perform(group.id) + end.to change(Namespace::AggregationSchedule, :count).by(1) + + expect(group.aggregation_schedule).to be_present + 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) @@ -27,19 +41,6 @@ describe Namespaces::ScheduleAggregationWorker, '#perform' do end.not_to change(Namespace::AggregationSchedule, :count) 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) - - expect do - worker.perform(group.id) - end.to change(Namespace::AggregationSchedule, :count).by(1) - - expect(group.aggregation_schedule).to be_present - end - end end context 'when group is not the root ancestor' do @@ -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