From d0a06c03b77ff28dbc92750389dfc73e4689a8d8 Mon Sep 17 00:00:00 2001 From: Mathieu Benoit Date: Thu, 27 Aug 2026 05:39:08 -0400 Subject: [PATCH] =?UTF-8?q?[FIX]=20LongTest=20:=20rapport=20=C3=A9crit=20V?= =?UTF-8?q?M=20par=20VM,=20retrait=20ssh=20sans=20bloc=20nu?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Descente de dix étages arrêtée pendant l'installation du quatrième : quatre machines réelles restaient, et « --detruire » répondait « aucun rapport : rien à défaire ». Le rapport ne s'écrivait qu'à la fin, donc le seul enregistrement du couple (alias du parent, VMID) mourait avec le processus. Il fallait les retrouver par leur NOM, ce que tout ce fichier s'applique à éviter. En nettoyant à la main, second défaut : appelé sans nom à écrire — un retrait pur, légitime, les machines n'existant plus — _write_ssh_config_entry écrivait « Host » NU suivi d'un « HostName » vide dans le ~/.ssh/config réel, puis mourait sur IndexError en annonçant l'ajout. Constaté, puis retiré du fichier. Les deux correctifs meurent sous mutation : 3 tests et 2 tests. --- EN --- A ten-level descent stopped during the fourth level's install: four real machines were left, and "--detruire" answered "no report: nothing to undo". The report was only written at the end, so the sole record of the (parent alias, VMID) pair died with the process. They had to be found by NAME, which this whole file works to avoid. Cleaning up by hand surfaced a second defect: called with no name to write — a pure removal, legitimate since the machines are gone — _write_ssh_config_entry wrote a BARE "Host" followed by an empty "HostName" into the real ~/.ssh/config, then died on IndexError while announcing the addition. Observed, then removed. Both fixes die under mutation: 3 tests and 2 tests. Assisted-by: claude-opus-5 (cherry picked from commit e7c8b540c630b33c6eb1b1a2f51c01497e0b3ca0) --- LongTest/deep_proxmox.py | 69 +++++++++++++++---- script/todo/todo.py | 12 ++++ script/todo/todo_i18n.py | 4 ++ test/test_proxmox_form.py | 42 ++++++++++++ test/test_todo_longtest.py | 132 +++++++++++++++++++++++++++++++++++-- 5 files changed, 238 insertions(+), 21 deletions(-) diff --git a/LongTest/deep_proxmox.py b/LongTest/deep_proxmox.py index 9bd0d26..4f5fc94 100755 --- a/LongTest/deep_proxmox.py +++ b/LongTest/deep_proxmox.py @@ -130,9 +130,10 @@ def alias_etage(niveau, parent_alias): class Descente: """Un étage après l'autre, et ce qu'on en sait.""" - def __init__(self, plan, journal, dry_run=False): + def __init__(self, plan, journal, dry_run=False, chemin_json=None): self.plan = plan self.journal = journal + self.chemin_json = chemin_json self.dry_run = dry_run self.etages = [] self.interrompu = False @@ -520,6 +521,8 @@ class Descente: self.interrompu = True break alias = nom + # Le domaine libvirt existe : le rapport doit exister aussi. + self._sauver(etage) else: prepare = self.preparer_parent(parent) if not prepare: @@ -538,6 +541,7 @@ class Descente: # sert. Sans lui, une VM abandonnée juste après « qm create » # n'était nommée nulle part. etage["parent_alias"] = parent_alias + self._sauver(etage) alias = alias_etage(niveau, parent_alias) if not self.dry_run: self.ecrire_alias(alias, adresse, parent_alias) @@ -560,6 +564,7 @@ class Descente: ("pmxcfs", lambda: self.reparer_pmxcfs(cible)), ): etage["etape"] = etape + self._sauver(etage) if not action(): self.etages.append(etage) return self.rapport(interrompu=True) @@ -571,6 +576,7 @@ class Descente: etage["ok"] = not self.dry_run etage["secondes"] = int(time.time() - debut) self.etages.append(etage) + self._sauver() self.dire(f" ✓ étage {niveau} en {etage['secondes']} s") parent, parent_alias = cible, alias return self.rapport(interrompu=self.interrompu) @@ -589,8 +595,52 @@ class Descente: identity_file=prive, ) + def _etat(self, interrompu, en_cours=None): + """Le rapport, à cet instant. `en_cours` : l'étage pas encore rangé.""" + etages = list(self.etages) + if en_cours is not None and en_cours not in etages: + etages.append(en_cours) + return { + "demandee": self.plan["demandee"], + "atteignable": self.plan["atteignable"], + "atteinte": sum(1 for e in etages if e.get("ok")), + "interrompu": interrompu, + # Sans ce champ, un rapport d'essai à blanc se lisait comme une + # descente réussie — et « --detruire » s'en servait. + "dry_run": self.dry_run, + "etages": etages, + } + + def _sauver(self, en_cours=None): + """Écrit le rapport PARTIEL, dès qu'une VM existe. + + Il ne s'écrivait qu'à la fin. Une descente tuée au quatrième étage — + c'est arrivé — laissait quatre machines réelles et « --detruire » + répondait « aucun rapport : rien à défaire » : le seul enregistrement + du couple (alias du parent, VMID) mourait avec le processus. Il fallait + alors les retrouver et les détruire à la main, c'est-à-dire par leur + nom, ce que tout le reste de ce fichier s'applique à ne pas faire. + + Marqué « interrompu » jusqu'au bout : un rapport partiel ne doit jamais + se lire comme une descente terminée. + """ + if self.dry_run or not self.chemin_json: + return + temporaire = self.chemin_json + ".tmp" + try: + with open(temporaire, "w", encoding="utf-8") as fh: + json.dump( + self._etat(interrompu=True, en_cours=en_cours), + fh, + indent=2, + ) + os.replace(temporaire, self.chemin_json) + except OSError as err: + self.dire(f" ⚠ rapport non écrit : {err}") + def rapport(self, interrompu=False): - atteint = sum(1 for e in self.etages if e["ok"]) + etat = self._etat(interrompu) + atteint = etat["atteinte"] print("") if self.dry_run: self.dire( @@ -611,16 +661,7 @@ class Descente: f"{e.get('secondes', '—')} s" if e["ok"] else e["etape"] ) self.dire(f" {marque} étage {e['niveau']:2d} {detail}") - return { - "demandee": self.plan["demandee"], - "atteignable": self.plan["atteignable"], - "atteinte": atteint, - "interrompu": interrompu, - # Sans ce champ, un rapport d'essai à blanc se lisait comme une - # descente réussie — et « --detruire » s'en servait. - "dry_run": self.dry_run, - "etages": self.etages, - } + return etat def dernier_rapport(): @@ -868,9 +909,9 @@ def principal(argv=None): print(f"\n journal : {journal}") if args.dry_run: print(" --dry-run : rien ne sera créé.\n") - descente = Descente(plan, journal, args.dry_run) - rapport = descente.parcourir() chemin = journal[:-4] + ("-dryrun.json" if args.dry_run else ".json") + descente = Descente(plan, journal, args.dry_run, chemin) + rapport = descente.parcourir() with open(chemin, "w", encoding="utf-8") as fh: json.dump(rapport, fh, indent=2) print(f"\n rapport : {chemin}\n") diff --git a/script/todo/todo.py b/script/todo/todo.py index cd9acf1..2a2dd43 100755 --- a/script/todo/todo.py +++ b/script/todo/todo.py @@ -1468,6 +1468,18 @@ class TODO( existing = self._ssh_config_drop_hosts( existing, names + [n for n in also_drop if n not in names] ).rstrip("\n") + if not names: + # Retirer sans réécrire est un appel légitime : les machines + # n'existent plus. Sans ce retour, un « Host » NU était écrit dans + # le ~/.ssh/config de l'utilisateur — un bloc sans nom, suivi d'un + # « HostName » vide, qui s'applique alors à rien et brouille la + # lecture du fichier. + with open(cfg, "w", encoding="utf-8") as fh: + fh.write(existing + "\n" if existing else "") + os.chmod(cfg, 0o600) + retires = ", ".join(also_drop) + print(f"🗑 {t('Removed from ~/.ssh/config:')} {retires}") + return block = ( f"Host {' '.join(names)}\n" f" HostName {ip}\n" diff --git a/script/todo/todo_i18n.py b/script/todo/todo_i18n.py index b292f80..d94bf33 100644 --- a/script/todo/todo_i18n.py +++ b/script/todo/todo_i18n.py @@ -278,6 +278,10 @@ TRANSLATIONS = { "fr": "Ajouté à ~/.ssh/config :", "en": "Added to ~/.ssh/config:", }, + "Removed from ~/.ssh/config:": { + "fr": "Retiré de ~/.ssh/config :", + "en": "Removed from ~/.ssh/config:", + }, "SSH address input method": { "fr": "Méthode de saisie de l'adresse SSH", "en": "SSH address input method", diff --git a/test/test_proxmox_form.py b/test/test_proxmox_form.py index adbc7fc..7276470 100644 --- a/test/test_proxmox_form.py +++ b/test/test_proxmox_form.py @@ -1057,6 +1057,48 @@ class TestLAncienNomSEnVa(unittest.TestCase): ligne.rstrip() for ligne in fh if ligne.startswith("Host ") ] + def test_dropping_the_last_entry_writes_no_nameless_block(self): + """Retirer sans réécrire est un appel légitime : les machines + n'existent plus. + + Constaté dans le vrai ~/.ssh/config de l'utilisateur : l'appel écrivait + « Host » NU, suivi d'un « HostName » vide, puis mourait sur un + IndexError en annonçant l'ajout. Le bloc sans nom s'applique à rien et + brouille la lecture du fichier.""" + import os + + self.todo._write_ssh_config_entry( + ["deep-1"], "erplibre", "10.10.10.150" + ) + self.todo._write_ssh_config_entry( + ["deep-2"], "erplibre", "10.10.10.151", proxy_jump="deep-1" + ) + self.todo._write_ssh_config_entry( + [], "erplibre", "", also_drop=("deep-1", "deep-2") + ) + self.assertEqual(self._hosts(), []) + with open( + os.path.join(self.maison, ".ssh/config"), encoding="utf-8" + ) as fh: + reste = fh.read() + self.assertNotIn("Host", reste) + self.assertNotIn("HostName", reste) + # Et le fichier garde ses droits : ssh refuse un config trop ouvert. + self.assertEqual( + oct(os.stat(os.path.join(self.maison, ".ssh/config")).st_mode)[ + -3: + ], + "600", + ) + + def test_dropping_one_entry_leaves_the_others_untouched(self): + for nom, ip in (("garde-a", "10.0.0.1"), ("part", "10.0.0.2")): + self.todo._write_ssh_config_entry([nom], "erplibre", ip) + self.todo._write_ssh_config_entry( + [], "erplibre", "", also_drop=("part",) + ) + self.assertEqual(self._hosts(), ["Host garde-a"]) + def test_the_old_short_entry_is_retired(self): # L'état d'avant : une entrée écrite sous l'ancienne convention. self.todo._write_ssh_config_entry( diff --git a/test/test_todo_longtest.py b/test/test_todo_longtest.py index 7ad9c73..5be410b 100644 --- a/test/test_todo_longtest.py +++ b/test/test_todo_longtest.py @@ -10,9 +10,14 @@ sans virtualisation. Ce fichier-ci vérifie la frontière, et que l'essai à blanc du test long dit quelque chose sans rien créer. """ +import contextlib +import io +import json import os +import shutil import subprocess import sys +import tempfile import unittest sys.argv = ["todo.py"] @@ -180,9 +185,14 @@ class TestLEssaiABlanc(unittest.TestCase): self.assertEqual(rapport["atteinte"], 0) self.assertTrue(all(not e["ok"] for e in rapport["etages"])) - def test_the_first_level_is_wide_and_the_others_are_not(self): - # 12 vCPU au quatrième étage ont gelé un noyau invité ; deux - # avançaient. + def test_the_plan_shrinks_towards_the_bottom(self): + """Chaque étage annoncé est plus étroit que son parent, sur les trois + ressources. + + Deux vCPU à chaque étage imbriqué donnaient un parent aussi étroit que + son enfant : cent pour cent de surengagement, et l'hyperviseur à servir + par-dessus. Mesuré : l'installation de l'étage 4 dépassait 2 h 50 + contre 793 s pour l'étage 3.""" # Par expression exacte : la ligne « machine : … Mo … Go » du haut # contient les mêmes unités et décalait l'index d'un cran. import re @@ -193,10 +203,17 @@ class TestLEssaiABlanc(unittest.TestCase): re.M, ) self.assertEqual(len(plan), 4, plan) - niveaux = {int(n): int(v) for n, v, _r, _d in plan} - self.assertGreater(niveaux[1], 1, "le premier étage peut être large") - for niveau in (2, 3, 4): - self.assertEqual(niveaux[niveau], 2, f"étage {niveau}") + etages = sorted( + (int(n), int(v), int(r), int(d)) for n, v, r, d in plan + ) + for parent, enfant in zip(etages, etages[1:]): + for i, quoi in ((1, "vCPU"), (2, "RAM"), (3, "disque")): + self.assertGreater( + parent[i], enfant[i], f"étage {parent[0]} : {quoi}" + ) + # Et le plus profond reçoit ce qu'un Proxmox de test demande, pas ce + # qui reste. + self.assertEqual(etages[-1][1], 2, "vCPU du plus profond") class TestDefaireSansEffacerAutreChose(unittest.TestCase): @@ -272,6 +289,107 @@ class TestDefaireSansEffacerAutreChose(unittest.TestCase): self.assertIn("dry_run=args.dry_run", principal) +class TestUnRapportQuiSurvitAuProcessus(unittest.TestCase): + """Le rapport ne s'écrivait qu'à la FIN de la descente. + + Constaté : une descente de dix étages arrêtée pendant l'installation du + quatrième laissait quatre machines réelles, et « --detruire » répondait + « aucun rapport de descente : rien à défaire ». Le seul enregistrement du + couple (alias du parent, VMID) mourait avec le processus — il fallait + retrouver ces VM à la main, c'est-à-dire par leur nom, ce que tout le + reste de ce fichier s'applique à ne pas faire. + """ + + def setUp(self): + sys.path.insert(0, os.path.join(RACINE, "LongTest")) + import deep_proxmox + + self.dp = deep_proxmox + self.dossier = tempfile.mkdtemp(prefix="longtest-rapport-") + self.addCleanup(shutil.rmtree, self.dossier, ignore_errors=True) + + def _descente_tuee(self, a_l_etage): + """Une descente dont l'installation MEURT à l'étage donné. + + Rien de réel n'est touché : aucune des méthodes qui créent une machine + ou écrivent dans ~/.ssh/config n'est appelée pour de vrai. + """ + niveaux = [ + {"niveau": n, "vcpu": 2, "ram": 4096, "disque": 25} + for n in (1, 2, 3) + ] + plan = {"demandee": 3, "atteignable": 3, "niveaux": niveaux} + chemin = os.path.join(self.dossier, "rapport.json") + d = self.dp.Descente(plan, None, False, chemin) + + appels = [] + d.creer_etage1 = lambda res: "deep-pve-1" + d.preparer_parent = lambda parent: {"stockage": "local-lvm"} + d.creer_enfant = lambda parent, niveau, res, prep: ( + 100 + niveau, + f"10.10.10.{niveau}", + ) + d.ecrire_alias = lambda *a, **k: None + d.attendre_ssh = lambda cible, delai: 1 + d.redemarrer_et_verifier = lambda cible: True + d.reparer_pmxcfs = lambda cible: True + + def installer(cible): + appels.append(cible) + if len(appels) >= a_l_etage: + raise KeyboardInterrupt("descente tuée") + return True + + d.installer_proxmox = installer + with contextlib.redirect_stdout(io.StringIO()): + with self.assertRaises(KeyboardInterrupt): + d.parcourir() + with open(chemin, encoding="utf-8") as fh: + return json.load(fh) + + def test_a_killed_descent_still_names_what_it_created(self): + rapport = self._descente_tuee(a_l_etage=3) + # Le couple (parent, VMID) des étages imbriqués créés : c'est de lui + # seul que « --detruire » se sert. + self.assertEqual( + self.dp.a_defaire(rapport), + [ + (3, "deep-pve-1+deep-pve-2", 103, self.dp.nom_etage(3)), + (2, "deep-pve-1", 102, self.dp.nom_etage(2)), + ], + ) + + def test_the_report_exists_as_soon_as_the_first_vm_does(self): + """Tuée pendant l'installation de l'étage 1, il n'y a aucun VMID à + noter — mais le domaine libvirt existe, et sans rapport « --detruire » + ne le regardait même pas.""" + rapport = self._descente_tuee(a_l_etage=1) + self.assertTrue(rapport["etages"]) + self.assertEqual(self.dp.a_defaire(rapport), []) + + def test_a_partial_report_never_reads_as_a_finished_descent(self): + rapport = self._descente_tuee(a_l_etage=3) + self.assertTrue(rapport["interrompu"]) + self.assertLess(rapport["atteinte"], rapport["demandee"]) + # Et il n'est pas écarté comme un essai à blanc : c'est bien de VRAIES + # machines qu'il parle. + self.assertFalse(rapport["dry_run"]) + + def test_a_dry_run_writes_no_partial_report(self): + """Un plan n'a rien créé : lui laisser écrire un rapport ferait + détruire d'après un plan.""" + plan = { + "demandee": 1, + "atteignable": 1, + "niveaux": [{"niveau": 1, "vcpu": 2, "ram": 4096, "disque": 25}], + } + chemin = os.path.join(self.dossier, "blanc.json") + d = self.dp.Descente(plan, None, True, chemin) + with contextlib.redirect_stdout(io.StringIO()): + d._sauver({"niveau": 1, "vmid": 101, "parent_alias": "x"}) + self.assertFalse(os.path.exists(chemin)) + + class TestLeMenu(unittest.TestCase): def test_the_mixin_is_wired_into_TODO(self): todo = TODO.__new__(TODO)