Repository navigation
feat(vm-instance): pin a static IP on secondary interfaces on both network backends - #4038
lf (lfinmauritius) wants to merge 2 commits into
Conversation
Helm's `lookup` propagates the discovery error when the requested API group is not served, so the vm-instance chart could not render at all on clusters without the Kube-OVN CRDs. Gate the IP lookup behind `.Capabilities.APIVersions.Has "kubeovn.io/v1/IP"`. The regression suite mocks the IP object through kubernetesProvider and pins both sides: with the API advertised the annotations carry the mocked MAC and IP, without it they are absent even though the object exists. helm-unittest's fake lookup never errors on an unserved kind, so only the second case distinguishes the guard; it fails on main and passes here. Assisted-By: GPT-5 <[email protected]> Signed-off-by: Loïc Fontaine <[email protected]>
…twork backends networks[] exposes a single field per interface, name. A tenant can attach a VM to a VPC network but cannot choose the interface's address, and nothing reports the allocated one either. Add an optional networks[].ipAddress and pin it through whichever backend the cluster serves, picked from API discovery, never from a lookup. Kube-OVN (kubeovn.io/v1 IP served) reads a per-network annotation key, <nad>.<namespace>.ovn.kubernetes.io/ip_address, on the launcher pod. Cozyplane (sdn.cozystack.io/v1alpha1 VPC served, Kube-OVN absent) reads one launcher pod annotation, sdn.cozystack.io/networks: a JSON list with an entry per pinned NIC, selected by the interface name Multus hands to the CNI. KubeVirt names a secondary interface "pod" plus the first eleven hex digits of the sha256 of the network name, so each entry carries that name together with the VPC the NetworkAttachmentDefinition names; an entry without a name would be read as a leg of the pod itself. Both keys reuse the <namespace>/<network> expressions the multus reference is composed from, so a pinned network and an attached network cannot disagree; a key the backend does not recognise is ignored without any admission error. With neither backend served the render fails: a value that would be dropped silently is worse than a render that says why it stopped. Without ipAddress nothing changes: the rendering is byte-identical on every backend, so existing VMs are untouched. The suite drives both branches from the capability set, mocks the Kube-OVN IP object to pin that the historical pod-network annotations keep rendering next to the new key, and is red on the previous template for every pinning case. Co-Authored-By: Claude Fable 5.1 <[email protected]> Signed-off-by: lfneosequentia <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe VM instance gains optional static IPv4 pinning for ChangesStatic IP pinning
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant VMInstanceValues
participant HelmTemplate
participant ClusterAPIs
participant NetworkBackend
VMInstanceValues->>HelmTemplate: Provide networks[].ipAddress
HelmTemplate->>ClusterAPIs: Check served Kube-OVN and Cozyplane APIs
ClusterAPIs-->>HelmTemplate: Return capabilities
HelmTemplate->>NetworkBackend: Render selected IP annotations
NetworkBackend-->>HelmTemplate: Accept pinned address or reject unsupported rendering
Merge Risk: 🔵 Low · up to The PR adds tenant-configurable static IPs for secondary VM interfaces and safely routes them to the selected network backend. Merge is reasonable with explicit owner awareness that backend-side validation, conflict handling, tenant isolation, and address cleanup must be confirmed for both supported backends. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Out of Scope Changes checkExplanation The PR includes changes beyond issue Resolution Either update or add linked issue requirements for Cozyplane support and the Kube-OVN lookup guard, or move those changes into separate pull requests. Keep this pull request limited to the issue
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
lf (@lfinmauritius) NOT LGTM: the Kube-OVN part is a reasonable feature, but half of this targets a backend main does not ship.
A fixed address for a VM inside its VPC is a fair ask, and the Kube-OVN key is right. The vpc chart sets the NAD provider to <nad>.<namespace>.ovn, so <network>.<namespace>.ovn.kubernetes.io/ip_address is the annotation Kube-OVN reads for that attachment. That part is small and I'd take it.
The Cozyplane branch should go. Nothing on main serves sdn.cozystack.io/v1alpha1 VPC. The only in-tree source is the open draft #3149, and the sdn.cozystack.io/networks format depends on an unmerged change in the Cozyplane repo. The entry name pod plus 11 hex digits of sha256 is also a KubeVirt internal. The template would carry both as a contract no test on main can check. Once Cozyplane lands, pinning for it can follow in the same place.
What I need to merge this:
- Only
ipAddressplus the Kube-OVN annotation. Keep thefailwhen Kube-OVN is absent, since the value would otherwise be dropped silently. - A rebase. The branch conflicts with main in
packages/apps/vm-instance/README.mdand the generatedpackages/system/vm-instance-rd/cozyrds/vm-instance.yaml, so re-runmake generateafter it. - A run on a Kube-OVN cluster. The description says this path was only unit-tested, and it is the only path main can use. Show a VM coming up with the pinned address on the secondary NIC.
- Replace
Co-Authored-By: Claude Fable 5.1 <[email protected]>withAssisted-by: LLM, seedocs/agents/contributing.md.
Minor: ipAddress is a bare string in the schema. A typo like 10.0.0.300 passes validation and only fails when the pod is created. An IPv4 format check on the field would catch it earlier.
…4306) <!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does A guest whose interfaces are rendered by netplan ignores two things the platform already puts on the wire for it, and this adds an opt-in `netplanDhcp` value that supplies the cloud-init network-data making it ask for both. `virt-launcher` starts a DHCP server on **every** bridge-bound interface, secondary ones included — `newDHCPConfigurator` in `pkg/network/setup/launcher/configurator.go` gates only on `iface.Bridge != nil`, and `BridgeConfigGenerator.Generate` serves the address the CNI put on the pod-side interface along with that link's MTU. The chart renders `bridge: {}` for every network, so a VM joined to a VPC subnet is offered its reserved address and its MTU. cloud-init's fallback network configuration covers a single interface, so on Debian and Ubuntu nothing ever sends a DHCPDISCOVER on the second NIC and the interface stays inert, while AlmaLinux (NetworkManager auto-profile) and Windows come up. That asymmetry is the tell: with no DHCP server at all, none of the three would work. The same DHCP server advertises the launcher pod's search domains as option 119, and `UseDomains=` defaults to `no` in systemd-networkd, so a netplan-managed link keeps none of them. A cluster domain under `.local` then reaches the mDNS exclusion in resolved and every lookup answers `REFUSED`, which is #2085. `dhcp4-overrides: use-domains: true` is enough on its own: the domains on the wire already carry whatever cluster domain the cluster actually uses, so nothing needs to be hardcoded here and #2372 does not collide with this. ### This is a regression, not a missing feature `packages/apps/virtual-machine` carried exactly this, added in c63fb20 (*[apps] virtual-machine: add vpc support*, 2025-10-21) as `templates/secret-network-config.yaml` plus a `networkDataSecretRef`, gated on `.Values.systemDisk.image`. 08a5f98 (*feat(platform): remove virtual-machine application*, 2026-02-12) deleted the application and the template with it, and `vm-instance` never had one — `git log --all -S networkDataSecretRef -- packages/apps/vm-instance/` is empty. The gate could not be ported verbatim because `systemDisk.image` went away in the same split, which is the likeliest reason nobody did. #2166 is the other regression from that migration. `docs/vm-external-vlan.md` has since documented the absence as a property of the chart and prescribed a `write_files` + `netplan apply` workaround, so this PR updates that page too. ### Why it is opt-in The network-data renders only when `netplanDhcp` is `true`, and the value defaults to `false`. Keying it on `instanceProfile` instead cannot work: the profile defaults to `ubuntu` whatever image the disk holds, and it is a KubeVirt preference rather than a declaration of the guest's network stack. cloud-init passes a v2 configuration through verbatim only for the netplan renderer; sysconfig, NetworkManager and eni turn each `match` stanza into an interface literally named `cozy-en` or `cozy-eth`, so a non-netplan guest handed this configuration loses DHCP on its real NIC. Two cases where the value must stay off, both stated in its description and in `docs/vm-external-vlan.md`: - A guest not rendered by netplan (AlmaLinux, Rocky, Alpine), for the reason above. - A guest that configures a NIC statically through netplan, as the external-VLAN recipe does. Netplan writes one networkd file per definition ID and systemd-networkd applies only the first `.network` file in lexical order that matches a link, so `10-netplan-cozy-en.network` shadows `10-netplan-enp2s0.network` and the static address is never applied, whatever number the user's netplan file carries. Reproduced with `netplan generate` on Ubuntu 24.04. Matching by MAC with a v1 configuration would be renderer-independent and would not shadow a named interface, but it needs the chart to know the MACs at render time; `lookup` cannot supply them on a first install, so the chart would have to pin them, which is a larger change that overlaps #4038. ### What this does not do A VM that leaves `netplanDhcp` unset renders exactly as before: no network-data Secret, and no cloud-init disk where there was none. Turning the value on for a VM with neither `cloudInit` nor `sshKeys` adds a cloud-init disk, and because KubeVirt uses the pinned firmware UUID as the NoCloud instance-id, a guest that has never seen that id treats its next boot as a first boot (new SSH host keys); the value's description says so. On a VM that already has a cloud-init disk the instance-id does not change, and it is derived from the namespace, the name and `cloudInitSeed`, so recreating the VM keeps it too; cloud-init applies network configuration per instance by default, so such a guest picks the network-data up once `cloudInitSeed` is changed. Static addressing is still `userData`. ### Tests `tests/network_data_test.yaml`, 6 cases: the rendered network-data asserted in full (both properties, not just the presence of the key), the VM carrying network-data with no user-data requested, user-data and network-data side by side, a VM on the default profile rendering neither the Secret nor a cloud-init disk, user-data alone when the value is off, and the value rather than the profile deciding. With the templates gated on the `ubuntu` profile again, the default-profile case and the value-off case go red. `helm unittest` green on all four suites of the package (32 tests). `make generate` output is committed (`values.schema.json`, `README.md`, `api/apps/v1alpha1/vminstance/types.go`, `packages/system/vm-instance-rd/`). `helm lint` fails on this package identically on an untouched tree (`_cluster.scheduling` nil pointer, `cozy-lib` dependency metadata), so it is not a signal here. ### Screenshots Not a UI change. ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> <!-- Trigger map walked file by file against the diff. `docs/vm-external-vlan.md` documents the absence this PR removes, which is the "make a documented workaround obsolete" trigger, so the website copies of that page need the same edit and issue 702 carries it. `netplanDhcp` is a new field in `values.schema.json`, which the provider models by hand, and issue 62 there carries it. No package added, renamed or removed; no hack/ or ApplicationDefinition change; the cozy-proxy annotations in templates/service.yaml are untouched. --> - [x] [cozystack/website](https://github.com/cozystack/website) - follow-up: cozystack/website#702 - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: cozystack/terraform-provider-cozystack#62 - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(vm-instance): a new `netplanDhcp` value, off by default, makes the chart supply cloud-init network-data that requests DHCP on every ethernet interface and accepts the DNS search domains offered over DHCP. A netplan guest (Ubuntu, Debian) attached to a VPC subnet then configures that interface on its own instead of leaving it unaddressed, and cluster service names under the cluster domain resolve inside the guest. Leave it off for guests not rendered by netplan and for guests that configure a NIC statically through netplan. VMs that do not set it are unchanged. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an opt-in setting to request DHCP on Ethernet interfaces in supported virtual machines while retaining DHCP-provided DNS search domains. * The setting is off by default. Enabling it can add a cloud-init disk and may interfere with static network settings or guests that do not use netplan. * **Documentation** * Updated setup guidance with considerations for enabling DHCP and configuring static networking on VLANs without a DHCP server. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Adds an optional
ipAddresstonetworks[](and to the deprecatedsubnets[]alias) of thevm-instancechart, so atenant can pin a static IPv4 on a secondary (VPC) interface. The address is handed to whichever network backend the
cluster serves, discovered through
.Capabilities.APIVersions.Has— never throughlookup:ipAddresskubeovn.io/v1IP(with or without Cozyplane)<network>.<namespace>.ovn.kubernetes.io/ip_address: <ip>sdn.cozystack.io/v1alpha1VPConlysdn.cozystack.io/networks: '[{"ip":"…","name":"pod<sha256(network)[:11]>","vpc":"<namespace>/<network>"}]'failwith a message naming the missing backendKube-OVN keeps precedence when both backends are served, consistent with #4013 and #4034. Without
ipAddresstherendered manifests are byte-identical to
mainfor all four combinations (checked withhelm templateon everyexisting test fixture), so no existing VM is affected.
The field is documented in
values.yaml;values.schema.json,README.md, the Go types and theApplicationDefinition are regenerated with
make generate(no drift).Why
networks[]only carriesname: a VM can join a VPC network but never receive a predictable address on it.Anything that must be reachable at a known address inside a tenant VPC (gateway, resolver, database peers have in
their config) has no way to express it, and the IPAM address changes on every recreation.
This supersedes #3900, which rendered the Kube-OVN annotation only. Both backends are covered here so that the same
values file works whichever CNI a cluster runs.
Cozyplane contract (measured, not inferred)
The Cozyplane delegate (lllamnyp/cozyplane#27) selects the annotation entry whose
nameequals the interface nameMultus passes (
CNI_IFNAME). On KubeVirt 1.8 that name ispod+ the first 11 hex characters ofsha256(network name), which the template computes withsha256sum $net.name | trunc 11on the same$net.nameused for the MultusnetworkName. An entry withoutnameis consumed by the primary attachment and moves the launcher'seth0ontothe VPC, so the template always names the interface.
Tests
tests/network_ipaddress_test.yaml(single suite, header explains the mocks): 5 Kube-OVN cases (pin, only pinnedinterfaces get the key, coexistence with the existing
lookupannotations,subnetsalias, both backends ⇒ Kube-OVN),3 Cozyplane cases (pin, list, alias), no backend ⇒
failedTemplate, no address ⇒ nothing rendered on either.main(the two "no address" guards pass there by design).make testgreen with the existing suites;helm lintclean.networks: [{name: subnet-…, ipAddress: 172.16.10.50}]⇒ Portv108.172-16-10-50owned by the VM, address visible in the guest, kept across stop/start, a second VM asking thesame address is refused by the CNI (
ip 172.16.10.50 is already taken in VPC). Kube-OVN: unit tests + identicalrender (no Kube-OVN cluster available to me).
Notes
templates/vm.yaml); will rebase onmainonce it lands.🤖 Generated with Claude Code
https://claude.ai/code/session_01LoggBxo8kGq53hnRQoiYfi
Summary by CodeRabbit
New Features
Documentation
ipAddresssettings.Tests
Assisted by Codex