diff --git a/lib/kitchen/driver/vro.rb b/lib/kitchen/driver/vro.rb index a0f80de..32e207b 100644 --- a/lib/kitchen/driver/vro.rb +++ b/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. diff --git a/spec/vro_spec.rb b/spec/vro_spec.rb index bff6df9..4388c08 100644 --- a/spec/vro_spec.rb +++ b/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