diff --git a/CHANGELOG.md b/CHANGELOG.md index a35002d2b82..e55df3825b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,9 @@ follows [Semantic Versioning](https://semver.org) guidelines. ### Fixed +- Fixed reinstalling a server while its node couldn't be reached being taken as "there's no VM": the reinstall then + skipped removing the old VM and failed later on its taken VMID. It now fails straight away with the node's error, so + it can just be retried. - Fixed every error from Proxmox filling the log with three copies of the same stack trace -- about 256 lines per error, every few seconds while a page polled a server whose VM was gone. An error is now logged once, with one trace, and its message is Proxmox's own ("Proxmox returned 500: Configuration file ... does not exist"). diff --git a/app/Services/Servers/ServerBuildDispatchService.php b/app/Services/Servers/ServerBuildDispatchService.php index e430400383c..dbd89b5c585 100644 --- a/app/Services/Servers/ServerBuildDispatchService.php +++ b/app/Services/Servers/ServerBuildDispatchService.php @@ -49,12 +49,13 @@ public function rebuild(ServerDeploymentData $deployment): void // A retry after a failed install can find no VM at all -- the build // may have died before or during the clone. Stopping and deleting a VM // that does not exist fails on Proxmox's "does not exist", which would - // fail the whole chain, so go straight to building instead. If the VM - // does exist after all, the clone refuses its VMID and nothing is lost. + // fail the whole chain, so go straight to building instead. vmExists() + // throws on any other Proxmox error, so a node that can't be asked fails + // the reinstall up front rather than being taken for "no VM". $jobs = [ - ...($this->buildService->isVmDeleted($deployment->server) - ? [] - : $this->getChainedDeleteJobs($deployment->server)), + ...($this->buildService->vmExists($deployment->server) + ? $this->getChainedDeleteJobs($deployment->server) + : []), ...$this->getChainedBuildJobs($deployment), ]; diff --git a/app/Services/Servers/ServerBuildService.php b/app/Services/Servers/ServerBuildService.php index 134efb71e69..40cb8154065 100644 --- a/app/Services/Servers/ServerBuildService.php +++ b/app/Services/Servers/ServerBuildService.php @@ -48,24 +48,13 @@ public function isVmCreated(Server $server): bool return true; } - public function isVmDeleted(Server $server): bool - { - try { - $this->configRepository->setServer($server)->getConfig(); - } catch (ProxmoxConnectionException $e) { - return true; - } - - return false; - } - /** * Whether the server's VM exists on its node. * - * Unlike isVmDeleted(), only Proxmox's own "does not exist" answer counts as - * missing. Any other failure -- the node unreachable, a bad token, a 5xx -- - * is rethrown, so an outage can never be mistaken for a VM that's gone and - * lead to a server being dropped from Convoy while its VM keeps running. + * Only Proxmox's own "does not exist" answer counts as missing. Any other + * failure -- the node unreachable, a bad token, a 5xx -- is rethrown, so an + * outage can never be mistaken for a VM that's gone and lead to a server + * being dropped from Convoy while its VM keeps running. * * @throws ProxmoxConnectionException */ diff --git a/tests/Feature/Controllers/Client/Servers/RetryInstallTest.php b/tests/Feature/Controllers/Client/Servers/RetryInstallTest.php index 307b73ef6dc..4ee3482e705 100644 --- a/tests/Feature/Controllers/Client/Servers/RetryInstallTest.php +++ b/tests/Feature/Controllers/Client/Servers/RetryInstallTest.php @@ -117,3 +117,20 @@ function fakeMissingVm(): void Queue::assertNothingPushed(); }); + +it('fails a retry up front when the node cannot be asked', function () { + Http::fake(['*' => Http::response(['data' => null, 'message' => 'proxy error'], 503)]); + + [$user, $_, $node, $server] = createServerModel(); + $server->update(['status' => Status::INSTALL_FAILED->value]); + + $this->actingAs($user)->postJson("/api/client/servers/{$server->uuid}/settings/reinstall", [ + 'template_uuid' => createTemplateFor($node)->uuid, + 'account_password' => 'Rt-Aa1!bcdefgh9', + 'start_on_completion' => false, + ])->assertServerError(); + + // Not taken for "no VM": nothing is queued and the server is left as it was. + expect($server->fresh()->status)->toBe(Status::INSTALL_FAILED->value); + Queue::assertNothingPushed(); +});