From 5146e0a0f85fa7202229ec1b063311afb9316810 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Fri, 4 Apr 2025 11:56:35 -0700 Subject: [PATCH] provide host_id to neutron early on Cases exist where you can request a port to be created, in such a way where neutron has to defer assignment of addressing. In Neutron's world, it wants to map the host so it can be ensured that *correct* physical network is available and mapped to for the address range. Networking-baremetal helps backfill physical connection context so Neutron can make the appropriate address assignment. In order to ensure we do the right thing while also ensuring appropriate security around the state of the port and the bind, we need to go ahead and facilitate an *initial* context to neutron so address assignment can occur, but explicitly limits the provided information to a highly limited scope to prevent actual binding to a physical port. With the assignment of addresses, it becomes possible to begin to generate, or eventually *fix* network configuration metadata being provided to a baremetal node. In this *entire* process, we also identified you can create neutron ports with binding information "out of the gate" to be immediately bound. Thanks metalsmith! So the code pattern now checks the port, and unbinds the VIF before rebinding it. Related-Bug: #2106073 Change-Id: Ic53c626afe641ce63d71a7858e65df1fb250e3c0 --- doc/source/admin/networking.rst | 52 ++++++++++++-- ironic/common/neutron.py | 49 ++++++++++++- ironic/conductor/manager.py | 4 ++ ironic/drivers/modules/network/common.py | 63 ++++++++++++++++ ironic/tests/unit/common/test_neutron.py | 72 +++++++++++++++++++ .../drivers/modules/network/test_common.py | 55 ++++++++++++-- .../initial-port-bind-5a1ffb083f6631d4.yaml | 16 +++++ 7 files changed, 298 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/initial-port-bind-5a1ffb083f6631d4.yaml diff --git a/doc/source/admin/networking.rst b/doc/source/admin/networking.rst index 627bf1452d..e0e03b0a3a 100644 --- a/doc/source/admin/networking.rst +++ b/doc/source/admin/networking.rst @@ -107,9 +107,10 @@ the :oslo.config:option:`enabled_network_interfaces` setting. VIF Attachment flow ------------------- -When creating a VIF, the action occurs against the Neutron Networking Service, -such as by using the ``openstack port create`` command, and then the port ID -is submitted to Ironic to facilitate a VIF attachment. +When creating a virtual interface (VIF), the action occurs against the +Neutron Networking Service, such as by using the ``openstack port create`` +command, and then the port ID is submitted to Ironic to facilitate a VIF +attachment. .. NOTE:: Instances created with Nova can have a Neutron port created under a variety @@ -149,10 +150,47 @@ ports, the maximum number of VIFs is determined by the number of configured and available ports represented in Ironic, as framed by the suitablity criteria noted above. -When Ironic goes to attach *any* supplied, selected, or even self-created -VIFs, Ironic explicitly sets the physical port's MAC address for which the -VIF will be bound. If a node is already in an ``ACTIVE`` state, then the -vif attachment is updated with Neutron. +Ironic goes to attach *any* supplied, selected, or even self-created +VIFs, in two distinct steps. + +If a host is *not* active: + +The first step, as a result of +`bug 2106073 `_, +Ironic provides an initial ``host_id`` to Neutron so neutron can perform +any deferred address assignment to enable appropriate network mapping. This +initial binding lacks all detail to enable binding to an exact port. +For a short period of time, the VIF binding may show as "ACTIVE" in Neutron. + +.. NOTE:: + If the port was bound in *advance* of being submitted to Ironic, + and we must perform an initial bind, the port will be unbound and rebound + as part of the workflow. + +Once we're handing the instance over to the user, Ironic sets the physical +port's MAC address, and provides additional context, such as the physical +switch and switchport metadata which may be used. This action effectively +"re-binds" the port, and the port state in Neutron can change as a result +if binding is successful or fails. That may, ultimately, have no realistic +impact to availability of ability to pass traffic over an interface, but +can be rooted in the overall ``network_interface`` driver in use as well. + +.. NOTE:: + Ironic's ``network_interface`` driver selection does influence the base + binding model behavior and as such the resulting state as reported by + Neutron. Specifically ``flat`` assumes a pre-wired, static, never changing + physical network. Neutron port states indicating a failure when using the + ``flat`` interface is often more a sign of the ``networking-baremetal`` + ML2 plugin not being configured in Neutron. + The ``neutron`` interface is far more dynamic and the binding state can + generally be relied upon if any operator configured ML2 plugins are + functional. + +If the host in in an *active* state: + +Ironic explicitly sets the physical port's MAC address for which the +VIF will be bound, and is immediately attached to the host with any +required metadata for port binding, which is then transmitted to Neutron. When Ironic goes to unbind the VIF, Ironic makes a request for Neutron to "reset" the assigned MAC address in order to avoid conflicts with Neutron's diff --git a/ironic/common/neutron.py b/ironic/common/neutron.py index c3ca005dc9..cb32d92cc6 100644 --- a/ironic/common/neutron.py +++ b/ironic/common/neutron.py @@ -141,16 +141,61 @@ def unbind_neutron_port(port_id, client=None, context=None, reset_mac=True): raise exception.NetworkError(msg) -def update_port_address(port_id, address, context=None): +def unbind_neutron_port_if_bound(port_id, client=None, context=None): + """Check and if bound, unbind a neutron port. + + A wrapper around unbind_neutron_port which checks if the port is already + bound, and if so then triggers the port to be unbound. This is critical + early on if a user has pre-bound the port. + + If the port is missing from Neutron, then a NetworkError exception will + also be raised because in this code path, we explicitly expect the port + to already exist and be accessible. + + :param port_id: Neutron Port ID + :param client: Optional Neutron client. + :param context: request context + :type context: ironic.common.context.RequestContext + :raises: NetworkError + """ + is_bound = None + if not client: + client = get_client(context=context) + try: + port = client.get_port(port_id) + if not (port.binding_host_id is None or port.binding_host_id == ''): + is_bound = True + except openstack_exc.ResourceNotFound: + msg = (_('The neutron port %(port_id)s was not found when we expect ' + 'it to exist. We cannot proceed.') % {'port_id': port_id}) + raise exception.NetworkError(msg) + except openstack_exc.OpenStackCloudException as e: + msg = (_('An unknown error was encountered while attempting ' + 'to retrieve neutron port %(port_id)s. Error: ' + '%(err)s') % {'port_id': port_id, 'err': e}) + raise exception.NetworkError(msg) + if is_bound: + # If bound, then trigger an unbind using the already created + # context and client. + LOG.debug('Attempting to unbind port %s because it is already ' + 'bound in Neutron.', port_id) + # Don't use a client or overall context + unbind_neutron_port(port_id) + + +def update_port_address(port_id, address, context=None, client=None): """Update a port's mac address. :param port_id: Neutron port id. :param address: new MAC address. :param context: request context :type context: ironic.common.context.RequestContext + :param client: A neutron client object. :raises: FailedToUpdateMacOnPort """ - client = get_client(context=context) + if not client: + client = get_client(context=context) + port_attrs = {'mac_address': address} try: diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 304f95c5ce..afe2a4fcad 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -3337,6 +3337,10 @@ class ConductorManager(base_manager.BaseConductorManager): with task_manager.acquire(context, node_id, purpose='attach vif') as task: task.driver.network.validate(task) + # NOTE(TheJulia): It is the responsibility of the vif_attach + # method provided by the driver to raise an appropriate + # exception should it identify an error which would apply + # in *its* context of execution. task.driver.network.vif_attach(task, vif_info) LOG.info("VIF %(vif_id)s successfully attached to node %(node_id)s", {'vif_id': vif_info['id'], 'node_id': node_id}) diff --git a/ironic/drivers/modules/network/common.py b/ironic/drivers/modules/network/common.py index 9e790d456e..018a0ef024 100644 --- a/ironic/drivers/modules/network/common.py +++ b/ironic/drivers/modules/network/common.py @@ -304,6 +304,65 @@ def plug_port_to_tenant_network(task, port_like_obj, client=None): raise exception.NetworkError(msg) +def update_port_host_id(task, vif_id, client=None): + """Send an initial host_id value to Neutron to enable allocation. + + :param task: A TaskManager instance. + :param vif_id: The VIF ID to "attach" to the host. + :param client: A neutron client object + :raises: NetworkError if failed to update Neutron port. + :raises: VifNotAttached if tenant VIF is not associated with port_like_obj. + """ + + node = task.node + + if not vif_id: + # Nothing to do here, there *is* no VIF to update. + return + + if not client: + client = neutron.get_client(task.context) + + LOG.debug('Seeding network configuration for VIF %(vif_id)s to node ' + '%(node_id)s', + {'vif_id': vif_id, 'node_id': node.uuid}) + # We cannot perform an initial/early bind if the port is already + # bound. For example, Metalsmith users pre-pass the host_id to + # neutron, but then causes this patch to fail in such a case. + neutron.unbind_neutron_port_if_bound(vif_id, context=task.context, + client=client) + + # NOTE(TheJulia): Just provide enough data through to neutron + # to start initial binding, but ultimately this entire binding + # sequence won't succeed because it cannot succeed. No binding + # profile is included, so an ML2 plugin cannot act on the change + # and effectively neutron should treat the port update as a NOOP, + # except if deferred addressing is the case, then that should + # trigger. + port_attrs = {'binding:vnic_type': neutron.VNIC_BAREMETAL, + 'binding:host_id': node.uuid, + 'binding:profile': {}} + try: + # This cannot be done with the prior/existing client, because they + # bring a different context and thus API rights. In other words, + # we, as an adminy service need to do this. + neutron.update_neutron_port(task.context, vif_id, port_attrs, + client=None) + except openstack_exc.OpenStackCloudException as e: + # If his action has failed, the possibility exists that the port + # was previously bound *and* allocated to another network, and thus + # cannot be properly bound later *either*. This exception should + # cause the deployment flow to never be triggered if there is an + # underlying issue which only neutron can see like wrong segment + # for the vif. + msg = (_('Could not seed network configuration for VIF %(vif)s' + 'to node %(node)s, possible network, state, or access ' + 'permission issue. %(exc)s') % + {'vif': vif_id, 'node': node.uuid, 'exc': e}) + LOG.error(msg) + raise exception.NetworkError(msg) + + class VIFPortIDMixin(object): """VIF port ID mixin class for non-neutron network interfaces. @@ -576,6 +635,10 @@ class NeutronVIFPortIDMixin(VIFPortIDMixin): # NOTE(vsaienko) allow to attach VIF to active instance. if task.node.provision_state == states.ACTIVE: plug_port_to_tenant_network(task, port_like_obj, client=client) + else: + # Sends *just* a host_id to trigger neutron to do the initial + # address work (if needed...) + update_port_host_id(task, vif_id, client=client) def vif_detach(self, task, vif_id): """Detach a virtual network interface from a node diff --git a/ironic/tests/unit/common/test_neutron.py b/ironic/tests/unit/common/test_neutron.py index ceb6763b2e..93cebe5c57 100644 --- a/ironic/tests/unit/common/test_neutron.py +++ b/ironic/tests/unit/common/test_neutron.py @@ -1254,6 +1254,78 @@ class TestUnbindPort(base.TestCase): mock_log.info.assert_called_once_with('Port %s was not found while ' 'unbinding.', port_id) + @mock.patch.object(neutron, 'get_client', autospec=True) + def test_unbind_neutron_port_if_bound(self, mock_client, mock_unp): + fake_client = mock.Mock() + fake_client.get_port.return_value.binding_host_id = 'foo' + mock_client.return_value = fake_client + port_id = 'fake-port-id' + neutron.unbind_neutron_port_if_bound(port_id, context=self.context) + mock_client.assert_called_once_with(context=self.context) + fake_client.get_port.assert_called_once_with(port_id) + mock_unp.assert_has_calls([ + mock.call(None, 'fake-port-id', + {'binding:host_id': '', 'binding:profile': {}}, + None), + mock.call(None, 'fake-port-id', + {'mac_address': None}, + None) + ]) + + @mock.patch.object(neutron, 'get_client', autospec=True) + def test_unbind_neutron_port_if_bound_not_bound(self, mock_client, + mock_unp): + fake_client = mock.Mock() + fake_client.get_port.return_value.binding_host_id = None + mock_client.return_value = fake_client + port_id = 'fake-port-id' + neutron.unbind_neutron_port_if_bound(port_id, context=self.context) + mock_client.assert_called_once_with(context=self.context) + fake_client.get_port.assert_called_once_with(port_id) + mock_unp.assert_not_called() + + @mock.patch.object(neutron, 'get_client', autospec=True) + def test_unbind_neutron_port_if_bound_not_bound_empty(self, mock_client, + mock_unp): + fake_client = mock.Mock() + # While it should be none, it can be set to empty value. + fake_client.get_port.return_value.binding_host_id = '' + mock_client.return_value = fake_client + port_id = 'fake-port-id' + neutron.unbind_neutron_port_if_bound(port_id, context=self.context) + mock_client.assert_called_once_with(context=self.context) + fake_client.get_port.assert_called_once_with(port_id) + mock_unp.assert_not_called() + + @mock.patch.object(neutron, 'get_client', autospec=True) + def test_unbind_neutron_port_if_bound_port_not_found(self, mock_client, + mock_unp): + fake_client = mock.Mock() + fake_client.get_port.side_effect = openstack_exc.ResourceNotFound() + mock_client.return_value = fake_client + port_id = 'fake-port-id' + self.assertRaises(exception.NetworkError, + neutron.unbind_neutron_port_if_bound, + port_id, context=self.context) + mock_client.assert_called_once_with(context=self.context) + fake_client.get_port.assert_called_once_with(port_id) + mock_unp.assert_not_called() + + @mock.patch.object(neutron, 'get_client', autospec=True) + def test_unbind_neutron_port_if_bound_port_osc_error(self, mock_client, + mock_unp): + fake_client = mock.Mock() + fake_client.get_port.side_effect = ( + openstack_exc.OpenStackCloudException()) + mock_client.return_value = fake_client + port_id = 'fake-port-id' + self.assertRaises(exception.NetworkError, + neutron.unbind_neutron_port_if_bound, + port_id, context=self.context) + mock_client.assert_called_once_with(context=self.context) + fake_client.get_port.assert_called_once_with(port_id) + mock_unp.assert_not_called() + class TestGetNetworkByUUIDOrName(base.TestCase): diff --git a/ironic/tests/unit/drivers/modules/network/test_common.py b/ironic/tests/unit/drivers/modules/network/test_common.py index e4e0d4cd86..7bd0b897fe 100644 --- a/ironic/tests/unit/drivers/modules/network/test_common.py +++ b/ironic/tests/unit/drivers/modules/network/test_common.py @@ -711,6 +711,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): self.neutron_port = {'id': '132f871f-eaec-4fed-9475-0d54465e0f00', 'mac_address': '52:54:00:cf:2d:32'} + @mock.patch.object(neutron_common, 'update_neutron_port', + autospec=True) @mock.patch.object(common.VIFPortIDMixin, '_save_vif_to_port_like_obj', autospec=True) @mock.patch.object(common, 'get_free_port_like_object', autospec=True) @@ -719,7 +721,7 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): @mock.patch.object(neutron_common, 'get_physnets_by_port_uuid', autospec=True) def test_vif_attach(self, mock_gpbpi, mock_upa, mock_client, mock_gfp, - mock_save): + mock_save, mock_update_port): vif = {'id': "fake_vif_id"} mock_gfp.return_value = self.port with task_manager.acquire(self.context, self.node.id) as task: @@ -727,6 +729,18 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): mock_client.assert_called_once_with(context=task.context) mock_upa.assert_called_once_with( "fake_vif_id", self.port.address, context=task.context) + mock_update_port.assert_has_calls([ + mock.call(None, 'fake_vif_id', + {'binding:host_id': '', 'binding:profile': {}}, + None), + mock.call(None, 'fake_vif_id', + {'mac_address': None}, None), + mock.call(task.context, mock.ANY, + {'binding:vnic_type': 'baremetal', + 'binding:host_id': mock.ANY, + 'binding:profile': {}}, + client=None) + ]) self.assertFalse(mock_gpbpi.called) mock_gfp.assert_called_once_with(task, 'fake_vif_id', set(), {'id': 'fake_vif_id'}) @@ -750,6 +764,10 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): {'id': 'fake_vif_id'}) self.assertFalse(mock_save.called) + @mock.patch.object(neutron_common, 'unbind_neutron_port_if_bound', + autospec=True) + @mock.patch.object(neutron_common, 'update_neutron_port', + autospec=True) @mock.patch.object(common.VIFPortIDMixin, '_save_vif_to_port_like_obj', autospec=True) @mock.patch.object(common, 'get_free_port_like_object', autospec=True) @@ -758,7 +776,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): @mock.patch.object(neutron_common, 'get_physnets_by_port_uuid', autospec=True) def test_vif_attach_with_physnet(self, mock_gpbpi, mock_upa, mock_client, - mock_gfp, mock_save): + mock_gfp, mock_save, mock_port_update, + mock_unbind): self.port.physical_network = 'physnet1' self.port.save() vif = {'id': "fake_vif_id"} @@ -767,14 +786,24 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.id) as task: self.interface.vif_attach(task, vif) mock_client.assert_called_once_with(context=task.context) + mock_unbind.assert_called_once_with("fake_vif_id", client=mock.ANY, + context=task.context) mock_upa.assert_called_once_with( "fake_vif_id", self.port.address, context=task.context) + mock_port_update.assert_called_once_with( + task.context, mock.ANY, + {'binding:vnic_type': 'baremetal', + 'binding:host_id': mock.ANY, + 'binding:profile': {}}, + client=None) mock_gpbpi.assert_called_once_with(mock_client.return_value, 'fake_vif_id') mock_gfp.assert_called_once_with(task, 'fake_vif_id', {'physnet1'}, {'id': 'fake_vif_id'}) mock_save.assert_called_once_with(self.port, "fake_vif_id") + @mock.patch.object(neutron_common, 'update_neutron_port', + autospec=True) @mock.patch.object(common.VIFPortIDMixin, '_save_vif_to_port_like_obj', autospec=True) @mock.patch.object(common, 'plug_port_to_tenant_network', autospec=True) @@ -784,7 +813,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): @mock.patch.object(neutron_common, 'get_physnets_by_port_uuid', autospec=True) def test_vif_attach_active_node(self, mock_gpbpi, mock_upa, mock_client, - mock_gfp, mock_plug, mock_save): + mock_gfp, mock_plug, mock_save, + mock_port_update): self.node.provision_state = states.ACTIVE self.node.save() vif = {'id': "fake_vif_id"} @@ -794,6 +824,7 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): mock_client.assert_called_once_with(context=task.context) mock_upa.assert_called_once_with( "fake_vif_id", self.port.address, context=task.context) + mock_port_update.assert_not_called() self.assertFalse(mock_gpbpi.called) mock_gfp.assert_called_once_with(task, 'fake_vif_id', set(), {'id': 'fake_vif_id'}) @@ -828,6 +859,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): mock_save.assert_called_once_with(self.port, "fake_vif_id") mock_plug.assert_called_once_with(task, self.port, mock.ANY) + @mock.patch.object(neutron_common, 'update_neutron_port', + autospec=True) @mock.patch.object(common.VIFPortIDMixin, '_save_vif_to_port_like_obj', autospec=True) @mock.patch.object(common, 'get_free_port_like_object', autospec=True) @@ -836,7 +869,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): @mock.patch.object(neutron_common, 'get_physnets_by_port_uuid', autospec=True) def test_vif_attach_portgroup_no_address(self, mock_gpbpi, mock_upa, - mock_client, mock_gfp, mock_save): + mock_client, mock_gfp, mock_save, + mock_port_update): pg = obj_utils.create_test_portgroup( self.context, node_id=self.node.id, address=None) mock_gfp.return_value = pg @@ -844,6 +878,19 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.id) as task: self.interface.vif_attach(task, vif) mock_client.assert_called_once_with(context=task.context) + mock_port_update.assert_has_calls([ + mock.call(None, 'fake_vif_id', + {'binding:host_id': '', 'binding:profile': {}}, + None), + mock.call(None, 'fake_vif_id', + {'mac_address': None}, None), + mock.call(task.context, mock.ANY, + {'binding:vnic_type': 'baremetal', + 'binding:host_id': mock.ANY, + 'binding:profile': {}}, + client=None) + ]) + self.assertFalse(mock_gpbpi.called) mock_gfp.assert_called_once_with(task, 'fake_vif_id', set(), {'id': 'fake_vif_id'}) diff --git a/releasenotes/notes/initial-port-bind-5a1ffb083f6631d4.yaml b/releasenotes/notes/initial-port-bind-5a1ffb083f6631d4.yaml new file mode 100644 index 0000000000..2e2a5d878b --- /dev/null +++ b/releasenotes/notes/initial-port-bind-5a1ffb083f6631d4.yaml @@ -0,0 +1,16 @@ +--- +fixes: + - | + Fixes interface binding logic as it relates to Neutron VIF attachments, + such that an initial neutron port update occurs to ensure Neutron performs + any necessary address assignments, which will now result in the port + entering an ``ACTIVE`` state. The state may change later on as the port + is updated as part of any workflow actions with supplied port configuration + information to allow ML2 plugins to finalize any port binding actions, when + appropriate. The base bug which identified this issue is + `bug 2106073 `_, which will + require additional work to completely fix. + + Related, this logic *also* detaches any previously bound VIF which might + have been supplied to Ironic. To have done so in advance of attachment is + erroneous, yet understandable behavior.