From 111593bc38d7e25c6a130d0ca4ea30aef7ac1825 Mon Sep 17 00:00:00 2001 From: Jonatan Date: Sat, 17 Oct 2026 12:48:02 +0200 Subject: [PATCH] Fix OCI management after changing Proxmox tags --- .github/workflows/test-oci-registries.yml | 6 + oci/remote/install_oci.sh | 11 +- oci/remote/oci_instance_transaction.py | 16 +- oci/tests/test_external_tags.py | 189 ++++++++++++++++++++++ 4 files changed, 213 insertions(+), 9 deletions(-) create mode 100644 oci/tests/test_external_tags.py diff --git a/.github/workflows/test-oci-registries.yml b/.github/workflows/test-oci-registries.yml index 9612c0d3..235bb0c1 100644 --- a/.github/workflows/test-oci-registries.yml +++ b/.github/workflows/test-oci-registries.yml @@ -32,3 +32,9 @@ jobs: env: PYTHONDONTWRITEBYTECODE: '1' run: python3 -m unittest discover -s oci/tests -p 'test_registry_*.py' -v + - name: Run external OCI settings regressions + env: + PYTHONDONTWRITEBYTECODE: '1' + run: | + python3 -m unittest discover -s oci/tests -p 'test_external_tags.py' -v + python3 -m unittest discover -s oci/tests -p 'test_instance_reconcile.py' -v diff --git a/oci/remote/install_oci.sh b/oci/remote/install_oci.sh index 7d1d8e85..3b549001 100755 --- a/oci/remote/install_oci.sh +++ b/oci/remote/install_oci.sh @@ -1025,15 +1025,18 @@ apply_extra_hosts() { } # Settings an update gives back to the rebuilt container before it starts: -# the LAN leg of a suite or stack member and its start order. +# the LAN leg of a suite or stack member, its start order and current tags. apply_kept_settings() { local key value while IFS=$'\t' read -r key value; do [[ -n $key ]] || continue - [[ $key == net1 || $key == startup ]] \ + [[ $key == net1 || $key == startup || $key == tags ]] \ || die "$(translate "Unsupported kept setting:") $key" - oci_quiet pct set "$VMID" "--$key" "$value" \ - || die "$(translate "Could not restore the container setting:") $key" + if [[ $key == tags && -z $value ]]; then + oci_quiet pct set "$VMID" --delete tags + else + oci_quiet pct set "$VMID" "--$key" "$value" + fi || die "$(translate "Could not restore the container setting:") $key" done < <(jq -r '.kept_proxmox_settings // {} | to_entries[] | [.key, .value] | @tsv' "$DEPLOYMENT_FILE") } diff --git a/oci/remote/oci_instance_transaction.py b/oci/remote/oci_instance_transaction.py index 59579967..cdde917b 100644 --- a/oci/remote/oci_instance_transaction.py +++ b/oci/remote/oci_instance_transaction.py @@ -415,10 +415,9 @@ def candidate_contract(record, operation, proposal=None): return with_default_healthcheck(result) -# Resource settings edited in Proxmox that the new container keeps. # Proxmox settings the rebuilt container gets back exactly as they are: the -# LAN leg ProxMenux adds to a suite or stack member, and the start order. -KEPT_AS_IS = ('net1', 'startup') +# LAN leg ProxMenux adds to a suite or stack member, the start order and tags. +KEPT_AS_IS = ('net1', 'startup', 'tags') ADOPTABLE = {'memory': ('resources', 'memory_mb'), 'swap': ('resources', 'swap_mb'), 'cores': ('resources', 'cores'), 'cpulimit': ('resources', 'cores'), 'cpuunits': ('resources', 'cpu_units'), 'onboot': (None, 'onboot')} @@ -1051,6 +1050,9 @@ def apply(root, vmid, archive, operation, proposal=None, registry_digest=None, i 'original_gpu_devices': original_gpu, 'desired_gpu_devices': desired_gpu, 'was_running': run('pct', 'status', str(vmid)).strip() == b'status: running'} state['kept_config'] = {key: cfg[key] for key in KEPT_AS_IS if key in cfg} + # The installer adds default tags. Preserve an explicit absence too, so + # deleting all tags in Proxmox does not bring them back on the next update. + state['kept_config']['tags'] = cfg.get('tags', '') if not coordinated and state['kept_config']: # Applied by the installer before the new container starts, so the # application has its LAN leg from the first second. @@ -1058,9 +1060,10 @@ def apply(root, vmid, archive, operation, proposal=None, registry_digest=None, i if coordinated: state['coordinated'] = coordinated state['preserved_stack_config'] = {key: cfg[key] for key in ('net1', 'startup', 'hookscript') if key in cfg} + state['preserved_stack_config']['tags'] = cfg.get('tags', '') if operation == 'update': state['preserved_stack_config'].update({key: cfg[key] for key in - ('net0', 'features', 'cmode', 'console', 'tty', 'cpuunits', 'tags') if key in cfg}) + ('net0', 'features', 'cmode', 'console', 'tty', 'cpuunits') if key in cfg}) checkpoint(journal, state, 'prepared') pending = copy.deepcopy(record) pending.update(status='updating', pending_transaction=str(journal), transaction_id=state['id']) @@ -1141,7 +1144,10 @@ def apply(root, vmid, archive, operation, proposal=None, registry_digest=None, i restore_firewall(vmid, state) if coordinated: for key, value in state['preserved_stack_config'].items(): - run('pct', 'set', str(vmid), '--' + key, value) + if key == 'tags' and not value: + run('pct', 'set', str(vmid), '--delete', 'tags') + else: + run('pct', 'set', str(vmid), '--' + key, value) check_runtime_mounts(run('pct', 'config', str(vmid)), candidate['deployment']) gpu_devices.check(run('pct', 'config', str(vmid)), candidate['deployment']) state['staged_config_sha256'] = sha(owned(vmid, record['installation_id'])) diff --git a/oci/tests/test_external_tags.py b/oci/tests/test_external_tags.py new file mode 100644 index 00000000..a28dd420 --- /dev/null +++ b/oci/tests/test_external_tags.py @@ -0,0 +1,189 @@ +"""Proxmox tags can change without preventing OCI management or being lost.""" +import copy +import io +import json +import os +from pathlib import Path +import re +import shutil +import subprocess +import sys +import tempfile +import unittest +from contextlib import redirect_stdout +from unittest.mock import patch + +ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(ROOT / 'remote')) + +import oci_instance_reconcile as reconcile +import oci_instance_transaction as transaction +from oci_installation_state import sha + +MARKER = '11111111-1111-4111-8111-111111111111' +BASE = (f'description: proxmenux-instance={MARKER}\n' + 'rootfs: local-lvm:vm-200-disk-0,size=8G\n' + 'unprivileged: 1\ncores: 2\nmemory: 512\n' + 'net0: name=eth0,bridge=vmbr0,hwaddr=02:00:00:00:00:01,ip=dhcp\n' + 'net1: name=eth1,bridge=vmbr1,ip=dhcp\nstartup: order=20\n').encode() + + +def config(tags): + return BASE + (f'tags: {tags}\n'.encode() if tags is not None else b'') + + +def record(tags='oci;proxmenux'): + before = config(tags) + return { + 'schema_version': 1, 'vmid': 200, 'status': 'installed', + 'installation_id': MARKER, + 'template': {'id': 'test-app', 'container_contract': {'volumes': []}}, + 'deployment': { + 'vmid': 200, 'rootfs': {'storage': 'local-lvm', 'size_gb': 8}, + 'network': {}, 'resources': {'cores': 2, 'memory_mb': 512}, + 'mounts': [], 'devices': [], + }, + 'observed': { + 'config': before.decode(), 'config_sha256': sha(before), + 'image': {'architecture': 'amd64', 'manifest_digest': 'sha256:old', + 'defaults': {'Volumes': {}}}, + }, + } + + +class ExternalTagsTests(unittest.TestCase): + def test_adding_editing_reordering_and_removing_tags_need_no_adoption(self): + for old, new in ((None, 'homelab'), ('oci;proxmenux', 'homelab;oci;proxmenux'), + ('oci;proxmenux', 'proxmenux;oci'), ('oci;proxmenux', None), + ('oci;proxmenux', '')): + with self.subTest(old=old, new=new): + saved, current = record(old), config(new) + self.assertIsNone(reconcile.propose(saved, current)) + self.assertEqual(transaction.external_changes(saved, current), {}) + + def test_tags_can_change_together_with_cpu_and_memory(self): + current = config('homelab').replace(b'cores: 2', b'cores: 4').replace(b'memory: 512', b'memory: 2048') + self.assertIsNone(reconcile.propose(record(), current)) + self.assertEqual(transaction.external_changes(record(), current), {'cores': 4, 'memory': 2048}) + + def test_tag_changes_do_not_bypass_identity_or_network_checks(self): + for current in (config('homelab').replace(MARKER.encode(), b'22222222-2222-4222-8222-222222222222'), + config('homelab').replace(b'bridge=vmbr0', b'bridge=vmbr2')): + with self.subTest(config=current): + with self.assertRaises(ValueError): + reconcile.propose(record(), current) + with self.assertRaises(ValueError): + transaction.external_changes(record(), current) + + def test_new_volume_still_requires_adoption_when_tags_also_change(self): + saved = record() + current = config('homelab') + b'mp0: local-lvm:vm-200-disk-1,mp=/data,size=8G,backup=1\n' + with tempfile.TemporaryDirectory() as folder: + root = Path(folder) + path = root / '200/oci-compose.json' + path.parent.mkdir() + path.write_text(json.dumps(saved)) + with patch.object(reconcile.instances, 'ROOT', root): + proposal = reconcile.propose(saved, current) + self.assertEqual(proposal['candidate']['deployment']['mounts'][0]['container_path'], '/data') + self.assertEqual(len(proposal['details']), 1) + self.assertEqual(json.loads(path.read_text()), saved) + + +class Prepared(Exception): + """Stop a mocked transaction before it can stop or modify any container.""" + + +class TagReplayTests(unittest.TestCase): + def prepare(self, folder, tags, operation, coordinated=None): + current, saved = config(tags), record() + archive = folder / 'image.tar' + archive.write_bytes(b'fixture') + states = [] + + def checkpoint(_path, state, phase): + self.assertEqual(phase, 'prepared') + states.append(copy.deepcopy(state)) + raise Prepared() + + def command(*args): + if args[:2] == ('pct', 'config'): + return current + if args[:3] == ('pvesh', 'get', '/cluster/ha/resources'): + return b'[]' + if args[:2] == ('pct', 'status'): + return b'status: running' + self.fail(f'Unexpected host command: {args}') + + proposal = {'operation': 'recreate', 'candidate': copy.deepcopy(saved)} if operation == 'recreate' else None + image = {'architecture': 'amd64', 'os': 'linux', 'manifest_digest': 'sha256:new', 'defaults': {}} + with (patch.object(transaction.instances, 'read', return_value=saved), + patch.object(transaction, 'run', side_effect=command), + patch.object(transaction, 'check_archive'), + patch.object(transaction, 'image_from_archive', return_value=image), + patch.object(transaction.work_backup, 'locate', side_effect=lambda directory, *_: directory), + patch.object(transaction, 'open_log'), + patch.object(transaction, 'checkpoint', side_effect=checkpoint), + redirect_stdout(io.StringIO()), self.assertRaises(Prepared)): + transaction.apply(folder, 200, archive, operation, proposal=proposal, coordinated=coordinated) + return states[0] + + @unittest.skipUnless(shutil.which('bash') and shutil.which('jq'), 'bash and jq are needed') + def test_update_and_modify_replay_current_tags_including_no_tags(self): + installer = (ROOT / 'remote/install_oci.sh').read_text() + function = re.search(r'^apply_kept_settings\(\) \{\n.*?^\}\n', installer, re.M | re.S) + self.assertIsNotNone(function) + for operation in ('update', 'recreate'): + for tags in ('homelab;oci;proxmenux', 'oci', None, ''): + with self.subTest(operation=operation, tags=tags), tempfile.TemporaryDirectory() as name: + folder = Path(name) + state = self.prepare(folder, tags, operation) + kept = state['runtime_deployment']['kept_proxmox_settings'] + deployment = folder / 'deployment.json' + deployment.write_text(json.dumps({'kept_proxmox_settings': kept})) + calls = folder / 'calls' + script = ''' +pct() { printf '%s\\0' "$@" >> "$CALLS"; printf '\\n' >> "$CALLS"; } +oci_quiet() { "$@"; } +translate() { printf '%s' "$1"; } +die() { printf '%s\\n' "$*" >&2; exit 1; } +''' + done = subprocess.run(['bash', '-euc', script + function.group(0) + '\napply_kept_settings'], + env=dict(os.environ, VMID='200', DEPLOYMENT_FILE=str(deployment), CALLS=str(calls)), + capture_output=True, text=True) + self.assertEqual(done.returncode, 0, done.stderr) + commands = [line.split(b'\0')[:-1] for line in calls.read_bytes().splitlines()] + expected = [b'set', b'200', b'--tags', tags.encode()] if tags else [b'set', b'200', b'--delete', b'tags'] + self.assertIn(expected, commands) + self.assertIn([b'set', b'200', b'--net1', b'name=eth1,bridge=vmbr1,ip=dhcp'], commands) + self.assertIn([b'set', b'200', b'--startup', b'order=20'], commands) + + def test_stack_operations_also_preserve_tags_and_their_absence(self): + for operation in ('update', 'recreate'): + for tags in ('homelab;oci', None, ''): + with self.subTest(operation=operation, tags=tags), tempfile.TemporaryDirectory() as name: + state = self.prepare(Path(name), tags, operation, coordinated={'stack': 'test'}) + self.assertEqual(state['preserved_stack_config']['tags'], tags or '') + + @unittest.skipUnless(shutil.which('bash') and shutil.which('jq'), 'bash and jq are needed') + def test_installer_still_refuses_unsupported_kept_settings(self): + installer = (ROOT / 'remote/install_oci.sh').read_text() + function = re.search(r'^apply_kept_settings\(\) \{\n.*?^\}\n', installer, re.M | re.S).group(0) + with tempfile.TemporaryDirectory() as name: + deployment = Path(name) / 'deployment.json' + deployment.write_text(json.dumps({'kept_proxmox_settings': {'unprivileged': '0'}})) + script = ''' +oci_quiet() { echo 'must not run' >&2; exit 99; } +translate() { printf '%s' "$1"; } +die() { printf '%s\\n' "$*" >&2; exit 1; } +''' + done = subprocess.run(['bash', '-euc', script + function + '\napply_kept_settings'], + env=dict(os.environ, VMID='200', DEPLOYMENT_FILE=str(deployment)), + capture_output=True, text=True) + self.assertEqual(done.returncode, 1) + self.assertIn('Unsupported kept setting: unprivileged', done.stderr) + self.assertNotIn('must not run', done.stderr) + + +if __name__ == '__main__': + unittest.main()