Revert "Changes signature of LockHelper#with_release_locks to take release names"

This reverts commit 0cb8b416.

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

@@ -23,9 +23,8 @@ module Bosh::Director
     # Binds release DB record(s) to a plan
     # @return [void]
     def bind_releases
-      releases = @deployment_plan.releases
-      with_release_locks(releases.map(&:name)) do
-        releases.each do |release|
+      with_release_locks(@deployment_plan) do
+        @deployment_plan.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.releases.map(&:name)) do
+            with_release_locks(@deployment_plan) 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
@@ -13,6 +21,13 @@ 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}")
@@ -20,12 +35,31 @@ module Bosh::Director
           lock { yield }
     end
 
-    def with_release_lock(release_name, opts = {})
-      with_release_locks([release_name], opts) { yield }
+    # 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 }
     end
 
-    def with_release_locks(release_names, opts = {})
+    # 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 = {})
       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)
@@ -39,6 +73,13 @@ 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', name: 'r1')
-        r2 = instance_double('Bosh::Director::DeploymentPlan::ReleaseVersion', name: 'r2')
+        r1 = instance_double('Bosh::Director::DeploymentPlan::ReleaseVersion')
+        r2 = instance_double('Bosh::Director::DeploymentPlan::ReleaseVersion')
 
         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).with(['r1', 'r2']).and_yield
+        expect(assembler).to receive(:with_release_locks).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(%w(foo bar)).and_yield.ordered
+          expect(job).to receive(:with_release_locks).with(deployment_plan).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(%w(foo bar)).and_yield.ordered
+          expect(job).to receive(:with_release_locks).with(deployment_plan).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,12 +50,11 @@ module Bosh::Director
     end
 
     describe :with_release_lock do
-      it 'creates a lock for the given name' do
+      it 'should support a release name' do
         lock = double(:lock)
         allow(Lock).to receive(:new).with('lock:release:bar', { timeout: 5 }).
-            and_return(lock)
-        expect(lock).to receive(:lock).ordered
-        expect(lock).to receive(:release).ordered
+          and_return(lock)
+        expect(lock).to receive(:lock).and_yield
 
         called = false
         @test_instance.with_release_lock('bar', timeout: 5) do
@@ -66,12 +65,21 @@ module Bosh::Director
     end
 
     describe :with_release_locks do
-      it 'creates locks for each release name in a consistent order' 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])
+
         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
@@ -79,7 +87,7 @@ module Bosh::Director
         expect(lock_a).to receive(:release).ordered
 
         called = false
-        @test_instance.with_release_locks(['b', 'a'], timeout: 5) do
+        @test_instance.with_release_locks(deployment_plan, timeout: 5) do
           called = true
         end
         expect(called).to be(true)