diff --git a/app/Actions/ArchiveAndTearDown.php b/app/Actions/ArchiveAndTearDown.php index 43e42d1..674f874 100644 --- a/app/Actions/ArchiveAndTearDown.php +++ b/app/Actions/ArchiveAndTearDown.php @@ -173,19 +173,25 @@ class ArchiveAndTearDown return false; } - // Zwei Läufe über dieselbe Instanz — der Zeitplan und ein Betreiber von - // Hand — würden zweimal sichern und zweimal löschen; der zweite fände - // dabei eine Maschine vor, die der erste gerade wegnimmt. Niemand - // wartet auf die Sperre: wer sie nicht bekommt, hat nichts - // hinzuzufügen, weil der Lauf, der sie hält, dasselbe tut. Die Frist - // deckt den ganzen Dump ab, sonst liefe sie mitten im Sichern aus. - $lock = Cache::lock('instance-teardown:'.$instance->uuid, $this->backupWaitSeconds + 600); - - if (! $lock->get()) { - return false; - } + $lock = null; try { + // Zwei Läufe über dieselbe Instanz — der Zeitplan und ein Betreiber + // von Hand — würden zweimal sichern und zweimal löschen; der zweite + // fände dabei eine Maschine vor, die der erste gerade wegnimmt. + // Niemand wartet auf die Sperre: wer sie nicht bekommt, hat nichts + // hinzuzufügen, weil der Lauf, der sie hält, dasselbe tut. + // + // Innerhalb des try, nicht davor: im Betrieb ist der Sperrspeicher + // Redis, und ein Redis, das gerade nicht da ist, wirft schon beim + // Anlegen der Sperre — vor dem try nähme dieser Wurf im Zeitplan + // alle übrigen Instanzen mit. + $lock = Cache::lock('instance-teardown:'.$instance->uuid, $this->lockSeconds()); + + if (! $lock->get()) { + return false; + } + return $this->tearDown($instance); } catch (Throwable $e) { // Keine Ausnahme entkommt: dieser Ablauf läuft im Zeitplan über @@ -194,10 +200,31 @@ class ArchiveAndTearDown return false; } finally { - $lock->release(); + try { + $lock?->release(); + } catch (Throwable) { + // Dieselbe Verbindung wie oben und derselbe Weg nach draussen — + // diesmal sogar am Erfolgsfall vorbei. Eine Sperre, die sich + // nicht freigeben lässt, läuft von selbst ab; ein Wurf von hier + // machte aus einem fertigen Abbau einen gescheiterten Zeitplan. + } } } + /** + * Wie lange die Sperre halten muss. + * + * Der ganze Lauf, nicht nur der Dump: im schlechtesten Fall wartet er erst + * die Frist fürs Herunterfahren und danach die fürs Sichern ab. Eine kürzere + * Sperre liefe ausgerechnet kurz vor dem Löschen aus — also genau dann, wenn + * ein zweiter Lauf am meisten Schaden anrichtet. Der Zuschlag deckt die + * Ablagenprüfung, die Nachschau und die HTTP-Umläufe dazwischen. + */ + private function lockSeconds(): int + { + return $this->shutdownWaitSeconds + $this->backupWaitSeconds + 600; + } + /** * Der eigentliche Ablauf. Jeder Riegel wirft, der Aufrufer oben fängt und * schreibt den Grund an den Datensatz — ein Wurf ist hier Ablaufsteuerung @@ -219,14 +246,29 @@ class ArchiveAndTearDown $pve = $this->pve->forHost($host); $vmid = (int) $vmid; + // Zuerst die Ablage, und das aus zwei Gründen. Der offensichtliche: eine + // Instanz, deren Archiv nirgends hin kann, wird gar nicht erst + // angefasst. Der wichtigere: `nodeStorage()` ruft `->throw()` und ist + // damit das Lebenszeichen des Knotens, das der Nachtrags-Zweig + // darunter braucht. + $storage = $this->archiveStorage($pve, $node); + // Der Weg zurück aus einem Absturz zwischen Löschen und Buchen: die // Maschine ist fort, das Archiv steht aber schon am Datensatz. Ohne // diesen Zweig versuchte jeder weitere Lauf, einen Geist // herunterzufahren, und die Instanz bliebe für immer im Fehler stehen. - // Bewusst an `archive_volid` gebunden und nicht an `vmExists()` allein: - // die Frage beantwortet auch ein hakendes Proxmox mit „nein", und ohne - // ein verbrieftes Archiv wäre das „schon weg" eine Maschine, die - // niemand mehr anfasst. + // + // Zwei Bedingungen, weil `vmExists()` allein die Frage nicht beantwortet: + // es ist `->get(…)->successful()` OHNE `->throw()`, und Proxmox antwortet + // auf die Konfiguration einer nicht vorhandenen VM mit 500 — demselben + // Code, den ein hakender Knoten liefert (siehe `isTemplate()` im + // Client). „Gibt es nicht" und „ich konnte nicht fragen" sehen also + // gleich aus. Ein verbrieftes `archive_volid` schliesst die Maschine + // aus, die nie archiviert wurde; die Ablagenprüfung darüber schliesst + // den Knoten aus, der gerade gar nichts beantwortet. Ohne beides würde + // hier eine LAUFENDE Kundenmaschine als abgebaut verbucht und danach + // nie wieder angesehen — der Zustand entsteht im Regelbetrieb, sobald + // ein Lauf nach dem Sichern am Löschen scheitert. if ($instance->archive_volid !== null && ! $pve->vmExists($node, $vmid)) { $instance->update(['torn_down_at' => now(), 'teardown_error' => null]); @@ -239,10 +281,6 @@ class ArchiveAndTearDown return true; } - // Vor dem Herunterfahren: eine Instanz, deren Archiv nirgends hin - // kann, wird gar nicht erst angefasst. - $storage = $this->archiveStorage($pve, $node); - $shutdownUpid = $pve->shutdownVm($node, $vmid, $this->shutdownWaitSeconds); $this->awaitTask($pve, $node, $shutdownUpid, $this->shutdownWaitSeconds, 'Das Herunterfahren'); @@ -404,12 +442,24 @@ class ArchiveAndTearDown $ctime = $juengstes['ctime'] ?? null; if ($ctime === null) { - // Eine echte Ablage nennt zu jedem Eintrag ein `ctime`; fehlt es, - // ist das Alter des Archivs schlicht nicht feststellbar. Dann wird - // es angenommen statt abgelehnt — es LIEGT ein Archiv dieser VM da, - // und der Fall, den diese Aktion zu verhindern hat, ist das Löschen - // ohne jedes Archiv. Laut genug, dass es auffällt, wenn es je öfter - // vorkommt. + // Ohne `ctime` ist das Alter eines Archivs nicht feststellbar. Ein + // EINZELNER solcher Eintrag wird angenommen statt abgelehnt: es + // liegt ein Archiv dieser VM da, und der Fall, den diese Aktion zu + // verhindern hat, ist das Löschen ohne jedes Archiv. + // + // MEHRERE dagegen nicht. Auf `local` liegen neben dem frischen Dump + // die nächtlichen Sicherungen derselben VM; ohne Zeitpunkt lassen + // sie sich nicht ordnen, und vermerkt würde irgendeiner davon — + // womöglich der von vorgestern. Eine Reihe, die sich nicht sortieren + // lässt, ist keine Grundlage für ein Löschen. + if (count($eintraege) > 1) { + throw new RuntimeException( + 'Auf "'.$storage.'" liegen '.count($eintraege).' Archive für VM '.$vmid + .', und keines nennt einen Zeitpunkt. Welches davon die Sicherung dieses Laufes ' + .'ist, lässt sich nicht sagen; es wird nichts gelöscht.' + ); + } + Log::warning('Abbau: das Archiv nennt keinen Zeitpunkt, sein Alter ist ungeprüft.', [ 'vmid' => $vmid, 'storage' => $storage, @@ -440,15 +490,33 @@ class ArchiveAndTearDown return $volid; } - /** Der Grund gehört an den Datensatz, wo ein Betreiber ihn ohne Logdurchsicht findet. */ + /** + * Der Grund gehört an den Datensatz, wo ein Betreiber ihn ohne + * Logdurchsicht findet. + * + * Beide Schritte einzeln abgesichert, und das ist kein Zierrat: diese + * Methode läuft IM `catch`. Ein Wurf von hier — eine Datenbank, die gerade + * nicht schreibt — ersetzte den gefangenen Fehlschlag und käme doch nach + * draussen, wo er im Zeitplan die übrigen Instanzen mitrisse. + */ private function recordFailure(Instance $instance, Throwable $e): void { - $instance->update(['teardown_error' => $e->getMessage()]); + try { + $instance->update(['teardown_error' => $e->getMessage()]); + } catch (Throwable) { + // Der Grund liess sich nicht vermerken. Mehr als der Eintrag unten + // geht dann nicht — und gelöscht hat dieser Lauf ohnehin nichts. + } - Log::error('Abbau einer gekündigten Instanz gestoppt.', [ - 'instance' => $instance->uuid, - 'vmid' => $instance->vmid, - 'reason' => $e->getMessage(), - ]); + try { + Log::error('Abbau einer gekündigten Instanz gestoppt.', [ + 'instance' => $instance->uuid, + 'vmid' => $instance->vmid, + 'reason' => $e->getMessage(), + ]); + } catch (Throwable) { + // Auch das Protokoll ist nicht erreichbar. Dann schweigt dieser + // eine Lauf, statt den ganzen Zeitplan mitzunehmen. + } } } diff --git a/tests/Feature/Cancellation/ArchiveAndTearDownTest.php b/tests/Feature/Cancellation/ArchiveAndTearDownTest.php index 6ee7472..c61fd3c 100644 --- a/tests/Feature/Cancellation/ArchiveAndTearDownTest.php +++ b/tests/Feature/Cancellation/ArchiveAndTearDownTest.php @@ -6,6 +6,7 @@ use App\Models\Instance; use App\Services\Proxmox\FakeProxmoxClient; use App\Services\Proxmox\ProxmoxClient; use Illuminate\Support\Carbon; +use Illuminate\Support\Facades\Cache; /** * Eine Instanz im Zustand NACH dem Laufzeitende, so wie ihn @@ -69,7 +70,8 @@ it('loescht auch dann nicht, wenn das Herunterfahren scheitert', function () { expect(app(ArchiveAndTearDown::class)($instance))->toBeFalse() ->and($pve->backupCalls)->toBe([]) - ->and($pve->deletedVmids)->toBe([]); + ->and($pve->deletedVmids)->toBe([]) + ->and($instance->fresh()->teardown_error)->not->toBeNull(); }); it('faellt nicht ueber eine Instanz, die schon abgebaut ist', function () { @@ -166,7 +168,8 @@ it('fährt gar nichts herunter, wenn die Ablage keine Sicherungen aufnimmt', fun expect(app(ArchiveAndTearDown::class)($instance))->toBeFalse() ->and($pve->shutdownCalls)->toBe([]) ->and($pve->backupCalls)->toBe([]) - ->and($pve->deletedVmids)->toBe([]); + ->and($pve->deletedVmids)->toBe([]) + ->and($instance->fresh()->teardown_error)->toContain('local'); }); it('sichert nicht, wenn der Gast nach dem Herunterfahren weiterläuft', function () { @@ -227,6 +230,28 @@ it('merkt sich das frische Archiv, nicht das danebenliegende alte', function () ->and($instance->fresh()->archived_at)->not->toBeNull(); }); +it('löscht nicht gegen mehrere Archive, von denen keines einen Zeitpunkt nennt', function () { + // Auf `local` liegen neben dem frischen Dump die nächtlichen Sicherungen + // derselben VM. Fehlt allen der Zeitpunkt, lassen sie sich nicht ordnen — + // vermerkt würde irgendeines, womöglich das von vorgestern. Eine Reihe, die + // sich nicht sortieren lässt, ist keine Grundlage für ein Löschen. Ein + // EINZELNES Archiv ohne Zeitpunkt bleibt dagegen gültig; das ist der Fall + // der ersten Prüfung dieser Datei. + $pve = new FakeProxmoxClient; + $pve->backups['201'] = [ + ['volid' => 'local:backup/vzdump-qemu-201-a.vma.zst', 'size' => 4096], + ['volid' => 'local:backup/vzdump-qemu-201-b.vma.zst', 'size' => 4096], + ]; + app()->instance(ProxmoxClient::class, $pve); + $instance = abgelaufeneInstanz(vmid: 201); + + expect(app(ArchiveAndTearDown::class)($instance))->toBeFalse() + ->and($pve->deletedVmids)->toBe([]) + ->and($instance->fresh()->torn_down_at)->toBeNull() + ->and($instance->fresh()->archive_volid)->toBeNull() + ->and($instance->fresh()->teardown_error)->not->toBeNull(); +}); + it('löscht nicht, wenn das Archiv keine Kennung trägt', function () { // Ein Archiv, dessen Ort nicht notiert werden kann, ist in zwölf Monaten // keins: niemand findet es wieder, und niemand räumt es je auf. @@ -375,6 +400,60 @@ it('trägt den Abbau nach, wenn die Maschine schon fort ist und das Archiv fests ->and($instance->fresh()->torn_down_at)->not->toBeNull(); }); +it('bucht keine Maschine als abgebaut, wenn der Knoten gar nicht antwortet', function () { + // Die Eingangslage entsteht im Regelbetrieb: ein Lauf hat gesichert und ist + // am Löschen gescheitert, weil der Knoten hängt. Am nächsten Tag steht + // `archive_volid` am Datensatz, und `vmExists()` — ein GET ohne `->throw()` — + // antwortet wegen desselben hakenden Knotens mit „nein". Ohne ein + // Lebenszeichen davor würde hier eine LAUFENDE Kundenmaschine als abgebaut + // verbucht und danach nie wieder angesehen. + $pve = new class extends FakeProxmoxClient + { + public function nodeStorage(string $node): array + { + throw new RuntimeException('Proxmox: 595 Connection refused'); + } + }; + app()->instance(ProxmoxClient::class, $pve); + $instance = abgelaufeneInstanz(vmid: 201); + $instance->update([ + 'archive_volid' => 'local:backup/vzdump-qemu-201.vma.zst', + 'archived_at' => now()->subDay(), + 'teardown_error' => 'Proxmox: VM is locked (backup)', + ]); + + expect(app(ArchiveAndTearDown::class)($instance))->toBeFalse() + ->and($pve->deletedVmids)->toBe([]) + ->and($instance->fresh()->torn_down_at)->toBeNull() + ->and($instance->fresh()->teardown_error)->toContain('Connection refused'); +}); + +it('lässt keinen zweiten Lauf über dieselbe Instanz', function () { + // Zwei Läufe nebeneinander — der Zeitplan und ein Betreiber von Hand — + // würden zweimal sichern und zweimal löschen; der zweite fände dabei eine + // Maschine vor, die der erste gerade wegnimmt. Hier hält bereits jemand die + // Sperre, und der Lauf hat nichts hinzuzufügen. + $pve = new FakeProxmoxClient; + $pve->backups['201'] = [['volid' => 'local:backup/vzdump-qemu-201.vma.zst', 'ctime' => now()->timestamp]]; + app()->instance(ProxmoxClient::class, $pve); + $instance = abgelaufeneInstanz(vmid: 201); + + $fremdeSperre = Cache::lock('instance-teardown:'.$instance->uuid, 60); + expect($fremdeSperre->get())->toBeTrue(); + + expect(app(ArchiveAndTearDown::class)($instance))->toBeFalse() + ->and($pve->shutdownCalls)->toBe([]) + ->and($pve->backupCalls)->toBe([]) + ->and($pve->deletedVmids)->toBe([]) + ->and($instance->fresh()->torn_down_at)->toBeNull(); + + // Und die fremde Sperre steht noch: der abgewiesene Lauf gibt im `finally` + // nicht frei, was ihm nie gehört hat. + expect(Cache::lock('instance-teardown:'.$instance->uuid, 60)->get())->toBeFalse(); + + $fremdeSperre->release(); +}); + it('rührt eine Instanz ohne Maschine nicht an', function () { // Ein Datensatz ohne VMID hat nichts, was sich sichern oder löschen ließe. // Er wird nicht stillschweigend als abgebaut gebucht: das wäre ein Haken an