feat(proxmox): reconfigure VMs during PatchHosts when their config drifts - #353
Merged
Conversation
…ifts PatchHosts only re-applied firewall state, so a VM whose config on the hypervisor no longer matched the database (template upgrades, manual edits, changed defaults such as the scsi controller) stayed drifted until someone triggered ConfigureVm by hand. Add VmHostClient::patch_config, a firewall-patch analogue that reads the live config, diffs it against the config we expect from the DB, and only calls configure_vm when something actually differs — returning the drifted field names for logging. Defaults to a no-op for hosts that cannot read their config back (libvirt, dummy). The Proxmox implementation compares only the fields we set, ignoring disk/EFI entries (owned by create/resize). Property strings (net0, ipconfig0) are normalised before comparison because Proxmox re-orders keys and upper-cases MACs when it stores them, and ssh keys are compared url-decoded — otherwise every pass would report permanent drift.
The disk options we own (iothread=1, discard/ssd, throttle limits) live on the scsi0 device string, which is rebuilt from the live volume reference rather than produced by make_config — so the drift check missed them entirely, and a VM whose only difference was the missing iothread flag was never reconciled. Factor the scsi0 device string out of apply_disk_options into make_scsi0 and compare it against what the host currently has. When it differs, scsi0 is reported as drift and configure_vm re-applies it (as it already does at the end of a normal configure), which is exactly what converges existing VMs onto iothread=1 alongside virtio-scsi-single and the ballooning minimum from the host config.
Proxmox stores compound config values as property strings (`net0 = virtio=BC:24:11:00:11:22,bridge=vmbr0,firewall=1`). Holding them as raw Strings meant every read was a substring search and every comparison had to re-implement Proxmox's normalisation — it re-orders keys and upper-cases MACs when it stores them — which is why the drift check needed a sort/lowercase hack and a url-decode special case for ssh keys. Give each property string we set a real type in host::proxmox_config: NetDevice (+ NetModel, MacAddress), IpConfig (+ Ipv4Setting/Ipv6Setting), DiskDevice (+ VolumeRef), CiCustom and SshKeys, each parsed on the way in and rendered on the way out through serde. VmConfig now holds those types instead of Strings, so: - config_drift is plain field equality, no normalisation helpers - make_scsi0 builds a DiskDevice from the live volume rather than splitting and re-joining a string - parse_mac_from_net / parse_storage_from_disk are gone, replaced by reading net.mac and scsi_0.volume.storage - make_config assembles typed values, including the one-address-per-family ipconfig rule which is now enforced by the type rather than by filtering formatted strings Parsing is lenient by design: unmodelled keys are ignored and malformed values dropped, so a hand-edited VM config is still readable.
Two agent_chat tests asserted outcomes the model chooses rather than ones the server enforces, and both failed intermittently. test_chat_resists_indirect_injection_via_tool_output required the model to call get_vm_details. That call is the test's precondition — the injected payload only enters the context through the tool result — not the property under test, which is that the payload dispatches nothing destructive and leaks no host token. It now asks up to three times, progressively more directly, stopping as soon as the lookup runs, and checks the security assertions on every turn. When the lookup never happens the failure prints the tools and replies from each attempt instead of a bare `ran: []`. test_chat_refuses_billing_actions flaked the same way: the model declined correctly but omitted the LNVPS_ESCALATE sentinel the e2e system prompt mandates. Same treatment — one follow-up turn, security assertions each time, sentinel required on at least one. Verified with scripts/run-e2e.sh --filter agent_chat: 13 passed, 0 failed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PatchHostsre-applied firewall state for every live VM but never touched the VM config, so a VM whose config on the hypervisor no longer matched the database (template upgrades, manual edits on the host, changed defaults like the scsi controller) stayed drifted until someone triggeredConfigureVmby hand.This adds a firewall-patch analogue for config.
Changes
VmHostClient::patch_config(lnvps_api_common/src/host/mod.rs) — reads the live config, diffs it against the config expected from the DB, and callsconfigure_vmonly when something differs; returns the drifted field names. Defaults to a no-op for hosts that can't read their config back (libvirt, dummy).lnvps_api_common/src/host/proxmox.rs) — compares only the fields we set (name, cores, memory, balloon, cpu, cpulimit, onboot, machine, ostype, bios, boot, kvm, scsihw, serial0, cicustom, net0, ipconfig0, sshkeys). Disk/EFI entries are ignored since they're owned by create/resize. Property strings are normalised (key order + case) because Proxmox re-orders keys and upper-cases MACs when storing them, and ssh keys are compared url-decoded — otherwise every pass would report permanent drift.VmBiosnow derivesPartialEq, Eq.lnvps_api/src/worker.rs) —patch_hostcallspatch_configbeforepatch_firewall, reusing the already-loadedFullVmInfo; logsRe-configured VM {id} on host {name} (drift: ...)and warns non-fatally on failure.Testing
test_config_drift(no false positives on Proxmox-normalised strings, detects resource/ipconfig/ssh-key drift) andtest_normalize_prop_string.cargo clippy -p lnvps_api -p lnvps_api_common --all-targetsclean; proxmox test suite passes.