From 14513cc898a84e797d9d562b8f6ce6602fba2210 Mon Sep 17 00:00:00 2001 From: Jonatan <190036+jcastro@users.noreply.github.com> Date: Sat, 17 Oct 2026 17:19:24 +0200 Subject: [PATCH] Speed up OCI Modify entry by deferring backup probes and reusing verified config --- .../test_oci_update_preservation_wording.py | 1 + .github/workflows/test-oci-registries.yml | 4 + oci/src/proxmenux_oci/management.py | 29 ++++- oci/tests/test_modify_latency.py | 100 ++++++++++++++++++ 4 files changed, 131 insertions(+), 3 deletions(-) create mode 100644 oci/tests/test_modify_latency.py 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 9612c0d3..7625fe1c 100644 --- a/.github/workflows/test-oci-registries.yml +++ b/.github/workflows/test-oci-registries.yml @@ -32,3 +32,7 @@ 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 diff --git a/oci/src/proxmenux_oci/management.py b/oci/src/proxmenux_oci/management.py index cbf8e041..9b71d1b1 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}) @@ -397,12 +416,12 @@ 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']): + 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: - 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')) @@ -443,6 +462,10 @@ 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 diff --git a/oci/tests/test_modify_latency.py b/oci/tests/test_modify_latency.py new file mode 100644 index 00000000..788215ad --- /dev/null +++ b/oci/tests/test_modify_latency.py @@ -0,0 +1,100 @@ +"""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): + events=[];saved=record() + row={'vmid':115,'reason':'matched','stack':False,'pending':False,'status':'installed'} + def edit(*a,**kw): + events.append('editor') + 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): + result,events=self.run_editor({'operation':'recreate','candidate':record()},approved=False) + 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()