From 6a63ee541dcfd21d22ad74579779c0de9c83093c Mon Sep 17 00:00:00 2001
From: sumitag <sumit.aga@gmail.com>
Date: Wed, 5 Sep 2018 08:39:11 -0400
Subject: [PATCH 1/8] Implementation of Atomic tagging (squash of
 sumitag/atomic-tags)

---
 README.md                       |  2 ++
 lib/kitchen/driver/ec2.rb       | 11 +++++++++++
 spec/kitchen/driver/ec2_spec.rb |  4 ++--
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/README.md b/README.md
index a6e4ccd3..6de2870c 100644
--- a/README.md
+++ b/README.md
@@ -276,6 +276,8 @@ The Hash of EC tag name/value pairs which will be applied to the instance.
 
 The default is `{ "created-by" => "test-kitchen" }`.
 
+Tags are applied post creation for spot instances and during creation for standard instances.
+
 #### `user_data`
 
 The user_data script or the path to a script to feed the instance.
diff --git a/lib/kitchen/driver/ec2.rb b/lib/kitchen/driver/ec2.rb
index 96d652ed..a7a125fc 100644
--- a/lib/kitchen/driver/ec2.rb
+++ b/lib/kitchen/driver/ec2.rb
@@ -418,6 +418,17 @@ def submit_server
         end
         instance_data[:min_count] = 1
         instance_data[: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 }
+          instance_data[:tag_specifications] = [instance_tag_spec, volume_tag_spec]
+        end
         ec2.create_instance(instance_data)
       end
 
diff --git a/spec/kitchen/driver/ec2_spec.rb b/spec/kitchen/driver/ec2_spec.rb
index 06d01175..8dacca45 100644
--- a/spec/kitchen/driver/ec2_spec.rb
+++ b/spec/kitchen/driver/ec2_spec.rb
@@ -230,7 +230,7 @@
 
     it "submits the server request" do
       expect(generator).to receive(:ec2_instance_data).and_return({})
-      expect(client).to receive(:create_instance).with(min_count: 1, max_count: 1)
+      expect(client).to receive(:create_instance).with(min_count: 1, max_count: 1, tag_specifications: anything)
       driver.submit_server
     end
   end
@@ -246,7 +246,7 @@
         instance_initiated_shutdown_behavior: "terminate"
       )
       expect(client).to receive(:create_instance).with(
-        min_count: 1, max_count: 1, instance_initiated_shutdown_behavior: "terminate"
+        min_count: 1, max_count: 1, instance_initiated_shutdown_behavior: "terminate", tag_specifications: anything
       )
       driver.submit_server
     end

From 112a908799499d66565073967478818afeb1b930 Mon Sep 17 00:00:00 2001
From: Clinton Wolfe <clintoncwolfe@gmail.com>
Date: Tue, 16 Jun 2020 17:02:04 -0400
Subject: [PATCH 2/8] Linting for chefstyle

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
---
 lib/kitchen/driver/ec2.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/kitchen/driver/ec2.rb b/lib/kitchen/driver/ec2.rb
index a7a125fc..0e2436d8 100644
--- a/lib/kitchen/driver/ec2.rb
+++ b/lib/kitchen/driver/ec2.rb
@@ -423,10 +423,10 @@ def submit_server
             # 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 }
+            { key: k, value: v.to_s }
           end
-          instance_tag_spec = { :resource_type => "instance", :tags => tags }
-          volume_tag_spec = { :resource_type => "volume", :tags => tags }
+          instance_tag_spec = { resource_type: "instance", tags: tags }
+          volume_tag_spec = { resource_type: "volume", tags: tags }
           instance_data[:tag_specifications] = [instance_tag_spec, volume_tag_spec]
         end
         ec2.create_instance(instance_data)

From 39fe2ecd1b12a212d3b453eb2a0ca096ca188ce6 Mon Sep 17 00:00:00 2001
From: Clinton Wolfe <clintoncwolfe@gmail.com>
Date: Tue, 16 Jun 2020 17:17:45 -0400
Subject: [PATCH 3/8] Expand testing of tagging at creation

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
---
 spec/kitchen/driver/ec2_spec.rb | 48 +++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/spec/kitchen/driver/ec2_spec.rb b/spec/kitchen/driver/ec2_spec.rb
index 8dacca45..edd0c118 100644
--- a/spec/kitchen/driver/ec2_spec.rb
+++ b/spec/kitchen/driver/ec2_spec.rb
@@ -35,6 +35,18 @@
       security_group_ids: ["sg-56789"],
     }
   end
+  let(:tag_spec) do
+    [
+      {
+        resource_type: "instance",
+        tags: [ { key: "created-by", value: "test-kitchen" } ],
+      },
+      {
+        resource_type: "volume",
+        tags: [ { key: "created-by", value: "test-kitchen" } ],
+      },
+    ]
+  end
   let(:platform)      { Kitchen::Platform.new(name: "fooos-99") }
   let(:transport)     { Kitchen::Transport::Dummy.new }
   let(:provisioner)   { Kitchen::Provisioner::Dummy.new }
@@ -230,7 +242,7 @@
 
     it "submits the server request" do
       expect(generator).to receive(:ec2_instance_data).and_return({})
-      expect(client).to receive(:create_instance).with(min_count: 1, max_count: 1, tag_specifications: anything)
+      expect(client).to receive(:create_instance).with(min_count: 1, max_count: 1, tag_specifications: tag_spec)
       driver.submit_server
     end
   end
@@ -246,7 +258,7 @@
         instance_initiated_shutdown_behavior: "terminate"
       )
       expect(client).to receive(:create_instance).with(
-        min_count: 1, max_count: 1, instance_initiated_shutdown_behavior: "terminate", tag_specifications: anything
+        min_count: 1, max_count: 1, instance_initiated_shutdown_behavior: "terminate", tag_specifications: tag_spec
       )
       driver.submit_server
     end
@@ -817,6 +829,38 @@
         include_examples "common create"
       end
     end
+
+    context "and setting tags" do
+      let(:tag_spec) do
+        [
+          {
+            resource_type: "instance",
+            tags: [
+              { key: "string", value: "a_string" },
+              { key: "integer", value: "1" },
+            ],
+          },
+          {
+            resource_type: "volume",
+            tags: [
+              { key: "string", value: "a_string" },
+              { key: "integer", value: "1" },
+            ],
+          },
+        ]
+      end
+
+      before do
+        config[:tags] = {
+          "string" => "a_string",
+          "integer" => 1,
+        }
+        expect(generator).to receive(:ec2_instance_data).and_return({})
+        expect(client).to receive(:create_instance).with(max_count: 1, min_count: 1, tag_specifications: tag_spec).and_return(server)
+      end
+
+      include_examples "common create"
+    end
   end
 
   describe "#destroy" do

From f8a310d5c55044e841d5b4daa16daded3de80aa0 Mon Sep 17 00:00:00 2001
From: Clinton Wolfe <clintoncwolfe@gmail.com>
Date: Thu, 25 Jun 2020 11:30:00 -0400
Subject: [PATCH 4/8] Create spot instances using create_instances

This allows spot instances to be tagged at creation time.
The tag-after-create code still needs to be removed, and there
is still some refactoring to be done.
Also, the ability to use "ondemand" as the spot price is
currently broken.

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
---
 lib/kitchen/driver/ec2.rb | 94 +++++++++++++++++++++++----------------
 1 file changed, 55 insertions(+), 39 deletions(-)

diff --git a/lib/kitchen/driver/ec2.rb b/lib/kitchen/driver/ec2.rb
index 0e2436d8..a07a7dcf 100644
--- a/lib/kitchen/driver/ec2.rb
+++ b/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 }
@@ -297,13 +297,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 +402,19 @@ 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
+
+        # TODO: @cwolfe DRY up push into instance generator
         instance_data[:min_count] = 1
         instance_data[:max_count] = 1
+
+        # TODO: @cwolfe DRY up push into instance generator
         if config[:tags] && !config[:tags].empty?
           tags = config[:tags].map do |k, v|
             # we convert the value to a string because
@@ -456,7 +453,7 @@ def expand_config(conf, key)
         configs
       end
 
-      def submit_spots(state)
+      def submit_spots
         configs = [config]
         expanded = []
         keys = %i{instance_type subnet_id}
@@ -473,7 +470,7 @@ def submit_spots(state)
         configs.each do |conf|
           begin
             @config = conf
-            return submit_spot(state)
+            return submit_spot
           rescue => e
             errs.append(e)
           end
@@ -481,47 +478,66 @@ def submit_spots(state)
         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 = ""
+          spot_price = determine_current_on_demand_price
         else
           spot_price = config_spot_price
         end
-        request_data = {
-          spot_price: spot_price,
-          launch_specification: instance_generator.ec2_instance_data,
+        spot_options = {
+          max_price: spot_price,
+          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
+        instance_data[:instance_market_options] = {
+          market_type: "spot",
+          spot_options: spot_options,
+        }
+
+        # TODO: @cwolfe move these to instance generator
+        instance_data[:min_count] = 1
+        instance_data[:max_count] = 1
+
+        # TODO: @cwolfe DRY up push into instance generator
+        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 }
+          instance_data[:tag_specifications] = [instance_tag_spec, volume_tag_spec]
+        end
+
+        # 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
 
-        response = ec2.client.request_spot_instances(request_data)
-        response[:spot_instance_requests][0][:spot_instance_request_id]
+      def determine_current_on_demand_price
+        "" # TODO @cwolfe this fails with create_instances()
       end
 
       def tag_server(server)

From 026db096ae623c556b8e6ca57533d0622b27406f Mon Sep 17 00:00:00 2001
From: Clinton Wolfe <clintoncwolfe@gmail.com>
Date: Thu, 25 Jun 2020 11:56:33 -0400
Subject: [PATCH 5/8] Remove tag-after-create code, push tag config into
 instance generator

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
---
 lib/kitchen/driver/aws/instance_generator.rb  |  14 +++
 lib/kitchen/driver/ec2.rb                     |  99 +--------------
 .../driver/aws/instance_generator_spec.rb     | 117 +++++++++++++++---
 3 files changed, 121 insertions(+), 109 deletions(-)

diff --git a/lib/kitchen/driver/aws/instance_generator.rb b/lib/kitchen/driver/aws/instance_generator.rb
index f5a6e359..1f54cac6 100644
--- a/lib/kitchen/driver/aws/instance_generator.rb
+++ b/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
diff --git a/lib/kitchen/driver/ec2.rb b/lib/kitchen/driver/ec2.rb
index a07a7dcf..80e25188 100644
--- a/lib/kitchen/driver/ec2.rb
+++ b/lib/kitchen/driver/ec2.rb
@@ -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
 
@@ -410,22 +394,6 @@ def submit_server
           debug("- #{key} = #{value.inspect}")
         end
 
-        # TODO: @cwolfe DRY up push into instance generator
-        instance_data[:min_count] = 1
-        instance_data[:max_count] = 1
-
-        # TODO: @cwolfe DRY up push into instance generator
-        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 }
-          instance_data[:tag_specifications] = [instance_tag_spec, volume_tag_spec]
-        end
         ec2.create_instance(instance_data)
       end
 
@@ -503,23 +471,6 @@ def submit_spot
           spot_options: spot_options,
         }
 
-        # TODO: @cwolfe move these to instance generator
-        instance_data[:min_count] = 1
-        instance_data[:max_count] = 1
-
-        # TODO: @cwolfe DRY up push into instance generator
-        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 }
-          instance_data[:tag_specifications] = [instance_tag_spec, volume_tag_spec]
-        end
-
         # 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
@@ -527,7 +478,7 @@ def submit_spot
         Retryable.retryable(
           tries: config[:spot_wait] / config[:retryable_sleep],
           sleep: lambda { |_n| config[:retryable_sleep] },
-          on: ::Aws::EC2::Errors::SpotMaxPriceTooLow,
+          on: ::Aws::EC2::Errors::SpotMaxPriceTooLow
         ) do |retries|
           c = retries * config[:retryable_sleep]
           t = config[:spot_wait]
@@ -540,46 +491,6 @@ def determine_current_on_demand_price
         "" # TODO @cwolfe this fails with create_instances()
       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)
-        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
-
-      # 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)
-        end
-      end
-
       # Normally we could use `server.wait_until_running` but we actually need
       # to check more than just the instance state
       def wait_until_ready(server, state)
diff --git a/spec/kitchen/driver/aws/instance_generator_spec.rb b/spec/kitchen/driver/aws/instance_generator_spec.rb
index 66c0c00d..9e12255c 100644
--- a/spec/kitchen/driver/aws/instance_generator_spec.rb
+++ b/spec/kitchen/driver/aws/instance_generator_spec.rb
@@ -99,7 +99,9 @@
         image_id: nil,
         key_name: nil,
         subnet_id: nil,
-        private_ip_address: nil
+        private_ip_address: nil,
+        max_count: 1,
+        min_count: 1
       )
     end
 
@@ -122,7 +124,42 @@
           image_id: "ami-123",
           key_name: nil,
           subnet_id: "s-456",
-          private_ip_address: "0.0.0.0"
+          private_ip_address: "0.0.0.0",
+          max_count: 1,
+          min_count: 1
+        )
+      end
+    end
+
+    context "when provided with tags" do
+      let(:config) do
+        {
+          region: "us-east-2",
+          tags: {
+            string_tag: "string",
+            integer_tag: 1,
+          },
+        }
+      end
+
+      it "includes tag specifications" do
+        expect(generator.ec2_instance_data).to include(
+          tag_specifications: [
+            {
+              resource_type: "instance",
+              tags: [
+                { key: :string_tag, value: "string" },
+                { key: :integer_tag, value: "1" },
+              ],
+            },
+            {
+              resource_type: "volume",
+              tags: [
+                { key: :string_tag, value: "string" },
+                { key: :integer_tag, value: "1" },
+              ],
+            },
+          ]
         )
       end
     end
@@ -147,7 +184,9 @@
           image_id: "ami-123",
           key_name: "key",
           subnet_id: "s-456",
-          private_ip_address: "0.0.0.0"
+          private_ip_address: "0.0.0.0",
+          max_count: 1,
+          min_count: 1
         )
       end
     end
@@ -288,7 +327,9 @@
           image_id: "ami-123",
           key_name: "key",
           subnet_id: "s-456",
-          private_ip_address: "0.0.0.0"
+          private_ip_address: "0.0.0.0",
+          max_count: 1,
+          min_count: 1
         )
       end
     end
@@ -308,7 +349,9 @@
           key_name: nil,
           subnet_id: nil,
           private_ip_address: nil,
-          placement: { availability_zone: "eu-west-1c" }
+          placement: { availability_zone: "eu-west-1c" },
+          max_count: 1,
+          min_count: 1
         )
       end
     end
@@ -328,7 +371,9 @@
           key_name: nil,
           subnet_id: nil,
           private_ip_address: nil,
-          placement: { availability_zone: "eu-east-1c" }
+          placement: { availability_zone: "eu-east-1c" },
+          max_count: 1,
+          min_count: 1
         )
       end
     end
@@ -346,7 +391,9 @@
           image_id: nil,
           key_name: nil,
           subnet_id: nil,
-          private_ip_address: nil
+          private_ip_address: nil,
+          max_count: 1,
+          min_count: 1
         )
       end
     end
@@ -367,6 +414,8 @@
           key_name: nil,
           subnet_id: nil,
           private_ip_address: nil,
+          max_count: 1,
+          min_count: 1,
           placement: { availability_zone: "eu-east-1c",
                        tenancy: "dedicated" }
         )
@@ -388,7 +437,9 @@
           key_name: nil,
           subnet_id: nil,
           private_ip_address: nil,
-          placement: { tenancy: "default" }
+          placement: { tenancy: "default" },
+          max_count: 1,
+          min_count: 1
         )
       end
     end
@@ -409,6 +460,8 @@
           key_name: nil,
           subnet_id: nil,
           private_ip_address: nil,
+          max_count: 1,
+          min_count: 1,
           placement: { availability_zone: "eu-east-1c",
                        tenancy: "dedicated" }
         )
@@ -430,7 +483,9 @@
           key_name: nil,
           subnet_id: nil,
           private_ip_address: nil,
-          placement: { tenancy: "default" }
+          placement: { tenancy: "default" },
+          max_count: 1,
+          min_count: 1
         )
       end
     end
@@ -450,7 +505,9 @@
           image_id: nil,
           key_name: nil,
           subnet_id: "s-456",
-          private_ip_address: nil
+          private_ip_address: nil,
+          max_count: 1,
+          min_count: 1
         )
       end
     end
@@ -475,7 +532,9 @@
             device_index: 0,
             associate_public_ip_address: true,
             delete_on_termination: true,
-          }]
+          }],
+          max_count: 1,
+          min_count: 1
         )
       end
 
@@ -500,7 +559,9 @@
               associate_public_ip_address: true,
               delete_on_termination: true,
               subnet_id: "s-456",
-            }]
+            }],
+            max_count: 1,
+            min_count: 1
           )
         end
       end
@@ -527,7 +588,9 @@
               associate_public_ip_address: true,
               delete_on_termination: true,
               groups: ["sg-789"],
-            }]
+            }],
+            max_count: 1,
+            min_count: 1
           )
         end
 
@@ -566,7 +629,9 @@
               associate_public_ip_address: true,
               delete_on_termination: true,
               private_ip_address: "0.0.0.0",
-            }]
+            }],
+            max_count: 1,
+            min_count: 1
           )
         end
       end
@@ -599,6 +664,10 @@
           user_data: "foo",
           iam_profile_name: "iam-123",
           associate_public_ip: true,
+          tags: {
+            string_tag: "string",
+            integer_tag: 1,
+          },
         }
       end
 
@@ -630,7 +699,25 @@
             private_ip_address: "0.0.0.0",
           }],
           placement: { availability_zone: "eu-west-1a" },
-          user_data: Base64.encode64("foo")
+          user_data: Base64.encode64("foo"),
+          max_count: 1,
+          min_count: 1,
+          tag_specifications: [
+            {
+              resource_type: "instance",
+              tags: [
+                { key: :string_tag, value: "string" },
+                { key: :integer_tag, value: "1" },
+              ],
+            },
+            {
+              resource_type: "volume",
+              tags: [
+                { key: :string_tag, value: "string" },
+                { key: :integer_tag, value: "1" },
+              ],
+            },
+          ]
         )
       end
     end

From ea470fd6dc7e78e200bb85c8719d35a9eacc5e22 Mon Sep 17 00:00:00 2001
From: Clinton Wolfe <clintoncwolfe@gmail.com>
Date: Thu, 25 Jun 2020 13:38:41 -0400
Subject: [PATCH 6/8] Update unit test

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
---
 spec/kitchen/driver/ec2_spec.rb | 253 +++-----------------------------
 1 file changed, 18 insertions(+), 235 deletions(-)

diff --git a/spec/kitchen/driver/ec2_spec.rb b/spec/kitchen/driver/ec2_spec.rb
index edd0c118..b12adcda 100644
--- a/spec/kitchen/driver/ec2_spec.rb
+++ b/spec/kitchen/driver/ec2_spec.rb
@@ -35,18 +35,6 @@
       security_group_ids: ["sg-56789"],
     }
   end
-  let(:tag_spec) do
-    [
-      {
-        resource_type: "instance",
-        tags: [ { key: "created-by", value: "test-kitchen" } ],
-      },
-      {
-        resource_type: "volume",
-        tags: [ { key: "created-by", value: "test-kitchen" } ],
-      },
-    ]
-  end
   let(:platform)      { Kitchen::Platform.new(name: "fooos-99") }
   let(:transport)     { Kitchen::Transport::Dummy.new }
   let(:provisioner)   { Kitchen::Provisioner::Dummy.new }
@@ -242,7 +230,7 @@
 
     it "submits the server request" do
       expect(generator).to receive(:ec2_instance_data).and_return({})
-      expect(client).to receive(:create_instance).with(min_count: 1, max_count: 1, tag_specifications: tag_spec)
+      expect(client).to receive(:create_instance)
       driver.submit_server
     end
   end
@@ -258,18 +246,13 @@
         instance_initiated_shutdown_behavior: "terminate"
       )
       expect(client).to receive(:create_instance).with(
-        min_count: 1, max_count: 1, instance_initiated_shutdown_behavior: "terminate", tag_specifications: tag_spec
+        instance_initiated_shutdown_behavior: "terminate"
       )
       driver.submit_server
     end
   end
 
   describe "#submit_spot" do
-    let(:state) { {} }
-    let(:response) do
-      { spot_instance_requests: [{ spot_instance_request_id: "id" }] }
-    end
-
     before do
       expect(driver).to receive(:instance).at_least(:once).and_return(instance)
       allow(Time).to receive(:now).and_return(Time.now)
@@ -277,119 +260,19 @@
 
     it "submits the server request" do
       expect(generator).to receive(:ec2_instance_data).and_return({})
-      expect(actual_client).to receive(:request_spot_instances).with(
-        spot_price: "",
-        launch_specification: {},
-        valid_until: Time.now + config[:spot_wait],
-        block_duration_minutes: 60
-      ).and_return(response)
-      expect(actual_client).to receive(:wait_until)
-      expect(client).to receive(:get_instance_from_spot_request).with("id")
-      driver.submit_spot(state)
-      expect(state).to eq(spot_request_id: "id")
-    end
-  end
-
-  describe "#tag_server" do
-    context "with no tags specified" do
-      it "does not raise" do
-        config[:tags] = nil
-        expect { driver.tag_server(server) }.not_to raise_error
-      end
-    end
-
-    context "with standard string tags" do
-      it "tags the server" do
-        config[:tags] = { key1: "value1", key2: "value2" }
-        expect(server).to receive(:create_tags).with(
-          tags: [
-            { key: "key1", value: "value1" },
-            { key: "key2", value: "value2" },
-          ]
-        )
-        driver.tag_server(server)
-      end
-      it "tags the server with String keys" do
-        config[:tags] = { key1: "value1", "key2" => "value2" }
-        expect(server).to receive(:create_tags).with(
-          tags: [
-            { key: "key1", value: "value1" },
-            { key: "key2", value: "value2" },
-          ]
-        )
-        driver.tag_server(server)
-      end
-    end
-
-    context "with a tag that includes a Integer value" do
-      it "tags the server" do
-        config[:tags] = { key1: "value1", key2: 1 }
-        expect(server).to receive(:create_tags).with(
-          tags: [
-            { key: "key1", value: "value1" },
-            { key: "key2", value: "1" },
-          ]
-        )
-        driver.tag_server(server)
-      end
-    end
-
-    context "with a tag that includes a Nil value" do
-      it "tags the server" do
-        config[:tags] = { key1: "value1", key2: nil }
-        expect(server).to receive(:create_tags).with(
-          tags: [
-            { key: "key1", value: "value1" },
-            { key: "key2", value: "" },
-          ]
-        )
-        driver.tag_server(server)
-      end
-    end
-  end
-
-  describe "#tag_volumes" do
-    let(:volume) { double("aws volume resource") }
-    before do
-      allow(server).to receive(:volumes).and_return([volume])
-    end
-    context "with standard string tags" do
-      it "tags the instance volumes" do
-        config[:tags] = { key1: "value1", key2: "value2" }
-        expect(volume).to receive(:create_tags).with(
-          tags: [
-            { key: "key1", value: "value1" },
-            { key: "key2", value: "value2" },
-          ]
-        )
-        driver.tag_volumes(server)
-      end
-    end
-
-    context "with a tag that includes a Integer value" do
-      it "tags the instance volumes" do
-        config[:tags] = { key1: "value1", key2: 2 }
-        expect(volume).to receive(:create_tags).with(
-          tags: [
-            { key: "key1", value: "value1" },
-            { key: "key2", value: "2" },
-          ]
-        )
-        driver.tag_volumes(server)
-      end
-    end
-
-    context "with a tag that includes a Nil value" do
-      it "tags the instance volumes" do
-        config[:tags] = { key1: "value1", key2: nil }
-        expect(volume).to receive(:create_tags).with(
-          tags: [
-            { key: "key1", value: "value1" },
-            { key: "key2", value: "" },
-          ]
-        )
-        driver.tag_volumes(server)
-      end
+      expect(client).to receive(:create_instance).with(
+        instance_market_options: {
+          market_type: "spot",
+          spot_options: {
+            max_price: "", # TODO: @cwolfe
+            spot_instance_type: "persistent",
+            instance_interruption_behavior: "stop",
+            valid_until: Time.now + config[:spot_wait],
+            block_duration_minutes: 60,
+          },
+        }
+      ).and_return(server)
+      driver.submit_spot
     end
   end
 
@@ -436,40 +319,6 @@
     end
   end
 
-  describe "#wait_until_volumes_ready" do
-    let(:aws_instance) { double("aws instance") }
-    let(:msg) { "volumes to be ready" }
-    let(:volume) { double("aws volume resource") }
-
-    before do
-      expect(driver).to receive(:wait_with_destroy).with(server, state, msg).and_yield(aws_instance)
-    end
-    it "first checks instance existence" do
-      expect(aws_instance).to receive(:exists?).and_return(false)
-      expect(driver.wait_until_volumes_ready(server, state)).to eq(false)
-    end
-    it "second, it checks for prescence of described volumes" do
-      expect(aws_instance).to receive(:exists?).and_return(true)
-      expect(actual_client).to receive_message_chain(:describe_volumes, :volumes, :length).and_return(0)
-      expect(aws_instance).to receive(:volumes).and_return([])
-      expect(driver.wait_until_volumes_ready(server, state)).to eq(false)
-    end
-    it "third, it compares the described volumes and instance volumes" do
-      expect(aws_instance).to receive(:exists?).and_return(true)
-      expect(actual_client).to receive_message_chain(:describe_volumes, :volumes, :length).and_return(2)
-      expect(aws_instance).to receive(:volumes).and_return([volume])
-      expect(driver.wait_until_volumes_ready(server, state)).to eq(false)
-    end
-    context "when it exists, and both client and instance agree on volumes" do
-      it "returns true" do
-        expect(aws_instance).to receive(:exists?).and_return(true)
-        expect(actual_client).to receive_message_chain(:describe_volumes, :volumes, :length).and_return(1)
-        expect(aws_instance).to receive(:volumes).and_return([volume])
-        expect(driver.wait_until_volumes_ready(server, state)).to eq(true)
-      end
-    end
-  end
-
   describe "#fetch_windows_admin_password" do
     let(:msg) { "to fetch windows admin password" }
     let(:aws_instance) { double("aws instance") }
@@ -571,11 +420,7 @@
       it "successfully creates and tags the instance" do
         expect(server).to receive(:wait_until_exists)
         expect(driver).to receive(:update_username)
-        expect(driver).to receive(:tag_server).with(server)
-        expect(driver).to receive(:tag_volumes).with(server)
-        expect(driver).to receive(:wait_until_volumes_ready).with(server, state)
         expect(driver).to receive(:wait_until_ready).with(server, state)
-        allow(actual_client).to receive(:describe_images).with({ image_ids: [server.image_id] }).and_return(ec2_stub)
         expect(transport).to receive_message_chain("connection.wait_until_ready")
         driver.create(state)
         expect(state[:server_id]).to eq(id)
@@ -608,7 +453,7 @@
 
       context "price is numeric" do
         before do
-          expect(driver).to receive(:submit_spots).with(state).and_return(server)
+          expect(driver).to receive(:submit_spots).and_return(server)
         end
 
         include_examples "common create"
@@ -617,7 +462,7 @@
       context "price is on-demand" do
         before do
           config[:spot_price] = "on-demand"
-          expect(driver).to receive(:submit_spots).with(state).and_return(server)
+          expect(driver).to receive(:submit_spots).and_return(server)
         end
 
         include_examples "common create"
@@ -626,7 +471,7 @@
       context "instance_type is an array" do
         before do
           config[:instance_type] = %w{t1 t2}
-          expect(driver).to receive(:submit_spot).with(state).and_return(server)
+          expect(driver).to receive(:submit_spot).and_return(server)
         end
 
         include_examples "common create"
@@ -641,21 +486,6 @@
       end
     end
 
-    context "instance is not ebs-backed" do
-      before do
-        ec2_stub.images[0].root_device_type = "instance-store"
-      end
-
-      it "does not tag volumes or wait for volumes to be ready" do
-        expect(driver).to_not receive(:tag_volumes).with(server)
-        expect(driver).to_not receive(:wait_until_volumes_ready).with(server, state)
-      end
-
-      after do
-        ec2_stub.images[0].root_device_type = "ebs"
-      end
-    end
-
     context "instance is a windows machine" do
       before do
         expect(driver).to receive(:windows_os?).and_return(true)
@@ -829,38 +659,6 @@
         include_examples "common create"
       end
     end
-
-    context "and setting tags" do
-      let(:tag_spec) do
-        [
-          {
-            resource_type: "instance",
-            tags: [
-              { key: "string", value: "a_string" },
-              { key: "integer", value: "1" },
-            ],
-          },
-          {
-            resource_type: "volume",
-            tags: [
-              { key: "string", value: "a_string" },
-              { key: "integer", value: "1" },
-            ],
-          },
-        ]
-      end
-
-      before do
-        config[:tags] = {
-          "string" => "a_string",
-          "integer" => 1,
-        }
-        expect(generator).to receive(:ec2_instance_data).and_return({})
-        expect(client).to receive(:create_instance).with(max_count: 1, min_count: 1, tag_specifications: tag_spec).and_return(server)
-      end
-
-      include_examples "common create"
-    end
   end
 
   describe "#destroy" do
@@ -890,21 +688,6 @@
       end
     end
 
-    context "when state has a spot request" do
-      let(:state) { { server_id: "id", hostname: "name", spot_request_id: "spot" } }
-
-      it "destroys the server" do
-        expect(client).to receive(:get_instance).with("id").and_return(server)
-        expect(instance).to receive_message_chain("transport.connection.close")
-        expect(server).to receive(:terminate)
-        expect(actual_client).to receive(:cancel_spot_instance_requests).with(
-          spot_instance_request_ids: ["spot"]
-        )
-        driver.destroy(state)
-        expect(state).to eq({})
-      end
-    end
-
     context "when the state has an automatic security group" do
       let(:state) { { auto_security_group_id: "sg-asdf" } }
 

From f9370cb1c36751c5c66a8236fd82a1155f4ef865 Mon Sep 17 00:00:00 2001
From: Clinton Wolfe <clintoncwolfe@gmail.com>
Date: Thu, 25 Jun 2020 17:12:59 -0400
Subject: [PATCH 7/8] Omit max_price field when spot_price is set to on-demand

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
---
 lib/kitchen/driver/ec2.rb       | 11 +++++------
 spec/kitchen/driver/ec2_spec.rb |  1 -
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/lib/kitchen/driver/ec2.rb b/lib/kitchen/driver/ec2.rb
index 80e25188..c4c578fb 100644
--- a/lib/kitchen/driver/ec2.rb
+++ b/lib/kitchen/driver/ec2.rb
@@ -453,12 +453,11 @@ def submit_spot
         request_duration = config[:spot_wait]
         config_spot_price = config[:spot_price].to_s
         if %w{ondemand on-demand}.include?(config_spot_price)
-          spot_price = determine_current_on_demand_price
+          spot_price = ""
         else
           spot_price = config_spot_price
         end
         spot_options = {
-          max_price: spot_price,
           spot_instance_type: "persistent", # Cannot use one-time with valid_until
           valid_until: Time.now + request_duration,
           instance_interruption_behavior: "stop",
@@ -466,6 +465,10 @@ def submit_spot
         if config[:block_duration_minutes]
           spot_options[:block_duration_minutes] = config[:block_duration_minutes]
         end
+        unless spot_price == "" # i.e. on-demand
+          spot_options[:max_price] = spot_price
+        end
+
         instance_data[:instance_market_options] = {
           market_type: "spot",
           spot_options: spot_options,
@@ -487,10 +490,6 @@ def submit_spot
         end
       end
 
-      def determine_current_on_demand_price
-        "" # TODO @cwolfe this fails with create_instances()
-      end
-
       # Normally we could use `server.wait_until_running` but we actually need
       # to check more than just the instance state
       def wait_until_ready(server, state)
diff --git a/spec/kitchen/driver/ec2_spec.rb b/spec/kitchen/driver/ec2_spec.rb
index b12adcda..225a0766 100644
--- a/spec/kitchen/driver/ec2_spec.rb
+++ b/spec/kitchen/driver/ec2_spec.rb
@@ -264,7 +264,6 @@
         instance_market_options: {
           market_type: "spot",
           spot_options: {
-            max_price: "", # TODO: @cwolfe
             spot_instance_type: "persistent",
             instance_interruption_behavior: "stop",
             valid_until: Time.now + config[:spot_wait],

From 85787793c1f9eec7f57ea4b4069d808f4a1459a9 Mon Sep 17 00:00:00 2001
From: Tim Smith <tsmith@chef.io>
Date: Thu, 2 Jul 2020 13:03:06 -0700
Subject: [PATCH 8/8] Update README.md

Co-authored-by: Tyler Ball <tball@chef.io>
---
 README.md | 1 -
 1 file changed, 1 deletion(-)

diff --git a/README.md b/README.md
index 6de2870c..5106534e 100644
--- a/README.md
+++ b/README.md
@@ -276,7 +276,6 @@ The Hash of EC tag name/value pairs which will be applied to the instance.
 
 The default is `{ "created-by" => "test-kitchen" }`.
 
-Tags are applied post creation for spot instances and during creation for standard instances.
 
 #### `user_data`