Repository navigation
Conversation
00a2d00 to
f625af3
Compare
|
Rebased onto current The change is ready for review:
Happy to adjust anything you want changed. |
|
@MorningLightMountain713 or @TheTrunk ready for review when you have a moment. Rebased onto current |
MorningLightMountain713
left a comment
MorningLightMountain713
left a comment
There was a problem hiding this comment.
Thanks for this, and for the thorough tests. Accepting SDN VNets is the right call: a VNet is a Linux bridge on the node, the VM attaches to it exactly as it does to vmbr0, and nothing inside FluxOS cares which one it is. I'd like the check to work a different way before we merge, though.
Check per node, not cluster-wide. /cluster/sdn/vnets returns the cluster's SDN config. A VNet that has been created but not yet applied is listed, and so is a VNet whose zone is restricted to other nodes. In both cases the pre-flight passes and the VM then fails to start, which is the exact failure the pre-flight exists to catch.
GET /nodes/{node}/network?type=any_bridge returns the bridges on that node including the SDN VNets applied there. It's the same list the Proxmox UI offers in a VM's NIC bridge dropdown. That gives one call, scoped to the target node, reflecting applied state, and get_sdn_vnets goes away. Since you have an SDN setup, could you confirm against it that an applied VNet shows up there with "type": "vnet" and that a pending one doesn't?
As a bonus it tightens the existing check, which currently accepts any interface (eth0, bond0, vmbr0.50), none of which a VM can attach to.
Comments. We keep comments to the contract or constraint the next reader needs, stated as it stands. The reasoning for a change goes in the commit message, not the code. Details inline.
Docs. Please add a line to docs/configuration.md: when the VNet is in a VLAN zone, the tag is applied by the host, so network.vlan in the fluxnode config must be left unset. Otherwise the guest tags a second time and gets no network.
| """Validate that the network is a classic bridge or an SDN VNet. | ||
|
|
||
| if not res: | ||
| return False | ||
| Classic bridges come from /nodes/{node}/network. SDN VNets do not | ||
| appear there; they are listed by /cluster/sdn/vnets. Provisioning | ||
| already accepts either name as the VM bridge, so the pre-flight | ||
| check must accept both. A token without SDN access fails closed. | ||
| """ | ||
| res = await self.api.get_networks(node) | ||
|
|
||
| network_exists = next(filter(lambda x: x.get("iface") == network, res.payload), None) | ||
| if res and res.payload and any(x.get("iface") == network for x in res.payload): | ||
| return True | ||
|
|
||
| return bool(network_exists) | ||
| # SDN VNets are not returned by /nodes/{node}/network. | ||
| vnets = await self.api.get_sdn_vnets() | ||
| return bool(vnets and vnets.payload and any(v.get("vnet") == network for v in vnets.payload)) |
There was a problem hiding this comment.
With type=any_bridge this collapses to a single lookup. Something like:
async def validate_network(self, node: str, network: str) -> bool:
"""Whether `network` is a bridge a VM on `node` can attach to: a Linux or
OVS bridge, or an SDN VNet applied on that node."""
res = await self.api.get_networks(node)
return bool(res and res.payload and any(x.get("iface") == network for x in res.payload))On the comments as written:
- "Provisioning already accepts either name as the VM bridge, so the pre-flight check must accept both" is the argument for this PR, which belongs in the commit message. Six months from now the reader needs to know what the function accepts, not why it was changed.
- "SDN VNets do not appear there; they are listed by /cluster/sdn/vnets" describes the implementation line by line, and with
any_bridgeit's no longer true. # SDN VNets are not returned by /nodes/{node}/network.repeats the docstring. A comment should earn its place by saying something the code can't, and this one doesn't.
|
|
||
| return res | ||
|
|
||
| async def get_sdn_vnets(self) -> ApiResponse: |
There was a problem hiding this comment.
This can be dropped in favour of filtering get_networks, its only caller being validate_network:
async def get_networks(self, node: str) -> ApiResponse:
"""The bridges on `node` a VM can attach to, SDN VNets applied there included."""
return await self._do_get(f"nodes/{node}/network?type=any_bridge")A docstring here should state what the call returns, not what some other endpoint doesn't.
|
|
||
| assert result is False | ||
|
|
||
| async def test_validate_network_classic_unavailable_sdn_ok( |
There was a problem hiding this comment.
With a single lookup this case goes away: if the node's network listing fails, validation fails. That's the behaviour we want, because we can't vouch for a bridge we couldn't list.
|
|
||
| assert result is False | ||
|
|
||
| async def test_validate_network_vlan_iface_not_treated_as_vnet( |
There was a problem hiding this comment.
vmbr0.50 is a VLAN interface, not a bridge, so a VM can't attach to it and this should fail validation. With type=any_bridge the API won't return it. I'd swap this for tests that the request carries type=any_bridge, and that an entry with "type": "vnet" passes the same way a "type": "bridge" one does.
The # A classic VLAN interface is a bridge name, not an SDN VNet. comment goes with it.
|
Heads-up: master moved under this PR, so it will need a rebase onto What changed, briefly:
For this PR, with the res = await self.api.get_networks(node)
return bool(res) and any(x["iface"] == network for x in res.payload) |
836f9ee to
8010970
Compare
|
Rebased onto current Confirmed on my cluster. The implementation matches the review:
|
8010970 to
fc0c910
Compare
validate previously listed every interface on the node, so it accepted
names a VM cannot attach to (eth0, bond0, vmbr0.50) and rejected SDN
VNets, which provisioning already passes through as the VM bridge.
get_networks now requests /nodes/{node}/network?type=any_bridge, the
bridges on that node including the SDN VNets applied there. Validation
is a single lookup against that list. If the listing fails, validation
fails. A VNet that exists in the cluster config but is not applied on
the node fails, which is the case the pre-flight exists to catch.
When the VNet is in a VLAN zone the host applies the tag, so
network.vlan must be left unset or the guest tags a second time.
de67da0 to
0fea49e
Compare
Summary
arcane-mage validatepreviously checked/nodes/{node}/networkwith no filter. That lists every interface on the node, including ones a VM cannot attach to (eth0,bond0,vmbr0.50), and it does not include SDN VNets. Provisioning already accepts a VNet name as the VM bridge, so the pre-flight was both stricter and looser than the path that creates the VM.This change makes the pre-flight match provisioning.
Validation logic
Provisioner.validate_networkdoes one lookup:GET /nodes/{node}/network?type=any_bridgeThat is the bridges on the target node, including the SDN VNets applied there. It is the same list the Proxmox UI offers in a VM's NIC bridge dropdown. A name passes if it matches an
ifacein that list.Behaviour:
vmbr0) pass.flux50) pass. They appear with"type": "vnet"./cluster/sdn/vnetsis not consulted.eth0,bond0,vmbr0.50) fail. They are not returned bytype=any_bridge.Why this is safe
Provisioning already treated the network name as an opaque bridge string and passed it through to the VM config. The pre-flight now accepts exactly the names the hypervisor will accept on that node, and nothing else. No new permissions are required.
Test coverage
type: vnet) → passget_networksrequestsnodes/{node}/network?type=any_bridgeDocs
docs/configuration.mdnotes that when the VNet is in a VLAN zone the tag is applied by the host, sonetwork.vlanmust be left unset.Checklist
master(typed responses, 3.0.0)