feat: start a stopped VM through the owner's /machine/{ref}/start route - #294
Conversation
With aleph-vm 2.1 giving the owner an authenticated route to start a VM
they stopped, VmClient.start_instance now calls
/control/machine/{ref}/start through the authenticated operation path and
falls back to the legacy /control/allocation/notify push on 404 for CRNs
running pre-2.1 aleph-vm. notify_allocation keeps the legacy call for
initial allocation.
|
Failed to retrieve llama text: POST 502: Bad Gateway |
foxpatch-aleph
left a comment
There was a problem hiding this comment.
Correct and well-scoped change. start_instance now prefers the authenticated /control/machine/{ref}/start route and falls back to the legacy notify_allocation push on 404, which keeps compatibility with pre-2.1 CRNs. I verified the 200/404/403 paths empirically against the real VmClient and they behave as described; the 404 check also correctly avoids falling back on network errors since perform_operation returns None in that case. The new tests match the existing style and assert both request counts and URLs, and the enum test was extended. Two minor nits: the return type annotation should be Tuple[Optional[int], str] to match perform_operation (status is None on aiohttp.ClientError, like every sibling method), and the docstring has a small grammar typo.
foxpatch-aleph
left a comment
There was a problem hiding this comment.
The fallback logic is correct and well-tested: perform_operation returns (status, text) instead of raising on non-2xx, so the 404 check works as intended, the URL built by create_vm_control_payload matches the mocked routes, and the three new tests correctly cover the default route, the 404 fallback (both requests), and non-404 passthrough. The main nit is the return type annotation: start_instance declares Tuple[int, str] but now delegates to perform_operation, which returns (None, str(e)) on aiohttp.ClientError, so it should be Tuple[Optional[int], str] to match stop_instance/reboot_instance. Other minor observations: a log line when falling back would help diagnose 404s from proxies or mixed-version CRNs, and a test asserting no fallback on a client error (status=None) would round out coverage.
src/aleph/sdk/client/vm_client.py (line 221): The annotation -> Tuple[int, str] is now inaccurate: perform_operation returns Tuple[Optional[int], str] and yields (None, str(e)) on aiohttp.ClientError (see lines 156-158). Previously this method delegated to notify_allocation, which does guarantee an int. It should be Tuple[Optional[int], str] for consistency with stop_instance, reboot_instance, etc. Runtime behavior is fine either way (None == 404 is False, so the fallback won't misfire), but callers typed against int would break on network errors, and mypy would flag this.
src/aleph/sdk/client/vm_client.py (line 230): Consider logging here (e.g. logger.debug("start route returned 404, falling back to notify_allocation")). A 404 can also come from a reverse proxy or a 2.1+ node for other reasons, in which case the legacy unauthenticated notify push fires silently — a log line would help operators diagnose mixed-version fleets.
tests/unit/test_vm_client.py (line 98): Nice coverage of the 200/404/403 cases. One gap: no test asserts the behavior when perform_operation returns (None, ...) on a aiohttp.ClientError — i.e., that start_instance returns (None, error) without attempting the fallback. Worth adding for completeness since that's the remaining branch of the new logic.
foxpatch-aleph
left a comment
There was a problem hiding this comment.
This PR cleanly repoints start_instance at the owner-authenticated /control/machine/{ref}/start route with a per-call 404 fallback to the legacy notify_allocation push. The logic is correct: perform_operation never raises on HTTP error statuses, so a 404 reliably reaches the fallback branch, while ClientError returns (None, error) and correctly skips it. Return-type annotations are consistent with sibling operations, and the enum path construction was verified to produce the expected /control/machine/{ref}/start path. The three new tests precisely cover the new-route, fallback, and no-fallback-on-other-errors cases. Only minor notes: the 404 fallback cannot distinguish a missing route from a genuine machine-not-found 404 on 2.1+ CRNs (which silently degrades to pre-PR behavior, so not a regression), a small docstring grammar nit, and the 403 test could additionally assert the response body is surfaced.
src/aleph/sdk/client/vm_client.py (line 230): Note that this 404-based fallback can't distinguish "route doesn't exist (pre-2.1 CRN)" from a genuine "machine not found" 404 on a 2.1+ CRN — in the latter case the caller silently gets legacy notify_allocation behavior. Not a regression (that was the pre-PR behavior), but worth a sentence in the docstring if that distinction matters.
src/aleph/sdk/client/vm_client.py (line 222): Minor grammar nit: "Start a VM its owner stopped." reads awkwardly — "Start a VM stopped by its owner." would be cleaner.
tests/unit/test_vm_client.py (line 114): Consider also asserting the error body is surfaced, e.g. assert response_text == "Unauthorized sender", to confirm the non-404 error response is passed through rather than swallowed.
Summary
With aleph-vm 2.1 giving the owner an authenticated route to start a VM they stopped (aleph-vm #1245),
VmClient.start_instancenow defaults to the authenticatedPOST /control/machine/{ref}/startand falls back to the legacy/control/allocation/notifypush on 404 for CRNs running pre-2.1 aleph-vm.VmOperation.STARTstart_instancecallsperform_operation(vm_id, START); on 404 it falls back tonotify_allocation(per-call, no caching)notify_allocationis unchanged and remains the call for initial allocation (instance creation), which is not a restartTests
test_start_instance_uses_machine_start_route— new route by defaulttest_start_instance_falls_back_to_notify_on_404— fallback issues both requeststest_start_instance_does_not_fall_back_on_other_errors— 403 surfaces directly