fix(engine-b): clean re-clone on lost task ref; idempotent firewall rules

- Clone recovery: once the lock clears but the task ref is lost, destroy the
  possibly-incomplete VM and re-clone instead of advancing onto it.
- applyFirewall clears existing rules before adding, so retries don't accumulate
  duplicate Proxmox firewall entries.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
feat/portal-design
nexxo 2026-07-25 13:04:48 +02:00
parent 9162446b1f
commit 66e8a4bbf6
5 changed files with 48 additions and 14 deletions

View File

@ -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);

View File

@ -130,6 +130,16 @@ class FakeProxmoxClient implements ProxmoxClient
return in_array($vmid, $this->clonedVmids, true) || in_array($vmid, $this->runningVmids, true);
}
/** @var array<int, int> */
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;

View File

@ -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();
}

View File

@ -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} */

View File

@ -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 () {