fix: make messages say what was verified (source questions from the Italian review)

oci/remote/oci_immich_recognition.py

@@ -228,16 +228,18 @@ def apply(root, primary_id, vmid, images, acceleration, render_device, gfx_overr
 
 
 def revert(root, primary_id, vmid, images, previous, was_running):
-    """Give the container and the records the choice they had."""
+    """Give the container and the records the choice they had. Returns
+    whether they have it again."""
     try:
         if oci_stack_modify.is_running(vmid):
             stop(vmid)
         apply(root, primary_id, vmid, images, *previous)
     except (OSError, ValueError, KeyError, RuntimeError, subprocess.SubprocessError) as error:
         msg_warn(f"{translate('The previous recognition choice could not be put back:')} {error}")
-        return
+        return False
     if was_running:
         subprocess.run(['pct', 'start', str(vmid)], capture_output=True, check=False)
+    return True
 
 
 def change(root, primary_id, acceleration, render_device=None, gfx_override=None):
@@ -262,17 +264,17 @@ def change(root, primary_id, acceleration, render_device=None, gfx_override=None
             msg_info(translate('Preparing the machine learning container for the new choice...'))
             apply(root, primary_id, vmid, images, acceleration, render_device, gfx_override)
             msg_ok(translate('Machine learning container prepared'))
-        except BaseException:
-            revert(root, primary_id, vmid, images, previous, was_running)
+        except BaseException as error:
+            error.recognition_kept = revert(root, primary_id, vmid, images, previous, was_running)
             raise
     try:
         # The image of the new choice replaces the one in use, as an update does.
         oci_stack_native.run(primary_id, acknowledge_external_data=True)
-    except BaseException:
+    except BaseException as error:
         with instances.locked(root):
             # An update left halfway keeps its own record of what to restore.
-            if not instances.read(root, primary_id).get('pending_stack_transaction'):
-                revert(root, primary_id, vmid, images, previous, was_running)
+            error.recognition_kept = (not instances.read(root, primary_id).get('pending_stack_transaction')
+                                      and revert(root, primary_id, vmid, images, previous, was_running))
         raise
     if was_running and not oci_stack_modify.is_running(vmid):
         # An update leaves each container as it found it, and this one was
@@ -297,7 +299,12 @@ def main():
         return 1
     except (OSError, ValueError, KeyError, RuntimeError, StopIteration, subprocess.SubprocessError) as error:
         if not getattr(error, 'oci_reported', False):
-            msg_error(f"{translate('The recognition of Immich was not changed:')} {error}")
+            # Only an error raised before anything was touched, or one that
+            # was put back, leaves the previous choice in place.
+            kept = getattr(error, 'recognition_kept', True)
+            message = (translate('The recognition of Immich was not changed:') if kept
+                       else translate('The recognition change of Immich did not complete:'))
+            msg_error(f"{message} {error}")
         return 1
     msg_ok(translate('Recognition of Immich changed; the application was updated with the new choice.'))
     return 0

oci/remote/oci_instance_transaction.py

@@ -768,29 +768,33 @@ def release_stage(state, discard=False):
     """After a commit the holder CT only keeps its own rootfs: every parked
     volume went back to the application. After a verified recovery it holds
     the disks of the failed attempt, which the restored backup replaced:
-    `discard` removes them with it. Otherwise anything still attached keeps it."""
+    `discard` removes them with it. Otherwise anything still attached keeps it.
+    Returns whether the holder CT was removed."""
     stage = state.get('stage')
     if not stage:
-        return
+        return False
     try:
         config = owned(stage, 'proxmenux-transaction=' + state['id'])
     except (ValueError, RuntimeError, subprocess.CalledProcessError):
-        return
+        return False
     held = mounts(config) or any(re.fullmatch(r'unused[0-9]+', key) for key in parse_config(config))
     if held and not discard:
-        return
+        return False
     if discard:
         stop(stage)
     run('pct', 'destroy', str(stage))
+    return True
 
 
 def discard_stage(state):
     """Remove the holder CT of an operation that ended in a verified recovery.
-    A failure here leaves it in place and does not undo the recovery."""
+    A failure here leaves it in place and does not undo the recovery.
+    Returns whether the holder CT was removed."""
     try:
-        release_stage(state, discard=True)
+        return release_stage(state, discard=True)
     except (OSError, ValueError, RuntimeError, subprocess.SubprocessError) as error:
         log(f'cleanup: {error}')
+        return False
 
 
 def gib(size):
@@ -1139,8 +1143,13 @@ def complete_recovery(root, journal, state):
     checkpoint(journal, state, 'rolled-back')
     if show:
         # A member of a stack is released when the whole stack is back.
-        discard_stage(state)
-        msg_ok(translate('Recovery completed. The disks of the failed attempt were removed; the backup is kept.'))
+        if discard_stage(state):
+            msg_ok(translate('Recovery completed. The disks of the failed attempt were removed; the backup is kept.'))
+        else:
+            msg_ok(translate('Recovery completed; the backup is kept.'))
+            if state.get('stage'):
+                msg_warn(f"{translate('The disks of the failed attempt could not be removed and stay in the temporary container:')} "
+                         f"CT {state['stage']}")
         if state.get('original_host_sources') or state.get('desired_host_sources'):
             msg_info2(translate('Shared host files are kept as they are; the backup does not restore their content.'))
 

oci/remote/oci_restore_recovery.py

@@ -943,7 +943,7 @@ def start_order(entry):
     return int(order[1]) if order else 9999
 
 
-def recover(root, plan, start=False, host_firewall=False):
+def recover(root, plan, start=False, host_firewall=()):
     for bridge, subnet in plan['bridges'].items():
         msg_info(f"{translate('Creating the private network...')} {bridge}")
         create_bridge(bridge, subnet)
@@ -974,7 +974,8 @@ def recover(root, plan, start=False, host_firewall=False):
         if not rule:
             continue
         text = f"TCP {rule['port']} {translate('from')} {rule['source']}"
-        if not host_firewall:
+        # Each rule is confirmed on its own; one answer does not cover the rest.
+        if rule['vmid'] not in host_firewall:
             plan['notes'].append(f"CT {entry['vmid']}: {translate('its host firewall rule was not added:')} {text}")
         elif add_firewall_rule(rule):
             msg_ok(f"{translate('Host firewall rule added:')} {text}")
@@ -1020,7 +1021,8 @@ def main():
     parser.add_argument('action', choices=('list', 'plan', 'recover'))
     parser.add_argument('--root', type=Path, default=instances.ROOT)
     parser.add_argument('--start', action='store_true')
-    parser.add_argument('--host-firewall', action='store_true')
+    parser.add_argument('--host-firewall', type=int, action='append', default=[], metavar='VMID',
+                        help='Add the host firewall rule of this container; one option for each confirmed rule')
     parser.add_argument('--automatic', action='store_true',
                         help='Only the applications that need no question: the ones the cluster keeps a record of')
     args = parser.parse_args()
@@ -1041,13 +1043,13 @@ def main():
             failed = 0
             if args.automatic:
                 plans = [plan for plan in plans if automatic(plan)]
-                args.start = args.host_firewall = False
+                args.start, args.host_firewall = False, []
             for plan in plans:
                 containers = ', '.join(f"CT {entry['vmid']}" for entry in plan['restored'])
                 msg_info2(f"{plan['title']} · {containers}")
                 if plan['blockers']:
                     failed += 1
-                    msg_error(translate('This application cannot be recovered yet; nothing was changed:'))
+                    msg_error(translate('This application cannot be recovered yet:'))
                     for line in plan['blockers']:
                         msg_info2(f"  - {line}")
                     lost = [entry for entry in plan['restored'] if entry.get('unrecoverable')]

oci/src/proxmenux_oci/management.py

@@ -186,7 +186,7 @@ def offer_recovery(project, ui):
     lines = '\n'.join(f"  CT {row['vmid']}  {row['hostname']}" for row in found)
     text = (f"{translate('These containers were restored from a backup and this host has no record of their OCI application:')}"
             f"\n\n{lines}\n\n"
-            f"{translate('Until they are registered again they cannot be updated or managed from here. The recovery checks first that everything they need is on this host and changes nothing otherwise.')}"
+            f"{translate('Until they are registered again they cannot be updated or managed from here. The recovery checks first that everything they need is on this host; an application that lacks something is not registered.')}"
             f"\n\n{translate('Recover them now?')}")
     if not ui.confirm(text, default=True):
         return
@@ -194,8 +194,7 @@ def offer_recovery(project, ui):
     for rule in _restored_firewall_rules(project):
         if ui.confirm(translate('CT {vmid} is a host monitor and had a rule in the host firewall. Allow TCP port {port} from {subnet} through the firewall of this host? Existing firewall rules are not changed.').format(
                 vmid=rule['vmid'], port=rule['port'], subnet=rule['source']), default=False):
-            command.append('--host-firewall')
-            break
+            command.extend(['--host-firewall', str(rule['vmid'])])
     if ui.confirm(translate('Start the applications once they are registered? Answer No if the original containers are still running on another host: both would use the same addresses.'),
                   default=False):
         command.append('--start')
@@ -277,7 +276,7 @@ def manage_instance(project, ui, row, action=None, lifecycle_args=()):
     # A container that came back from another node or from an older backup
     # carries the record that describes it; the one of this host is not used.
     if carry_records(project, vmids=[row['vmid']], verify=True).get(row['vmid']) == 'stale':
-        ui.message(translate('This container was restored or came back from another host after its record on this host was written. Open this menu again to recover it; nothing was changed.'),
+        ui.message(translate('This container was restored or came back from another host after its record on this host was written. Open this menu again to recover it; the container was not modified.'),
                    translate('OCI management'))
         return False
     row = check_selected(project, row)
@@ -370,7 +369,7 @@ def manage_instance(project, ui, row, action=None, lifecycle_args=()):
     if action is None:
         return False
     if action == 'recover' and not ui.review(
-            translate('The previous native backup will be restored. Shared host directories are not reverted. Displaced disks are kept.'),
+            translate('The previous native backup will be restored. Shared host directories are not reverted. The disks of the failed attempt are removed.'),
             translate('Recover OCI'), question=translate('Recover now?'), default=True):
         return False
     return _run_lifecycle([sys.executable, str(project / 'remote/oci_instance_transaction.py'),

oci/tests/test_immich_recognition.py

@@ -171,7 +171,8 @@ class RevertTests(unittest.TestCase):
                 patch.object(recognition, "stop", side_effect=lambda vmid: calls.append(("stop", vmid))), \
                 patch.object(recognition, "apply", side_effect=lambda *arguments: calls.append(("apply",) + arguments[2:])), \
                 patch.object(recognition.subprocess, "run", side_effect=lambda command, **_: calls.append(tuple(command[:3]))):
-            recognition.revert(Path("/nonexistent"), 100, 101, {"cpu": "image"}, ("cpu", None, None), True)
+            kept = recognition.revert(Path("/nonexistent"), 100, 101, {"cpu": "image"}, ("cpu", None, None), True)
+        self.assertTrue(kept)
         self.assertEqual(calls, [("stop", 101), ("apply", 101, {"cpu": "image"}, "cpu", None, None),
                                  ("pct", "start", "101")])
 
@@ -180,9 +181,26 @@ class RevertTests(unittest.TestCase):
                 patch.object(recognition, "apply", side_effect=RuntimeError("pct set: locked")), \
                 patch.object(recognition, "msg_warn") as warned, \
                 patch.object(recognition.subprocess, "run") as started:
-            recognition.revert(Path("/nonexistent"), 100, 101, {}, ("cpu", None, None), True)
+            kept = recognition.revert(Path("/nonexistent"), 100, 101, {}, ("cpu", None, None), True)
         self.assertIn("pct set: locked", warned.call_args[0][0])
         started.assert_not_called()
+        self.assertFalse(kept)
+
+    def test_the_error_only_says_nothing_changed_when_the_choice_was_kept(self):
+        def reported(error):
+            with patch.object(recognition.os, "geteuid", return_value=0), \
+                    patch.object(recognition.sys, "argv", ["x", "100", "--acceleration", "cpu"]), \
+                    patch.object(recognition, "change", side_effect=error), \
+                    patch.object(recognition, "translate", side_effect=lambda text: text), \
+                    patch.object(recognition, "msg_error") as shown:
+                self.assertEqual(recognition.main(), 1)
+            return shown.call_args[0][0]
+        self.assertIn("was not changed", reported(ValueError("already on that choice")))
+        put_back = RuntimeError("update failed"); put_back.recognition_kept = True
+        self.assertIn("was not changed", reported(put_back))
+        left = RuntimeError("update failed"); left.recognition_kept = False
+        self.assertIn("did not complete", reported(left))
+        self.assertNotIn("was not changed", reported(left))
 
 
 class RocmProbeTests(unittest.TestCase):

oci/tests/test_restore_recovery.py

@@ -546,5 +546,55 @@ class RegistrationTests(unittest.TestCase):
         self.assertNotIn("deployment", services[2])
 
 
+class FirewallConsentTests(unittest.TestCase):
+    """A host firewall rule is added only for the container it was confirmed for."""
+
+    def recover(self, confirmed):
+        rule = lambda vmid, port: {"vmid": vmid, "port": port, "source": "192.0.2.0/24"}
+        plan = {"bridges": {}, "renumbered": {}, "hold": False, "notes": [],
+                "restored": [{"vmid": 120, "firewall": rule(120, 61208)}, {"vmid": 121, "firewall": rule(121, 19999)}]}
+        added = []
+        with patch.object(recovery, "restore_host_files"), patch.object(recovery, "clean_format_directories"), \
+                patch.object(recovery, "register", return_value=[120, 121]), patch.object(recovery.carried, "carry"), \
+                patch.object(recovery, "add_firewall_rule", side_effect=lambda rule: added.append(rule["vmid"]) or True), \
+                patch.object(recovery, "msg_info"), patch.object(recovery, "msg_ok"):
+            recovery.recover(Path("/nonexistent"), plan, host_firewall=confirmed)
+        return added, plan["notes"]
+
+    def test_one_confirmed_rule_does_not_add_the_others(self):
+        added, notes = self.recover([121])
+        self.assertEqual(added, [121])
+        self.assertEqual([note[:7] for note in notes], ["CT 120:"])
+
+    def test_without_an_answer_no_rule_is_added(self):
+        added, notes = self.recover([])
+        self.assertEqual(added, [])
+        self.assertEqual(len(notes), 2)
+
+    def test_the_menu_asks_for_each_rule_and_passes_only_the_confirmed_ones(self):
+        sys.path.insert(0, str(ROOT / "src"))
+        from proxmenux_oci import management
+        rules = [{"vmid": 120, "port": 61208, "source": "192.0.2.0/24"},
+                 {"vmid": 121, "port": 19999, "source": "192.0.2.0/24"}]
+        answers = iter([True, False, True, False])  # recover, rule of 120, rule of 121, start
+
+        class Ui:
+            asked = []
+
+            def confirm(self, text, default=False):
+                self.asked.append(text)
+                return next(answers)
+
+        ui = Ui()
+        with patch.object(management, "restored_applications", return_value=[{"vmid": 120, "hostname": "glances"},
+                                                                             {"vmid": 121, "hostname": "netdata"}]), \
+                patch.object(management, "_restored_firewall_rules", return_value=rules), \
+                patch.object(management, "_run_lifecycle") as run:
+            management.offer_recovery(ROOT, ui)
+        command = run.call_args[0][0]
+        self.assertEqual(len(ui.asked), 4)
+        self.assertEqual(command[command.index("--host-firewall"):], ["--host-firewall", "121"])
+
+
 if __name__ == "__main__":
     unittest.main()

oci/tests/test_stage_release.py

@@ -54,8 +54,24 @@ class StageReleaseTests(unittest.TestCase):
                 patch.object(transaction, "stop"), \
                 patch.object(transaction, "run", side_effect=RuntimeError("pct failed with exit code 255")), \
                 patch.object(transaction, "log") as logged:
-            transaction.discard_stage(STATE)
+            removed = transaction.discard_stage(STATE)
         self.assertIn("pct failed", logged.call_args[0][0])
+        self.assertFalse(removed)
+
+    def test_the_result_says_whether_the_container_was_removed(self):
+        with patch.object(transaction, "owned", return_value=HOLDING), patch.object(transaction, "stop"), \
+                patch.object(transaction, "run"):
+            self.assertTrue(transaction.discard_stage(STATE))
+            self.assertFalse(transaction.release_stage(STATE))
+        with patch.object(transaction, "owned", side_effect=ValueError("not ours")):
+            self.assertFalse(transaction.discard_stage(STATE))
+        self.assertFalse(transaction.discard_stage({"id": "x"}))
+
+    def test_the_recovery_only_says_the_disks_were_removed_when_they_were(self):
+        source = (ROOT / "remote/oci_instance_transaction.py").read_text()
+        claim = source.index("The disks of the failed attempt were removed")
+        self.assertIn("if discard_stage(state):", source[source.rindex("\n", 0, claim - 80):claim])
+        self.assertIn("could not be removed and stay in the temporary container", source)
 
 
 if __name__ == "__main__":