diff --git a/app/Provisioning/Steps/Customer/CloneVirtualMachine.php b/app/Provisioning/Steps/Customer/CloneVirtualMachine.php index ec7d03d..feb05fb 100644 --- a/app/Provisioning/Steps/Customer/CloneVirtualMachine.php +++ b/app/Provisioning/Steps/Customer/CloneVirtualMachine.php @@ -3,6 +3,7 @@ namespace App\Provisioning\Steps\Customer; use App\Models\ProvisioningRun; +use App\Models\RunResource; use App\Provisioning\StepResult; use App\Services\Proxmox\ProxmoxClient; @@ -48,13 +49,20 @@ class CloneVirtualMachine extends CustomerStep // Clone once, keyed to the reserved vmid; record the task ref immediately. $upid = $run->context('clone_upid'); if ($upid === null) { - // Crash-recovery: if the VM already exists, a prior attempt cloned it - // (but lost the task ref) — don't re-clone. Wait until the clone lock - // clears before advancing, in case the copy is still running. + // Crash-recovery: the VM exists but the clone task ref was lost, so we + // can't confirm the clone succeeded. If it's still locked, keep waiting; + // once unlocked, destroy the possibly-incomplete VM and re-clone cleanly. if ($pve->vmExists($node, $vmid)) { - return empty($pve->vmStatus($node, $vmid)['lock']) - ? StepResult::advance() - : StepResult::poll(10, 'finishing recovered clone'); + if (! empty($pve->vmStatus($node, $vmid)['lock'])) { + return StepResult::poll(10, 'finishing recovered clone'); + } + + $pve->deleteVm($node, $vmid); + RunResource::query()->where('run_id', $run->id)->where('kind', 'vmid')->delete(); + $run->forgetContext('vmid'); + $run->forgetContext('clone_upid'); + + return StepResult::retry(5, 're-cloning after a lost clone task reference'); } $upid = $pve->cloneVm($node, $templateVmid, $vmid, $name); diff --git a/app/Services/Proxmox/FakeProxmoxClient.php b/app/Services/Proxmox/FakeProxmoxClient.php index 6d5edb0..c9e4cec 100644 --- a/app/Services/Proxmox/FakeProxmoxClient.php +++ b/app/Services/Proxmox/FakeProxmoxClient.php @@ -130,6 +130,16 @@ class FakeProxmoxClient implements ProxmoxClient return in_array($vmid, $this->clonedVmids, true) || in_array($vmid, $this->runningVmids, true); } + /** @var array */ + public array $deletedVmids = []; + + public function deleteVm(string $node, int $vmid): void + { + $this->deletedVmids[] = $vmid; + $this->clonedVmids = array_values(array_diff($this->clonedVmids, [$vmid])); + $this->runningVmids = array_values(array_diff($this->runningVmids, [$vmid])); + } + public function guestAgentPing(string $node, int $vmid): bool { return $this->guestAgentUp; diff --git a/app/Services/Proxmox/HttpProxmoxClient.php b/app/Services/Proxmox/HttpProxmoxClient.php index 789f637..4bc5969 100644 --- a/app/Services/Proxmox/HttpProxmoxClient.php +++ b/app/Services/Proxmox/HttpProxmoxClient.php @@ -89,6 +89,11 @@ class HttpProxmoxClient implements ProxmoxClient return $this->http()->get("/nodes/{$node}/qemu/{$vmid}/status/current")->successful(); } + public function deleteVm(string $node, int $vmid): void + { + $this->http()->asForm()->delete("/nodes/{$node}/qemu/{$vmid}", ['purge' => 1])->throw(); + } + public function guestAgentPing(string $node, int $vmid): bool { return $this->http()->asForm()->post("/nodes/{$node}/qemu/{$vmid}/agent/ping")->successful(); @@ -134,6 +139,12 @@ class HttpProxmoxClient implements ProxmoxClient public function applyFirewall(string $node, int $vmid, array $rules): void { + // Clear existing rules first so a retry doesn't accumulate duplicates. + $existing = $this->http()->get("/nodes/{$node}/qemu/{$vmid}/firewall/rules")->throw()->json('data', []); + for ($pos = count($existing) - 1; $pos >= 0; $pos--) { + $this->http()->delete("/nodes/{$node}/qemu/{$vmid}/firewall/rules/{$pos}")->throw(); + } + foreach ($rules as $rule) { $this->http()->asForm()->post("/nodes/{$node}/qemu/{$vmid}/firewall/rules", $rule)->throw(); } diff --git a/app/Services/Proxmox/ProxmoxClient.php b/app/Services/Proxmox/ProxmoxClient.php index 981b69d..065200d 100644 --- a/app/Services/Proxmox/ProxmoxClient.php +++ b/app/Services/Proxmox/ProxmoxClient.php @@ -45,6 +45,8 @@ interface ProxmoxClient /** Whether a VM with this id already exists on the node. */ public function vmExists(string $node, int $vmid): bool; + public function deleteVm(string $node, int $vmid): void; + public function guestAgentPing(string $node, int $vmid): bool; /** @return array{exitcode: int, out-data: string} */ diff --git a/tests/Feature/Provisioning/CustomerStepsTest.php b/tests/Feature/Provisioning/CustomerStepsTest.php index 5c8a11c..f9c00ac 100644 --- a/tests/Feature/Provisioning/CustomerStepsTest.php +++ b/tests/Feature/Provisioning/CustomerStepsTest.php @@ -121,21 +121,24 @@ it('polls while cloning and fails on a clone error', function () { expect(app(CloneVirtualMachine::class)->execute($run2)->type)->toBe('fail'); }); -it('recovers a clone whose task ref was lost (no duplicate clone)', function () { +it('recovers a lost-task clone by waiting for the lock then re-cloning cleanly', function () { $s = fakeServices(); ['run' => $run] = reservedRun([], ['vmid' => 101]); - // Simulate a crash after the clone but before the upid was persisted: - // vmid reserved + breadcrumb, no clone_upid, and the VM already exists. + // Crash after cloneVm() but before clone_upid was persisted: vmid reserved + + // breadcrumb, no clone_upid, and the VM already exists. RunResource::create(['run_id' => $run->id, 'kind' => 'vmid', 'external_id' => '101']); $run->mergeContext(['vmid' => 101]); - $s['pve']->clonedVmids[] = 101; // VM exists + $s['pve']->clonedVmids[] = 101; - expect(app(CloneVirtualMachine::class)->execute($run->fresh())->type)->toBe('advance') - ->and(count($s['pve']->clonedVmids))->toBe(1); // no second clone - - // still locked (clone copy running) → poll, don't advance onto an incomplete VM + // still cloning (locked) → poll, don't touch it $s['pve']->vmLock = 'clone'; expect(app(CloneVirtualMachine::class)->execute($run->fresh())->type)->toBe('poll'); + + // unlocked but task status unknown → destroy the maybe-incomplete VM + re-clone + $s['pve']->vmLock = null; + expect(app(CloneVirtualMachine::class)->execute($run->fresh())->type)->toBe('retry'); + expect($s['pve']->deletedVmids)->toContain(101) + ->and(RunResource::where('run_id', $run->id)->where('kind', 'vmid')->exists())->toBeFalse(); }); it('fails clone when the plan has no template', function () {