From 6b0be2d6208a670484423222301f5898851c824c Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Sat, 29 Aug 2026 18:47:16 -0700 Subject: [PATCH] fix: clear destroyed servers from state and stop masking create failures A handful of related problems in the create/destroy lifecycle. `destroy` never removed `server_id` and `hostname` from the state hash. Test Kitchen writes state back in an `ensure` block even when an action fails, so when `wait_for_server` gives up and tears the machine down, the state file is still left naming a server that no longer exists. The next `kitchen destroy` then runs the destroy workflow a second time against a machine that is already gone. `destroy` now clears both keys once the workflow succeeds, which also makes it a no-op the second time it is called in a process. `wait_for_server` replaced the connection failure with whatever the cleanup raised. If the machine was unreachable *and* the destroy workflow then failed, the user saw the vRO error and no hint of why the machine was being destroyed in the first place. The cleanup failure is now logged -- including a warning that the server may have been left behind -- and the original connection failure is what propagates. `set_workflow_vars` cleared the memoized client but not the memoized output parameters, so anything reading `output_parameters` after switching from the create workflow to the destroy workflow would still be looking at the create workflow's output. `output_parameter_value` raised `NoMethodError` on nil for any parameter the workflow did not return, which made `output_parameter_empty?` blow up instead of answering the question it was asked. It now returns an empty string for a missing parameter. That in turn removes a dead branch: `output_parameter_empty?` tested `.nil?` on the result of a `.to_s`, which can never be nil, and computed the value twice to do it. Also requires `timeout` explicitly rather than relying on a dependency having pulled it in, and updates a `rubocop:disable` comment that still used the old `Style/AccessorMethodName` namespace -- cookstyle printed a deprecation warning about it on every run. Signed-off-by: Tim Smith --- lib/kitchen/driver/vro.rb | 62 ++++++++++++++++++++++------- spec/vro_spec.rb | 82 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 125 insertions(+), 19 deletions(-) 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