From e3c9162982b027a3a7274ca48679f14b8c678f6f Mon Sep 17 00:00:00 2001 From: MacRimi Date: Sun, 18 Oct 2026 11:40:17 +0200 Subject: [PATCH] Carry over OCI variables changed in Proxmox and keep the LXC update badge on old checks --- .../test_oci_update_preservation_wording.py | 2 + AppImage/components/virtual-machines.tsx | 9 ++- lang/es.json | 6 ++ oci/remote/oci_instance_reconcile.py | 53 +++++++++++++++- oci/remote/oci_update_current.py | 3 + oci/src/proxmenux_oci/management.py | 36 +++++++---- oci/tests/test_instance_reconcile.py | 61 +++++++++++++++++++ oci/tests/test_modify_menu.py | 21 ++++++- tests/lxc_updates/test_observed_state_ui.cjs | 6 +- 9 files changed, 171 insertions(+), 26 deletions(-) diff --git a/.github/scripts/tests/test_oci_update_preservation_wording.py b/.github/scripts/tests/test_oci_update_preservation_wording.py index 477196e9..e3fdae92 100644 --- a/.github/scripts/tests/test_oci_update_preservation_wording.py +++ b/.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), diff --git a/AppImage/components/virtual-machines.tsx b/AppImage/components/virtual-machines.tsx index a538627a..1bf5b78c 100644 --- a/AppImage/components/virtual-machines.tsx +++ b/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 (!stale && (!uc.available || !uc.count || uc.count <= 0)) return null + if (!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 ? : } - {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 diff --git a/lang/es.json b/lang/es.json index 077ac4c6..b54e843c 100644 --- a/lang/es.json +++ b/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", diff --git a/oci/remote/oci_instance_reconcile.py b/oci/remote/oci_instance_reconcile.py index 07291125..cd8632a7 100644 --- a/oci/remote/oci_instance_reconcile.py +++ b/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 diff --git a/oci/remote/oci_update_current.py b/oci/remote/oci_update_current.py index b57f7049..f4f94038 100644 --- a/oci/remote/oci_update_current.py +++ b/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(): diff --git a/oci/src/proxmenux_oci/management.py b/oci/src/proxmenux_oci/management.py index 27ee09ad..2744032d 100644 --- a/oci/src/proxmenux_oci/management.py +++ b/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( - f"{translate('These Proxmox resources were added outside ProxMenux:')}\n\n{summary}\n\n" - + translate('They will be added to oci-compose before continuing. Unsupported or changed resources are not imported.'), + # Variables reordered in Proxmox change nothing that needs a decision. + if summary and not ui.review( + 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,15 +473,24 @@ 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') - if proposal is None: - completed = _run_lifecycle(command, title) - else: - # Saved environment/secrets must never be passed on the command line. - with tempfile.NamedTemporaryFile(mode='w', suffix='.json') as file: - json.dump(proposal, file) - file.flush() - completed = _run_lifecycle(command + ['--proposal', file.name], title) - if completed and not getattr(ui, 'unattended', False): + # 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: + # Saved environment/secrets must never be passed on the command line. + with tempfile.NamedTemporaryFile(mode='w', suffix='.json') as file: + json.dump(proposal, file) + file.flush() + completed = _run_lifecycle(command + ['--proposal', file.name], title) + 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')), diff --git a/oci/tests/test_instance_reconcile.py b/oci/tests/test_instance_reconcile.py index b0fb25a1..bd9da868 100644 --- a/oci/tests/test_instance_reconcile.py +++ b/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() diff --git a/oci/tests/test_modify_menu.py b/oci/tests/test_modify_menu.py index 601c588e..0f80a0a7 100644 --- a/oci/tests/test_modify_menu.py +++ b/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() diff --git a/tests/lxc_updates/test_observed_state_ui.cjs b/tests/lxc_updates/test_observed_state_ui.cjs index 7b659bed..4f2de690 100644 --- a/tests/lxc_updates/test_observed_state_ui.cjs +++ b/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[1], "vmLxc.updates.checkStale") +assert.equal(ui.badge(legacy).props.children[0].type, "Arrow", + "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)