Merge rebased BOSH PR 915

Rebase PR against extracted CPI core in bosh-aws-cpi-release.

[#104456366](https://www.pivotaltracker.com/story/show/104456366)

Signed-off-by: Beyhan Veli <beyhan.veli@sap.com>

src/bosh_aws_cpi/lib/cloud/aws/helpers.rb

@@ -1,15 +1,13 @@
-# Copyright (c) 2009-2012 VMware, Inc.
-
 module Bosh::AwsCloud
 
   module Helpers
     def default_ephemeral_disk_mapping
-      [
-        {
-          device_name: '/dev/sdb',
-          virtual_name: 'ephemeral0',
-        }
-      ]
+       [
+         {
+           :device_name => '/dev/sdb',
+           :virtual_name => 'ephemeral0',
+         },
+       ]
     end
 
     def ebs_ephemeral_disk_mapping(volume_size_in_gb, volume_type)

src/bosh_aws_cpi/lib/cloud/aws/instance_manager.rb

@@ -82,7 +82,7 @@ def create(agent_id, stemcell_id, resource_pool, networks_spec, disk_locality, e
 
       @logger.info("Creating new instance with: #{instance_params.inspect}")
 
-      aws_instance = create_aws_instance(instance_params, resource_pool["spot_bid_price"])
+      aws_instance = create_aws_instance(instance_params, resource_pool)
 
       instance = Instance.new(aws_instance, @registry, @elb, @logger)
 
@@ -142,23 +142,32 @@ def build_instance_params(stemcell_id, resource_pool, networks_spec, disk_locali
       return instance_params, block_device_info
     end
 
-    def create_aws_instance(instance_params, spot_bid_price)
-      if spot_bid_price
-        @logger.info("Launching spot instance...")
-        spot_manager = Bosh::AwsCloud::SpotManager.new(@region)
-        spot_manager.create(instance_params, spot_bid_price)
-      else
-        # Retry the create instance operation a couple of times if we are told that the IP
-        # address is in use - it can happen when the director recreates a VM and AWS
-        # is too slow to update its state when we have released the IP address and want to
-        # realocate it again.
-        errors = [AWS::EC2::Errors::InvalidIPAddress::InUse, AWS::EC2::Errors::RequestLimitExceeded]
-        Bosh::Common.retryable(sleep: instance_create_wait_time, tries: 20, on: errors) do |tries, error|
-          @logger.info("Launching on demand instance...")
-          @logger.warn("IP address was in use: #{error}") if tries > 0
-          @region.instances.create(instance_params)
+    def create_aws_spot_instance(instance_params, spot_bid_price)
+      @logger.info("Launching spot instance...")
+      spot_manager = Bosh::AwsCloud::SpotManager.new(@region)
+
+      spot_manager.create(instance_params, spot_bid_price)
+    end
+
+    def create_aws_instance(instance_params, resource_pool)
+      if resource_pool["spot_bid_price"]
+        begin
+          return create_aws_spot_instance instance_params, resource_pool["spot_bid_price"]
+        rescue Bosh::Clouds::VMCreationFailed => e
+          raise unless resource_pool["spot_ondemand_fallback"]
         end
       end
+
+      # Retry the create instance operation a couple of times if we are told that the IP
+      # address is in use - it can happen when the director recreates a VM and AWS
+      # is too slow to update its state when we have released the IP address and want to
+      # realocate it again.
+      errors = [AWS::EC2::Errors::InvalidIPAddress::InUse, AWS::EC2::Errors::RequestLimitExceeded]
+      Bosh::Common.retryable(sleep: instance_create_wait_time, tries: 20, on: errors) do |tries, error|
+        @logger.info("Launching on demand instance...")
+        @logger.warn("IP address was in use: #{error}") if tries > 0
+        @region.instances.create(instance_params)
+      end
     end
 
     def instance_create_wait_time; 30; end

src/bosh_aws_cpi/lib/cloud/aws/spot_manager.rb

@@ -12,6 +12,7 @@ def initialize(region)
     end
 
     def create(instance_params, spot_bid_price)
+      @instance_params = instance_params
       spot_request_spec = create_spot_request_spec(instance_params, spot_bid_price)
       @logger.debug("Requesting spot instance with: #{spot_request_spec.inspect}")
 
@@ -22,12 +23,12 @@ def create(instance_params, spot_bid_price)
         raise Bosh::Clouds::VMCreationFailed.new(false), e.inspect
       end
 
-      request_spot_instance
+      wait_for_spot_instance
     end
 
     private
 
-    def request_spot_instance
+    def wait_for_spot_instance
       instance = nil
 
       # Query the spot request state until it becomes "active".
@@ -43,20 +44,17 @@ def request_spot_instance
             fail_spot_creation("VM spot instance creation failed: #{status.inspect}")
           when 'open'
             if status[:status] != nil && status[:status][:code] == 'price-too-low'
-              fail_spot_creation("Cannot create VM spot instance because bid price is too low: #{status.inspect}")
+              fail_spot_creation("Cannot create VM spot instance because bid price is too low: #{status.inspect}. Reverting to creating ondemand instance")
             end
           when 'active'
             @logger.info("Spot request instances fulfilled: #{status.inspect}")
             instance = @region.instances[status[:instance_id]]
-            true
         end
       end
 
       instance
     rescue Bosh::Common::RetryCountExceeded
-      @logger.warn("Timed out waiting for spot request #{@spot_instance_requests.inspect} to be fulfilled")
-      cancel_pending_spot_requests
-      raise Bosh::Clouds::VMCreationFailed.new(true)
+      fail_spot_creation("Timed out waiting for spot request #{@spot_instance_requests.inspect} to be fulfilled. Reverting to creating ondemand instance")
     end
 
     def create_spot_request_spec(instance_params, spot_price)
@@ -103,7 +101,7 @@ def spot_instance_request_status
     end
 
     def fail_spot_creation(message)
-      @logger.error(message)
+      @logger.warn(message)when ephemeral disk size is not specified
       cancel_pending_spot_requests
       raise Bosh::Clouds::VMCreationFailed.new(false), message
     end

src/bosh_aws_cpi/spec/unit/instance_manager_spec.rb

@@ -38,10 +38,12 @@
         security_groups: ['baz'],
         private_ip_address: '1.2.3.4',
         availability_zone: 'us-east-1a',
-        block_device_mappings: [{
-                                    device_name: '/dev/sdb',
-                                    virtual_name: 'ephemeral0',
-                                }]
+        block_device_mappings: [
+          {
+            :device_name => '/dev/sdb',
+            :virtual_name => 'ephemeral0',
+          },
+        ]
       }
     end
 
@@ -197,6 +199,33 @@
         # Trigger spot instance request
         create_instance
       end
+
+      it 'should raise an exception when spot creation fails' do
+        expect(instance_manager).to receive(:create_aws_spot_instance).and_raise(Bosh::Clouds::VMCreationFailed.new(false))
+
+        expect {
+          create_instance
+        }.to raise_error(Bosh::Clouds::VMCreationFailed)
+      end
+
+      context 'when spot_ondemand_fallback is configured' do
+        let(:resource_pool) do
+          {
+            'spot_bid_price' => 0.15,
+            'spot_ondemand_fallback' => true,
+            'instance_type' => 'm1.small',
+            'key_name' => 'bar',
+          }
+        end
+
+        it 'should create an on demand instance when spot creation fails AND we have enabled spot_ondemand_fallback' do
+          expect(instance_manager).to receive(:create_aws_spot_instance).and_raise(Bosh::Clouds::VMCreationFailed.new(false))
+
+          expect(aws_instances).to receive(:create).and_return(aws_instance)
+
+          create_instance
+        end
+      end
     end
 
     it 'should retry creating the VM when AWS::EC2::Errors::InvalidIPAddress::InUse raised' do
@@ -522,26 +551,26 @@ def set_ephemeral_disk_size(size)
               end
             end
           end
+        end
 
-          context 'when instance type does not have instance storage' do
-            before { set_instance_type 't2.small' }
+        context 'when instance type does not have instance storage' do
+          before { set_instance_type 't2.small' }
 
-            it 'uses a default 10GB ebs storage for ephemeral disk' do
-              allow(aws_instances).to receive(:create) { aws_instance }
+          it 'uses a default 10GB ebs storage for ephemeral disk' do
+            allow(aws_instances).to receive(:create) { aws_instance }
 
-              create_instance
+            create_instance
 
-              expect(aws_instances).to have_received(:create) do |instance_params|
-                expect(instance_params[:block_device_mappings]).to eq([
-                  {
-                    device_name: '/dev/sdb',
-                    ebs: {
-                      volume_size: 10,
-                      volume_type: 'standard',
-                      delete_on_termination: true,
-                    }
-                  }])
-              end
+            expect(aws_instances).to have_received(:create) do |instance_params|
+              expect(instance_params[:block_device_mappings]).to eq([
+                {
+                  device_name: '/dev/sdb',
+                  ebs: {
+                    volume_size: 10,
+                    volume_type: 'standard',
+                    delete_on_termination: true,
+                  }
+                }])
             end
           end
         end

src/bosh_aws_cpi/spec/unit/spot_manager_spec.rb

@@ -72,7 +72,7 @@
       }
     }).and_return(spot_instance_requests)
 
-    spot_manager.create(instance_params, spot_bid_price)
+    expect(spot_manager.create(instance_params, spot_bid_price)).to be(instance)
   end
 
   context 'when instance does not have security groups' do
@@ -83,11 +83,11 @@
         expect(request_params[:launch_specification][:network_interfaces][0]).to_not include(:groups)
       end.and_return(spot_instance_requests)
 
-      spot_manager.create(instance_params, spot_bid_price)
+      expect(spot_manager.create(instance_params, spot_bid_price)).to be(instance)
     end
   end
 
-  it 'should wait total_spot_instance_request_wait_time() seconds for a SPOT instance to be started, and then fail (but allow retries)' do
+  it 'should fail to return an instance when starting a spot instance times out' do
     spot_instance_requests = {
       spot_instance_request_set: [
         { spot_instance_request_id: 'sir-12345c' }
@@ -96,20 +96,16 @@
     }
 
     expect(aws_client).to receive(:describe_spot_instance_requests).
-      exactly(10).times.with({ spot_instance_request_ids: ['sir-12345c'] }).
+      exactly(Bosh::AwsCloud::SpotManager::RETRY_COUNT).times.with({ spot_instance_request_ids: ['sir-12345c'] }).
       and_return({ spot_instance_request_set: [{ state: 'open' }] })
 
     # When erroring, should cancel any pending spot requests
     expect(aws_client).to receive(:cancel_spot_instance_requests)
 
-    expect(Bosh::Common).to receive(:retryable).
-      with(sleep: 0.01, tries: 10, on: [AWS::EC2::Errors::InvalidSpotInstanceRequestID::NotFound]).
-      and_call_original
-
     expect {
       spot_manager.create(instance_params, spot_bid_price)
-    }.to raise_error(Bosh::Clouds::VMCreationFailed){ |error|
-      expect(error.ok_to_retry).to eq true
+    }.to raise_error(Bosh::Clouds::VMCreationFailed) { |error|
+      expect(error.ok_to_retry).to eq false
     }
   end
 
@@ -130,8 +126,9 @@
     }.to_not raise_error
   end
 
-  it 'should fail VM creation (no retries) when spot bid price is below market price' do
+  it 'should immediately fail to return an instance when spot bid price is too low' do
     expect(aws_client).to receive(:describe_spot_instance_requests).
+      exactly(1).times.
       with({ spot_instance_request_ids: ['sir-12345c'] }).
       and_return(
       {
@@ -241,7 +238,7 @@
         }
       }).and_return(spot_instance_requests)
 
-      spot_manager.create(instance_params, spot_bid_price)
+      expect(spot_manager.create(instance_params, spot_bid_price)).to be(instance)
     end
   end
 end