fix: clear destroyed servers from state and stop masking create failures

lib/kitchen/driver/vro.rb

@@ -16,6 +16,8 @@
 # limitations under the License.
 #
 
+require "timeout" unless defined?(Timeout)
+
 require "kitchen"
 require "vcoworkflows"
 require_relative "version"
@@ -87,14 +89,27 @@ def create(state)
 
       # Runs the destroy workflow for the server named in state.
       #
-      # @param state [Hash] instance state naming the server
+      # Clears +server_id+ and +hostname+ afterwards. Test Kitchen persists
+      # the state hash even when an action fails, so leaving a destroyed
+      # server named in it means the next +kitchen destroy+ runs the destroy
+      # workflow again against a machine that is already gone.
+      #
+      # @param state [Hash] mutable instance state naming the server; loses
+      #   +server_id+ and +hostname+
       # @return [void]
       def destroy(state)
         return if state[:server_id].nil?
 
-        info("Executing the destroy-server workflow for #{state[:hostname]} (#{state[:server_id]})...")
+        server_id = state[:server_id]
+        hostname  = state[:hostname]
+
+        info("Executing the destroy-server workflow for #{hostname} (#{server_id})...")
         execute_destroy_workflow(state)
-        info("Server #{state[:hostname]} (#{state[:server_id]}) destroyed.")
+
+        state.delete(:server_id)
+        state.delete(:hostname)
+
+        info("Server #{hostname} (#{server_id}) destroyed.")
       end
 
       # Connection settings for the vRO API.
@@ -131,16 +146,20 @@ def verify_ssl?
 
       # Points the driver at a different workflow.
       #
-      # Clears the memoized client, which is what makes it safe to run the
-      # destroy workflow after the create workflow in the same process.
+      # Clears the memoized client and the memoized output parameters, which
+      # is what makes it safe to run the destroy workflow after the create
+      # workflow in the same process. Without the second reset, anything
+      # reading {#output_parameters} after the switch would still be looking
+      # at the previous workflow's output.
       #
       # @param name [String] workflow name
       # @param id [String, nil] workflow id, when the name is ambiguous
       # @return [void]
       def set_workflow_vars(name, id)
-        @vro_client    = nil
-        @workflow_name = name
-        @workflow_id   = id
+        @vro_client        = nil
+        @output_parameters = nil
+        @workflow_name     = name
+        @workflow_id       = id
       end
 
       # Runs the create workflow and records what it produced.
@@ -221,17 +240,26 @@ def wait_for_workflow
       # @return [void]
       def wait_for_server(state)
         instance.transport.connection(state).wait_until_ready
-      rescue
+      rescue => e
         error("Server #{state[:hostname]} (#{state[:server_id]}) not reachable. Destroying server...")
-        destroy(state)
-        raise
+
+        begin
+          destroy(state)
+        rescue => destroy_error
+          # Report the leaked machine, but keep the connection failure as the
+          # error that surfaces -- it is the one that explains the run.
+          error("The destroy-server workflow also failed: #{destroy_error.message}. " \
+                "The server may still exist; check the vRO UI.")
+        end
+
+        raise e
       end
 
       # Sets input parameters on the current workflow.
       #
       # @param data [Hash] parameter names to values; names are stringified
       # @return [void]
-      def set_workflow_parameters(data) # rubocop:disable Style/AccessorMethodName
+      def set_workflow_parameters(data) # rubocop:disable Naming/AccessorMethodName
         data.each do |key, value|
           vro_client.parameter(key.to_s, value)
         end
@@ -245,15 +273,19 @@ def output_parameters
       end
 
       # @param key [String] output parameter name
-      # @return [String] the parameter's value, stringified
+      # @return [String] the parameter's value, stringified; an empty string
+      #   when the workflow did not return that parameter at all
       def output_parameter_value(key)
-        output_parameters[key].value.to_s
+        parameter = output_parameters[key]
+        return "" if parameter.nil?
+
+        parameter.value.to_s
       end
 
       # @param key [String] output parameter name
       # @return [Boolean] true when the parameter is missing or empty
       def output_parameter_empty?(key)
-        output_parameter_value(key).nil? || output_parameter_value(key).empty?
+        output_parameter_value(key).empty?
       end
 
       # Checks that the create workflow returned what the driver needs.

spec/vro_spec.rb

@@ -192,6 +192,17 @@
       expect(driver.workflow_id).to be_nil
     end
 
+    it "discards the memoized output parameters so they cannot leak between workflows" do
+      create_token = workflow_token(output: { "server_id" => "server-12345" })
+      allow(driver).to receive(:vro_client).and_return(workflow_client(token: create_token))
+      expect(driver.output_parameters.keys).to eq(["server_id"])
+
+      driver.set_workflow_vars("Destroy Workflow", "workflow-2")
+      allow(driver).to receive(:vro_client).and_return(workflow_client(token: workflow_token))
+
+      expect(driver.output_parameters).to be_empty
+    end
+
     it "discards the memoized client so the next call builds a new one" do
       first  = workflow_client
       second = workflow_client
@@ -284,6 +295,31 @@
       driver.destroy({})
     end
 
+    it "clears the destroyed server out of state" do
+      allow(driver).to receive(:execute_destroy_workflow)
+
+      driver.destroy(state)
+
+      expect(state).not_to have_key(:server_id)
+      expect(state).not_to have_key(:hostname)
+    end
+
+    it "leaves state alone when the destroy workflow fails" do
+      allow(driver).to receive(:execute_destroy_workflow).and_raise(RuntimeError)
+
+      expect { driver.destroy(state) }.to raise_error(RuntimeError)
+      expect(state[:server_id]).to eq("server-12345")
+    end
+
+    it "does not run the destroy workflow twice for the same server" do
+      allow(driver).to receive(:execute_destroy_workflow)
+
+      driver.destroy(state)
+      driver.destroy(state)
+
+      expect(driver).to have_received(:execute_destroy_workflow).once
+    end
+
     context "with the vRO API stubbed end to end" do
       let(:client) { workflow_client }
 
@@ -545,6 +581,29 @@
       expect { driver.wait_for_server(state) }
         .to raise_error(Kitchen::Transport::TransportFailed, "connection refused")
     end
+
+    context "when the cleanup destroy also fails" do
+      before do
+        allow(connection).to receive(:wait_until_ready)
+          .and_raise(Kitchen::Transport::TransportFailed, "connection refused")
+        allow(driver).to receive(:destroy).and_raise(RuntimeError, "vRO rejected the request")
+      end
+
+      it "still surfaces the connection failure, not the cleanup failure" do
+        allow(driver).to receive(:error)
+
+        expect { driver.wait_for_server(state) }
+          .to raise_error(Kitchen::Transport::TransportFailed, "connection refused")
+      end
+
+      it "warns that the server may have been left behind" do
+        expect(driver).to receive(:error).with(/not reachable/)
+        expect(driver).to receive(:error).with(/destroy-server workflow also failed/)
+
+        expect { driver.wait_for_server(state) }
+          .to raise_error(Kitchen::Transport::TransportFailed)
+      end
+    end
   end
 
   describe "#set_workflow_parameters" do
@@ -622,23 +681,38 @@
 
       expect(driver.output_parameter_value("ip_address")).to eq("")
     end
+
+    it "returns an empty string for a parameter the workflow never returned" do
+      allow(driver).to receive(:output_parameters).and_return({})
+
+      expect(driver.output_parameter_value("ip_address")).to eq("")
+    end
   end
 
   describe "#output_parameter_empty?" do
     it "is false when the value is neither nil nor empty" do
-      allow(driver).to receive(:output_parameter_value).with("test_key").and_return("test_value")
+      allow(driver).to receive(:output_parameters)
+        .and_return(workflow_output("test_key" => "test_value"))
 
       expect(driver.output_parameter_empty?("test_key")).to be false
     end
 
-    it "is true when the value is nil" do
-      allow(driver).to receive(:output_parameter_value).with("test_key").and_return(nil)
+    it "is true when vRO returned the parameter unset" do
+      allow(driver).to receive(:output_parameters)
+        .and_return(workflow_output("test_key" => nil))
 
       expect(driver.output_parameter_empty?("test_key")).to be true
     end
 
     it "is true when the value is empty" do
-      allow(driver).to receive(:output_parameter_value).with("test_key").and_return("")
+      allow(driver).to receive(:output_parameters)
+        .and_return(workflow_output("test_key" => ""))
+
+      expect(driver.output_parameter_empty?("test_key")).to be true
+    end
+
+    it "is true when the workflow did not return the parameter at all" do
+      allow(driver).to receive(:output_parameters).and_return({})
 
       expect(driver.output_parameter_empty?("test_key")).to be true
     end