Merge branch 'da-exclusive-lease-helpers' into 'master'

Add helper methods to stub Gitlab::ExclusiveLease

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

spec/lib/mattermost/session_spec.rb

@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe Mattermost::Session, type: :request do
+  include ExclusiveLeaseHelpers
+
   let(:user) { create(:user) }
 
   let(:gitlab_url) { "http://gitlab.com" }
@@ -97,26 +99,20 @@ describe Mattermost::Session, type: :request do
       end
     end
 
-    context 'with lease' do
-      before do
-        allow(subject).to receive(:lease_try_obtain).and_return('aldkfjsldfk')
-      end
+    context 'exclusive lease' do
+      let(:lease_key) { 'mattermost:session' }
 
       it 'tries to obtain a lease' do
-        expect(subject).to receive(:lease_try_obtain)
-        expect(Gitlab::ExclusiveLease).to receive(:cancel)
+        expect_to_obtain_exclusive_lease(lease_key, 'uuid')
+        expect_to_cancel_exclusive_lease(lease_key, 'uuid')
 
         # Cannot setup a session, but we should still cancel the lease
         expect { subject.with_session }.to raise_error(Mattermost::NoSessionError)
       end
-    end
 
-    context 'without lease' do
-      before do
-        allow(subject).to receive(:lease_try_obtain).and_return(nil)
-      end
+      it 'returns a NoSessionError error without lease' do
+        stub_exclusive_lease_taken(lease_key)
 
-      it 'returns a NoSessionError error' do
         expect { subject.with_session }.to raise_error(Mattermost::NoSessionError)
       end
     end

spec/models/ci/build_trace_chunk_spec.rb

@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
+  include ExclusiveLeaseHelpers
+
   set(:build) { create(:ci_build, :running) }
   let(:chunk_index) { 0 }
   let(:data_store) { :redis }
@@ -322,7 +324,7 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
 
   describe 'ExclusiveLock' do
     before do
-      allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain) { nil }
+      stub_exclusive_lease_taken
       stub_const('Ci::BuildTraceChunk::WRITE_LOCK_RETRY', 1)
     end
 

spec/models/concerns/reactive_caching_spec.rb

@@ -1,6 +1,7 @@
 require 'spec_helper'
 
 describe ReactiveCaching, :use_clean_rails_memory_store_caching do
+  include ExclusiveLeaseHelpers
   include ReactiveCachingHelpers
 
   class CacheTest
@@ -106,8 +107,8 @@ describe ReactiveCaching, :use_clean_rails_memory_store_caching do
       end
 
       it 'takes and releases the lease' do
-        expect_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return("000000")
-        expect(Gitlab::ExclusiveLease).to receive(:cancel).with(cache_key, "000000")
+        expect_to_obtain_exclusive_lease(cache_key, 'uuid')
+        expect_to_cancel_exclusive_lease(cache_key, 'uuid')
 
         go!
       end
@@ -153,11 +154,9 @@ describe ReactiveCaching, :use_clean_rails_memory_store_caching do
     end
 
     context 'when the lease is already taken' do
-      before do
-        expect_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(nil)
-      end
-
       it 'skips the calculation' do
+        stub_exclusive_lease_taken(cache_key)
+
         expect(instance).to receive(:calculate_reactive_cache).never
 
         go!

spec/services/clusters/applications/check_ingress_ip_address_service_spec.rb

@@ -1,11 +1,13 @@
 require 'spec_helper'
 
 describe Clusters::Applications::CheckIngressIpAddressService do
+  include ExclusiveLeaseHelpers
+
   let(:application) { create(:clusters_applications_ingress, :installed) }
   let(:service) { described_class.new(application) }
   let(:kubeclient) { double(::Kubeclient::Client, get_service: kube_service) }
   let(:ingress) { [{ ip: '111.222.111.222' }] }
-  let(:exclusive_lease) { instance_double(Gitlab::ExclusiveLease, try_obtain: true) }
+  let(:lease_key) { "check_ingress_ip_address_service:#{application.id}" }
 
   let(:kube_service) do
     ::Kubeclient::Resource.new(
@@ -22,11 +24,8 @@ describe Clusters::Applications::CheckIngressIpAddressService do
   subject { service.execute }
 
   before do
+    stub_exclusive_lease(lease_key, timeout: 15.seconds.to_i)
     allow(application.cluster).to receive(:kubeclient).and_return(kubeclient)
-    allow(Gitlab::ExclusiveLease)
-      .to receive(:new)
-      .with("check_ingress_ip_address_service:#{application.id}", timeout: 15.seconds.to_i)
-      .and_return(exclusive_lease)
   end
 
   describe '#execute' do
@@ -47,13 +46,9 @@ describe Clusters::Applications::CheckIngressIpAddressService do
     end
 
     context 'when the exclusive lease cannot be obtained' do
-      before do
-        allow(exclusive_lease)
-          .to receive(:try_obtain)
-          .and_return(false)
-      end
-
       it 'does not call kubeclient' do
+        stub_exclusive_lease_taken(lease_key, timeout: 15.seconds.to_i)
+
         subject
 
         expect(kubeclient).not_to have_received(:get_service)

spec/services/users/refresh_authorized_projects_service_spec.rb

@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe Users::RefreshAuthorizedProjectsService do
+  include ExclusiveLeaseHelpers
+
   # We're using let! here so that any expectations for the service class are not
   # triggered twice.
   let!(:project) { create(:project) }
@@ -10,12 +12,10 @@ describe Users::RefreshAuthorizedProjectsService do
 
   describe '#execute', :clean_gitlab_redis_shared_state do
     it 'refreshes the authorizations using a lease' do
-      expect_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain)
-        .and_return('foo')
-
-      expect(Gitlab::ExclusiveLease).to receive(:cancel)
-        .with(an_instance_of(String), 'foo')
+      lease_key = "refresh_authorized_projects:#{user.id}"
 
+      expect_to_obtain_exclusive_lease(lease_key, 'uuid')
+      expect_to_cancel_exclusive_lease(lease_key, 'uuid')
       expect(service).to receive(:execute_without_lease)
 
       service.execute

spec/support/helpers/exclusive_lease_helpers.rb

@@ -0,0 +1,36 @@
+module ExclusiveLeaseHelpers
+  def stub_exclusive_lease(key = nil, uuid = 'uuid', renew: false, timeout: nil)
+    key     ||= instance_of(String)
+    timeout ||= instance_of(Integer)
+
+    lease = instance_double(
+      Gitlab::ExclusiveLease,
+      try_obtain: uuid,
+      exists?: true,
+      renew: renew
+    )
+
+    allow(Gitlab::ExclusiveLease)
+      .to receive(:new)
+      .with(key, timeout: timeout)
+      .and_return(lease)
+
+    lease
+  end
+
+  def stub_exclusive_lease_taken(key = nil, timeout: nil)
+    stub_exclusive_lease(key, nil, timeout: timeout)
+  end
+
+  def expect_to_obtain_exclusive_lease(key, uuid = 'uuid', timeout: nil)
+    lease = stub_exclusive_lease(key, uuid, timeout: timeout)
+
+    expect(lease).to receive(:try_obtain)
+  end
+
+  def expect_to_cancel_exclusive_lease(key, uuid)
+    expect(Gitlab::ExclusiveLease)
+      .to receive(:cancel)
+      .with(key, uuid)
+  end
+end

spec/support/shared_examples/ci_trace_shared_examples.rb

@@ -247,8 +247,10 @@ shared_examples_for 'common trace features' do
         end
 
         context 'when another process has already been archiving', :clean_gitlab_redis_shared_state do
+          include ExclusiveLeaseHelpers
+
           before do
-            Gitlab::ExclusiveLease.new("trace:archive:#{trace.job.id}", timeout: 1.hour).try_obtain
+            stub_exclusive_lease_taken("trace:archive:#{trace.job.id}", timeout: 1.hour)
           end
 
           it 'blocks concurrent archiving' do

spec/support/shared_examples/uploaders/object_storage_shared_examples.rb

@@ -76,8 +76,10 @@ shared_examples "migrates" do |to_store:, from_store: nil|
   end
 
   context 'when migrate! is occupied by another process' do
+    include ExclusiveLeaseHelpers
+
     before do
-      @uuid = Gitlab::ExclusiveLease.new(subject.exclusive_lease_key, timeout: 1.hour.to_i).try_obtain
+      stub_exclusive_lease_taken(subject.exclusive_lease_key, timeout: 1.hour.to_i)
     end
 
     it 'does not execute migrate!' do
@@ -91,10 +93,6 @@ shared_examples "migrates" do |to_store:, from_store: nil|
 
       expect { subject.use_file }.to raise_error(ObjectStorage::ExclusiveLeaseTaken)
     end
-
-    after do
-      Gitlab::ExclusiveLease.cancel(subject.exclusive_lease_key, @uuid)
-    end
   end
 
   context 'migration is unsuccessful' do

spec/workers/project_cache_worker_spec.rb

@@ -1,14 +1,17 @@
 require 'spec_helper'
 
 describe ProjectCacheWorker do
+  include ExclusiveLeaseHelpers
+
   let(:worker) { described_class.new }
   let(:project) { create(:project, :repository) }
   let(:statistics) { project.statistics }
+  let(:lease_key) { "project_cache_worker:#{project.id}:update_statistics" }
+  let(:lease_timeout) { ProjectCacheWorker::LEASE_TIMEOUT }
 
   describe '#perform' do
     before do
-      allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain)
-        .and_return(true)
+      stub_exclusive_lease(lease_key, timeout: lease_timeout)
     end
 
     context 'with a non-existing project' do
@@ -63,9 +66,7 @@ describe ProjectCacheWorker do
   describe '#update_statistics' do
     context 'when a lease could not be obtained' do
       it 'does not update the repository size' do
-        allow(worker).to receive(:try_obtain_lease_for)
-          .with(project.id, :update_statistics)
-          .and_return(false)
+        stub_exclusive_lease_taken(lease_key, timeout: lease_timeout)
 
         expect(statistics).not_to receive(:refresh!)
 
@@ -75,9 +76,7 @@ describe ProjectCacheWorker do
 
     context 'when a lease could be obtained' do
       it 'updates the project statistics' do
-        allow(worker).to receive(:try_obtain_lease_for)
-          .with(project.id, :update_statistics)
-          .and_return(true)
+        stub_exclusive_lease(lease_key, timeout: lease_timeout)
 
         expect(statistics).to receive(:refresh!)
           .with(only: %i(repository_size))

spec/workers/project_migrate_hashed_storage_worker_spec.rb

@@ -1,53 +1,47 @@
 require 'spec_helper'
 
 describe ProjectMigrateHashedStorageWorker, :clean_gitlab_redis_shared_state do
+  include ExclusiveLeaseHelpers
+
   describe '#perform' do
     let(:project) { create(:project, :empty_repo) }
-    let(:pending_delete_project) { create(:project, :empty_repo, pending_delete: true) }
-
-    context 'when have exclusive lease' do
-      before do
-        lease = subject.lease_for(project.id)
-
-        allow(Gitlab::ExclusiveLease).to receive(:new).and_return(lease)
-        allow(lease).to receive(:try_obtain).and_return(true)
-      end
+    let(:lease_key) { "project_migrate_hashed_storage_worker:#{project.id}" }
+    let(:lease_timeout) { ProjectMigrateHashedStorageWorker::LEASE_TIMEOUT }
 
     it 'skips when project no longer exists' do
-        nonexistent_id = 999999999999
-
       expect(::Projects::HashedStorageMigrationService).not_to receive(:new)
-        subject.perform(nonexistent_id)
+
+      subject.perform(-1)
     end
 
     it 'skips when project is pending delete' do
+      pending_delete_project = create(:project, :empty_repo, pending_delete: true)
+
       expect(::Projects::HashedStorageMigrationService).not_to receive(:new)
 
       subject.perform(pending_delete_project.id)
     end
 
-      it 'delegates removal to service class' do
-        service = double('service')
-        expect(::Projects::HashedStorageMigrationService).to receive(:new).with(project, subject.logger).and_return(service)
-        expect(service).to receive(:execute)
+    it 'delegates removal to service class when have exclusive lease' do
+      stub_exclusive_lease(lease_key, 'uuid', timeout: lease_timeout)
+
+      migration_service = spy
+
+      allow(::Projects::HashedStorageMigrationService)
+        .to receive(:new).with(project, subject.logger)
+        .and_return(migration_service)
 
       subject.perform(project.id)
-      end
+
+      expect(migration_service).to have_received(:execute)
     end
 
-    context 'when dont have exclusive lease' do
-      before do
-        lease = subject.lease_for(project.id)
+    it 'skips when dont have lease when dont have exclusive lease' do
+      stub_exclusive_lease_taken(lease_key, timeout: lease_timeout)
 
-        allow(Gitlab::ExclusiveLease).to receive(:new).and_return(lease)
-        allow(lease).to receive(:try_obtain).and_return(false)
-      end
-
-      it 'skips when dont have lease' do
       expect(::Projects::HashedStorageMigrationService).not_to receive(:new)
 
       subject.perform(project.id)
     end
   end
 end
-end

spec/workers/propagate_service_template_worker_spec.rb

@@ -1,8 +1,11 @@
 require 'spec_helper'
 
 describe PropagateServiceTemplateWorker do
-  let!(:service_template) do
-    PushoverService.create(
+  include ExclusiveLeaseHelpers
+
+  describe '#perform' do
+    it 'calls the propagate service with the template' do
+      template = PushoverService.create(
         template: true,
         active: true,
         properties: {
@@ -12,18 +15,15 @@ describe PropagateServiceTemplateWorker do
           user_key: 'asdf',
           api_key: '123456789'
         })
-  end
 
-  before do
-    allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain)
-      .and_return(true)
-  end
+      stub_exclusive_lease("propagate_service_template_worker:#{template.id}",
+        timeout: PropagateServiceTemplateWorker::LEASE_TIMEOUT)
 
-  describe '#perform' do
-    it 'calls the propagate service with the template' do
-      expect(Projects::PropagateServiceTemplate).to receive(:propagate).with(service_template)
+      expect(Projects::PropagateServiceTemplate)
+        .to receive(:propagate)
+        .with(template)
 
-      subject.perform(service_template.id)
+      subject.perform(template.id)
     end
   end
 end

spec/workers/repository_remove_remote_worker_spec.rb

@@ -1,44 +1,50 @@
 require 'rails_helper'
 
 describe RepositoryRemoveRemoteWorker do
-  subject(:worker) { described_class.new }
+  include ExclusiveLeaseHelpers
 
   describe '#perform' do
-    let(:remote_name) { 'joe'}
     let!(:project) { create(:project, :repository) }
+    let(:remote_name) { 'joe'}
+    let(:lease_key) { "remove_remote_#{project.id}_#{remote_name}" }
+    let(:lease_timeout) { RepositoryRemoveRemoteWorker::LEASE_TIMEOUT }
 
-    context 'when it cannot obtain lease' do
-      it 'logs error' do
-        allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain) { nil }
-
-        expect_any_instance_of(Repository).not_to receive(:remove_remote)
-        expect(worker).to receive(:log_error).with('Cannot obtain an exclusive lease. There must be another instance already in execution.')
-
-        worker.perform(project.id, remote_name)
-      end
-    end
-
-    context 'when it gets the lease' do
-      before do
-        allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(true)
-      end
-
-      context 'when project does not exist' do
-        it 'returns nil' do
-          expect(worker.perform(-1, 'remote_name')).to be_nil
-        end
+    it 'returns nil when project does not exist' do
+      expect(subject.perform(-1, 'remote_name')).to be_nil
     end
 
     context 'when project exists' do
-        it 'removes remote from repository' do
-          masterrev = project.repository.find_branch('master').dereferenced_target
+      before do
+        allow(Project)
+          .to receive(:find_by)
+          .with(id: project.id)
+          .and_return(project)
+      end
 
+      it 'does not remove remote when cannot obtain lease' do
+        stub_exclusive_lease_taken(lease_key, timeout: lease_timeout)
+
+        expect(project.repository)
+          .not_to receive(:remove_remote)
+
+        expect(subject)
+          .to receive(:log_error)
+          .with('Cannot obtain an exclusive lease. There must be another instance already in execution.')
+
+        subject.perform(project.id, remote_name)
+      end
+
+      it 'removes remote from repository when obtain a lease' do
+        stub_exclusive_lease(lease_key, timeout: lease_timeout)
+        masterrev = project.repository.find_branch('master').dereferenced_target
         create_remote_branch(remote_name, 'remote_branch', masterrev)
 
-          expect_any_instance_of(Repository).to receive(:remove_remote).with(remote_name).and_call_original
+        expect(project.repository)
+          .to receive(:remove_remote)
+          .with(remote_name)
+          .and_call_original
 
-          worker.perform(project.id, remote_name)
-        end
+        subject.perform(project.id, remote_name)
       end
     end
   end
@@ -47,6 +53,7 @@ describe RepositoryRemoveRemoteWorker do
     rugged = Gitlab::GitalyClient::StorageSettings.allow_disk_access do
       project.repository.rugged
     end
+
     rugged.references.create("refs/remotes/#{remote_name}/#{branch_name}", target.id)
   end
 end

spec/workers/stuck_ci_jobs_worker_spec.rb

@@ -1,14 +1,21 @@
 require 'spec_helper'
 
 describe StuckCiJobsWorker do
+  include ExclusiveLeaseHelpers
+
   let!(:runner) { create :ci_runner }
   let!(:job) { create :ci_build, runner: runner }
-  let(:worker) { described_class.new }
-  let(:exclusive_lease_uuid) { SecureRandom.uuid }
+  let(:trace_lease_key) { "trace:archive:#{job.id}" }
+  let(:trace_lease_uuid) { SecureRandom.uuid }
+  let(:worker_lease_key) { StuckCiJobsWorker::EXCLUSIVE_LEASE_KEY }
+  let(:worker_lease_uuid) { SecureRandom.uuid }
+
+  subject(:worker) { described_class.new }
 
   before do
+    stub_exclusive_lease(worker_lease_key, worker_lease_uuid)
+    stub_exclusive_lease(trace_lease_key, trace_lease_uuid)
     job.update!(status: status, updated_at: updated_at)
-    allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(exclusive_lease_uuid)
   end
 
   shared_examples 'job is dropped' do
@@ -44,16 +51,19 @@ describe StuckCiJobsWorker do
 
       context 'when job was not updated for more than 1 day ago' do
         let(:updated_at) { 2.days.ago }
+
         it_behaves_like 'job is dropped'
       end
 
       context 'when job was updated in less than 1 day ago' do
         let(:updated_at) { 6.hours.ago }
+
         it_behaves_like 'job is unchanged'
       end
 
       context 'when job was not updated for more than 1 hour ago' do
         let(:updated_at) { 2.hours.ago }
+
         it_behaves_like 'job is unchanged'
       end
     end
@@ -65,11 +75,14 @@ describe StuckCiJobsWorker do
 
       context 'when job was not updated for more than 1 hour ago' do
         let(:updated_at) { 2.hours.ago }
+
         it_behaves_like 'job is dropped'
       end
 
-      context 'when job was updated in less than 1 hour ago' do
+      context 'when job was updated in less than 1
+       hour ago' do
         let(:updated_at) { 30.minutes.ago }
+
         it_behaves_like 'job is unchanged'
       end
     end
@@ -80,11 +93,13 @@ describe StuckCiJobsWorker do
 
     context 'when job was not updated for more than 1 hour ago' do
       let(:updated_at) { 2.hours.ago }
+
       it_behaves_like 'job is dropped'
     end
 
     context 'when job was updated in less than 1 hour ago' do
       let(:updated_at) { 30.minutes.ago }
+
       it_behaves_like 'job is unchanged'
     end
   end
@@ -93,6 +108,7 @@ describe StuckCiJobsWorker do
     context "when job is #{status}" do
       let(:status) { status }
       let(:updated_at) { 2.days.ago }
+
       it_behaves_like 'job is unchanged'
     end
   end
@@ -119,23 +135,27 @@ describe StuckCiJobsWorker do
     it 'is guard by exclusive lease when executed concurrently' do
       expect(worker).to receive(:drop).at_least(:once).and_call_original
       expect(worker2).not_to receive(:drop)
+
       worker.perform
-      allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(false)
+
+      stub_exclusive_lease_taken(worker_lease_key)
+
       worker2.perform
     end
 
     it 'can be executed in sequence' do
       expect(worker).to receive(:drop).at_least(:once).and_call_original
       expect(worker2).to receive(:drop).at_least(:once).and_call_original
+
       worker.perform
       worker2.perform
     end
 
-    it 'cancels exclusive lease after worker perform' do
-      worker.perform
+    it 'cancels exclusive leases after worker perform' do
+      expect_to_cancel_exclusive_lease(trace_lease_key, trace_lease_uuid)
+      expect_to_cancel_exclusive_lease(worker_lease_key, worker_lease_uuid)
 
-      expect(Gitlab::ExclusiveLease.new(described_class::EXCLUSIVE_LEASE_KEY, timeout: 1.hour))
-        .not_to be_exists
+      worker.perform
     end
   end
 end