Network: Add support for OVN uplink networks attached to a VLAN #14196
Conversation
Signed-off-by: Mark Bolton <[email protected]>
…witch ports Signed-off-by: Mark Bolton <[email protected]>
Signed-off-by: Mark Bolton <[email protected]>
boltmark
force-pushed
the
ovn-uplink-vlan
branch
from
1d70282
to
498b0b0
Compare
Signed-off-by: Mark Bolton <[email protected]>
|
Heads up @mionaalex - the "Documentation" label was applied to this issue. |
| @@ -119,8 +119,9 @@ type OVNSwitchPortOpts struct { | |||
| DHCPv4OptsID OVNDHCPOptionsUUID // Optional, if empty, no DHCPv4 enabled on port. | |||
| DHCPv6OptsID OVNDHCPOptionsUUID // Optional, if empty, no DHCPv6 enabled on port. | |||
| Parent OVNSwitchPort // Optional, if set a nested port is created. | |||
| VLAN uint16 // Optional, use with Parent to request a specific VLAN for nested port. | |||
| NestedVLAN uint16 // Optional, use with Parent to request a specific VLAN for nested port. | |||
There was a problem hiding this comment.
Shall we call this ParentVLAN to show it is related to Parent field?
| @@ -1269,6 +1270,10 @@ func (o *OVN) LogicalSwitchPortAdd(switchName OVNSwitch, portName OVNSwitchPort, | |||
| if opts.Location != "" { | |||
| args = append(args, "--", "set", "logical_switch_port", string(portName), fmt.Sprintf("external_ids:%s=%s", ovnExtIDLXDLocation, opts.Location)) | |||
| } | |||
|
|
|||
| if opts.VLAN != 0 { | |||
There was a problem hiding this comment.
If opts.VLAN == 0 we should clear the tag_request field (and confirm that doing that clears the tag field set by OVN) so that one can disable the VLAN setting later.
| @@ -3104,6 +3161,13 @@ func (n *ovn) Start() error { | |||
| return err | |||
| } | |||
|
|
|||
| // If VLAN config is provided to the OVN network, configure the necessary ports | |||
| // to handle traffic on the specified VLAN. | |||
| err = n.configurePortsForVLAN(n.config["vlan"], false) | |||
There was a problem hiding this comment.
Can we make this called, or be part of, startUplinkPort() as that can then avoid loading the uplink network from the DB again, and also it keeps the uplink port configuration together, rather than splitting it out.
| @@ -3218,6 +3282,12 @@ func (n *ovn) Update(newNetwork api.NetworkPut, targetNode string, clientType re | |||
| delete(newNetwork.Config, ovnVolatileUplinkIPv6) | |||
| } | |||
|
|
|||
| // Update port configurations for VLAN if necessary. | |||
| err = n.configurePortsForVLAN(newNetwork.Config["vlan"], true) | |||
There was a problem hiding this comment.
where do changes to uplink ports get applied during updates currently? Can we move this call there?
| @@ -2302,8 +2348,19 @@ func (n *ovn) setup(update bool) error { | |||
| return fmt.Errorf("Failed linking external router port to external switch port: %w", err) | |||
| } | |||
|
|
|||
| // If VLAN is provided, ensure localnet port is properly tagged. | |||
| localnetOpts := &openvswitch.OVNSwitchPortOpts{} | |||
| if n.config["vlan"] != "" { | |||
There was a problem hiding this comment.
what if vlan is unset, how is the vlan setting removed?
|
|
||
| ovs := openvswitch.NewOVS() | ||
| vars := n.uplinkPortBridgeVars(uplinkNet) | ||
| uplinkHostName := GetHostDevice(uplinkNet.Config()["parent"], uplinkNet.Config()["vlan"]) |
There was a problem hiding this comment.
Hrm, so referring to my question #12234 (comment) that we didn't discuss yet:
Should we add a vlan setting to the uplink network or each OVN network?
You've gone with adding the vlan setting to each OVN network in this PR.
Because each uplink network can be used by multiple OVN networks, this means there is the possibility of using multiple VLANs on a single uplink (which in principle would work as we are trunking VLANs through the uplink OVS bridge into the uplink interface).
But I don't think using the uplink's vlan setting in the call to GetHostDevice makes sense in that case.
I also see some other places in the existing code where we've called uplinkHostName := GetHostDevice and used the uplink's VLAN setting which needs to be reviewed as part of this PR.
Right now, say I had an eth0.101 interface (a physical port eth0, with an untagged VLAN 101 interface layered ontop) which I want to use for my uplink, you could create a physical network that has parent=eth0 and vlan=101 and when passing directly to an instance, LXD would check if the eth0.101 interface existed, and if not create it, before passing it to the instance.
For OVN uplink networks, I can't see that LXD would create the eth0.101 interface if it doesn't exist, but if it does exist we would then connect eth0.101 to the intermediate uplink OVS bridge like any other port.
But in this PR you are introducing per-OVN network uplink VLAN settings, so this VLAN setting on the uplink network conceptually conflicts with what we are trying to do, as you are trying to allow multiple OVN networks to trunk multiple VLANs through a single uplink network, whilst at the same time allowing the uplink port to be configured with a single untagged VLAN (which wouldn't work with tagged frames).
But stepping back a bit further, lets forget the uplink's existing vlan setting for a moment.
Say we go ahead with your proposal here of adding vlan to the OVN network itself, then if I have two OVN networks both connected to the same uplink but using two different uplink VLAN IDs, how do we allocate an IP for the OVN network's external (volatile) IP on the uplink network, as each OVN network is effectively connected to different uplink networks (because each VLAN on the uplink network is likely going to have different IP ranges, or at least conceptually separate IP ranges).
So going back to my question above:
Should we add a vlan setting to the uplink network or each OVN network?
I actually think we should keep the vlan setting on the uplink network.
Don't introduce a vlan setting on the OVN network.
This would then require that the administrator define separate uplink networks in LXD for each VLAN on the parent interface, along with the separate IP range settings (which is what we want).
So the question then becomes: How should we use the vlan setting on the uplink network?
Also it seems that whilst the OVN network doesn't create the vlan interface (e.g. eth0.101), if the uplink is a physical network it does seem to create it:
https://github.com/canonical/lxd/blob/main/lxd/network/driver_physical.go#L318
So, because we already have uplinkHostName := GetHostDevice(uplinkConfig["parent"], uplinkConfig["vlan"]) in startUplinkPortPhysical could it be that if you use an OVN network with a physical network as an uplink using a dedicated interface (not a bridge) that VLAN support already works?
There was a problem hiding this comment.
could it be that if you use an OVN network with a physical network as an uplink using a dedicated interface (not a bridge) that VLAN support already works?
I've just tested this and it works already, but only for physical uplinks, not bridged uplinks.
We should probably just add the last step from #12234 (comment) startUplinkPortBridgeNative and the equivalent to startUplinkPortBridgeOVS to this PR instead:
bridge vlan add dev vid 101
This PR adds support for OVN uplink networks attached to a VLAN. The implementation details closely track #12234 (comment).
Testing here: canonical/lxd-ci#302.
Closes #12234.