fix: track created disks in the state file so an interrupt cannot orphan them (#160)

Standalone disks were recorded in an instance variable, and the cleanup
that deletes them hangs off `rescue => e`, which catches StandardError.
Interrupt is not a StandardError, so Ctrl-C during a create -- the most
common way a create ends early -- skipped cleanup entirely and left a
billable disk behind with no record of it anywhere:

    disks:  tk-datadisk-...-extra-disk  20GB  (no users)
    state:  --- {}
    kitchen destroy -> Finished destroying (0m0.00s)

Record the disks in the Test Kitchen state file instead, which is written
whatever happens, and have `destroy` delete anything still recorded once
the instance is gone. This mirrors what the instance itself already does.

Two supporting changes are needed to make that reachable:

- Pin the zone before any billable resource exists. In region mode it is
  chosen at random, so cleanup would not otherwise know where to look.
- Record the server name before the insert request is issued rather than
  after it returns. Live testing turned up a case where GCE created the
  instance but the interrupted request never returned, so the state file
  had the disk but not the instance holding it, and cleanup failed with
  resourceInUseByAnotherResource.

Confirmed against a real project: an interrupted create is now fully
reclaimed by `kitchen destroy`, leaving no instances and no disks.

Author: tas50

lib/kitchen/driver/gce.rb

@@ -197,16 +197,22 @@ def create(state)
 
         server_name = generate_server_name
 
+        # Pin the zone before anything billable exists. In region mode it is
+        # picked at random, so cleanup would otherwise have no way to know
+        # where to look for the disks created below.
+        state[:zone] = zone
+
+        # Record the name before the request goes out, not after it returns.
+        # GCE starts billing as soon as the insert is accepted, and it may
+        # accept one whose response never reaches us -- an interrupted or
+        # timed-out request can leave an instance running that the state file
+        # knows nothing about, holding disks that cleanup then cannot delete.
+        # `destroy` copes with a name that never became an instance.
+        state[:server_name] = server_name
+
         info("Creating GCE instance <#{server_name}> in project #{project}, zone #{zone}...")
         operation = connection.insert_instance(project, zone, create_instance_object(server_name))
 
-        # GCE starts billing for the instance as soon as the insert is
-        # accepted, so record it before waiting on the operation. Anything that
-        # goes wrong from here on can then be torn down by the rescue below,
-        # and by `kitchen destroy` if the process does not survive to run it.
-        state[:server_name] = server_name
-        state[:zone]        = zone
-
         wait_for_operation(operation)
 
         state[:hostname] = ip_address_for(server_instance(server_name))
@@ -231,30 +237,38 @@ def create(state)
         raise
       end
 
-      # Destroys the GCE instance recorded in the state file.
+      # Destroys the GCE instance recorded in the state file, together with any
+      # standalone disks an earlier create left behind.
+      #
+      # An instance that no longer exists in GCE is treated as already
+      # destroyed, but the state file is still cleared: a create that fails
+      # after `insert_instance` records a server that may never have come into
+      # being, and leaving the name behind would make {#create}'s idempotency
+      # guard skip every subsequent retry.
       #
-      # Does nothing when the state file records no server. An instance that no
-      # longer exists in GCE is treated as already destroyed, but the state file
-      # is still cleared: a create that fails after `insert_instance` records a
-      # server that may never have come into being, and leaving the name behind
-      # would make {#create}'s idempotency guard skip every subsequent retry.
+      # Disks are deleted after the instance, which holds them until it is
+      # gone. A create interrupted before it reached `insert_instance` records
+      # no server at all, so they are cleaned up whether or not there is one.
       #
       # @param state [Hash] the Test Kitchen state hash, mutated in place to
-      #   remove `:server_name`, `:hostname` and `:zone`
+      #   remove `:server_name`, `:hostname`, `:zone` and `:created_disks`
       # @return [void]
       def destroy(state)
         @state      = state
         server_name = state[:server_name]
-        return if server_name.nil?
 
-        if server_exist?(server_name)
-          info("Destroying GCE instance <#{server_name}>...")
-          wait_for_operation(connection.delete_instance(project, zone, server_name))
-          info("GCE instance <#{server_name}> destroyed.")
-        else
-          info("GCE instance <#{server_name}> does not exist - assuming it has been already destroyed.")
+        unless server_name.nil?
+          if server_exist?(server_name)
+            info("Destroying GCE instance <#{server_name}>...")
+            wait_for_operation(connection.delete_instance(project, zone, server_name))
+            info("GCE instance <#{server_name}> destroyed.")
+          else
+            info("GCE instance <#{server_name}> does not exist - assuming it has been already destroyed.")
+          end
         end
 
+        delete_created_disks
+
         state.delete(:server_name)
         state.delete(:hostname)
         state.delete(:zone)
@@ -1041,21 +1055,28 @@ def create_attached_disk(unique_disk_name, disk_config)
         attached_disk
       end
 
-      # Names of the standalone disks this driver created during the current
-      # action, tracked so they can be cleaned up if creation fails.
+      # Names of the standalone disks this driver has created, held in the Test
+      # Kitchen state file rather than in memory.
+      #
+      # These disks bill from the moment GCE creates them, which is before the
+      # instance exists, and the driver's own cleanup cannot be relied on to
+      # remove them: `Interrupt` is not a `StandardError`, so pressing Ctrl-C
+      # during a create skips the rescue entirely. Test Kitchen writes the
+      # state file whatever happens, so recording the names there is what makes
+      # a later `kitchen destroy` able to find them.
       #
       # @return [Array<String>] the created disk names
       def created_disk_names
-        @created_disk_names ||= []
+        state[:created_disks] ||= []
       end
 
-      # Deletes every standalone disk created during a failed create, so a
-      # partial run does not leave billable disks behind.
+      # Deletes every standalone disk this driver created, so a partial run
+      # does not leave billable disks behind.
       #
       # @return [void]
       def delete_created_disks
         created_disk_names.each { |disk_name| delete_disk(disk_name) }
-        created_disk_names.clear
+        state.delete(:created_disks)
       end
 
       # Deletes a standalone persistent disk, tolerating one that is already

spec/kitchen/driver/gce/disk_build_spec.rb

@@ -345,6 +345,75 @@ def built_disks
     end
   end
 
+  # An interrupted `kitchen create` never reaches the driver's own cleanup:
+  # Interrupt is not a StandardError, so the rescue does not run. The only
+  # thing that survives is the state file, so anything already billable has to
+  # be recorded there rather than in memory.
+  describe "recording standalone disks in the state file" do
+    let(:driver_config) do
+      { disks: { boot: { boot: true }, data: { disk_size: 50 } } }
+    end
+
+    it "records the zone before creating anything billable" do
+      allow_successful_create
+      allow(compute).to receive(:insert_disk).and_raise(Interrupt)
+      state = {}
+
+      expect { driver.create(state) }.to raise_error(Interrupt)
+
+      expect(state[:zone]).to eq("test-zone-1a")
+    end
+
+    it "records a standalone disk as soon as GCE has created it" do
+      allow_successful_create
+      driver.state = state = {}
+
+      driver.create_attached_disk("tk-test-1-data", disk_size: 50)
+
+      expect(state[:created_disks]).to eq(["tk-test-1-data"])
+    end
+
+    # GCE may well create the instance even when the response never reaches
+    # us, so the name has to be recorded before the request goes out. Without
+    # it, cleanup finds a disk it cannot delete because an instance it does not
+    # know about is still holding it.
+    it "records the server name before the insert request is issued" do
+      allow_successful_create
+      allow(driver).to receive(:generate_server_name).and_return("tk-test-1")
+      allow(compute).to receive(:insert_instance).and_raise(Interrupt)
+      state = {}
+
+      expect { driver.create(state) }.to raise_error(Interrupt)
+
+      expect(state[:server_name]).to eq("tk-test-1")
+    end
+
+    it "leaves an interrupted create's disk recorded for a later destroy to find" do
+      allow_successful_create
+      allow(driver).to receive(:generate_server_name).and_return("tk-test-1")
+      allow(compute).to receive(:insert_instance).and_raise(Interrupt)
+      state = {}
+
+      expect { driver.create(state) }.to raise_error(Interrupt)
+
+      expect(state[:created_disks]).to eq(["tk-test-1-data"])
+    end
+
+    it "deletes disks an interrupted create left behind, even with no server recorded" do
+      allow(compute).to receive(:get_disk).and_return(ComputeApi.disk)
+      allow(compute).to receive(:get_zone_operation).and_return(ComputeApi.operation)
+      state = { zone: "test-zone-1a", created_disks: ["tk-test-1-data"] }
+
+      expect(compute).to receive(:delete_disk)
+        .with("test-project", "test-zone-1a", "tk-test-1-data")
+        .and_return(ComputeApi.operation)
+
+      driver.destroy(state)
+
+      expect(state).to be_empty
+    end
+  end
+
   describe "#delete_created_disks" do
     it "does nothing when no standalone disks were created" do
       expect(compute).not_to receive(:delete_disk)