Carry over OCI variables changed in Proxmox and keep the LXC update badge on old checks

.github/scripts/tests/test_oci_update_preservation_wording.py

@@ -3,6 +3,7 @@ import ast
 from contextlib import nullcontext
 import copy
 import json
+import os
 from pathlib import Path
 import re
 import runpy
@@ -129,6 +130,7 @@ class UpdateWording(unittest.TestCase):
         reconcile = ModuleType('oci_instance_reconcile')
         reconcile.propose = lambda *args: None
         scope = {'sys': SimpleNamespace(path=[], executable='python3'),
+                 'os': os, 'tempfile': tempfile, 'Path': Path,
                  'translate': lambda text: text, 'check_selected': lambda project, row: row,
                  '_selected_configuration': lambda vmid: instances.command('pct', 'config', str(vmid)),
                  'images': SimpleNamespace(offer_removal=lambda *args: None),

AppImage/components/virtual-machines.tsx

@@ -11,7 +11,7 @@ import { Badge } from "./ui/badge"
 import { Progress } from "./ui/progress"
 import { Button } from "./ui/button"
 import { Dialog, DialogContent, DialogHeader, DialogTitle, DialogFooter, DialogDescription } from "./ui/dialog"
-import { Server, Play, Square, Cpu, MemoryStick, HardDrive, Network, Power, RotateCcw, StopCircle, Container, ChevronDown, ChevronUp, ChevronRight, Terminal, Archive, Plus, PlusCircle, Loader2, Clock, Database, Shield, Bell, FileText, Settings2, Activity, Package, RefreshCw, EthernetPort, ArrowUpCircle, Info, CheckCircle2, EyeOff, Eye, Trash2, Check, X, AlertTriangle, AlertCircle, Search, Pin, Tag as TagIcon } from 'lucide-react'
+import { Server, Play, Square, Cpu, MemoryStick, HardDrive, Network, Power, RotateCcw, StopCircle, Container, ChevronDown, ChevronUp, ChevronRight, Terminal, Archive, Plus, PlusCircle, Loader2, Database, Shield, Bell, FileText, Settings2, Activity, Package, RefreshCw, EthernetPort, ArrowUpCircle, Info, CheckCircle2, EyeOff, Eye, Trash2, Check, X, AlertTriangle, AlertCircle, Search, Pin, Tag as TagIcon } from 'lucide-react'
 import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "./ui/select"
 import { Checkbox } from "./ui/checkbox"
 import { Switch } from "./ui/switch"
@@ -2863,8 +2863,7 @@ const handleDownloadLogs = async (vmid: number, vmName: string) => {
     onClick?: () => void,
   ) => {
     if (!uc) return null
-    const stale = uc.stale === true || (!!uc.last_check && isLxcUpdateStale(uc))
+    if (!uc.available || !uc.count || uc.count <= 0) return null
-    if (!stale && (!uc.available || !uc.count || uc.count <= 0)) return null
     const last = uc.last_check
       ? new Date(uc.last_check).toLocaleString()
       : "—"
@@ -2912,8 +2911,8 @@ const handleDownloadLogs = async (vmid: number, vmName: string) => {
             space — matters most on the LXC card row where uptime,
             IP and other chips compete for width. Count alone with
             the up-arrow reads unambiguously as "pending updates". */}
-        {stale ? <Clock className={iconSize} /> : <ArrowUpCircle className={iconSize} />}
+        <ArrowUpCircle className={iconSize} />
-        {stale ? t("vmLxc.updates.checkStale") : uc.count}
+        {uc.count}
         {/* Chevron only when the badge is wired up as a clickable
             shortcut — its absence on the dashboard card avoids
             implying interactivity where there isn't any (the whole

lang/es.json

@@ -2988,6 +2988,7 @@
   "Inaccessible device during VM startup": "Dispositivo inaccesible durante el inicio de la VM",
   "Inactive": "Inactivo",
   "Inactive (entry in fstab, not currently mounted)": "Inactivo (entrada en fstab, no montado actualmente)",
+  "Include these changes in oci-compose?": "¿Incluir estos cambios en el oci-compose?",
   "Include these resources in oci-compose?": "¿Incorporar estos recursos al oci-compose?",
   "Includes /etc/network (may drop SSH immediately)": "Incluye /etc/network (puede eliminar SSH inmediatamente)",
   "Includes /etc/zfs": "Incluye /etc/zfs",
@@ -6781,6 +6782,7 @@
   "These Proxmox resources were added outside ProxMenux:": "Se añadieron estos recursos en Proxmox fuera de ProxMenux:",
   "These VMIDs are not free:": "Estos VMID no están libres:",
   "These are the changes that will be made": "Estos son los cambios que se harán",
+  "These changes were made in Proxmox outside ProxMenux:": "Estos cambios se hicieron en Proxmox fuera de ProxMenux:",
   "These container mount paths have an extension-like suffix:": "Estas rutas de montaje del contenedor tienen un sufijo parecido a una extensión:",
   "These containers were restored from a backup and this host has no record of their OCI application:": "Estos contenedores se restauraron desde un backup y este host no tiene el registro de su aplicación OCI:",
   "These host backups run with this job and expect its storage:": "Estos backups del host se ejecutan con este trabajo y esperan su almacenamiento:",
@@ -6791,6 +6793,7 @@
   "These values are asked during the installation:": "Estos valores se preguntan durante la instalación:",
   "They are described in the image documentation:": "Están descritos en la documentación de la imagen:",
   "They will be added to oci-compose before continuing. Unsupported or changed resources are not imported.": "Se añadirán al oci-compose antes de continuar. No se importarán recursos incompatibles o modificados.",
+  "They will be saved in oci-compose before continuing.": "Se guardarán en el oci-compose antes de continuar.",
   "This CIFS share is mounted with restrictive permissions.": "Este recurso compartido CIFS está montado con permisos restrictivos.",
   "This GPU is considered incompatible with GPU passthrough to a VM in ProxMenux.": "Esta GPU se considera incompatible con el passthrough de GPU a una VM en ProxMenux.",
   "This NFS share is fully restricted — even the host root cannot write to it.": "Este recurso compartido NFS está completamente restringido: ni siquiera la raíz del host puede escribir en él.",
@@ -7644,6 +7647,7 @@
   "active VF(s)": "FV activas",
   "active VFs": "FV activas",
   "active Virtual Functions. Changing its driver binding would destroy every VF.": "Funciones virtuales activas. Cambiar la vinculación del controlador destruiría todos los VF.",
+  "added in Proxmox": "añadida en Proxmox",
   "after copy; source tree is incomplete.": "después de la copia; El árbol fuente está incompleto.",
   "after update.": "después de la actualización.",
   "all guests": "todas las VM y CT",
@@ -8010,6 +8014,7 @@
   "remapped users": "usuarios reasignados",
   "remove the (now-empty) directory if possible": "elimine el directorio (ahora vacío) si es posible",
   "removed from Proxmox": "eliminado de Proxmox",
+  "removed in Proxmox": "eliminada en Proxmox",
   "removed successfully from Proxmox.": "eliminado correctamente de Proxmox.",
   "requires running the official installer, which restarts the Monitor service.": "requiere ejecutar el instalador oficial, que reinicia el servicio del Monitor.",
   "requires the package": "requiere el paquete",
@@ -8113,6 +8118,7 @@
   "using": "usando",
   "using direct path (not guaranteed to survive reboots).": "utilizando una ruta directa (no se garantiza que sobreviva a los reinicios).",
   "vCPUs:": "vCPU:",
+  "value changed in Proxmox": "valor cambiado en Proxmox",
   "vfio-pci IDs configured": "ID de vfio-pci configurados",
   "vfio-pci IDs in /etc/modprobe.d/vfio.conf": "ID de vfio-pci en /etc/modprobe.d/vfio.conf",
   "video": "vídeo",

oci/remote/oci_instance_reconcile.py

@@ -9,6 +9,7 @@ import oci_instances as instances
 import oci_gpu_devices as gpu_devices
 import oci_host_mounts as host_mounts
 import oci_instance_transaction as transaction
+import oci_native_modify as native_modify
 from oci_installation_state import parse_config, sha
 from oci_ui import translate
 
@@ -88,6 +89,43 @@ def _device(key, value):
     return device
 
 
+ENVIRONMENT_KEYS = ('env', 'lxc.environment.runtime')
+
+
+def _environment(record, config):
+    """Variables changed in Proxmox since the last operation. Values are
+    applied but never listed: they may be passwords."""
+    before = native_modify.environment(record['observed']['config'].encode())
+    now = native_modify.environment(config)
+    defaults = {item.split('=', 1)[0] for item in record['observed']['image'].get('defaults', {}).get('Env', [])
+                if '=' in item}
+    removed = sorted(before.keys() - now.keys())
+    # An image default comes back with a new container; its absence cannot be kept.
+    if any(name in defaults for name in removed):
+        raise ValueError(f"{translate('These manual changes cannot be adopted safely:')} env")
+    changed = sorted(name for name in now if before.get(name) != now[name])
+    return {name: now[name] for name in changed}, removed
+
+
+def _adopt_environment(deployment, values, removed):
+    items = deployment.setdefault('environment', [])
+    details = []
+    for name, value in values.items():
+        matches = [item for item in items if item['name'] == name]
+        if len(matches) > 1:
+            raise ValueError(translate('Duplicate variable in the contract; review it before editing'))
+        if matches:
+            matches[0]['value'] = value
+            details.append(f"env {name}: {translate('value changed in Proxmox')}")
+        else:
+            items.append({'name': name, 'value': value, 'sensitive': True})
+            details.append(f"env {name}: {translate('added in Proxmox')}")
+    for name in removed:
+        items[:] = [item for item in items if item['name'] != name]
+        details.append(f"env {name}: {translate('removed in Proxmox')}")
+    return details
+
+
 def propose(record, config):
     if (record.get('status') != 'installed' or record.get('stack') or record.get('stack_member')
             or record.get('native_stack_intent') or record.get('deployment', {}).get('stack_managed')):
@@ -101,16 +139,20 @@ def propose(record, config):
     # was checked above, so a presentation-only change needs no adoption.
     if 'description' in changed:
         changed.remove('description')
+    environment = None
+    if any(key in ENVIRONMENT_KEYS for key in changed):
+        changed = [key for key in changed if key not in ENVIRONMENT_KEYS]
+        environment = _environment(record, config)
     new_keys = [key for key in changed if key not in before and re.fullmatch(r'(mp|dev)[0-9]+', key)]
     unsupported = [key for key in changed if key not in new_keys and key not in transaction.ADOPTABLE
                    and key not in transaction.KEPT_AS_IS]
     if unsupported:
         raise ValueError(f"{translate('These manual changes cannot be adopted safely:')} {', '.join(unsupported)}")
-    if not new_keys:
+    if not new_keys and environment is None:
         return None
     candidate = copy.deepcopy(record)
     deployment = candidate['deployment']
-    details = []
+    details = _adopt_environment(deployment, *environment) if environment else []
     for key in new_keys:
         if key.startswith('mp'):
             mount = _mount(key, current[key], record['vmid'])
@@ -122,8 +164,13 @@ def propose(record, config):
             deployment.setdefault('devices', []).append(device)
             kind = 'GPU' if gpu_devices.gpu_path(device['host_path']) else 'USB/Coral/NPU'
             details.append(f"{key}: {device['host_path']} ({kind})")
+    skipped = [*new_keys, *(ENVIRONMENT_KEYS if environment else ())]
     filtered = b'\n'.join(line for line in config.splitlines()
-                          if not any(line.startswith(key.encode() + b': ') for key in new_keys)) + b'\n'
+                          if not any(line.startswith(key.encode() + b': ') for key in skipped))
+    if environment:
+        filtered += b''.join(b'\n' + line for line in record['observed']['config'].encode().splitlines()
+                             if any(line.startswith(key.encode() + b': ') for key in ENVIRONMENT_KEYS))
+    filtered += b'\n'
     for key, value in transaction.external_changes(record, filtered).items():
         section, name = transaction.ADOPTABLE[key]
         target = deployment.setdefault(section, {}) if section else deployment

oci/remote/oci_update_current.py

@@ -260,6 +260,9 @@ def update(vmid, acknowledge_external_data=False, proposal=None, keep_backup=Non
             transaction.apply(instances.ROOT, vmid, archive, operation, proposal=proposal,
                               registry_digest=digest, acknowledge_external_data=acknowledge_external_data,
                               keep_backup=file_storage)
+    marker = os.environ.get('OCI_RECREATED_MARKER')
+    if marker:
+        Path(marker).touch()
 
 
 def main():

oci/src/proxmenux_oci/management.py

@@ -441,11 +441,12 @@ def manage_instance(project, ui, row, action=None, lifecycle_args=()):
             return False
         if adoption:
             summary = '\n'.join(adoption['details'])
-            if not ui.review(
+            # Variables reordered in Proxmox change nothing that needs a decision.
-                    f"{translate('These Proxmox resources were added outside ProxMenux:')}\n\n{summary}\n\n"
+            if summary and not ui.review(
-                    + translate('They will be added to oci-compose before continuing. Unsupported or changed resources are not imported.'),
+                    f"{translate('These changes were made in Proxmox outside ProxMenux:')}\n\n{summary}\n\n"
+                    + translate('They will be saved in oci-compose before continuing.'),
                     translate('Review external OCI changes'),
-                    question=translate('Include these resources in oci-compose?'), default=False):
+                    question=translate('Include these changes in oci-compose?'), default=False):
                 return False
             try:
                 record = reconcile.commit(instances.ROOT, row['vmid'], adoption)
@@ -472,6 +473,12 @@ def manage_instance(project, ui, row, action=None, lifecycle_args=()):
         if '--acknowledge-external-data' not in command:
             command.append('--acknowledge-external-data')
         title = translate('Modify OCI') if proposal else translate('Update OCI')
+        # The downloaded image is only worth mentioning when a new container
+        # was created from it, not after a change applied to the running one.
+        with tempfile.TemporaryDirectory() as folder:
+            marker = Path(folder) / 'recreated'
+            os.environ['OCI_RECREATED_MARKER'] = str(marker)
+            try:
                 if proposal is None:
                     completed = _run_lifecycle(command, title)
                 else:
@@ -480,7 +487,10 @@ def manage_instance(project, ui, row, action=None, lifecycle_args=()):
                         json.dump(proposal, file)
                         file.flush()
                         completed = _run_lifecycle(command + ['--proposal', file.name], title)
-        if completed and not getattr(ui, 'unattended', False):
+            finally:
+                os.environ.pop('OCI_RECREATED_MARKER', None)
+            recreated = marker.exists()
+        if completed and recreated and not getattr(ui, 'unattended', False):
             images.offer_removal(ui, [row['vmid']])
         return completed
     action = ui.choose(translate('Interrupted operation'), [('status', translate('View status')),

oci/tests/test_instance_reconcile.py

@@ -155,5 +155,66 @@ class ProposalTests(unittest.TestCase):
             self.assertEqual(json.loads(history[0].read_text())['deployment']['mounts'], [])
 
 
+class EnvironmentAdoptionTests(unittest.TestCase):
+    marker = '11111111-1111-1111-1111-111111111111'
+
+    def record(self, env):
+        config = f'description: proxmenux-instance={self.marker}\nenv: {env}\n'.encode()
+        return {
+            'vmid': 200, 'status': 'installed', 'installation_id': self.marker,
+            'deployment': {'mounts': [], 'devices': [], 'environment': [
+                {'name': 'APP_PASSWORD', 'value': 'first-secret', 'sensitive': True}]},
+            'observed': {'config': config.decode(), 'config_sha256': sha(config),
+                         'archive_path': '/unused.tar', 'resolved_registry_digest': 'sha256:test',
+                         'image': {'manifest_digest': 'sha256:test', 'defaults': {'Env': ['PATH=/bin']}}},
+        }
+
+    def propose(self, record, env):
+        config = f'description: proxmenux-instance={self.marker}\nenv: {env}\n'.encode()
+        with (patch.object(reconcile.instances, 'ROOT', Path('/inert')),
+              patch.object(reconcile.instances, 'identity', return_value=self.marker),
+              patch.object(reconcile.instances, 'location', return_value=Path(__file__)),
+              patch.object(reconcile.gpu_devices, 'capture', return_value={}),
+              patch.object(reconcile.host_mounts, 'capture_sources', return_value={}),
+              patch.object(reconcile.transaction, 'preflight')):
+            return reconcile.propose(record, config)
+
+    def test_a_password_changed_in_proxmox_is_adopted_without_showing_it(self):
+        record = self.record('PATH=/bin\0APP_PASSWORD=first-secret')
+        proposal = self.propose(record, 'PATH=/bin\0APP_PASSWORD=my-own-secret\0TZ=Europe/Madrid')
+        environment = proposal['candidate']['deployment']['environment']
+        self.assertEqual(environment, [
+            {'name': 'APP_PASSWORD', 'value': 'my-own-secret', 'sensitive': True},
+            {'name': 'TZ', 'value': 'Europe/Madrid', 'sensitive': True}])
+        self.assertEqual(proposal['details'], ['env APP_PASSWORD: value changed in Proxmox',
+                                               'env TZ: added in Proxmox'])
+        self.assertNotIn('my-own-secret', str(proposal['details']))
+        self.assertEqual(record['deployment']['environment'][0]['value'], 'first-secret')
+
+    def test_reordered_variables_need_no_decision(self):
+        record = self.record('PATH=/bin\0APP_PASSWORD=first-secret')
+        proposal = self.propose(record, 'APP_PASSWORD=first-secret\0PATH=/bin')
+        self.assertEqual(proposal['details'], [])
+
+    def test_removing_an_image_default_is_refused(self):
+        record = self.record('PATH=/bin\0APP_PASSWORD=first-secret')
+        with self.assertRaisesRegex(ValueError, 'env'):
+            self.propose(record, 'APP_PASSWORD=first-secret')
+
+    def test_removing_a_saved_variable_removes_it_from_the_contract(self):
+        record = self.record('PATH=/bin\0APP_PASSWORD=first-secret')
+        proposal = self.propose(record, 'PATH=/bin')
+        self.assertEqual(proposal['candidate']['deployment']['environment'], [])
+        self.assertEqual(proposal['details'], ['env APP_PASSWORD: removed in Proxmox'])
+
+    def test_other_changes_next_to_the_variables_are_still_refused(self):
+        record = self.record('PATH=/bin')
+        config = (f'description: proxmenux-instance={self.marker}\nenv: PATH=/bin\0A=1\n'
+                  'net0: name=eth0,bridge=vmbr1\n').encode()
+        with (patch.object(reconcile.instances, 'identity', return_value=self.marker),
+              self.assertRaisesRegex(ValueError, 'net0')):
+            reconcile.propose(record, config)
+
+
 if __name__ == '__main__':
     unittest.main()

oci/tests/test_modify_menu.py

@@ -169,12 +169,13 @@ class ModifyMenuTests(unittest.TestCase):
 
 
 class ModifyDispatchTests(unittest.TestCase):
-    def call_manager(self, proposal, lifecycle):
+    def call_manager(self, proposal, lifecycle, ui=None, offer=None):
         import oci_instances
         import oci_instance_reconcile
         saved = record()
         row = {'vmid': 115, 'reason': 'matched', 'stack': False, 'pending': False, 'status': 'installed'}
-        with patch.object(management, 'carry_records', return_value={}), \
+        with patch.object(management.images, 'offer_removal', offer or Mock()), \
+                patch.object(management, 'carry_records', return_value={}), \
                 patch.object(management, 'check_selected', return_value=row), \
                 patch.object(management, '_work_backup_ready', return_value=True), \
                 patch.object(oci_instances, 'read', return_value=saved), \
@@ -182,7 +183,7 @@ class ModifyDispatchTests(unittest.TestCase):
                 patch.object(oci_instance_reconcile, 'propose', return_value=None), \
                 patch.object(recreation, 'edit_recreation', return_value=proposal), \
                 patch.object(management, '_run_lifecycle', side_effect=lifecycle):
-            return management.manage_instance(ROOT, Mock(), row, action='modify')
+            return management.manage_instance(ROOT, ui or Mock(), row, action='modify')
 
     def test_cancel_does_not_start_a_lifecycle_transaction(self):
         lifecycle = Mock()
@@ -203,6 +204,20 @@ class ModifyDispatchTests(unittest.TestCase):
         self.assertTrue(self.call_manager(proposal, lifecycle))
         self.assertEqual(seen, [proposal])
 
+    def test_the_image_question_only_follows_a_recreated_container(self):
+        proposal = {'operation': 'recreate', 'candidate': record(), 'base_config_sha256': 'fingerprint'}
+        ui = Mock(unattended=False)
+        for recreated in (False, True):
+            with self.subTest(recreated=recreated):
+                def lifecycle(command, title):
+                    if recreated:
+                        Path(management.os.environ['OCI_RECREATED_MARKER']).touch()
+                    return True
+                offer = Mock()
+                self.assertTrue(self.call_manager(proposal, lifecycle, ui, offer))
+                self.assertEqual(offer.called, recreated)
+                self.assertNotIn('OCI_RECREATED_MARKER', management.os.environ)
+
 
 if __name__ == '__main__':
     unittest.main()

tests/lxc_updates/test_observed_state_ui.cjs

@@ -162,8 +162,10 @@ assert.equal(ui.osStatus({ ...recent, available: false, count: 0 }).osUpdateStat
 assert.equal(ui.osStatus({ ...recent, error: "probe failed" }).osUpdateStatusKnown, false)
 assert.equal(ui.aggregate({ app_watches: [{ update_available: true, name: "App" }] }).last_check, null,
   "an app badge cannot invent an OS check timestamp")
-assert.equal(ui.badge(legacy).props.children[0].type, "Clock")
+assert.equal(ui.badge(legacy).props.children[0].type, "Arrow",
-assert.equal(ui.badge(legacy).props.children[1], "vmLxc.updates.checkStale")
+  "an old check keeps the same badge as a recent one")
+assert.equal(ui.badge(legacy).props.children[1], 51)
+assert.equal(ui.badge({ ...legacy, available: false, count: 0 }), null)
 assert.equal(ui.badge({ ...recent, available: true, count: 2 }).props.children[1], 2)
 assert.equal(ui.badge({ ...recent, available: false, count: 0 }), null)
 assert.equal(requests.length, 0)