refactor UpdateDeployment job to only contain control flow

* move finer grained control flow down to updater and planner.
* move domain knowledge into DeploymentPlan::Planner for better
  encapsulation.

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

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

@@ -6,6 +6,7 @@ module Bosh::Director
   # from the deployment manifest and the running environment.
   module DeploymentPlan
     class Planner
+      include LockHelper
       include DnsHelper
       include ValidationHelper
 
@@ -260,6 +261,27 @@ module Bosh::Director
       def rename_in_progress?
         @job_rename['old_name'] && @job_rename['new_name']
       end
+
+      def persist_updates!
+        #prior updates may have had release versions that we no longer use.
+        #remove the references to these stale releases.
+        stale_release_versions = (model.release_versions - releases.map(&:model))
+        stale_release_names = stale_release_versions.map {|version_model| version_model.release.name}
+        with_release_locks(stale_release_names) do
+          stale_release_versions.each do |release_version|
+            model.remove_release_version(release_version)
+          end
+        end
+
+        model.save
+      end
+
+      def update_stemcell_references!
+        current_stemcell_models = resource_pools.map { |pool| pool.stemcell.model }
+        model.stemcells.each do |deployment_stemcell|
+          deployment_stemcell.remove_deployment(model) unless current_stemcell_models.include?(deployment_stemcell)
+        end
+      end
     end
   end
 end

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

@@ -11,7 +11,23 @@ module Bosh::Director
         @multi_job_updater = multi_job_updater
       end
 
+
       def update
+        begin
+          @logger.info('Updating deployment')
+          assemble
+          update_jobs
+          @logger.info('Committing updates')
+          @deployment_plan.persist_updates!
+          @logger.info('Finished updating deployment')
+        ensure
+          @deployment_plan.update_stemcell_references!
+        end
+      end
+
+      private
+
+      def assemble
         @event_log.begin_stage('Preparing DNS', 1)
         @base_job.track_and_log('Binding DNS') do
           @assembler.bind_dns
@@ -34,7 +50,9 @@ module Bosh::Director
         @base_job.track_and_log('Binding configuration') do
           @assembler.bind_configuration
         end
+      end
 
+      def update_jobs
         @logger.info('Updating jobs')
         @multi_job_updater.run(
           @base_job,

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

 module Bosh::Director
   module Jobs
     class UpdateDeployment < BaseJob
-      include LockHelper
-
-      attr_reader :notifier
-
       @queue = :normal
 
       def self.job_type
         :update_deployment
       end
 
-      # @param [String] manifest_file Path to deployment manifest
-      # @param [Hash] options Deployment options
-      def initialize(manifest_file, cloud_config_id, options = {})
+      def initialize(manifest_file_path, options = {})
         @blobstore = App.instance.blobstores.blobstore
+        @manifest_file_path = manifest_file_path
+        @options = options
+      end
 
-        logger.info('Reading deployment manifest')
-        @manifest_file = manifest_file
-        @manifest = File.read(@manifest_file)
-        logger.debug("Manifest:\n#{@manifest}")
-
-        @cloud_config = Bosh::Director::Models::CloudConfig.find(id: cloud_config_id)
-        logger.debug("Cloud Config:\n#{@cloud_config.inspect}")
+      def perform
+        with_deployment_lock(deployment_plan) do
+          logger.info('Updating deployment')
+          notifier.send_start_event
+          prepare
+          compile
+          update
+          notifier.send_end_event
+          logger.info('Finished updating deployment')
 
-        logger.info('Creating deployment plan')
-        logger.info("Deployment plan options: #{options.pretty_inspect}")
+          "/deployments/#{deployment_plan.name}"
+        end
+      rescue Exception => e
+        notifier.send_error_event e
+        raise e
+      ensure
+        FileUtils.rm_rf(@manifest_file_path)
+      end
 
-        plan_options = {
-          'recreate' => !!options['recreate'],
-          'job_states' => options['job_states'] || {},
-          'job_rename' => options['job_rename'] || {}
-        }
+      # Job tasks
 
-        manifest_as_hash = Psych.load(@manifest)
-        @deployment_plan = DeploymentPlan::Planner.parse(manifest_as_hash, plan_options, event_log, logger)
-        logger.info('Created deployment plan')
+      def prepare
+        prepare_step = DeploymentPlan::Preparer.new(self, assembler)
+        prepare_step.prepare
+      end
 
-        nats_rpc = Config.nats_rpc
-        @notifier = DeploymentPlan::Notifier.new(@deployment_plan, nats_rpc, logger)
+      def compile
+        compile_step = PackageCompiler.new(self, deployment_plan)
+        compile_step.compile
+      end
 
-        resource_pools = @deployment_plan.resource_pools
-        @resource_pool_updaters = resource_pools.map do |resource_pool|
+      def update
+        resource_pool_updaters = deployment_plan.resource_pools.map do |resource_pool|
           ResourcePoolUpdater.new(resource_pool)
         end
+        resource_pools = DeploymentPlan::ResourcePools.new(event_log, resource_pool_updaters)
+        update_step = DeploymentPlan::Updater.new(self, event_log, resource_pools, assembler, deployment_plan, deployment_plan.model, multi_job_updater)
+        update_step.update
       end
 
-      def prepare
-        @assembler = DeploymentPlan::Assembler.new(@deployment_plan)
-        preparer = DeploymentPlan::Preparer.new(self, @assembler)
-        preparer.prepare
+      # Job dependencies
 
-        logger.info('Compiling and binding packages')
-        PackageCompiler.new(@deployment_plan).compile
+      def assembler
+        @assembler ||= DeploymentPlan::Assembler.new(deployment_plan)
       end
 
-      def update
-        resource_pools = DeploymentPlan::ResourcePools.new(event_log, @resource_pool_updaters)
-        job_updater_factory = JobUpdaterFactory.new(@blobstore)
-        multi_job_updater = DeploymentPlan::BatchMultiJobUpdater.new(job_updater_factory)
-        updater = DeploymentPlan::Updater.new(self, event_log, resource_pools, @assembler, @deployment_plan, multi_job_updater)
-        updater.update
+      def notifier
+        @notifier ||= DeploymentPlan::Notifier.new(deployment_plan, Config.nats_rpc, logger)
       end
 
-      def update_stemcell_references
-        current_stemcells = Set.new
-        @deployment_plan.resource_pools.each do |resource_pool|
-          current_stemcells << resource_pool.stemcell.model
-        end
-
-        deployment = @deployment_plan.model
-        stemcells = deployment.stemcells
-        stemcells.each do |stemcell|
-          unless current_stemcells.include?(stemcell)
-            stemcell.remove_deployment(deployment)
-          end
+      def deployment_plan
+        @deployment_plan ||= begin
+          logger.info('Reading deployment manifest')
+          manifest_text = File.read(@manifest_file_path)
+          logger.debug("Manifest:\n#{manifest_text}")
+          deployment_manifest = Psych.load(manifest_text)
+
+          plan_options = {
+            'recreate' => !!@options['recreate'],
+            'job_states' => @options['job_states'] || {},
+            'job_rename' => @options['job_rename'] || {}
+          }
+          logger.info('Creating deployment plan')
+          logger.info("Deployment plan options: #{plan_options.pretty_inspect}")
+
+          plan = DeploymentPlan::Planner.parse(deployment_manifest, plan_options, event_log, logger)
+          logger.info('Created deployment plan')
+          plan
         end
       end
 
-      def perform
-        with_deployment_lock(@deployment_plan) do
-          logger.info('Preparing deployment')
-          notifier.send_start_event
-          prepare
-          begin
-            deployment = @deployment_plan.model
-            logger.info('Finished preparing deployment')
-            logger.info('Updating deployment')
-            update
-
-            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
-                # previous partial deployments release version references
-                # to be able to delete these release versions later.
-                @deployment_plan.releases.each do |release|
-                  deployment.add_release_version(release.model)
-                end
-              end
-            end
-
-            deployment.manifest = @manifest
-            deployment.cloud_config = @cloud_config
-            deployment.save
-            notifier.send_end_event
-            logger.info('Finished updating deployment')
-            "/deployments/#{deployment.name}"
-          ensure
-            update_stemcell_references
-          end
+      def multi_job_updater
+        @multi_job_updater ||= begin
+          DeploymentPlan::BatchMultiJobUpdater.new(JobUpdaterFactory.new(@blobstore))
         end
-      rescue Exception => e
-        notifier.send_error_event e
-        raise e
-      ensure
-        FileUtils.rm_rf(@manifest_file)
       end
     end
   end

bosh-director/spec/spec_helper.rb

@@ -202,4 +202,74 @@ def check_event_log
   yield events
 end
 
+def strip_heredoc(str)
+  indent = str.scan(/^[ \t]*(?=\S)/).min.size || 0
+  str.gsub(/^[ \t]{#{indent}}/, '')
+end
+
+module ManifestHelper
+  class << self
+    def default_deployment_manifest(overrides = {})
+      {
+        'name' => 'deployment-name',
+        'releases' => [release],
+        'update' => {
+          'max_in_flight' => 10,
+          'canaries' => 2,
+          'canary_watch_time' => 1000,
+          'update_watch_time' => 1000,
+        },
+      }.merge(overrides)
+    end
+
+    def default_legacy_manifest(overrides = {})
+      (default_deployment_manifest.merge(default_iaas_manifest)).merge(overrides)
+    end
+
+    def default_iaas_manifest(overrides = {})
+      {
+        'networks' => [ManifestHelper::network],
+        'resource_pools' => [ManifestHelper::resource_pool],
+        'compilation' => {
+          'workers' => 1,
+          'network'=>'network-name',
+          'cloud_properties' => {},
+        },
+      }.merge(overrides)
+    end
+
+    def release(overrides = {})
+      {
+        'name' => 'release-name',
+        'version' => 'latest',
+      }.merge(overrides)
+    end
 
+    def network(overrides = {})
+      { 'name' => 'network-name', 'subnets' => [] }.merge(overrides)
+    end
+
+    def disk_pool(name='dp-name')
+      {'name' => name, 'disk_size' => 10000}.merge(overrides)
+    end
+
+    def job(overrides = {})
+      {
+        'name' => 'job-name',
+        'resource_pool' => 'rp-name',
+        'instances' => 1,
+        'networks' => [{'name' => 'network-name'}],
+        'templates' => [{'name' => 'template-name'}]
+      }.merge(overrides)
+    end
+
+    def resource_pool(overrides = {})
+      {
+        'name' => 'rp-name',
+        'network'=>'network-name',
+        'stemcell'=> {'name' => 'default','version'=>'1'},
+        'cloud_properties'=>{}
+      }.merge(overrides)
+    end
+  end
+end

bosh-director/spec/support/file_helpers.rb

+require 'spec_helper'
+
+module Support
+  module FileHelpers
+    class DeploymentDirectory
+      attr_reader :path, :artifacts_dir, :tarballs
+
+      def initialize
+        @path = Dir.mktmpdir('deployment-path')
+      end
+
+      def add_file(filepath, contents = nil)
+        full_path = File.join(path, filepath)
+        FileUtils.mkdir_p(File.dirname(full_path))
+
+        if contents
+          File.open(full_path, 'w') { |f| f.write(contents) }
+        else
+          FileUtils.touch(full_path)
+        end
+
+        full_path
+      end
+    end
+  end
+end
+
+RSpec.configure do |config|
+  config.include(Support::FileHelpers)
+end

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

@@ -4,43 +4,21 @@ module Bosh::Director
   module DeploymentPlan
     describe Planner do
       subject { described_class.new('fake-dep-name') }
-
       let(:event_log) { instance_double('Bosh::Director::EventLog::Log') }
-
-      describe 'parse' do
-        it 'parses disk_pools' do
-          manifest = minimal_manifest
-          manifest['disk_pools'] = [
-            {
-              'name' => 'disk_pool1',
-              'disk_size' => 3000,
-            },
-            {
-              'name' => 'disk_pool2',
-              'disk_size' => 1000,
-            },
-          ]
-          planner = Planner.parse(manifest, {}, event_log, logger)
-          expect(planner.disk_pools.length).to eq(2)
-          expect(planner.disk_pool('disk_pool1').disk_size).to eq(3000)
-          expect(planner.disk_pool('disk_pool2').disk_size).to eq(1000)
-        end
-      end
-
       def minimal_manifest
         {
           'name' => 'minimal',
           # 'director_uuid'  => 'deadbeef',
 
           'releases' => [{
-            'name'    => 'appcloud',
-            'version' => '0.1' # It's our dummy valid release from spec/assets/valid_release.tgz
-          }],
+              'name'    => 'appcloud',
+              'version' => '0.1' # It's our dummy valid release from spec/assets/valid_release.tgz
+            }],
 
           'networks' => [{
-            'name' => 'a',
-            'subnets' => [],
-          }],
+              'name' => 'a',
+              'subnets' => [],
+            }],
 
           'compilation' => {
             'workers' => 1,
@@ -59,6 +37,27 @@ module Bosh::Director
         }
       end
 
+
+      describe 'parse' do
+        it 'parses disk_pools' do
+          manifest = minimal_manifest
+          manifest['disk_pools'] = [
+            {
+              'name' => 'disk_pool1',
+              'disk_size' => 3000,
+            },
+            {
+              'name' => 'disk_pool2',
+              'disk_size' => 1000,
+            },
+          ]
+          planner = Planner.parse(manifest, {}, event_log, logger)
+          expect(planner.disk_pools.length).to eq(2)
+          expect(planner.disk_pool('disk_pool1').disk_size).to eq(3000)
+          expect(planner.disk_pool('disk_pool2').disk_size).to eq(1000)
+        end
+      end
+
       describe '#initialize' do
         it 'raises an error if name is not given' do
           expect {
@@ -184,6 +183,97 @@ module Bosh::Director
           end
         end
       end
+
+      describe '#persist_updates!' do
+        subject { Planner.parse(manifest, {}, Config.event_log, Config.logger) }
+        let(:manifest) do
+          ManifestHelper.default_legacy_manifest(
+            'releases' => [
+              ManifestHelper.release('name' => 'same', 'version' => '123'),
+              ManifestHelper.release('name' => 'new', 'version' => '123'),
+            ]
+          )
+        end
+        before { Bosh::Director::App.new(Bosh::Director::Config.load_file(asset('test-director-config.yml'))) }
+
+        context 'given prior deployment with old release versions' do
+          let(:stale_release_version) do
+            release = Bosh::Director::Models::Release.create(name: 'stale')
+            Bosh::Director::Models::ReleaseVersion.create(release: release, version: '123')
+          end
+          let(:same_release_version) do
+            release = Bosh::Director::Models::Release.create(name: 'same')
+            Bosh::Director::Models::ReleaseVersion.create(release: release, version: '123')
+          end
+          let(:new_release_version) do
+            release = Bosh::Director::Models::Release.create(name: 'new')
+            Bosh::Director::Models::ReleaseVersion.create(release: release, version: '123')
+          end
+          let(:assembler) { Assembler.new subject }
+
+          before do
+            expect(new_release_version).to exist
+            old_deployment = Bosh::Director::Models::Deployment.create(name: manifest['name'])
+            old_deployment.add_release_version stale_release_version
+            old_deployment.add_release_version same_release_version
+            assembler.bind_deployment
+            assembler.bind_releases
+          end
+
+          it 'updates the release version on the deployment to be the ones from the provided manifest' do
+            deployment = subject.model
+
+            expect(deployment.release_versions).to include(stale_release_version)
+            subject.persist_updates!
+            expect(deployment.release_versions).to_not include(stale_release_version)
+            expect(deployment.release_versions).to include(same_release_version)
+            expect(deployment.release_versions).to include(new_release_version)
+          end
+
+          it 'locks the stale releases when removing them' do
+            expect(subject).to receive(:with_release_locks).with(['stale'])
+            subject.persist_updates!
+          end
+
+          it 'saves the deployment model' do
+            deployment = subject.model
+            deployment.name = 'new-deployment-name'
+            subject.persist_updates!
+            expect(deployment.reload.name).to eq('new-deployment-name')
+          end
+        end
+      end
+
+      describe '#update_stemcell_references!' do
+        subject { Planner.parse(manifest, {}, Config.event_log, Config.logger) }
+        let(:manifest) { ManifestHelper.default_legacy_manifest }
+        before { Bosh::Director::App.new(Bosh::Director::Config.load_file(asset('test-director-config.yml'))) }
+
+        context "when the stemcells associated with the resource pools have diverged from the stemcells associated with the planner" do
+          let(:stemcell_model_1) { Bosh::Director::Models::Stemcell.create(name: 'default', version: '1', cid: 'abc') }
+          let(:stemcell_model_2) { Bosh::Director::Models::Stemcell.create(name: 'stem2', version: '1.0', cid: 'def') }
+
+          before do
+            old_deployment = Bosh::Director::Models::Deployment.create(name: manifest['name'])
+            old_deployment.add_stemcell stemcell_model_1
+            old_deployment.add_stemcell stemcell_model_2
+            assembler = Assembler.new(subject)
+            assembler.bind_deployment
+            assembler.bind_stemcells
+          end
+
+          it 'it removes the given deployment from any stemcell it should not be associated with' do
+            deployment_model = subject.model
+            expect(stemcell_model_1.deployments).to include(deployment_model)
+            expect(stemcell_model_2.deployments).to include(deployment_model)
+
+            subject.update_stemcell_references!
+
+            expect(stemcell_model_1.reload.deployments).to include(deployment_model)
+            expect(stemcell_model_2.reload.deployments).to_not include(deployment_model)
+          end
+        end
+      end
     end
   end
 end

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

@@ -2,32 +2,57 @@ require 'spec_helper'
 require 'bosh/director/deployment_plan/multi_job_updater'
 require 'bosh/director/job_updater'
 
-describe Bosh::Director::DeploymentPlan::Updater do
-  subject { described_class.new(base_job, event_log, resource_pools, assembler, deployment_plan, multi_job_updater) }
-  let(:base_job) { instance_double('Bosh::Director::Jobs::BaseJob') }
-  let(:event_log) { instance_double('Bosh::Director::EventLog::Log', begin_stage: nil) }
-  let(:resource_pools) { instance_double('Bosh::Director::DeploymentPlan::ResourcePools') }
-  let(:assembler) { instance_double('Bosh::Director::DeploymentPlan::Assembler') }
-  let(:deployment_plan) { instance_double('Bosh::Director::DeploymentPlan::Planner', jobs_starting_on_deploy: jobs) }
-  let(:jobs) { instance_double('Array') }
-  let(:multi_job_updater) { instance_double('Bosh::Director::DeploymentPlan::SerialMultiJobUpdater') }
+module Bosh::Director
+  describe DeploymentPlan::Updater do
+    subject { DeploymentPlan::Updater.new(base_job, event_log, resource_pools, assembler, deployment_plan, multi_job_updater) }
+    let(:base_job) { Jobs::BaseJob.new }
+    let(:event_log) { instance_double('Bosh::Director::EventLog::Log', begin_stage: nil) }
+    let(:resource_pools) { instance_double('Bosh::Director::DeploymentPlan::ResourcePools') }
+    let(:assembler) { instance_double('Bosh::Director::DeploymentPlan::Assembler') }
+    let(:deployment_plan) { instance_double('Bosh::Director::DeploymentPlan::Planner') }
+    let(:manifest) { ManifestHelper.default_legacy_manifest }
+    let(:releases) { [] }
+    let(:jobs) { instance_double('Array') }
+    let(:multi_job_updater) { instance_double('Bosh::Director::DeploymentPlan::SerialMultiJobUpdater') }
 
-  before { allow(base_job).to receive(:logger).and_return(logger) }
-  before { allow(base_job).to receive(:track_and_log).and_yield }
-  before { allow(Bosh::Director::Config).to receive(:dns_enabled?).and_return(true) }
+    before do
+      allow(base_job).to receive(:logger).and_return(logger)
+      allow(base_job).to receive(:track_and_log).and_yield
+      allow(Bosh::Director::Config).to receive(:dns_enabled?).and_return(true)
+    end
+
+    describe '#update' do
+
+      it 'runs deployment plan update stages in the correct order' do
+        expect(assembler).to receive(:bind_dns).with(no_args).ordered
+        expect(assembler).to receive(:delete_unneeded_vms).with(no_args).ordered
+        expect(assembler).to receive(:delete_unneeded_instances).with(no_args).ordered
+        expect(resource_pools).to receive(:update).with(no_args).ordered
+        expect(base_job).to receive(:task_checkpoint).with(no_args).ordered
+        expect(assembler).to receive(:bind_instance_vms).with(no_args).ordered
+        expect(assembler).to receive(:bind_configuration).with(no_args).ordered
+        expect(deployment_plan).to receive(:jobs_starting_on_deploy).and_return(jobs)
+        expect(multi_job_updater).to receive(:run).with(base_job, deployment_plan, jobs).ordered
+        expect(resource_pools).to receive(:refill).with(no_args).ordered
+        expect(deployment_plan).to receive(:persist_updates!).ordered
+        expect(deployment_plan).to receive(:update_stemcell_references!).ordered
+        subject.update
+      end
+
+      context 'when update fails' do
+        let(:some_error) { RuntimeError.new('oops') }
+        before do
+          allow(assembler).to receive(:bind_dns).with(no_args)
+          allow(assembler).to receive(:delete_unneeded_vms).with(no_args).and_raise(some_error)
+        end
 
-  describe '#update' do
-    it 'runs deployment plan update stages in a specific order' do
-      expect(assembler).to receive(:bind_dns).with(no_args).ordered
-      expect(assembler).to receive(:delete_unneeded_vms).with(no_args).ordered
-      expect(assembler).to receive(:delete_unneeded_instances).with(no_args).ordered
-      expect(resource_pools).to receive(:update).with(no_args).ordered
-      expect(base_job).to receive(:task_checkpoint).with(no_args).ordered
-      expect(assembler).to receive(:bind_instance_vms).with(no_args).ordered
-      expect(assembler).to receive(:bind_configuration).with(no_args).ordered
-      expect(multi_job_updater).to receive(:run).with(base_job, deployment_plan, jobs).ordered
-      expect(resource_pools).to receive(:refill).with(no_args).ordered
-      subject.update
+        it 'still updates the stemcell references' do