Network: Add support for OVN uplink networks attached to a VLAN #14196

boltmark · 2024-10-03T09:03:48Z

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.

tomponline · 2024-10-04T09:23:05Z

lxd/network/openvswitch/ovn.go

@@ -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.


Shall we call this ParentVLAN to show it is related to Parent field?

tomponline · 2024-10-04T09:24:01Z

lxd/network/openvswitch/ovn.go

@@ -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 {


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.

tomponline · 2024-10-04T09:36:01Z

lxd/network/driver_ovn.go

@@ -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)


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.

tomponline · 2024-10-04T09:36:37Z

lxd/network/driver_ovn.go

@@ -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)


where do changes to uplink ports get applied during updates currently? Can we move this call there?

tomponline · 2024-10-04T09:37:06Z

lxd/network/driver_ovn.go

@@ -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"] != "" {


what if vlan is unset, how is the vlan setting removed?

tomponline · 2024-10-04T10:10:11Z

lxd/network/driver_ovn.go

+
+	ovs := openvswitch.NewOVS()
+	vars := n.uplinkPortBridgeVars(uplinkNet)
+	uplinkHostName := GetHostDevice(uplinkNet.Config()["parent"], uplinkNet.Config()["vlan"])


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?

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

Thanks @tomponline. I've pushed changes which better handle the case where the uplink parent is a bridge and the vlan config is set -- we now avoid using the physical interface lxd creates on top of the bridge. This is WIP as I have a couple questions:

We should probably just add the last step from #12234 (comment) startUplinkPortBridgeNative and the equivalent to startUplinkPortBridgeOVS to this PR instead:

Can you help me to understand what we would need to add to startUplinkPortBridgeOVS?

Also, since the vlan config is set on the uplink network, I'm not sure what the best approach is to ensure ports are reconfigured when the uplink network vlan config is changed. Would we need to add a check in the network update handler which notifies cluster members to update their ports if (1) the updated network is used as an uplink for an OVN network and (2) the vlan config is changed? This seems a bit indirect so I'm wondering if there is something better we can do.

tomponline · 2024-10-14T11:54:37Z

lxd/network/driver_ovn.go

@@ -1853,7 +1866,7 @@ func (n *ovn) deleteUplinkPortPhysical(uplinkNet Network) error {
 	uplinkHostName := GetHostDevice(uplinkConfig["parent"], uplinkConfig["vlan"])

 	// Detect if uplink interface is a native bridge.
-	if IsNativeBridge(uplinkHostName) {
+	if IsNativeBridge(uplinkHostName) || IsNativeBridge(uplinkConfig["parent"]) && uplinkConfig["vlan"] != "" {


do we need brackets for the logic here?

Sure, added parens to improve readability

…parents Signed-off-by: Mark Bolton <[email protected]>

This PR adds API extension `network_ovn_uplink_vlan`, which adds support for using a bridge network with a specified VLAN ID as the uplink for an OVN network. Related: canonical/lxd-ci#302, #14196.

boltmark requested a review from tomponline October 3, 2024 09:03

boltmark force-pushed the ovn-uplink-vlan branch from 1d70282 to 498b0b0 Compare October 3, 2024 09:18

github-actions bot added the Documentation Documentation needs updating label Oct 3, 2024

tomponline reviewed Oct 4, 2024

View reviewed changes

boltmark force-pushed the ovn-uplink-vlan branch from 61bcd0b to 0d0a24d Compare October 10, 2024 04:00

github-actions bot removed the Documentation Documentation needs updating label Oct 10, 2024

tomponline reviewed Oct 14, 2024

View reviewed changes

boltmark force-pushed the ovn-uplink-vlan branch from 0d0a24d to 29cbe12 Compare October 15, 2024 03:51

lxd/network: Support VLAN tagging for OVN uplinks with native bridge …

920700b

…parents Signed-off-by: Mark Bolton <[email protected]>

boltmark force-pushed the ovn-uplink-vlan branch from 29cbe12 to 920700b Compare October 15, 2024 03:54

tomponline approved these changes Oct 15, 2024

View reviewed changes

tomponline merged commit dcaabf6 into canonical:main Oct 15, 2024
29 checks passed

tomponline mentioned this pull request Oct 16, 2024

Add testing for OVN uplink attached to VLAN canonical/lxd-ci#302

Merged

boltmark mentioned this pull request Oct 18, 2024

api: Add network_ovn_uplink_vlan API extension #14296

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network: Add support for OVN uplink networks attached to a VLAN #14196

Network: Add support for OVN uplink networks attached to a VLAN #14196

boltmark commented Oct 3, 2024

tomponline Oct 4, 2024

tomponline Oct 4, 2024 •

edited

Loading

tomponline Oct 4, 2024

tomponline Oct 4, 2024

tomponline Oct 4, 2024

tomponline Oct 4, 2024

tomponline Oct 4, 2024

boltmark Oct 10, 2024

tomponline Oct 14, 2024

boltmark Oct 15, 2024

Network: Add support for OVN uplink networks attached to a VLAN #14196

Network: Add support for OVN uplink networks attached to a VLAN #14196

Conversation

boltmark commented Oct 3, 2024

Choose a reason for hiding this comment

tomponline Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomponline Oct 4, 2024 •

edited

Loading