Merge pull request #496 from clintoncwolfe/cw/atomic_tags_v3

Tag on-demand instances at creation time

README.md

@@ -276,6 +276,7 @@ The Hash of EC tag name/value pairs which will be applied to the instance.
 
 The default is `{ "created-by" => "test-kitchen" }`.
 
+
 #### `user_data`
 
 The user_data script or the path to a script to feed the instance.

lib/kitchen/driver/aws/instance_generator.rb

@@ -115,8 +115,22 @@ def ec2_instance_data # rubocop:disable Metrics/MethodLength, Metrics/AbcSize
             key_name: config[:aws_ssh_key_id],
             subnet_id: config[:subnet_id],
             private_ip_address: config[:private_ip_address],
+            min_count: 1,
+            max_count: 1,
           }
 
+          if config[:tags] && !config[:tags].empty?
+            tags = config[:tags].map do |k, v|
+              # we convert the value to a string because
+              # nils should be passed as an empty String
+              # and Integers need to be represented as Strings
+              { key: k, value: v.to_s }
+            end
+            instance_tag_spec = { resource_type: "instance", tags: tags }
+            volume_tag_spec = { resource_type: "volume", tags: tags }
+            i[:tag_specifications] = [instance_tag_spec, volume_tag_spec]
+          end
+
           availability_zone = config[:availability_zone]
           if availability_zone
             if availability_zone =~ /^[a-z]$/i

lib/kitchen/driver/ec2.rb

@@ -227,7 +227,7 @@ def create(state)
 
         if config[:spot_price]
           # Spot instance when a price is set
-          server = with_request_limit_backoff(state) { submit_spots(state) }
+          server = with_request_limit_backoff(state) { submit_spots }
         else
           # On-demand instance
           server = with_request_limit_backoff(state) { submit_server }
@@ -238,32 +238,16 @@ def create(state)
           server.wait_until_exists(before_attempt: logging_proc)
         end
 
+        state[:server_id] = server.id
+        info("EC2 instance <#{state[:server_id]}> created.")
+
         # See https://github.com/aws/aws-sdk-ruby/issues/859
-        # Tagging can fail with a NotFound error even though we waited until the server exists
-        # Waiting can also fail, so we have to also retry on that. If it means we re-tag the
-        # instance, so be it.
-        # Tagging an instance is possible before volumes are attached. Tagging the volumes after
-        # instance creation is consistent.
+        # Waiting can fail, so we have to retry on that.
         Retryable.retryable(
           tries: 10,
           sleep: lambda { |n| [2**n, 30].min },
           on: ::Aws::EC2::Errors::InvalidInstanceIDNotFound
         ) do |r, _|
-          info("Attempting to tag the instance, #{r} retries")
-          tag_server(server)
-
-          # Get information about the AMI (image) used to create the image.
-          image_data = ec2.client.describe_images({ image_ids: [server.image_id] })[0][0]
-
-          state[:server_id] = server.id
-          info("EC2 instance <#{state[:server_id]}> created.")
-
-          # instance-store backed images do not have attached volumes, so only
-          # wait for the volumes to be ready if the instance EBS-backed.
-          if image_data.root_device_type == "ebs"
-            wait_until_volumes_ready(server, state)
-            tag_volumes(server)
-          end
           wait_until_ready(server, state)
         end
 
@@ -297,13 +281,6 @@ def destroy(state)
               warn("Received #{e}, instance was probably already destroyed. Ignoring")
             end
           end
-          if state[:spot_request_id]
-            debug("Deleting spot request <#{state[:server_id]}>")
-            ec2.client.cancel_spot_instance_requests(
-              spot_instance_request_ids: [state[:spot_request_id]]
-            )
-            state.delete(:spot_request_id)
-          end
           # If we are going to clean up an automatic security group, we need
           # to wait for the instance to shut down. This slightly breaks the
           # subsystem encapsulation, sorry not sorry.
@@ -409,15 +386,14 @@ def instance_generator
         @instance_generator = Aws::InstanceGenerator.new(config, ec2, instance.logger)
       end
 
-      # Fog AWS helper for creating the instance
+      # AWS helper for creating the instance
       def submit_server
         instance_data = instance_generator.ec2_instance_data
         debug("Creating EC2 instance in region #{config[:region]} with properties:")
         instance_data.each do |key, value|
           debug("- #{key} = #{value.inspect}")
         end
-        instance_data[:min_count] = 1
-        instance_data[:max_count] = 1
+
         ec2.create_instance(instance_data)
       end
 
@@ -445,7 +421,7 @@ def expand_config(conf, key)
         configs
       end
 
-      def submit_spots(state)
+      def submit_spots
         configs = [config]
         expanded = []
         keys = %i{instance_type subnet_id}
@@ -462,94 +438,55 @@ def submit_spots(state)
         configs.each do |conf|
           begin
             @config = conf
-            return submit_spot(state)
+            return submit_spot
           rescue => e
             errs.append(e)
           end
         end
         raise ["Could not create a spot instance:", errs].flatten.join("\n")
       end
 
-      def submit_spot(state)
+      def submit_spot
         debug("Creating EC2 Spot Instance..")
+        instance_data = instance_generator.ec2_instance_data
 
-        spot_request_id = create_spot_request
-        # deleting the instance cancels the request, but deleting the request
-        # does not affect the instance
-        state[:spot_request_id] = spot_request_id
-        ec2.client.wait_until(
-          :spot_instance_request_fulfilled,
-          spot_instance_request_ids: [spot_request_id]
-        ) do |w|
-          w.max_attempts = config[:spot_wait] / config[:retryable_sleep]
-          w.delay = config[:retryable_sleep]
-          w.before_attempt do |attempts|
-            c = attempts * config[:retryable_sleep]
-            t = config[:spot_wait]
-            info "Waited #{c}/#{t}s for spot request <#{spot_request_id}> to become fulfilled."
-          end
-        end
-        ec2.get_instance_from_spot_request(spot_request_id)
-      end
-
-      def create_spot_request
         request_duration = config[:spot_wait]
         config_spot_price = config[:spot_price].to_s
         if %w{ondemand on-demand}.include?(config_spot_price)
           spot_price = ""
         else
           spot_price = config_spot_price
         end
-        request_data = {
-          spot_price: spot_price,
-          launch_specification: instance_generator.ec2_instance_data,
+        spot_options = {
+          spot_instance_type: "persistent", # Cannot use one-time with valid_until
           valid_until: Time.now + request_duration,
+          instance_interruption_behavior: "stop",
         }
         if config[:block_duration_minutes]
-          request_data[:block_duration_minutes] = config[:block_duration_minutes]
+          spot_options[:block_duration_minutes] = config[:block_duration_minutes]
         end
-
-        response = ec2.client.request_spot_instances(request_data)
-        response[:spot_instance_requests][0][:spot_instance_request_id]
-      end
-
-      def tag_server(server)
-        if config[:tags] && !config[:tags].empty?
-          tags = config[:tags].map do |k, v|
-            # we convert the value to a string because
-            # nils should be passed as an empty String
-            # and Integers need to be represented as Strings
-            { key: k.to_s, value: v.to_s }
-          end
-          server.create_tags(tags: tags)
+        unless spot_price == "" # i.e. on-demand
+          spot_options[:max_price] = spot_price
         end
-      end
 
-      def tag_volumes(server)
-        if config[:tags] && !config[:tags].empty?
-          tags = config[:tags].map do |k, v|
-            { key: k.to_s, value: v.to_s }
-          end
-          server.volumes.each do |volume|
-            volume.create_tags(tags: tags)
-          end
-        end
-      end
+        instance_data[:instance_market_options] = {
+          market_type: "spot",
+          spot_options: spot_options,
+        }
 
-      # Compares the requested volume count vs what has actually been set to be
-      # attached to the instance. The information requested through
-      # ec2.client.described_volumes is updated before the instance volume
-      # information.
-      def wait_until_volumes_ready(server, state)
-        wait_with_destroy(server, state, "volumes to be ready") do |aws_instance|
-          described_volume_count = 0
-          ready_volume_count = 0
-          if aws_instance.exists?
-            described_volume_count = ec2.client.describe_volumes(filters: [
-              { name: "attachment.instance-id", values: ["#{state[:server_id]}"] }]).volumes.length
-            aws_instance.volumes.each { ready_volume_count += 1 }
-          end
-          (described_volume_count > 0) && (described_volume_count == ready_volume_count)
+        # The preferred way to create a spot instance is via request_spot_instances()
+        # However, it does not allow for tagging to occur at creation time.
+        # create_instances() allows creation of tagged spot instances, but does
+        # not retry if the price could not be satisfied immediately.
+        Retryable.retryable(
+          tries: config[:spot_wait] / config[:retryable_sleep],
+          sleep: lambda { |_n| config[:retryable_sleep] },
+          on: ::Aws::EC2::Errors::SpotMaxPriceTooLow
+        ) do |retries|
+          c = retries * config[:retryable_sleep]
+          t = config[:spot_wait]
+          info "Waited #{c}/#{t}s for spot request to become fulfilled."
+          ec2.create_instance(instance_data)
         end
       end