Merge pull request #473 from jcastro/fix/oci-external-tags

Fix OCI management after changing Proxmox tags

.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

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" \
+    if [[ $key == tags && -z $value ]]; then
-      || die "$(translate "Could not restore the container setting:") $key"
+      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")
 }
 

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.
+# LAN leg ProxMenux adds to a suite or stack member, the start order and tags.
-KEPT_AS_IS = ('net1', 'startup')
+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,6 +1144,9 @@ 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():
+            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'])

oci/remote/oci_instances.py

@@ -230,8 +230,9 @@ def identity(config):
     return match.group(1) if match else None
 
 
-def same_config_except_notes(record, config):
+def same_config_except_notes(record, config, ignore_tags=False):
-    """Accept a presentation-only note edit, never a changed OCI identity or LXC setting."""
+    """Accept a presentation-only note edit, never a changed OCI identity or LXC setting.
+    With `ignore_tags`, Proxmox tags may differ too: the operation keeps them as they are."""
     previous = record['observed']['config'].encode()
     expected = record['installation_id']
     if identity(previous) != expected or identity(config) != expected:
@@ -242,7 +243,8 @@ def same_config_except_notes(record, config):
         notes = [line for line in lines if line.startswith(b'description: ')]
         if len(notes) != 1:
             return None
-        return b''.join(line for line in lines if not line.startswith(b'description: '))
+        dropped = (b'description: ', b'tags: ') if ignore_tags else (b'description: ',)
+        return b''.join(line for line in lines if not line.startswith(dropped))
 
     original = without_notes(previous)
     return original is not None and original == without_notes(config)

oci/remote/oci_stack_native.py

@@ -192,7 +192,7 @@ class NativeAdapter:
                 config = instances.command('pct', 'config', str(vmid))
                 if instances.identity(config) != record['installation_id']:
                     raise ValueError(translate('The identity of a member was replaced'))
-                if (not self.journal.exists() or not self.state().get('replacement_intent')) and not instances.same_config_except_notes(record, config):
+                if (not self.journal.exists() or not self.state().get('replacement_intent')) and not instances.same_config_except_notes(record, config, ignore_tags=True):
                     raise ValueError(translate('A member configuration changed during the preparation'))
             else:
                 if Path('/etc/pve/lxc/%s.conf' % vmid).exists():

oci/tests/test_external_tags.py

@@ -0,0 +1,203 @@
+"""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_native_stack_check_accepts_tag_edits_and_nothing_else(self):
+        saved = record()
+        instances = transaction.instances
+        for new in ('homelab;oci;proxmenux', 'proxmenux;oci', '', None):
+            with self.subTest(new=new):
+                self.assertTrue(instances.same_config_except_notes(saved, config(new), ignore_tags=True))
+                self.assertFalse(instances.same_config_except_notes(saved, config(new)))
+        changed = config('homelab').replace(b'cores: 2', b'cores: 4')
+        self.assertFalse(instances.same_config_except_notes(saved, changed, ignore_tags=True))
+        other = config('homelab').replace(MARKER.encode(), b'22222222-2222-4222-8222-222222222222')
+        self.assertFalse(instances.same_config_except_notes(saved, other, ignore_tags=True))
+        source = (ROOT / 'remote/oci_stack_native.py').read_text()
+        self.assertIn('same_config_except_notes(record, config, ignore_tags=True)', source)
+
+    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()