Preserve Modify fallback and detached disks after native edits

oci/remote/oci_instance_transaction.py

@@ -320,6 +320,36 @@ def mounts(config):
     return result
 
 
+def unused_volumes(config):
+    return {key: value for key, value in parse_config(config).items()
+            if re.fullmatch(r'unused[0-9]+', key)}
+
+
+def move_retained_unused(state, to_stage):
+    """Unused disks are not in vzdump. Park and return them separately, using
+    PVE's volume reassignment API, never --delete (which frees an unused disk).
+    The journal's keys and both ownership markers also make retries safe."""
+    keys = set(state.get('retained_unused', {}))
+    if not keys:
+        return
+    vmid, stage = state['vmid'], state['stage']
+    guest_marker = state['record']['installation_id']
+    holder_marker = 'proxmenux-transaction=' + state['id']
+    source, target = (vmid, stage) if to_stage else (stage, vmid)
+    source_marker, target_marker = ((guest_marker, holder_marker) if to_stage
+                                    else (holder_marker, guest_marker))
+    for key in sorted(keys):
+        present = unused_volumes(owned(source, source_marker))
+        destination = unused_volumes(owned(target, target_marker))
+        if key not in present:
+            if key in destination:
+                continue  # Already reassigned before an interruption.
+            raise ValueError(translate('There are extra disks or bind mounts outside the journal; the rootfs is not replaced'))
+        if key in destination or (to_stage and present.keys() - keys):
+            raise ValueError(translate('There are extra disks or bind mounts outside the journal; the rootfs is not replaced'))
+        run('pct', 'move-volume', str(source), key, '--target-vmid', str(target), '--target-volume', key)
+
+
 def is_file_bind(mount, files=()):
     return mount['type'] == 'host-bind' and (mount['source'] in files or Path(mount['source']).is_file())
 
@@ -492,7 +522,16 @@ def preflight(record, candidate, config, coordinated=None, in_place=False, basel
     runtime_settings.check(config, deployment, record['vmid'])
     coordinated_keys = {'hookscript', *KEPT_AS_IS} if coordinated else set(KEPT_AS_IS)
     preserved_keys = {'nameserver', 'searchdomain', 'timezone', 'protection'} if in_place else set()
-    if '[' in config.decode() or keys - BASIC - runtime_keys - coordinated_keys - preserved_keys - {k for k in keys if re.fullmatch(r'(mp|dev)[0-9]+', k) or (in_place and re.fullmatch(r'unused[0-9]+', k))}:
+    detached = unused_volumes(config)
+    if baseline and detached != unused_volumes(record['observed'].get('config', '').encode()):
+        raise ValueError(translate('The container disks do not match the saved record'))
+    used_volumes = {value.split(',', 1)[0] for key, value in cfg.items()
+                    if key == 'rootfs' or re.fullmatch(r'mp[0-9]+', key)}
+    if (len(set(detached.values())) != len(detached)
+            or any(not re.fullmatch(r'[A-Za-z0-9_.-]+:[^\s,]+', value) or value in used_volumes
+                   for value in detached.values())):
+        raise ValueError(translate('The container disks do not match the saved record'))
+    if '[' in config.decode() or keys - BASIC - runtime_keys - coordinated_keys - preserved_keys - {k for k in keys if re.fullmatch(r'(mp|dev|unused)[0-9]+', k)}:
         raise ValueError(translate('The container has advanced Proxmox settings outside the supported profile'))
     unprivileged = deployment.get('security', {}).get('unprivileged', True) is not False
     if (desired.get('security', {}).get('unprivileged', True) is not False) != unprivileged \
@@ -549,7 +588,9 @@ def preflight(record, candidate, config, coordinated=None, in_place=False, basel
                 or mount.get('ro', '0') != ('1' if old[target].get('read_only') else '0')):
             raise ValueError(translate('A container mount has a source, backup or permission different from the saved record'))
     for target in old.keys() & new.keys():
-        if any(old[target].get(k) != new[target].get(k) for k in ('type', 'source', 'size_gb')):
+        directory_bind = old[target]['type'] == new[target]['type'] == 'host-bind'
+        fixed = ('type', 'size_gb') if directory_bind else ('type', 'source', 'size_gb')
+        if any(old[target].get(k) != new[target].get(k) for k in fixed):
             raise ValueError(translate('Changing the storage or size of a disk requires a migration; empty disks are not created'))
     protected = ('/bin', '/sbin', '/etc', '/usr', '/lib', '/lib64', '/proc', '/sys', '/dev', '/run')
     for target in new.keys() - old.keys():
@@ -1043,6 +1084,7 @@ def apply(root, vmid, archive, operation, proposal=None, registry_digest=None, i
                                   for p, m in actual.items() if p in required and not m['volume'].startswith('/')])
     state = {'schema_version': 1, 'id': directory.name, 'vmid': vmid, 'operation': operation,
              'record': record, 'candidate_contract': candidate, 'before_config': before.decode(),
+             'retained_unused': unused_volumes(before),
              'original_description': description,
              'archive': str(archive), 'archive_sha256': filehash(archive),
              'registry_digest': registry_digest or image['manifest_digest'],
@@ -1116,6 +1158,7 @@ def apply(root, vmid, archive, operation, proposal=None, registry_digest=None, i
         else:
             owned(stage, 'proxmenux-transaction=' + state['id'])
             run('pct', 'move-volume', str(vmid), mount['key'], '--target-vmid', str(stage), '--target-volume', mount['key'])
+    move_retained_unused(state, to_stage=True)
     checkpoint(journal, state, 'data-parked')
     if show:
         msg_ok(translate('Data volumes protected'))
@@ -1125,8 +1168,8 @@ def apply(root, vmid, archive, operation, proposal=None, registry_digest=None, i
         msg_info(translate('Installing the new image...') if update else translate('Recreating the container...'))
     current = owned(vmid, record['installation_id'])
     expected = b''.join(line for line in before.splitlines(keepends=True)
-                        if not re.match(rb'mp[0-9]+: ', line))
-    if mounts(current) or current != expected:
+                        if not re.match(rb'(mp|unused)[0-9]+: ', line))
+    if mounts(current) or unused_volumes(current) or current != expected:
         raise ValueError(translate('A concurrent change was detected; the container is not removed'))
     host_mounts.verify_sources(original_sources)
     gpu_devices.verify(original_gpu)
@@ -1141,6 +1184,7 @@ def apply(root, vmid, archive, operation, proposal=None, registry_digest=None, i
     install_candidate(root, journal, state, progress)
     restore_description(vmid, state)
     restore_firewall(vmid, state)
+    move_retained_unused(state, to_stage=False)
     if coordinated:
         for key, value in state['preserved_stack_config'].items():
             run('pct', 'set', str(vmid), '--' + key, value)
@@ -1340,8 +1384,9 @@ def recover(root, journal):
         stop(vmid)
         current = owned(vmid, record['installation_id'])
         gpu_devices.check_recovery_entries(current, state)
-        if any(re.fullmatch(r'unused[0-9]+', k) for k in parse_config(current)):
+        if unused_volumes(current).keys() - state.get('retained_unused', {}).keys():
             raise ValueError(translate('There are extra disks or bind mounts outside the journal; the rootfs is not replaced'))
+        move_retained_unused(state, to_stage=True)
         known_binds = {(m['source'], m['container_path'])
                        for plan in (state['record']['deployment'], state['candidate_contract']['deployment'])
                        for m in plan.get('mounts', []) if m['type'] == 'host-bind'}
@@ -1369,6 +1414,7 @@ def recover(root, journal):
     host_mounts.verify_sources(state.get('original_host_sources', {}))
     run('pct', 'restore', str(vmid), backup, '--force', '1', '--storage',
         state['record']['deployment']['rootfs']['storage'])
+    move_retained_unused(state, to_stage=False)
     state['restore_config_sha256'] = sha(owned(vmid, state['record']['installation_id']))
     checkpoint(journal, state, 'checking-recovery')
     if show:

oci/remote/oci_native_modify.py

@@ -86,6 +86,24 @@ def changes(record, candidate, config):
     if 'mounts' in changed:
         if any(transaction.is_file_bind(m) for plan in (before, after) for m in plan.get('mounts', [])):
             return None
+        if transaction.nested_mounts.without(config) != config:
+            # The installer also regenerates the recursive bind entries and
+            # hooks associated with these mounts.
+            return None
+        old_mounts = {m['container_path']: m for m in before.get('mounts', [])}
+        for mount in after.get('mounts', []):
+            original = old_mounts.get(mount['container_path'])
+            # Permission grants and installer-specific mount attributes need
+            # their normal installer, including when only their source changes.
+            if mount.get('grant_access'):
+                return None
+            if original:
+                fields = {k for k in original.keys() | mount.keys() if original.get(k) != mount.get(k)}
+                native_fields = {'read_only', 'backup', 'create_if_missing'}
+                if original['type'] == mount['type'] == 'host-bind':
+                    native_fields.add('source')
+                if fields - native_fields:
+                    return None
     return changed
 
 
@@ -174,8 +192,14 @@ def plan(record, candidate, config):
                 original = next(m for m in old['mounts'] if m['container_path'] == target)
                 if mount == original: continue
                 fields = dict(p.split('=', 1) for p in actual[target]['value'].split(',')[1:] if '=' in p)
-                fields['ro'] = '1' if mount.get('read_only') else '0'
-                values[actual[target]['key']] = actual[target]['volume']+','+','.join(k+'='+v for k, v in fields.items())
+                source = actual[target]['volume']
+                if mount['type'] == 'host-bind':
+                    source = host_mounts.valid_path(mount['source'])
+                    if not Path(source).is_dir() and not mount.get('create_if_missing'):
+                        raise ValueError(translate('The host directory does not exist'))
+                fields.update(mp=target, backup='1' if mount['backup'] else '0',
+                              ro='1' if mount.get('read_only') else '0')
+                values[actual[target]['key']] = source+','+','.join(k+'='+v for k, v in fields.items())
                 continue
             key = next('mp'+str(n) for n in range(256) if 'mp'+str(n) not in used)
             used.add(key)
@@ -203,6 +227,12 @@ def verify_values(config, operation):
             if environment(config) != dict(item.split('=', 1) for item in value.split('\0')):
                 raise ValueError(translate('Proxmox could not apply the container settings'))
         elif re.fullmatch(r'(mp|dev|net)[0-9]+', key):
+            if re.fullmatch(r'mp[0-9]+', key):
+                source = value.split(',', 1)[0]
+                actual_source = cfg.get(key, '').split(',', 1)[0]
+                allocating = re.fullmatch(r'[A-Za-z0-9_-]+:[0-9]+', source)
+                if (not allocating and actual_source != source) or (allocating and not actual_source.startswith(source.split(':')[0]+':')):
+                    raise ValueError(translate('Proxmox could not apply the container settings'))
             desired = dict(p.split('=', 1) for p in value.split(',') if '=' in p)
             actual = dict(p.split('=', 1) for p in cfg.get(key, '').split(',') if '=' in p)
             defaults = {'uid': '0', 'gid': '0', 'deny-write': '0', 'ro': '0', 'backup': '0', 'mode': '0660', 'firewall': '0'}

oci/remote/oci_update_current.py

@@ -224,10 +224,9 @@ def update(vmid, acknowledge_external_data=False, proposal=None, keep_backup=Non
                     backup_native_settings(vmid, keep_backup)
                     return oci_native_modify.apply(instances.ROOT, record, desired, config,
                                                    acknowledge_external_data=acknowledge_external_data)
-        if (operation == 'recreate'
-                and record['template']['container_contract']['image']['reference']
-                == desired['template']['container_contract']['image']['reference']):
-            raise ValueError(translate('These settings require an application-specific adaptation. The image was not downloaded and the container was not recreated.'))
+        # An unsupported native edit still uses the existing installer. Decide
+        # the route before changing anything; actual validation/API failures
+        # must not silently switch to a destructive fallback.
         transaction.preflight(record, desired, config)
         msg_ok(translate('Container checked'))
         current = record['observed']['image'] if operation == 'update' else None