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/workflows/test-oci-registries.yml b/.github/workflows/test-oci-registries.yml index 7625fe1c..cab053e6 100644 --- a/.github/workflows/test-oci-registries.yml +++ b/.github/workflows/test-oci-registries.yml @@ -35,4 +35,8 @@ jobs: - name: Run OCI Modify latency regressions env: PYTHONDONTWRITEBYTECODE: '1' - run: python3 -m unittest discover -s oci/tests -p test_modify_latency.py -v + 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_work_backup.py -v + python3 -m unittest discover -s oci/tests -p test_recreation.py -v 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 9b71d1b1..db75d597 100644 --- a/oci/src/proxmenux_oci/management.py +++ b/oci/src/proxmenux_oci/management.py @@ -332,7 +332,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 @@ -416,8 +416,6 @@ 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 action != 'modify' and not _work_backup_ready(project, ui, row['vmid']): - return False record = instances.read(instances.ROOT, row['vmid']) import oci_instance_reconcile as reconcile try: @@ -462,13 +460,13 @@ def manage_instance(project, ui, row, action=None, lifecycle_args=()): wizard.close() if not approved: return False - # Opening or cancelling the editor needs no disk/storage probes. - # Keep the existing backup checks after the user confirms edits. - if not _work_backup_ready(project, ui, row['vmid']): - return False 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 @@ -518,7 +516,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/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()