Changes signature of LockHelper#with_release_locks to take release names

Instead of taking a deployment planner and grabbing the releases then
the names, we only pass it the release names.

Also DRY's up the LockHelper#with_release_lock (singular) to use
release locks

[#91451310]
Signed-off-by: Jonathan Barnes <jonathan@pivotal.io>

bosh-director/lib/bosh/director/deployment_plan/assembler.rb

@@ -23,8 +23,9 @@ module Bosh::Director
     # Binds release DB record(s) to a plan
     # @return [void]
     def bind_releases
-      with_release_locks(@deployment_plan) do
-        @deployment_plan.releases.each do |release|
+      releases = @deployment_plan.releases
+      with_release_locks(releases.map(&:name)) do
+        releases.each do |release|
           release.bind_model
         end
       end

bosh-director/lib/bosh/director/jobs/update_deployment.rb

@@ -89,7 +89,7 @@ module Bosh::Director
             logger.info('Updating deployment')
             update
 
-            with_release_locks(@deployment_plan) do
+            with_release_locks(@deployment_plan.releases.map(&:name)) do
               deployment.db.transaction do
                 deployment.remove_all_release_versions
                 # Now we know that deployment has succeeded and can remove

bosh-director/lib/bosh/director/lock_helper.rb

 module Bosh::Director
-  # Helper for managing BOSH locks.
   module LockHelper
-
-    # Surround with deployment lock.
-    #
-    # @param [DeploymentPlan|String] deployment plan or name.
-    # @option opts [Number] timeout how long to wait before giving up
-    # @return [void]
-    # @yield [void] block to surround
     def with_deployment_lock(deployment, opts = {})
       if deployment.respond_to?(:name)
         name = deployment.name
@@ -21,13 +13,6 @@ module Bosh::Director
       Lock.new("lock:deployment:#{name}", :timeout => timeout).lock { yield }
     end
 
-    # Surround with stemcell lock.
-    #
-    # @param [String] name stemcell name.
-    # @param [String] version stemcell version.
-    # @option opts [Number] timeout how long to wait before giving up
-    # @return [void]
-    # @yield [void] block to surround
     def with_stemcell_lock(name, version, opts = {})
       timeout = opts[:timeout] || 10
       Config.logger.info("Acquiring stemcell lock on #{name}:#{version}")
@@ -35,31 +20,12 @@ module Bosh::Director
           lock { yield }
     end
 
-    # Surround with release lock.
-    #
-    # @param [String] release name.
-    # @option opts [Number] timeout how long to wait before giving up
-    # @return [void]
-    # @yield [void] block to surround
-    def with_release_lock(release, opts = {})
-      timeout = opts[:timeout] || 10
-      Config.logger.info("Acquiring release lock on #{release}")
-      Lock.new("lock:release:#{release}", :timeout => timeout).lock { yield }
+    def with_release_lock(release_name, opts = {})
+      with_release_locks([release_name], opts) { yield }
     end
 
-    # Surround with deployment releases lock.
-    #
-    # @param [DeploymentPlan] deployment plan.
-    # @option opts [Number] timeout how long to wait before giving up
-    # @return [void]
-    # @yield [void] block to surround
-    def with_release_locks(deployment_plan, opts = {})
+    def with_release_locks(release_names, opts = {})
       timeout = opts[:timeout] || 10
-      release_names = deployment_plan.releases.map do |release|
-        release.name
-      end
-
-      # Sorting to enforce lock order to avoid deadlocks
       locks = release_names.sort.map do |release_name|
         Config.logger.info("Acquiring release lock: #{release_name}")
         Lock.new("lock:release:#{release_name}", :timeout => timeout)
@@ -73,13 +39,6 @@ module Bosh::Director
       end
     end
 
-    # Surround with compile lock.
-    #
-    # @param [String|Number] package_id package id.
-    # @param [String|Number] stemcell_id stemcell id.
-    # @option opts [Number] timeout how long to wait before giving up
-    # @return [void]
-    # @yield [void] block to surround
     def with_compile_lock(package_id, stemcell_id, opts = {})
       timeout = opts[:timeout] || 15 * 60 # 15 minutes
 

bosh-director/spec/unit/deployment_plan/assembler_spec.rb

@@ -18,15 +18,15 @@ module Bosh::Director
       end
 
       it 'should bind releases' do
-        r1 = instance_double('Bosh::Director::DeploymentPlan::ReleaseVersion')
-        r2 = instance_double('Bosh::Director::DeploymentPlan::ReleaseVersion')
+        r1 = instance_double('Bosh::Director::DeploymentPlan::ReleaseVersion', name: 'r1')
+        r2 = instance_double('Bosh::Director::DeploymentPlan::ReleaseVersion', name: 'r2')
 
         expect(deployment_plan).to receive(:releases).and_return([r1, r2])
 
         expect(r1).to receive(:bind_model)
         expect(r2).to receive(:bind_model)
 
-        expect(assembler).to receive(:with_release_locks).and_yield
+        expect(assembler).to receive(:with_release_locks).with(['r1', 'r2']).and_yield
         assembler.bind_releases
       end
 

bosh-director/spec/unit/jobs/update_deployment_spec.rb

@@ -247,7 +247,7 @@ describe Bosh::Director::Jobs::UpdateDeployment do
           expect(notifier).to receive(:send_start_event).ordered
           expect(job).to receive(:prepare).ordered
           expect(job).to receive(:update).ordered
-          expect(job).to receive(:with_release_locks).with(deployment_plan).and_yield.ordered
+          expect(job).to receive(:with_release_locks).with(%w(foo bar)).and_yield.ordered
           expect(notifier).to receive(:send_end_event).ordered
           expect(job).to receive(:update_stemcell_references).ordered
 
@@ -272,7 +272,7 @@ describe Bosh::Director::Jobs::UpdateDeployment do
           expect(notifier).to receive(:send_start_event).ordered
           expect(job).to receive(:prepare).ordered
           expect(job).to receive(:update).ordered
-          expect(job).to receive(:with_release_locks).with(deployment_plan).and_yield.ordered
+          expect(job).to receive(:with_release_locks).with(%w(foo bar)).and_yield.ordered
           expect(notifier).to receive(:send_end_event).ordered
           expect(job).to receive(:update_stemcell_references).ordered
 

bosh-director/spec/unit/lock_helper_spec.rb

@@ -50,11 +50,12 @@ module Bosh::Director
     end
 
     describe :with_release_lock do
-      it 'should support a release name' do
+      it 'creates a lock for the given name' do
         lock = double(:lock)
         allow(Lock).to receive(:new).with('lock:release:bar', { timeout: 5 }).
-          and_return(lock)
-        expect(lock).to receive(:lock).and_yield
+            and_return(lock)
+        expect(lock).to receive(:lock).ordered
+        expect(lock).to receive(:release).ordered
 
         called = false
         @test_instance.with_release_lock('bar', timeout: 5) do
@@ -65,21 +66,12 @@ module Bosh::Director
     end
 
     describe :with_release_locks do
-      it 'should support a deployment plan' do
-        deployment_plan = double(:deployment_plan)
-        release_a = double(:release_a)
-        allow(release_a).to receive(:name).and_return('a')
-        release_b = double(:release_b)
-        allow(release_b).to receive(:name).and_return('b')
-        allow(deployment_plan).to receive(:releases).and_return([release_a, release_b])
-
+      it 'creates locks for each release name in a consistent order' do
         lock_a = double(:lock_a)
-        allow(Lock).to receive(:new).with('lock:release:a', { timeout: 5 }).
-          and_return(lock_a)
+        allow(Lock).to receive(:new).with('lock:release:a', { timeout: 5 }).and_return(lock_a)
 
         lock_b = double(:lock_b)
-        allow(Lock).to receive(:new).with('lock:release:b', { timeout: 5 }).
-          and_return(lock_b)
+        allow(Lock).to receive(:new).with('lock:release:b', { timeout: 5 }).and_return(lock_b)
 
         expect(lock_a).to receive(:lock).ordered
         expect(lock_b).to receive(:lock).ordered
@@ -87,7 +79,7 @@ module Bosh::Director
         expect(lock_a).to receive(:release).ordered
 
         called = false
-        @test_instance.with_release_locks(deployment_plan, timeout: 5) do
+        @test_instance.with_release_locks(['b', 'a'], timeout: 5) do
           called = true
         end
         expect(called).to be(true)