Expose vlan trunking in metadata/configdrive#620
Conversation
joker-at-work
left a comment
joker-at-work
left a comment
There was a problem hiding this comment.
Should we maybe extend nova/tests/unit/test_metadata.py's similar to test_network_data_response() to ensure these subports properly end up in a VM-visible part?
| for _vif in self['trunk_vifs']: | ||
| if vif['id'] == _vif['id']: |
There was a problem hiding this comment.
optional equivalent:
if any(vif['id'] == _vif['id'] for _vif in self['trunk_vifs']):
return| self._populate_trunk_info( | ||
| vif, current_neutron_port, context, client) |
There was a problem hiding this comment.
Why do we call it explicitly and not as part of _build_vif_model()? Feels like we're preparing future errors where we build a vif model but forget to add this call.
| instance=instance | ||
| ) | ||
|
|
||
| def _populate_trunk_info(self, vif, current_neutron_port, context, client): |
There was a problem hiding this comment.
Having context and client not as the first arguments feels inconsistent with the other methods of this class. Can we make them the first/second argument after self?
| port = self._show_port(context, port_id, | ||
| neutron_client=client) | ||
| subport_network = client.show_network( | ||
| port['network_id'])['network'] |
There was a problem hiding this comment.
This seems quite costly. Could we maybe using a "list port" endpoint filtering for the port ids and network ids in some kind of pre-fetch before we're trying to build the vif models?
We're doing something similar here in hammer for router ports.
| subport_vif = self._build_vif_model( | ||
| context, client, port, [subport_network], | ||
| [port_id]) | ||
| subport_profile = dict(subport_vif['profile'] or {}) |
There was a problem hiding this comment.
Why do we need to convert to a dict() here? What could subport_vif['profile'] be that we'll need this, a list of tuples?
| continue | ||
|
|
||
| parent_vif = None | ||
| if vif['type'] == 'trunk-subport': |
There was a problem hiding this comment.
I think we should use model.VIF_TYPE_TRUNK_SUBPORT here instead of the hard-coded string.
| vif_profile = vif.get('profile') | ||
| if not vif_profile: | ||
| continue |
There was a problem hiding this comment.
I think this could be well-expressed with the walrus-operator:
if not (vif_profile := vif.get('profile')):
continuesame below
| parent_vif = get_vif_from_network_info(parent_vif_id, | ||
| network_info) |
There was a problem hiding this comment.
We could™ instead of building a list of trunk_vifs build a list of tuples (vif, parent_vif) in the loop in line 200, having (vif, None) for parent vifs. Then we wouldn't have to iterate over network_info for every single subport again. parent_vif would be the vif we're a subport of, which we have in the first for. We wouldn't do the whole vif.get('profile') and `vif_profile.get('parent_name') necessarily though. Do you feel like it's needed?
|
|
||
| if nic_type == 'vlan': | ||
| link.update({ | ||
| 'vif_id': vif['id'], |
There was a problem hiding this comment.
vif_id is already part of link and has the same value.
| if nic_type == 'vlan': | ||
| link.update({ |
There was a problem hiding this comment.
optional: We could put and additional_link_data or something into the if above, so we don't have 2 if nic_type == 'vlan' 12 lines apart.
Add neutron trunk subports to network_data.json so cloud-init can bring up the VLAN subinterfaces in the guest. Subport VIFs hang off their parent in a new trunk_vifs field and get flattened into vlan links (vlan_link, vlan_id, vlan_mac_address) when the metadata is rendered. Based on upstream change [1] (blueprint expose-vlan-trunking [2], spec [3]), with a few cosmetic fixes and a performance improvement replacing the per-subport show_port / show_network calls with single bulk list_ports / list_networks calls. [1] https://review.opendev.org/c/openstack/nova/+/941227 [2] https://blueprints.launchpad.net/nova/+spec/expose-vlan-trunking [3] https://review.opendev.org/c/openstack/nova-specs/+/471815 Change-Id: I2b7c017a659adc0fd3c7ac363b29e2f67f145bea
leust
force-pushed
the
expose-vlan-trunking
branch
from
8b6306d to
8b0b977
Compare
2 participants
Add neutron trunk subports to network_data.json so cloud-init can bring up the VLAN subinterfaces in the guest. Subport VIFs hang off their parent in a new trunk_vifs field and get flattened into vlan links (vlan_link, vlan_id, vlan_mac_address) when the metadata is rendered.
Based on upstream change [1] (blueprint expose-vlan-trunking [2], spec [3]) with these fixes:
[1] https://review.opendev.org/c/openstack/nova/+/941227
[2] https://blueprints.launchpad.net/nova/+spec/expose-vlan-trunking
[3] https://review.opendev.org/c/openstack/nova-specs/+/471815
Change-Id: Iad2d56f85a0a8b2524bc047d64d8e9a7514e4aa4