diff --git a/.github/scripts/tests/test_oci_recovery_inventory_wording.py b/.github/scripts/tests/test_oci_recovery_inventory_wording.py index 77e7afaa..338698ae 100644 --- a/.github/scripts/tests/test_oci_recovery_inventory_wording.py +++ b/.github/scripts/tests/test_oci_recovery_inventory_wording.py @@ -5,6 +5,7 @@ import json from pathlib import Path import re import runpy +import subprocess import sys import tempfile from types import ModuleType, SimpleNamespace @@ -52,7 +53,8 @@ class InventoryMessages(unittest.TestCase): state = ModuleType('oci_installation_state') state.parse_config = lambda raw: dict(line.split(': ', 1) for line in raw.decode().splitlines()) scope = {'sys': SimpleNamespace(path=[]), 'source_text': lambda text: text or '', - 'translate': translate, 're': re} + 'translate': translate, 're': re, 'subprocess': subprocess, + '_selected_configuration': remover.guest_config} summary = extracted(MENU, '_removal_summary', scope) with patch.dict(sys.modules, {'oci_instances': instances, 'oci_remove': remover, 'oci_installation_state': state}): diff --git a/.github/scripts/tests/test_oci_removal_firewall_wording.py b/.github/scripts/tests/test_oci_removal_firewall_wording.py index bde90937..8041f923 100644 --- a/.github/scripts/tests/test_oci_removal_firewall_wording.py +++ b/.github/scripts/tests/test_oci_removal_firewall_wording.py @@ -56,7 +56,8 @@ class RemovalWordings(unittest.TestCase): state = ModuleType('oci_installation_state') state.parse_config = lambda raw: {} scope = {'sys': SimpleNamespace(path=[]), 'source_text': lambda value: value or '', - 'translate': translated or (lambda value: value), 're': re} + 'translate': translated or (lambda value: value), 're': re, + '_selected_configuration': remover.guest_config, 'subprocess': subprocess} with patch.dict(sys.modules, {'oci_instances': instances, 'oci_remove': remover, 'oci_installation_state': state}): return extract(MENU, '_removal_summary', scope)(Path('/inert'), 101) @@ -87,7 +88,8 @@ class RemovalWordings(unittest.TestCase): with patch.dict(sys.modules, {'oci_instances': instances, 'oci_remove': remover, 'oci_installation_state': state}): scope = {'sys': SimpleNamespace(path=[]), 'source_text': lambda value: value or '', - 'translate': lambda value: value, 're': re} + 'translate': lambda value: value, 're': re, + '_selected_configuration': remover.guest_config, 'subprocess': subprocess} preview = extract(MENU, '_removal_summary', scope)(Path('/inert'), 102) self.assertIn(WHOLE, preview) diff --git a/.github/scripts/tests/test_oci_update_preservation_wording.py b/.github/scripts/tests/test_oci_update_preservation_wording.py index 44146970..477196e9 100644 --- a/.github/scripts/tests/test_oci_update_preservation_wording.py +++ b/.github/scripts/tests/test_oci_update_preservation_wording.py @@ -130,6 +130,7 @@ class UpdateWording(unittest.TestCase): reconcile.propose = lambda *args: None scope = {'sys': SimpleNamespace(path=[], executable='python3'), '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), 'carry_records': lambda project, **options: {}, '_work_backup_ready': lambda project, ui, vmid: True, diff --git a/.github/workflows/test-oci-registries.yml b/.github/workflows/test-oci-registries.yml index a3edecdf..a04ea91a 100644 --- a/.github/workflows/test-oci-registries.yml +++ b/.github/workflows/test-oci-registries.yml @@ -45,6 +45,15 @@ jobs: env: PYTHONDONTWRITEBYTECODE: '1' run: python3 -m unittest discover -s oci/tests -p 'test_registry_*.py' -v + - name: Run OCI Modify latency regressions + env: + PYTHONDONTWRITEBYTECODE: '1' + run: | + python3 -m unittest discover -s oci/tests -p test_modify_latency.py -v + python3 -m unittest discover -s oci/tests -p test_menu_performance.py -v + python3 -m unittest discover -s oci/tests -p test_dialog_escape.py -v + python3 -m unittest discover -s oci/tests -p test_work_backup.py -v + python3 -m unittest discover -s oci/tests -p test_recreation.py -v - name: Run OCI modification menu regressions env: PYTHONDONTWRITEBYTECODE: '1' diff --git a/oci/remote/oci_work_backup.py b/oci/remote/oci_work_backup.py index b37a2c26..251ffe71 100644 --- a/oci/remote/oci_work_backup.py +++ b/oci/remote/oci_work_backup.py @@ -192,21 +192,25 @@ def application(root, vmid): return primary_id, members or [vmid] -def status(root, vmid): +def status(root, vmid, only_needed=False): """What the menu needs to know before an operation: whether the backup fits on the system disk, the storage chosen earlier and the ones that - have room.""" + have room. The menu can skip unused alternatives once a destination fits; + the default retains the complete status used by the maintenance CLI.""" primary_id, vmids = application(root, vmid) record = instances.read(root, primary_id) default = instances.location(root, primary_id).parent needed, free = required(vmids), available(default) saved = (record.get('deployment') or {}).get(KEY) - path = storage_path(saved) if saved else None + fits = free >= needed + path = storage_path(saved) if saved and not (only_needed and fits) else None saved_free = available(path) if path is not None and path.is_dir() else None - return {'needed': needed, 'free': free, 'fits': free >= needed, 'saved': saved, - 'saved_fits': saved_free is not None and saved_free >= needed, + saved_fits = saved_free is not None and saved_free >= needed + candidates = [] if only_needed and (fits or saved_fits) else file_storages() + return {'needed': needed, 'free': free, 'fits': fits, 'saved': saved, + 'saved_fits': saved_fits, 'candidates': [{'storage': item['storage'], 'free': item['free']} - for item in file_storages() if item['free'] >= needed]} + for item in candidates if item['free'] >= needed]} def choose(root, vmid, storage): diff --git a/oci/src/proxmenux_oci/management.py b/oci/src/proxmenux_oci/management.py index 11007edd..27ee09ad 100644 --- a/oci/src/proxmenux_oci/management.py +++ b/oci/src/proxmenux_oci/management.py @@ -106,13 +106,32 @@ def _config_hostname(vmid): return '' +def _selected_configuration(vmid): + """Reuse pct's output only while its complete native source is unchanged. + + The forced carried-record verification has just fetched a fresh config. + Older installations without that source-keyed cache still query pct. + The lifecycle backend independently validates again before any mutation. + """ + import oci_instances as instances + try: + import oci_carried_record + reader = getattr(oci_carried_record, 'configuration', None) + config = reader(instances.ROOT, vmid) if reader else None + if isinstance(config, str) and config: + return config.encode() + except (ImportError, OSError, ValueError, RuntimeError, subprocess.SubprocessError): + pass + return instances.command('pct', 'config', str(vmid)) + + def check_selected(project, row): """Validate only the chosen container; lifecycle backends validate its stack.""" sys.path.insert(0, str(project / 'remote')) import oci_instances as instances with instances.locked(instances.ROOT): record = instances.read(instances.ROOT, row['vmid']) - config = instances.command('pct', 'config', str(row['vmid'])) + config = _selected_configuration(row['vmid']) marker = instances.identity(config) reason = 'matched' if marker == record['installation_id'] else 'identity-unconfirmed' return public_row(record, {'reason': reason}) @@ -328,7 +347,7 @@ def _work_backup_ready(project, ui, vmid): import oci_instances as instances import oci_work_backup try: - state = oci_work_backup.status(instances.ROOT, vmid) + state = oci_work_backup.status(instances.ROOT, vmid, only_needed=True) except (OSError, ValueError, KeyError, RuntimeError, subprocess.SubprocessError): # What cannot be measured here is told by the engine when it stops. return True @@ -412,12 +431,10 @@ def manage_instance(project, ui, row, action=None, lifecycle_args=()): return _toggle_watchdog(project, ui, row['vmid'], watched) if action == 'remove': return _remove(project, ui, row['vmid']) - if not _work_backup_ready(project, ui, row['vmid']): - return False record = instances.read(instances.ROOT, row['vmid']) import oci_instance_reconcile as reconcile try: - current_config = instances.command('pct', 'config', str(row['vmid'])) + current_config = _selected_configuration(row['vmid']) adoption = reconcile.propose(record, current_config) except (OSError, ValueError, RuntimeError) as error: ui.message(str(error), translate('Review external OCI changes')) @@ -444,6 +461,10 @@ def manage_instance(project, ui, row, action=None, lifecycle_args=()): elif not ui.review(translate('If the image channel has a new version, the CT is stopped, backed up and verified, and replaced with the new image to update the container. If the update fails after the container was changed, the backup is restored. An update that stops halfway is recovered from the OCI management menu.'), translate('Update OCI'), question=translate('Update now?'), default=True): return False + # Opening/cancelling a confirmation needs no disk or storage probes. + # Both accepted operations retain this check before the backend starts. + if not _work_backup_ready(project, ui, row['vmid']): + return False command = [sys.executable, str(project / 'remote/oci_update_current.py'), str(row['vmid']), *lifecycle_args] # Host directories are configured by the user, who knows they are @@ -493,7 +514,10 @@ def _removal_summary(project, vmid): record = instances.read(instances.ROOT, member) except (OSError, ValueError, KeyError): record = {} - config = oci_remove.guest_config(member) + try: + config = _selected_configuration(member) + except (OSError, RuntimeError, subprocess.SubprocessError): + config = None cfg = parse_config(config) if config else {} name = (cfg.get('hostname') or (record.get('stack_member') or {}).get('name') or record.get('deployment', {}).get('hostname') or '') diff --git a/oci/src/proxmenux_oci/ui.py b/oci/src/proxmenux_oci/ui.py index 8238d3c2..6ff5955f 100644 --- a/oci/src/proxmenux_oci/ui.py +++ b/oci/src/proxmenux_oci/ui.py @@ -221,6 +221,10 @@ class DialogUI: def _run(self, widget: list[str], title: str | None = None) -> subprocess.CompletedProcess[str]: environment = os.environ.copy() + # ncurses otherwise waits a full second to distinguish Escape from + # the prefix of a cursor/function key. Keep a short sequence window, + # while respecting an explicit terminal-specific setting. + environment.setdefault("ESCDELAY", "150") if environment.get("TERM", "").casefold() in {"", "dumb", "unknown"}: environment["TERM"] = "xterm-256color" # dialog draws on the terminal and writes the selection to stderr. diff --git a/oci/tests/test_dialog_escape.py b/oci/tests/test_dialog_escape.py new file mode 100644 index 00000000..f5063878 --- /dev/null +++ b/oci/tests/test_dialog_escape.py @@ -0,0 +1,34 @@ +"""Escape delay is scoped to dialog and preserves terminal-specific overrides.""" +import os +from pathlib import Path +import subprocess +import sys +import unittest +from unittest.mock import patch + +sys.path.insert(0, str(Path(__file__).resolve().parents[1] / 'src')) +from proxmenux_oci.ui import DialogUI, UserCancelled + + +class DialogEscapeTests(unittest.TestCase): + def test_escape_uses_short_delay_without_changing_parent_environment(self): + with patch.dict(os.environ, {'TERM': 'xterm'}, clear=True), \ + patch('proxmenux_oci.ui.subprocess.run', return_value=subprocess.CompletedProcess([], 255, stderr='')) as run: + self.assertIsNone(DialogUI().choose('Menu', [('1', 'First')])) + self.assertEqual(run.call_args.kwargs['env']['ESCDELAY'], '150') + self.assertNotIn('ESCDELAY', os.environ) + + def test_explicit_delay_is_preserved(self): + with patch.dict(os.environ, {'ESCDELAY': '900'}), \ + patch('proxmenux_oci.ui.subprocess.run', return_value=subprocess.CompletedProcess([], 0, stderr='1')) as run: + self.assertEqual(DialogUI().choose('Menu', [('1', 'First')]), '1') + self.assertEqual(run.call_args.kwargs['env']['ESCDELAY'], '900') + + def test_escape_in_search_input_keeps_cancellation_behavior(self): + with patch('proxmenux_oci.ui.subprocess.run', return_value=subprocess.CompletedProcess([], 255, stderr='')): + with self.assertRaises(UserCancelled): + DialogUI().ask('Search') + + +if __name__ == '__main__': + unittest.main() diff --git a/oci/tests/test_menu_performance.py b/oci/tests/test_menu_performance.py new file mode 100644 index 00000000..94981ceb --- /dev/null +++ b/oci/tests/test_menu_performance.py @@ -0,0 +1,109 @@ +"""Menu speedups avoid unused probes, retaining confirmation and capacity checks.""" +from pathlib import Path +import sys +import tempfile +import unittest +from unittest.mock import Mock,patch +ROOT=Path(__file__).resolve().parents[1] +sys.path[:0]=[str(ROOT/'src'),str(ROOT/'remote')] +from proxmenux_oci import management as menu +import oci_instances as instances +import oci_instance_reconcile as reconcile +import oci_remove as remover +import oci_work_backup as backup + + +class UpdateEntryTests(unittest.TestCase): + def run_update(self,approved,ready): + events=[] + record={'status':'installed','vmid':110,'deployment':{},'template':{}} + row={'vmid':110,'reason':'matched','stack':False,'pending':False,'status':'installed'} + ui=Mock();ui.unattended=True + ui.review.side_effect=lambda *a,**kw:events.append('review') or approved + with patch.object(menu,'carry_records',return_value={}) as carry, \ + patch.object(menu,'check_selected',return_value=row), \ + patch.object(menu,'_selected_configuration',return_value=b'current config'), \ + patch.object(instances,'read',return_value=record),patch.object(reconcile,'propose',return_value=None), \ + patch.object(menu,'_work_backup_ready',side_effect=lambda *a:events.append('backup') or ready), \ + patch.object(menu,'_run_lifecycle',side_effect=lambda *a:events.append('lifecycle') or True): + result=menu.manage_instance(ROOT,ui,row,action='update') + carry.assert_called_once_with(ROOT,vmids=[110],verify=True) + return result,events + + def test_declining_update_does_not_probe_disks_or_storages(self): + result,events=self.run_update(False,False) + self.assertFalse(result);self.assertEqual(events,['review']) + + def test_accepted_update_checks_backup_space_before_starting(self): + result,events=self.run_update(True,True) + self.assertTrue(result);self.assertEqual(events,['review','backup','lifecycle']) + + def test_insufficient_space_still_prevents_the_update(self): + result,events=self.run_update(True,False) + self.assertFalse(result);self.assertEqual(events,['review','backup']) + + +class BackupDestinationTests(unittest.TestCase): + def status(self,local_free,saved_free=None,only_needed=True): + with tempfile.TemporaryDirectory() as directory: + root=Path(directory);saved= root/'nas';saved.mkdir() + record={'deployment':{backup.KEY:'nas'}} + with patch.object(backup,'application',return_value=(110,[110,111])), \ + patch.object(instances,'read',return_value=record), \ + patch.object(backup,'required',return_value=100) as required, \ + patch.object(backup,'available',side_effect=lambda p:saved_free if Path(p)==saved else local_free), \ + patch.object(backup,'storage_path',return_value=saved if saved_free is not None else None) as path, \ + patch.object(backup,'file_storages',return_value=[{'storage':'enough','free':200},{'storage':'small','free':50}]) as alternatives: + result=backup.status(root,110,only_needed=only_needed) + required.assert_called_once_with([110,111]) + return result,path.call_count,alternatives.call_count + + def test_local_room_avoids_saved_storage_and_alternative_queries(self): + result,saved,alternatives=self.status(200,saved_free=1000) + self.assertTrue(result['fits']);self.assertEqual((saved,alternatives),(0,0)) + + def test_saved_destination_that_fits_avoids_alternative_queries(self): + result,saved,alternatives=self.status(50,saved_free=200) + self.assertTrue(result['saved_fits']);self.assertEqual((saved,alternatives),(1,0)) + + def test_when_neither_fits_all_available_destinations_are_checked(self): + result,saved,alternatives=self.status(50,saved_free=40) + self.assertFalse(result['fits']);self.assertFalse(result['saved_fits']) + self.assertEqual(result['candidates'],[{'storage':'enough','free':200}]) + self.assertEqual(alternatives,1) + + def test_missing_saved_storage_still_offers_alternatives(self): + result,saved,alternatives=self.status(50) + self.assertEqual(result['candidates'],[{'storage':'enough','free':200}]) + self.assertEqual(alternatives,1) + + def test_complete_status_keeps_legacy_fields_and_all_candidates(self): + result,saved,alternatives=self.status(200,saved_free=300,only_needed=False) + self.assertTrue(result['fits']);self.assertTrue(result['saved_fits']) + self.assertEqual((saved,alternatives),(1,1)) + self.assertEqual(result['candidates'],[{'storage':'enough','free':200}]) + + +class RemovalPreviewTests(unittest.TestCase): + def preview(self,reader): + record={'vmid':110,'deployment':{},'template':{'catalog_ui':{'title':'Example'}}} + with patch.object(remover,'members_of',return_value=(110,record,[110])), \ + patch.object(instances,'read',return_value=record), \ + patch.object(menu,'_selected_configuration',side_effect=reader), \ + patch.object(remover,'guest_config',side_effect=AssertionError('Duplicate pct process')), \ + patch.object(remover,'host_directories',return_value=[]), \ + patch.object(remover,'private_bridge',return_value=None): + return menu._removal_summary(ROOT,110) + + def test_preview_reuses_verified_config_and_keeps_volume_and_host_path_details(self): + config=b'hostname: example\nrootfs: local:subvol-110-disk-0,size=8G\nmp0: /mnt/media,mp=/media,backup=0\n' + text=self.preview(lambda v:config) + self.assertIn('8G',text);self.assertIn('/mnt/media',text) + self.assertIn('not targeted for removal',text) + + def test_missing_guest_can_still_be_reviewed_without_a_second_probe(self): + text=self.preview(lambda v:(_ for _ in ()).throw(RuntimeError('guest missing'))) + self.assertIn('CT 110',text);self.assertIn('rootfs -',text) + + +if __name__=='__main__':unittest.main() diff --git a/oci/tests/test_modify_latency.py b/oci/tests/test_modify_latency.py new file mode 100644 index 00000000..f08359a5 --- /dev/null +++ b/oci/tests/test_modify_latency.py @@ -0,0 +1,103 @@ +"""Opening Modify avoids backup probes while preserving operation safeguards.""" +from pathlib import Path +import sys +import unittest +from unittest.mock import Mock,patch +ROOT=Path(__file__).resolve().parents[1] +sys.path[:0]=[str(ROOT/'src'),str(ROOT/'remote')] +from proxmenux_oci import management as menu,recreation +import oci_carried_record as carried +import oci_instances as instances +import oci_instance_reconcile as reconcile +from proxmenux_oci.ui import UserCancelled + + +def record(): + return {'vmid': 115, 'installation_id': 'test-installation', 'status': 'installed', + 'observed': {'config_sha256': 'native-config-fingerprint'}, + 'template': {'id': 'example-app', 'catalog_ui': {'title': 'Example'}, + 'container_contract': {'image': {'reference': 'example/app:latest'}}, + 'proxmox': {'installer_profile': {}}}, + 'deployment': {'vmid': 115, 'hostname': 'example', 'resources': {'cores': 2, 'memory_mb': 512}, + 'network': {'bridge': 'vmbr0', 'ipv4': 'dhcp', 'gateway': None}, + 'rootfs': {'storage': 'local-zfs', 'size_gb': 8}, 'mounts': [ + {'container_path': '/data', 'source': 'local-zfs', + 'type': 'managed-volume', 'size_gb': 8, 'backup': True}], + 'devices': [], 'security': {'unprivileged': True}, + 'environment': [{'name': 'PASSWORD', 'value': 'private-test-secret', + 'sensitive': True}]}} + + +class SelectedConfigTests(unittest.TestCase): + def test_valid_source_keyed_cache_avoids_another_pct_process(self): + with patch.object(carried,'configuration',create=True,return_value='description: current\n') as reader, \ + patch.object(instances,'command') as command: + self.assertEqual(menu._selected_configuration(115),b'description: current\n') + reader.assert_called_once_with(instances.ROOT,115) + command.assert_not_called() + + def test_old_or_unavailable_cache_falls_back_to_pct(self): + for reader in (None,Mock(return_value=None),Mock(return_value={}),Mock(side_effect=OSError('cache unavailable'))): + with self.subTest(reader=reader),patch.object(carried,'configuration',reader,create=True), \ + patch.object(instances,'command',return_value=b'live config') as command: + self.assertEqual(menu._selected_configuration(115),b'live config') + command.assert_called_once_with('pct','config','115') + + def test_identity_check_is_still_performed_on_the_selected_config(self): + r=record();r['installation_id']='11111111-1111-4111-8111-111111111111' + import contextlib + with patch.object(instances,'locked',return_value=contextlib.nullcontext()), \ + patch.object(instances,'read',return_value=r), \ + patch.object(menu,'_selected_configuration',return_value=b'description: another container\n'): + self.assertEqual(menu.check_selected(ROOT,{'vmid':115})['reason'],'identity-unconfirmed') + + +class EditorLatencyTests(unittest.TestCase): + def run_editor(self,proposal=None,backup_ready=True,stale=False,adoption_error=False,approved=True,declined=False): + events=[];saved=record() + row={'vmid':115,'reason':'matched','stack':False,'pending':False,'status':'installed'} + def edit(*a,**kw): + events.append('editor') + if declined:return None + if proposal is None:raise UserCancelled() + return proposal + def backup(*a,**kw):events.append('backup');return backup_ready + ui=Mock();ui.unattended=True;ui.review.return_value=approved + with patch.object(menu,'carry_records',return_value={115:'stale'} if stale else {}) as verify, \ + patch.object(menu,'check_selected',return_value=row), \ + patch.object(menu,'_selected_configuration',return_value=b'current config'), \ + patch.object(instances,'read',return_value=saved), \ + patch.object(reconcile,'propose',side_effect=ValueError('external changes') if adoption_error else None,return_value=None), \ + patch.object(recreation,'edit_recreation',side_effect=edit), \ + patch.object(menu,'_work_backup_ready',side_effect=backup), \ + patch.object(menu,'_run_lifecycle',side_effect=lambda *a:events.append('lifecycle') or True): + try:result=menu.manage_instance(ROOT,ui,row,action='modify') + except UserCancelled:result=False + verify.assert_called_once_with(ROOT,vmids=[115],verify=True) + return result,events + + def test_open_and_cancel_has_no_backup_or_storage_probes(self): + result,events=self.run_editor() + self.assertFalse(result);self.assertEqual(events,['editor']) + + def test_declining_the_review_has_no_backup_or_storage_probes(self): + # The review is part of the Modify menu: declining it and leaving the + # menu returns no proposal, so no probe or operation follows. + result,events=self.run_editor(declined=True) + self.assertFalse(result);self.assertEqual(events,['editor']) + + def test_confirmed_edit_keeps_backup_check_before_the_lifecycle(self): + result,events=self.run_editor({'operation':'recreate','candidate':record()}) + self.assertTrue(result);self.assertEqual(events,['editor','backup','lifecycle']) + + def test_missing_backup_space_still_prevents_the_lifecycle(self): + result,events=self.run_editor({'operation':'recreate','candidate':record()},backup_ready=False) + self.assertFalse(result);self.assertEqual(events,['editor','backup']) + + def test_restored_or_conflicting_instance_cannot_enter_editor(self): + for options in ({'stale':True},{'adoption_error':True}): + result,events=self.run_editor(**options) + self.assertFalse(result);self.assertEqual(events,[]) + + +if __name__=='__main__':unittest.main()