From 94184befc8674734b015cc5ed463f46c5868e21f Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Sat, 29 Aug 2026 19:09:48 -0700 Subject: [PATCH] fix: stop leaking servers and stranding instances on partial failures Six related failures, all found by walking the error paths rather than the happy ones. create walked away from a running server. Everything after create_server -- attaching a floating IP, resolving an address, waiting on the transport -- can fail on its own, and only the transport wait cleaned up after itself. An exhausted pool or an instance with no address of the requested family left a booted, billing server behind, and when allocate_floating_ip was set it also left a freshly allocated floating IP that nothing would ever release, because destroy can only give one back while it can still read it off the server. All of that now goes through one path that tears the server down before re-raising, and that path keeps the original failure even when the teardown itself fails, while saying a server may have been left behind. destroy could be stranded by a single odd address. IPAddr raises on anything it cannot parse, and that is an ArgumentError -- neither a Fog nor an Excon error, so nothing translated it. One unparsable entry in a server's address list aborted the destroy before it reached server.destroy. Unparsable addresses are now logged and skipped. destroy also handed users raw Excon backtraces where create has always raised ActionFailed. It now translates them the same way. clouds.yaml verify: false overrode an explicit disable_ssl_validation: false. The guard was a falsy check, so writing false in kitchen.yml -- a deliberate instruction to keep verifying -- read the same as leaving it unset. kitchen.yml now wins here as it does for every other key. Two API responses were read without checking their shape. A floating IP allocation whose body carried no address passed nil to associate_address and failed several steps later saying nothing useful; a volume create whose body carried no id raised NoMethodError. Both now fail where the problem is. Cinder reporting no status at all in the moments after a create is treated as not-ready rather than raising out of the middle of the wait. Finally, an image_ref of "/[/" reached the user as a raw RegexpError from inside create, and a lone "/" compiled to the empty pattern and quietly matched the first image in the account. Both are reported as configuration to fix. Signed-off-by: Tim Smith --- README.md | 6 +- lib/kitchen/driver/openstack.rb | 55 ++++++-- lib/kitchen/driver/openstack/clouds.rb | 6 +- lib/kitchen/driver/openstack/networking.rb | 40 ++++-- lib/kitchen/driver/openstack/server_helper.rb | 25 +++- lib/kitchen/driver/openstack/volume.rb | 11 +- spec/kitchen/driver/openstack/clouds_spec.rb | 26 ++++ .../driver/openstack/networking_spec.rb | 66 ++++++++++ .../driver/openstack/server_helper_spec.rb | 19 +++ spec/kitchen/driver/openstack/volume_spec.rb | 40 ++++++ spec/kitchen/driver/openstack_spec.rb | 119 ++++++++++++++++++ 11 files changed, 392 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index ed2fd36d..002fe902 100644 --- a/README.md +++ b/README.md @@ -512,7 +512,9 @@ clouds: Either is passed through to the HTTP connection. `verify: false` in `clouds.yaml` disables verification entirely, equivalent to setting -`disable_ssl_validation: true`. +`disable_ssl_validation: true` — unless you set `disable_ssl_validation` +yourself, in which case `kitchen.yml` wins as it does everywhere else, and +`disable_ssl_validation: false` keeps verification on. ## Troubleshooting @@ -528,6 +530,7 @@ kitchen create --log-level=debug | Symptom | Likely cause | | --- | --- | | `Image not found` / `Flavor not found` | The `_ref` matched nothing. Check `openstack image list`. A `/regex/` needs the surrounding slashes. | +| `Could not parse <...> as a regular expression` | A `/regex/` ref that does not compile — usually an unbalanced bracket. | | `Cannot specify both image_ref and image_id` | Set one, not both. Same for flavor and network. | | `Could not find an IP` | The instance has no address of the family you asked for. Check `use_ipv6`, and whether you need a floating IP. | | `Server is not attached to network ` | `openstack_network_name` does not match any network on the instance. | @@ -537,6 +540,7 @@ kitchen create --log-level=debug | `The security_groups config must be an array` | Use a list, even for a single group. | | Hangs at "Waiting for server to be ready" | The instance booted but SSH is unreachable. Check the security group allows port 22, that a floating IP is attached if you need one, and that `transport: username:` matches the image's default user. | | Times out reaching `ACTIVE` | Raise `glance_cache_wait_timeout`. First boot of a large image can be slow. | +| `Cinder accepted the volume but returned no id` | The volume create succeeded but the response was unreadable; check the Cinder endpoint and API microversion. | | TLS errors | Set `ssl_ca_file` (or `OS_CACERT`) to your CA bundle. | The instance is destroyed automatically if it never becomes reachable, so a diff --git a/lib/kitchen/driver/openstack.rb b/lib/kitchen/driver/openstack.rb index 8cf48695..14814f2f 100755 --- a/lib/kitchen/driver/openstack.rb +++ b/lib/kitchen/driver/openstack.rb @@ -156,14 +156,7 @@ def create(state) end info "OpenStack server ID <#{state[:server_id]}> created" - if config[:floating_ip] - attach_ip(server, config[:floating_ip]) - elsif config[:floating_ip_pool] - attach_ip_from_pool(server, config[:floating_ip_pool]) - end - state[:hostname] = get_ip(server) - wait_for_server(state) - add_ohai_hint(state) + finish_create(server, state) rescue Fog::Errors::Error, Excon::Errors::Error => e raise ActionFailed, e.message end @@ -195,6 +188,8 @@ def destroy(state) info "OpenStack instance <#{state[:server_id]}> destroyed." state.delete(:server_id) state.delete(:hostname) + rescue Fog::Errors::Error, Excon::Errors::Error => e + raise ActionFailed, e.message end # Reports what Nova currently thinks of the server. @@ -233,6 +228,50 @@ def doctor(state) # rubocop:disable Lint/UnusedMethodArgument private + # Everything +create+ does once the server is up and billing. + # + # Each step here can fail on its own -- a pool with no free addresses, a + # server with no address of the requested family, a transport that never + # comes up -- and every one of those leaves a running server behind that + # nothing else is going to clean up. Worse, when + # +allocate_floating_ip+ is set the run may also be holding a brand new + # floating IP, and +destroy+ can only give that back while it can still + # read it off the server. So a failure here tears the server down before + # re-raising, which is what +wait_for_server+ already did for its own + # step alone. + # + # @param server [Fog::OpenStack::Compute::Server] the server just built + # @param state [Hash] mutable instance state; gains `:hostname` + # @return [void] + # @raise [StandardError] whatever the failing step raised, after cleanup + def finish_create(server, state) + if config[:floating_ip] + attach_ip(server, config[:floating_ip]) + elsif config[:floating_ip_pool] + attach_ip_from_pool(server, config[:floating_ip_pool]) + end + state[:hostname] = get_ip(server) + wait_for_server(state) + add_ohai_hint(state) + rescue StandardError + # wait_for_server destroys the server itself and destroy deletes + # :server_id, so by the time that step fails there is nothing left to + # clean up and no second message worth printing. + raise if state[:server_id].nil? + + error "Destroying OpenStack server ID <#{state[:server_id]}> after a failed create." + begin + destroy(state) + rescue StandardError => cleanup_error + # The user needs to hear why the create failed, not why the tidying + # up afterwards also failed -- but they do need to know a server was + # left behind for them to deal with. + error "Could not destroy OpenStack server ID <#{state[:server_id]}>: " \ + "#{cleanup_error.message}. It may still be running." + end + raise + end + # Looks a server up without turning an unreachable cloud into a failure. # # @param server_id [String] the Nova server ID diff --git a/lib/kitchen/driver/openstack/clouds.rb b/lib/kitchen/driver/openstack/clouds.rb index de614e9a..4590030f 100644 --- a/lib/kitchen/driver/openstack/clouds.rb +++ b/lib/kitchen/driver/openstack/clouds.rb @@ -99,7 +99,11 @@ def apply_clouds_config # (though its env loader does sweep up any OS_* name into an implicit # cloud), and ENV_VAR_MAP deliberately mirrors the documented set -- # so for this driver clouds.yaml is the only source. - return unless cc[:ssl_verify_peer] == false && !config[:disable_ssl_validation] + # + # Nil, not falsy: `disable_ssl_validation: false` in kitchen.yml is a + # deliberate instruction to keep verifying, and kitchen.yml outranks + # clouds.yaml here exactly as it does for every key above. + return unless cc[:ssl_verify_peer] == false && config[:disable_ssl_validation].nil? config[:disable_ssl_validation] = true end diff --git a/lib/kitchen/driver/openstack/networking.rb b/lib/kitchen/driver/openstack/networking.rb index c7827ac0..bb7ec7ea 100644 --- a/lib/kitchen/driver/openstack/networking.rb +++ b/lib/kitchen/driver/openstack/networking.rb @@ -75,7 +75,15 @@ def allocate_ip_from_pool(pool) end resp = net.create_floating_ip(networks[0]["id"]) - ip = resp.body["floatingip"]["floating_ip_address"] + ip = resp.body.dig("floatingip", "floating_ip_address") + # Without this the nil travels on into associate_address, and the + # run fails several steps later with an error that says nothing + # about the allocation that actually went wrong. + if ip.nil? + raise ActionFailed, + "Allocated a floating IP from <#{pool}> but the response carried no address" + end + info "Created floating IP <#{ip}> from <#{pool}> pool" ip end @@ -195,11 +203,8 @@ def ip_from_named_network(server) # @param addresses [Array] address hashes, each with an `"addr"` key # @return [Array] the matching subset def filter_ips(addresses) - if config[:use_ipv6] - addresses.select { |i| IPAddr.new(i["addr"]).ipv6? } - else - addresses.select { |i| IPAddr.new(i["addr"]).ipv4? } - end + wanted = config[:use_ipv6] ? :ipv6? : :ipv4? + addresses.select { |i| ip_family_match?(i["addr"], wanted) } end # Normalizes and filters public/private address lists to the configured @@ -215,9 +220,30 @@ def parse_ips(pub, priv) # the caller's list here is the Fog server model's own address data. wanted = config[:use_ipv6] ? :ipv6? : :ipv4? [Array(pub), Array(priv)].map do |addrs| - addrs.select { |i| IPAddr.new(i).public_send(wanted) } + addrs.select { |i| ip_family_match?(i, wanted) } end end + + # Whether one address from Nova is of the IP family we want. + # + # IPAddr raises on anything it cannot parse, and that exception is an + # ArgumentError -- neither a Fog nor an Excon error, so neither + # +create+ nor +destroy+ would turn it into something a user can act + # on. A single odd entry in a server's address list used to abort a + # destroy before it reached +server.destroy+, stranding the instance, + # so an unparsable address is now logged and skipped instead. + # + # @param addr [String] an address as Nova reported it + # @param wanted [Symbol] `:ipv4?` or `:ipv6?` + # @return [Boolean] true when the address parses and matches + def ip_family_match?(addr, wanted) + IPAddr.new(addr.to_s).public_send(wanted) + rescue ArgumentError + # IPAddr::InvalidAddressError descends from IPAddr::Error, which + # descends from ArgumentError; catching the base covers both. + warn "Ignoring unparsable address <#{addr}> reported by OpenStack" + false + end end end end diff --git a/lib/kitchen/driver/openstack/server_helper.rb b/lib/kitchen/driver/openstack/server_helper.rb index 3b8f0586..2b9a2716 100644 --- a/lib/kitchen/driver/openstack/server_helper.rb +++ b/lib/kitchen/driver/openstack/server_helper.rb @@ -187,10 +187,15 @@ def find_network(network_ref) # @param collection [Enumerable] the Fog collection to search # @param name [String] id, name, or `/regex/` # @return [Object, nil] the first match, or nil + # @raise [Kitchen::ActionFailed] if the ref looks like a regex but is + # not a valid one def find_matching(collection, name) name = name.to_s - if name.start_with?("/") && name.end_with?("/") - regex = Regexp.new(name[1...-1]) + # A one-character "/" starts and ends with a slash, but the pattern + # between the slashes is empty and an empty Regexp matches the first + # resource in the collection -- which is never what anyone meant. + if name.length > 1 && name.start_with?("/") && name.end_with?("/") + regex = compile_ref_regex(name) # check for regex name match, skipping unnamed resources; Neutron # networks in particular are allowed to have no name collection.each { |single| return single if single.name && regex.match?(single.name) } @@ -203,6 +208,22 @@ def find_matching(collection, name) end nil end + + # Compiles the pattern out of a `/regex/` style ref. + # + # An unbalanced bracket or a stray quantifier is a typo in kitchen.yml, + # not a bug in the driver, so it is reported as configuration the user + # can go and fix rather than as a raw RegexpError backtrace from the + # middle of a create. + # + # @param name [String] the ref, slashes included + # @return [Regexp] the compiled pattern + # @raise [Kitchen::ActionFailed] if the pattern does not compile + def compile_ref_regex(name) + Regexp.new(name[1...-1]) + rescue RegexpError => e + raise ActionFailed, "Could not parse <#{name}> as a regular expression: #{e.message}" + end end end end diff --git a/lib/kitchen/driver/openstack/volume.rb b/lib/kitchen/driver/openstack/volume.rb index 0e4d423c..f5128bf2 100644 --- a/lib/kitchen/driver/openstack/volume.rb +++ b/lib/kitchen/driver/openstack/volume.rb @@ -96,7 +96,11 @@ def create_volume(config, os) bdm[:volume_size], opt ) - vol_id = resp[:body]["volume"]["id"] + vol_id = resp[:body].dig("volume", "id") if resp[:body].respond_to?(:dig) + # A create that answers 200 with a body we cannot read an id out of + # is not something to carry on from: without the id nothing can wait + # on the volume, attach it, or delete it again. + raise(ActionFailed, "Cinder accepted the volume but returned no id") if vol_id.nil? wait_for_volume(volume_service, vol_id, creation_timeout, attach_timeout) @@ -145,7 +149,10 @@ def wait_for_volume(volume_service, vol_id, creation_timeout, attach_timeout) @logger.debug "Waiting for volume to be ready for #{creation_timeout} seconds" vol_model.wait_for(creation_timeout) do sleep(1) - raise(ActionFailed, "Failed to make volume #{vol_id}") if status.casecmp("error") == 0 + # Cinder can answer with no status at all for the first moments + # after a create; that is "not ready yet", not a reason to raise + # NoMethodError out of the middle of the wait. + raise(ActionFailed, "Failed to make volume #{vol_id}") if status.to_s.casecmp("error") == 0 ready? end diff --git a/spec/kitchen/driver/openstack/clouds_spec.rb b/spec/kitchen/driver/openstack/clouds_spec.rb index 9bf0a249..986829ba 100644 --- a/spec/kitchen/driver/openstack/clouds_spec.rb +++ b/spec/kitchen/driver/openstack/clouds_spec.rb @@ -609,6 +609,32 @@ def use_clouds_file(content = clouds_yaml, secure: nil, env: {}) expect(driver[:disable_ssl_validation]).to be_nil end + # kitchen.yml outranks clouds.yaml for every other key, and writing + # `disable_ssl_validation: false` is a deliberate instruction to keep + # verifying. The guard used to be a plain falsy check, so the explicit + # false read the same as "unset" and clouds.yaml overrode it. + it "does not override an explicit disable_ssl_validation: false" do + cloud = clouds_yaml + cloud["clouds"]["mycloud"]["verify"] = false + use_clouds_file(cloud, env: { "OS_CLOUD" => "mycloud" }) + config[:disable_ssl_validation] = false + + driver.send(:apply_clouds_config) + + expect(driver[:disable_ssl_validation]).to be(false) + end + + it "leaves an explicit disable_ssl_validation: true alone" do + cloud = clouds_yaml + cloud["clouds"]["mycloud"]["verify"] = false + use_clouds_file(cloud, env: { "OS_CLOUD" => "mycloud" }) + config[:disable_ssl_validation] = true + + driver.send(:apply_clouds_config) + + expect(driver[:disable_ssl_validation]).to be(true) + end + it "carries a cacert from clouds.yaml into ssl_ca_file" do cloud = clouds_yaml cloud["clouds"]["mycloud"]["cacert"] = "/path/ca.crt" diff --git a/spec/kitchen/driver/openstack/networking_spec.rb b/spec/kitchen/driver/openstack/networking_spec.rb index 30fb3735..f01ca2ab 100644 --- a/spec/kitchen/driver/openstack/networking_spec.rb +++ b/spec/kitchen/driver/openstack/networking_spec.rb @@ -128,6 +128,43 @@ .to raise_error(Kitchen::ActionFailed, "Floating IP pool not found") end end + + # The list_networks response is guarded above; the create response was + # not, so a body without an address handed a nil straight on to + # associate_address and failed later with an error about the wrong thing. + context "when the allocation response carries no address" do + let(:net) do + fog_network( + list_networks: fog_response(networks_body), + create_floating_ip: fog_response("floatingip" => {}) + ) + end + + it "fails naming the pool" do + expect { driver.send(:attach_ip_from_pool, server, "swimmers") } + .to raise_error(Kitchen::ActionFailed, /from but the response carried no address/) + end + + it "does not try to attach a nil address" do + expect { driver.send(:attach_ip_from_pool, server, "swimmers") } + .to raise_error(Kitchen::ActionFailed) + expect(server).not_to have_received(:associate_address) + end + end + + context "when the allocation response has no floatingip key at all" do + let(:net) do + fog_network( + list_networks: fog_response(networks_body), + create_floating_ip: fog_response({}) + ) + end + + it "fails naming the pool" do + expect { driver.send(:attach_ip_from_pool, server, "swimmers") } + .to raise_error(Kitchen::ActionFailed, /from but the response carried no address/) + end + end end it "serializes pool access across threads" do @@ -415,6 +452,28 @@ expect(driver.send(:filter_ips, [{ "addr" => "1.2.3.4" }])).to eq([]) end + + # IPAddr raises ArgumentError on anything it cannot parse, which no caller + # up the stack translates -- one odd entry used to take out the whole run. + context "with an address Nova reported that will not parse" do + let(:addresses) { [{ "addr" => "1.2.3.4" }, { "addr" => "not-an-ip" }] } + + it "skips it and keeps the rest" do + expect(driver.send(:filter_ips, addresses)).to eq([{ "addr" => "1.2.3.4" }]) + end + + it "says which address it skipped" do + driver.send(:filter_ips, addresses) + + expect(logged_output.string).to include("Ignoring unparsable address ") + end + end + + context "with a nil address" do + it "skips it" do + expect(driver.send(:filter_ips, [{ "addr" => nil }])).to eq([]) + end + end end describe "#parse_ips" do @@ -478,5 +537,12 @@ expect(result_pub).to eq(%w{1.2.3.4}) expect(result_pub).not_to be(pub) end + + context "with an address that will not parse" do + it "skips it rather than raising out of the middle of a create" do + expect(driver.send(:parse_ips, ["1.2.3.4", "not-an-ip"], ["10.0.0.1 "])) + .to eq([%w{1.2.3.4}, []]) + end + end end end diff --git a/spec/kitchen/driver/openstack/server_helper_spec.rb b/spec/kitchen/driver/openstack/server_helper_spec.rb index a0d6e553..86c9153f 100644 --- a/spec/kitchen/driver/openstack/server_helper_spec.rb +++ b/spec/kitchen/driver/openstack/server_helper_spec.rb @@ -394,6 +394,25 @@ expect(driver.send(:find_matching, unnamed, "/.*/").id).to eq("222") end + + # A lone slash starts and ends with a slash, so it used to be compiled as + # the empty pattern -- which matches everything, and quietly booted + # whatever image happened to come back first. + it "does not treat a lone slash as a match-everything regex" do + expect(driver.send(:find_matching, collection, "/")).to be_nil + end + + # An unbalanced bracket is a typo in kitchen.yml, and used to surface as a + # raw RegexpError from the middle of a create. + it "reports an unparsable regex as configuration to fix" do + expect { driver.send(:find_matching, collection, "/[/") } + .to raise_error(Kitchen::ActionFailed, %r{Could not parse as a regular expression}) + end + + it "does not swallow the reason the pattern would not compile" do + expect { driver.send(:find_matching, collection, "/*/") } + .to raise_error(Kitchen::ActionFailed, /as a regular expression: /) + end end describe "#find_image" do diff --git a/spec/kitchen/driver/openstack/volume_spec.rb b/spec/kitchen/driver/openstack/volume_spec.rb index 3046ddd4..8a0901e5 100644 --- a/spec/kitchen/driver/openstack/volume_spec.rb +++ b/spec/kitchen/driver/openstack/volume_spec.rb @@ -113,6 +113,46 @@ def cinder(volumes: [volume_model], create_response: { body: { "volume" => { "id expect(vol_driver).to have_received(:volume).once end + # Without the id there is nothing to wait on, attach, or delete again, so + # this has to stop here rather than NoMethodError one line later. + context "when the create response carries no volume id" do + let(:cinder_service) { cinder(create_response: { body: { "volume" => {} } }) } + + it "fails with a message that names Cinder" do + expect { vol_driver.create_volume(config, os) } + .to raise_error(Kitchen::ActionFailed, "Cinder accepted the volume but returned no id") + end + end + + context "when the create response has no volume key at all" do + let(:cinder_service) { cinder(create_response: { body: {} }) } + + it "fails with a message that names Cinder" do + expect { vol_driver.create_volume(config, os) } + .to raise_error(Kitchen::ActionFailed, "Cinder accepted the volume but returned no id") + end + end + + context "when the create response body is not a hash" do + let(:cinder_service) { cinder(create_response: { body: "" }) } + + it "fails with a message that names Cinder" do + expect { vol_driver.create_volume(config, os) } + .to raise_error(Kitchen::ActionFailed, "Cinder accepted the volume but returned no id") + end + end + + # Cinder can answer with no status at all in the moments right after a + # create. That is "not ready yet", not a reason to raise NoMethodError out + # of the middle of the wait. + context "when Cinder reports no status yet" do + let(:cinder_service) { cinder(volumes: [volume_model(status: nil, ready: true)]) } + + it "treats it as not-an-error and carries on" do + expect(vol_driver.create_volume(config, os)).to eq("555") + end + end + # Regression: `Array#first` silently ignores a block, so the original # implementation waited on whichever volume happened to be first in the # account rather than the one it had just created. The id from the create diff --git a/spec/kitchen/driver/openstack_spec.rb b/spec/kitchen/driver/openstack_spec.rb index 5ac30e9c..0b3b0ec3 100755 --- a/spec/kitchen/driver/openstack_spec.rb +++ b/spec/kitchen/driver/openstack_spec.rb @@ -239,6 +239,93 @@ expect { driver.create(state) }.to raise_error(Kitchen::InstanceFailure, "nope") end end + + # Everything in here happens after create_server has returned, so a + # failure leaves a booted, billing instance behind. Regression coverage + # for a create that used to walk away from one. + describe "cleanup once the server is up" do + before { allow(driver).to receive(:destroy) } + + it "destroys the server when the floating IP cannot be attached" do + config[:floating_ip] = "1.2.3.4" + allow(driver).to receive(:attach_ip).and_raise(Kitchen::ActionFailed, "nope") + + expect { driver.create(state) }.to raise_error(Kitchen::ActionFailed, "nope") + expect(driver).to have_received(:destroy).with(state) + end + + it "destroys the server when the pool holds no free address" do + config[:floating_ip_pool] = "swimmers" + allow(driver).to receive(:attach_ip_from_pool) + .and_raise(Kitchen::ActionFailed, "No available IPs in pool ") + + expect { driver.create(state) }.to raise_error(Kitchen::ActionFailed, /No available IPs/) + expect(driver).to have_received(:destroy).with(state) + end + + it "destroys the server when no usable address can be found" do + allow(driver).to receive(:get_ip).and_raise(Kitchen::ActionFailed, "Could not find an IP") + + expect { driver.create(state) }.to raise_error(Kitchen::ActionFailed, "Could not find an IP") + expect(driver).to have_received(:destroy).with(state) + end + + it "says why it is tearing the server down" do + allow(driver).to receive(:get_ip).and_raise(Kitchen::ActionFailed, "Could not find an IP") + + expect { driver.create(state) }.to raise_error(Kitchen::ActionFailed) + expect(logged_output.string) + .to include("Destroying OpenStack server ID after a failed create.") + end + + it "re-raises rather than swallowing the failure" do + allow(driver).to receive(:get_ip).and_raise(Kitchen::ActionFailed, "Could not find an IP") + + expect { driver.create(state) }.to raise_error(Kitchen::ActionFailed) + end + + it "leaves a successful create alone" do + driver.create(state) + + expect(driver).not_to have_received(:destroy) + end + + # wait_for_server tears the server down itself and destroy clears + # :server_id, so there is nothing left for this to clean up a second + # time and no second message worth printing. + it "does not repeat the teardown wait_for_server already did" do + allow(driver).to receive(:wait_for_server) do |s| + s.delete(:server_id) + raise Kitchen::ActionFailed, "not reachable" + end + + expect { driver.create(state) }.to raise_error(Kitchen::ActionFailed, "not reachable") + expect(driver).not_to have_received(:destroy) + expect(logged_output.string).not_to include("after a failed create") + end + + context "when the teardown itself fails" do + before do + allow(driver).to receive(:get_ip).and_raise(Kitchen::ActionFailed, "Could not find an IP") + allow(driver).to receive(:destroy).and_raise(Kitchen::ActionFailed, "keystone is down") + end + + # The user needs to hear why the create failed, not why the tidying up + # afterwards also failed. + it "still reports the original failure" do + expect { driver.create(state) } + .to raise_error(Kitchen::ActionFailed, "Could not find an IP") + end + + it "warns that a server may have been left behind" do + expect { driver.create(state) }.to raise_error(Kitchen::ActionFailed) + + expect(logged_output.string) + .to include("Could not destroy OpenStack server ID : keystone is down. " \ + "It may still be running.") + end + end + end end describe "#finalize_config!" do @@ -585,6 +672,38 @@ def doctor_messages expect(net).not_to have_received(:list_floating_ips) end end + + context "when the server reports an address that will not parse" do + let(:server) { fog_server(public_ip_addresses: ["not-an-ip"], private_ip_addresses: []) } + + # IPAddr raises ArgumentError, which is neither a Fog nor an Excon + # error, so this used to abort the destroy before it ever reached + # server.destroy and strand the instance. + it "still destroys the server" do + driver.destroy(state) + + expect(server).to have_received(:destroy) + end + + it "says which address it ignored" do + driver.destroy(state) + + expect(logged_output.string).to include("Ignoring unparsable address ") + end + end + end + + context "when the cloud cannot be reached" do + before do + allow(driver).to receive(:compute) + .and_raise(Excon::Errors::SocketError.new(StandardError.new("no route to host"))) + end + + # create has always translated these; destroy used to hand the user a + # raw Excon backtrace instead. + it "reports an ActionFailed rather than a raw Excon error" do + expect { driver.destroy(state) }.to raise_error(Kitchen::ActionFailed, /no route to host/) + end end end