fix(neutron-understack): lift vlan trunk restrictions

[stevekeay]

The check was added to prevent the user from claiming a segmentation ID
that happens to be the same as the parent port of this vlan. We don't
want to allow that situation because the switch (stupidly) rejects that
configuration.

However, the way the check was implemented was too restrictive - it
banned a range of thousands of VLAN IDs. It did successfully prevent
all theoretical clashes present and future, but it also prevented
legitimate configurations and caused an upset customer.

We update this check so that it now compares the VLAN ID of the actual
native VLAN on this trunk interface, and only raises an error if the
user is configuring a number that would currently clash.

The theoretical valid range is 1-4094 according to 802.1Q.

However we would avoid VLAN 1, and we must avoid our nxos "reserved" vlan range 3872-3999.

On Nexus 9000 NX-OS, Cisco treats VLANs 4048-4093 as their own. These
might work, but even if they work today, they might randomly stop
working at some future point.

So this puts the realistic useable VLAN range at 2-3871 inclusive.

There was an existing config setting that seemed to do the right thing so I
piggy-backed on that rather than create another that does almost exactly
the same thing.

Result of testing in dev, first the setup:

openstack network create mynet-one
openstack network create mynet-two
openstack network create mynet-three

openstack subnet create mynet-one --no-dhcp --subnet-range 192.168.1.0/24 --network mynet-one
openstack subnet create mynet-three --no-dhcp --subnet-range 192.168.3.0/24 --network mynet-three --gateway none
openstack subnet create mynet-two --no-dhcp --subnet-range 192.168.2.0/24 --network mynet-two --gateway none

openstack port create myserver-mynet-one --network mynet-one
openstack port create myserver-mynet-three --network mynet-three
openstack port create myserver-mynet-two --network mynet-two

openstack network trunk create myserver-trunk --parent-port myserver-mynet-one
openstack network trunk set myserver-trunk --subport port=myserver-mynet-two,segmentation-type=vlan,segmentation-id=1802

openstack server create steve-test --port myserver-mynet-one --image 5048de36-4c01-4047-b638-195e639cb1e3 --flavor gp2.small

Now note that the above would not have worked on the previous code, we get the follwing error:

$ openstack network trunk set myserver-trunk --subport port=myserver-mynet-two,segmentation-type=vlan,segmentation-id=1802
Failed to add subports to trunk 'myserver-trunk': HttpException: 500: Server Error for url: https://neutron.iad3-dev.undercloud.rackspace.net/v2.0/trunks/09ca8703-ebe2-46b9-a144-43b63f34f1cf/add_subports, Segmentation ID: 1802 cannot be set to the Subport: 9395144f-d61a-4a40-b0e9-48cb5d9dbe7a as it falls outside of allowed ranges: 1-1799. Please use different Segmentation ID.

With the new code as per this PR, we are able to use vlan IDs outside the previous comfort zone.

I observed that the server was set up with a trunk on the switch having native vlan 1804.

We can now test the collision detection:

$ openstack network trunk set myserver-trunk --subport port=myserver-mynet-three,segmentation-type=vlan,segmentation-id=1804

Failed to add subports to trunk 'myserver-trunk': HttpException: 500: Server Error for url: https://neutron.iad3-dev.undercloud.rackspace.net/v2.0/trunks/09ca8703-ebe2-46b9-a144-43b63f34f1cf/add_subports, Segmentation ID: 1804 cannot be set to the Subport: 964bf1e7-2676-4971-85d2-b326361c6f2c because it matches the native VLAN on physical network: f20-2-network. Please use a different Segmentation ID.

$ openstack network trunk set myserver-trunk --subport port=myserver-mynet-three,segmentation-type=vlan,segmentation-id=1803
<success>

Here we note that using the native VLAN is rejected, whereas using other vlans is allowed.

[github-actions]

Python test results

63 tests   63 ✅  6s ⏱️
 1 suites   0 💤
 1 files     0 ❌

Results for commit 9b8e101.

♻️ This comment has been updated with latest results.

[mfencik]

Just a small thing, please test this in dev properly.

[cardoe]

Two things I'd like addressed before this merges.

1. Release the dynamic segment when the native VLAN check fails

In _bind_port_segment we call allocate_dynamic_segment and only afterwards run the native VLAN collision check via configure_trunk. If that check raises SubportSegmentationIDError, nothing releases the segment we just allocated, so it leaks (same class of problem as the vif-detach leak tracked by BM-BIND-04).

This is more reachable now than before. Subport tags used to be kept out of the segment allocation ranges, so a newly allocated native VLAN could never equal an existing tag. With the default range now [2, 3871] the allocator can hand back a VLAN that matches an existing subport tag. The precommit check only helps when a native segment already exists, so a first-time bind falls through to this path.

We either need to run the collision check before allocating, or release the segment in the failure path.

understack/python/neutron-understack/neutron_understack/understack_mech.py

Lines 412 to 429 in 8498de8

	dynamic_segment = context.allocate_dynamic_segment(
	segment={
	"network_type": p_const.TYPE_VLAN,
	"physical_network": physnet,
	},
	)
	LOG.debug("bind_port_segment: Native VLAN segment %s", dynamic_segment)
	trunk_details = context.current.get("trunk_details") or {}
	port_id = context.current["id"]
	if trunk_details:
	self.trunk_driver.configure_trunk(
	trunk_details=trunk_details,
	port_id=port_id,
	physical_network=physnet,
	native_vlan_id=dynamic_segment[api.SEGMENTATION_ID],
	)

2. Gate _check_trunk_native_vlan on baremetal ports

update_port_precommit calls the new _check_trunk_native_vlan for any port with trunk_details, without the is_baremetal_port guard the other paths use. A non-baremetal trunk port with a changed physical_network in its binding profile will hit network_segment_by_physnet and fetch_network_node_trunk_id(). That second call is the expensive one that was flagged in the review of #1377, and it is now also reached on the port update path, in addition to configure_trunk.

Please gate it the same way the other precommit/postcommit paths do.

understack/python/neutron-understack/neutron_understack/understack_mech.py

Lines 190 to 193 in 8498de8

	def update_port_precommit(self, context: PortContext):
	self._check_trunk_native_vlan(context)

understack/python/neutron-understack/neutron_understack/trunk.py

Lines 175 to 190 in 8498de8

	def check_trunk_native_vlan(
	self,
	trunk_details: dict,
	physical_network: str,
	native_vlan_id: int,
	) -> None:
	"""Check a candidate parent native VLAN against existing subports."""
	if trunk_details["trunk_id"] == utils.fetch_network_node_trunk_id():
	return
	self._check_native_vlan_conflict(
	subports=trunk_details.get("sub_ports", []),
	trunk_id=trunk_details["trunk_id"],
	physical_network=physical_network,
	native_vlan_id=native_vlan_id,

[cardoe]

My second comment was the second one as Milan and has been addressed.

[cardoe]

4ee9c29 a change to address my first comment if you want to cherry-pick that.