diff --git a/machine/providers/vultr.py b/machine/providers/vultr.py index c42cc99..df00d1f 100644 --- a/machine/providers/vultr.py +++ b/machine/providers/vultr.py @@ -14,6 +14,13 @@ "bom", "tlv", ] +# Overall budget for destroy_vm, and for confirming a single accepted delete. +# The overall budget stays well inside the callers' expectations of a command +# that finishes in a few minutes. +DESTROY_TIMEOUT = 240 +DESTROY_CONFIRM_TIMEOUT = 60 +DESTROY_POLL_INTERVAL = 5 + def _instance_to_vm(instance) -> VM: return VM( @@ -68,24 +75,51 @@ def get_vm(self, vm_id) -> VM: return _instance_to_vm(result) def destroy_vm(self, vm_id) -> bool: - # Vultr returns HTTP 500 if the instance is still pending or locked - # (e.g. during provisioning). Retry deletion with backoff. - for attempt in range(24): + # Vultr will not delete an instance that is still provisioning. It has + # signalled that in several ways over time (HTTP 500 "not currently + # active", HTTP 400 "currently locked", and — worst of all — by + # accepting the DELETE and then not acting on it), so retry on anything + # that is not a definitive answer, and confirm the instance really went + # away before reporting success. + deadline = time.monotonic() + DESTROY_TIMEOUT + last_error = None + while time.monotonic() < deadline: try: self._client.delete_instance(vm_id) - return True except VultrException as e: - error_msg = str(e) - if "500" in error_msg and ("not currently active" in error_msg or "currently locked" in error_msg): - info("Waiting for instance to become ready before destroying...") - time.sleep(5) - elif "404" in error_msg: + if e.status == 404: return True # already gone - else: - fatal_error(f"Error: machine with id {vm_id} not found: {e}") - fatal_error(f"Error: timed out waiting to destroy instance {vm_id}") + if e.status in (401, 403): + fatal_error(f"Error destroying machine with id {vm_id}: {e}") + last_error = e + info("Waiting for instance to become ready before destroying...") + time.sleep(DESTROY_POLL_INTERVAL) + continue + + if self._instance_is_gone(vm_id): + return True + info("Instance still present after delete was accepted, retrying...") + time.sleep(DESTROY_POLL_INTERVAL) + + if last_error: + fatal_error(f"Error: timed out destroying instance {vm_id}: {last_error}") + fatal_error(f"Error: timed out waiting for instance {vm_id} to be destroyed") return False + def _instance_is_gone(self, vm_id) -> bool: + """Poll the instance until the API reports it no longer exists.""" + deadline = time.monotonic() + DESTROY_CONFIRM_TIMEOUT + while True: + try: + self._client.get_instance(vm_id) + except VultrException as e: + if e.status == 404: + return True + raise + if time.monotonic() >= deadline: + return False + time.sleep(DESTROY_POLL_INTERVAL) + def list_vms(self, tag=None) -> list: try: params = {"tag": tag} if tag else None diff --git a/machine/subcommands/create.py b/machine/subcommands/create.py index 5d0e78b..61aad36 100644 --- a/machine/subcommands/create.py +++ b/machine/subcommands/create.py @@ -71,6 +71,12 @@ def command(context, name, tag, type, region, machine_size, image, wait_for_ip, # Verify SSH keys exist _verify_ssh_keys(provider, config.ssh_keys) + # Validate what will actually be used, not just what was passed on the + # command line, so that a bad value in the config file is caught too. + region = region if region is not None else config.region + image = image if image is not None else config.image + size = machine_size if machine_size is not None else config.machine_size + provider.validate_region(region) provider.validate_image(image) @@ -85,9 +91,9 @@ def command(context, name, tag, type, region, machine_size, image, wait_for_ip, vm = provider.create_vm( name=name, - region=region if region is not None else config.region, - image=image if image is not None else config.image, - size=machine_size if machine_size is not None else config.machine_size, + region=region, + image=image, + size=size, ssh_key_names=config.ssh_keys, tags=tags, user_data=user_data, diff --git a/tests/E2E.md b/tests/E2E.md index 631a3db..de21ad1 100644 --- a/tests/E2E.md +++ b/tests/E2E.md @@ -173,6 +173,8 @@ If credentials for a provider are not configured, that provider's test run will Each test cleans up after itself. All test instances use unique names prefixed with `e2etest-` so they are easy to identify. +A module-scoped `leak_check` fixture runs last and lists everything still tagged with the run's session id. Anything it finds is destroyed and then reported as a test failure — a leaked VM is a bug, not a warning, because a silent one accumulates until the provider's instance limit is reached (issue #102). + ## Cost Tests use the smallest available instance size and destroy VMs immediately after verification, so cost is minimal. diff --git a/tests/test_e2e.py b/tests/test_e2e.py index 18f9178..4941c89 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -233,6 +233,57 @@ def session_id(): return uuid.uuid4().hex[:8] +def _list_session_instances(config_file, session_id): + """Return every instance this test session created that still exists. + + ``list`` without ``--all`` is already filtered to the machines tagged with + this session id, so this sees exactly what the tests are responsible for. + """ + result = run_machine("list", "--output", "json", config_file=config_file, session_id=session_id) + if result.returncode != 0: + return None # cannot tell; do not claim a leak we did not observe + try: + return json.loads(result.stdout) + except json.JSONDecodeError: + return None + + +@pytest.fixture(scope="module", autouse=True) +def leak_check(config_file, session_id): + """Fail the run if any instance created by these tests outlives them. + + A destroy that quietly fails used to leave one VM running per CI run until + the provider's instance limit was hit (issue #102). The teardown below both + cleans up whatever survived and makes the leak visible. + """ + yield + + leftovers = _list_session_instances(config_file, session_id) + if not leftovers: + return + + names = [f"{i['name']} ({i['id']})" for i in leftovers] + for leftover in leftovers: + run_machine( + "--verbose", + "destroy", + "--no-confirm", + "--delete-dns", + str(leftover["id"]), + config_file=config_file, + session_id=session_id, + ) + still_there = _list_session_instances(config_file, session_id) + pytest.fail( + f"E2E tests leaked {len(leftovers)} instance(s): {', '.join(names)}. " + + ( + f"{len(still_there)} still running after cleanup — delete manually." + if still_there + else "They were cleaned up by the leak check." + ) + ) + + @pytest.fixture(scope="class") def instance(config_file, session_id): """Create a single instance with all features and destroy it after all tests. @@ -282,6 +333,8 @@ def instance(config_file, session_id): session_id=session_id, ) if destroy_result.returncode != 0: + # Do not fail here — the module-scoped leak_check fixture decides that, + # after it has had a chance to clean up. But make the reason visible. print(f"TEARDOWN WARNING: destroy exited {destroy_result.returncode}", flush=True) print(f" stdout: {destroy_result.stdout}", flush=True) print(f" stderr: {destroy_result.stderr}", flush=True) diff --git a/tests/test_vultr_destroy.py b/tests/test_vultr_destroy.py new file mode 100644 index 0000000..02142d5 --- /dev/null +++ b/tests/test_vultr_destroy.py @@ -0,0 +1,110 @@ +"""Tests for Vultr instance deletion. + +Vultr refuses to delete an instance that is still provisioning, and has been +observed to accept the DELETE without acting on it, which leaked one VM per e2e +run until the account instance limit was hit (issue #102). +""" + +import json + +import pytest +import requests + +from vultr import VultrException + +from machine.providers import vultr as vultr_module +from machine.providers.vultr import VultrProvider + + +def _exception(status, error="boom"): + response = requests.Response() + response.status_code = status + response.headers["content-type"] = "application/json" + response._content = json.dumps({"error": error}).encode() + return VultrException(response) + + +class FakeClient: + """Stands in for vultr.Vultr, scripting the API's replies.""" + + def __init__(self, delete_results, get_results): + self.delete_results = list(delete_results) + self.get_results = list(get_results) + self.delete_calls = 0 + self.get_calls = 0 + + def delete_instance(self, instance_id): + self.delete_calls += 1 + result = self.delete_results.pop(0) if self.delete_results else None + if isinstance(result, Exception): + raise result + return result + + def get_instance(self, instance_id): + self.get_calls += 1 + result = self.get_results.pop(0) if self.get_results else _exception(404, "not found") + if isinstance(result, Exception): + raise result + return result + + +@pytest.fixture +def provider(monkeypatch): + """A provider whose clock only advances when the code sleeps. + + destroy_vm budgets itself in minutes of wall clock, so the tests drive a + fake clock rather than waiting for it. + """ + now = [0.0] + monkeypatch.setattr(vultr_module, "Vultr", lambda api_key: None) + monkeypatch.setattr(vultr_module.time, "monotonic", lambda: now[0]) + monkeypatch.setattr(vultr_module.time, "sleep", lambda seconds: now.__setitem__(0, now[0] + seconds)) + return VultrProvider({"api-key": "test-key"}) + + +def test_destroy_succeeds_when_instance_disappears(provider): + """The happy path: the delete is accepted and the instance goes away.""" + provider._client = FakeClient(delete_results=[None], get_results=[_exception(404)]) + assert provider.destroy_vm("abc") is True + assert provider._client.delete_calls == 1 + + +def test_destroy_retries_when_delete_is_accepted_but_ignored(provider): + """The leak from issue #102: DELETE returns 204 but the instance survives.""" + provider._client = FakeClient( + delete_results=[None, None], + # First confirmation: still there for the whole confirm window, then gone. + get_results=[{"id": "abc"}] * 20 + [_exception(404)], + ) + assert provider.destroy_vm("abc") is True + assert provider._client.delete_calls == 2 + + +@pytest.mark.parametrize("status", [400, 500]) +def test_destroy_retries_while_instance_is_locked(provider, status): + """Vultr has signalled 'still provisioning' with more than one status code.""" + provider._client = FakeClient( + delete_results=[_exception(status, "instance is currently locked"), None], + get_results=[_exception(404)], + ) + assert provider.destroy_vm("abc") is True + assert provider._client.delete_calls == 2 + + +def test_destroy_treats_missing_instance_as_success(provider): + provider._client = FakeClient(delete_results=[_exception(404, "not found")], get_results=[]) + assert provider.destroy_vm("abc") is True + + +def test_destroy_gives_up_immediately_on_auth_failure(provider): + """Retrying a rejected API key would only stall for the full timeout.""" + provider._client = FakeClient(delete_results=[_exception(401, "invalid key")] * 10, get_results=[]) + with pytest.raises(SystemExit): + provider.destroy_vm("abc") + assert provider._client.delete_calls == 1 + + +def test_destroy_fails_when_instance_never_goes_away(provider): + provider._client = FakeClient(delete_results=[None] * 500, get_results=[{"id": "abc"}] * 500) + with pytest.raises(SystemExit): + provider.destroy_vm("abc")