Conversation
The error handling assumed an API that always answers, answers promptly, and never changes its mind. The live one does none of those, and every one of these failures ended a run with a Droplet already created. api_call rescued only DropletKit::Error. Faraday raises straight past it on a dropped connection, a timeout or a TLS failure, so allow_missing silently did not apply, lookup_droplet's rescue did not catch it, and a single reset connection anywhere in a ten minute poll killed create or destroy. It now rescues Faraday::Error and retries, with the important exception of the Droplet create: a dropped connection does not say whether the request arrived, and retrying one that did bills for a second Droplet nothing will ever destroy. That case raises and says to check the account first. A 429 became a hard failure. DigitalOcean allows 250 requests a minute and wait_for_public_ip polls up to 75 times per instance, so a dozen platforms in parallel meets the ceiling as a matter of course. 429 is now retried, waiting exactly as long as the RateLimit-Reset header says, up to a minute at a time. destroy's delete did not pass allow_missing, so a Droplet removed out of band between the poll and the delete raised, left :server_id in state, and made every later kitchen destroy fail the same way against a Droplet that was already gone. doctor tested Array(config[:ssh_key_ids]).empty? while create_server used normalize_list. They disagree on " " and ",", both of which doctor passed and both of which built a Droplet with no key installed. It now asks the same question create_server does. doctor also reported an unreachable API as a rejected token, sending people off to regenerate a credential that was fine. status called super, but Kitchen::Driver::Base#status only exists in test-kitchen 4.1 and up while the gemspec supports 3.0. It answers for itself now, and says what is actually unknown. vpcs is plural and every other list shaped setting takes a YAML list, so people write one; it went through as an array and earned an opaque 422. And normalize_list's nil for an unreadable value went into the body as "tags": null, which DigitalOcean accepts and ignores. Both now warn. Signed-off-by: Tim Smith <tim@mondoo.com>
Signed-off-by: Tim Smith <tim@mondoo.com>
|
CI caught something worth recording, because it changes what the code is for. The first push was green on Ruby 4.0 and red on 3.1, 3.2 and 3.4 — the same code, three different answers. The cause is dependency resolution, not Ruby: the gemspec allows That middleware is not a substitute for this PR: It skips POST, so a rate-limited create is never retried by it. And So the fix was in the specs, not the driver. The three that assert how long the driver waits now go through the create, which droplet_kit's middleware never touches, so they hold on either side of 3.20. The one that asserts a 429 does not fail the run sends four in a row, enough to outlast the middleware's three. Both behaviours are now pinned on every Locally that is against droplet_kit 3.19.0; CI covers 3.22.0. |
e713c9e to
d09069d
Compare
Why
The driver's error handling assumes an API that always answers, answers promptly, and never changes its mind between two calls. The live one does none of those. Every problem below ends a run with a Droplet already created and billing, which is the expensive kind of bug.
I found these while writing the integration suites in #141 — reading the driver against
droplet_kit's actual error surface rather than against the mocks, which encode the same assumptions the code does.What was wrong
api_callonly rescuedDropletKit::Error.droplet_kituses Faraday'snet_httpadapter, andfaraday-net_httpturnsErrno::ECONNRESET,Net::OpenTimeout,OpenSSL::SSL::SSLErrorand friends intoFaraday::ConnectionFailed/Faraday::TimeoutError/Faraday::SSLError. None of those is aDropletKit::Error, so they went straight past every rescue in the file:find_droplet'sallow_missing:contract silently did not applylookup_droplet'srescue Kitchen::ActionFaileddid not catch it, so#statuscould raise outwait_untilpolls for up to 600 seconds; one reset connection anywhere in that window killedcreateordestroyA 429 was a hard failure. DigitalOcean allows 250 requests a minute.
wait_for_public_ippolls every 8s for up to 600s — ~75GET /v2/droplets/:idper instance. Akitchen testacross a dozen platforms is over a thousand requests inside a ten-minute window. Meeting the ceiling is routine, not exotic, anddroplet_kithands back#reset_at,#limitand#remainingon the exception, all of which the driver threw away.destroy's delete omittedallow_missing: true. There is a window between the lastfind_dropletpoll and the delete in which someone else — a colleague, a cleanup script, the control panel — can remove the Droplet.DELETEthen returns 404, which raised, left:server_idin the state file, and made every laterkitchen destroyfail the same way against a Droplet that was already gone.doctorasked a different question fromcreate_server.doctorusedArray(config[:ssh_key_ids]).empty?;create_serverusednormalize_list. They disagree:ssh_key_idsrequired_configdoctor" "[]— no key","[]— no keyDigitalOcean then mails a root password, the transport has no way in, and
wait_until_readyhangs until it times out. That is precisely the failure the README's own troubleshooting section describes, sodoctoris the thing that should catch it.doctoralso reported an unreachable API asDigitalOcean rejected the configured access token: Failed to open TCP connection..., sending people off to regenerate a credential that was fine.#statuscalledsuper.Kitchen::Driver::Base#statusdoes not exist before test-kitchen 4.1 — I checked 3.0.0, 3.7.0, 3.9.1 and v4.0.0 — while the gemspec says>= 3.0. Latent, because nothing below 4.1 callsdriver.status, but the gem declared a floor its own advertised feature could not honour. Introduced by #138 after #137 set the floor.vpcsis plural and takes one UUID. DigitalOcean's schema isvpc_uuid: {type: string}, singular. But the key is plural, andssh_key_ids,tagsandfirewallsall accept YAML lists, so people write the obvious thing and get"vpc_uuid":["..."]on the wire → 422.normalize_listreturnednilinto the request body.tags: {env: prod}in akitchen.ymlserialized as"tags": null, which DigitalOcean accepts and silently ignores.attach_firewallswarned about this case;create_serverdid not.What changed
api_callrescuesFaraday::Errorand retries with exponential backoff, and rescuesDropletKit::RateLimitReachedseparately, sleeping untilRateLimit-Resetsays the window reopens (capped at a minute so a run does not sit silent).idempotent:keyword. A Droplet create is never retried on a dropped connection — the request may well have arrived, and a second attempt bills for a Droplet Test Kitchen has no record of and will never destroy. It raises and tells you to check the account first. A 429 is retried on a create, because a rate-limited request definitively did not happen.destroy's delete passesallow_missing: true.doctorusesnormalize_list, and reports an unreachable API as unreachable.#statusanswers for itself instead of delegating, and says what is actually unknown ("DigitalOcean does not know a Droplet <42>") rather than the base class's"does not support status checks", which is no longer true.vpcsaccepts a list, uses the first, and warns when there is more than one.null.24 new examples. The retry paths had no coverage at all before — no
429,RateLimit,Faraday,ConnectionFailedorTimeoutappeared anywhere inspec/.Not in this PR
PLATFORM_SLUG_MAPis badly stale — 15 of its 25 entries name images DigitalOcean has retired, includingubuntu-20and every Debian below 13. That is a separate change and I will send it separately.Verification
(175 before this PR.)