openstack/ironic
Code Issues Proposed changes

Merge "provide host_id to neutron early on"

Browse Source
This commit is contained in:
Zuul 2025-05-13 22:11:39 +00:00 committed by Gerrit Code Review
commit 1bdea20488
7 changed files with 298 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

52
doc/source/admin/networking.rst
View File

@ -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 <https://bugs.launchpad.net/ironic/+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

49
ironic/common/neutron.py
View File

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

4
ironic/conductor/manager.py
View File

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

63
ironic/drivers/modules/network/common.py
View File

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

72
ironic/tests/unit/common/test_neutron.py
View File

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

55
ironic/tests/unit/drivers/modules/network/test_common.py
View File

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

16
releasenotes/notes/initial-port-bind-5a1ffb083f6631d4.yaml Normal file
View File

@ -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 <https://bugs.launchpad.net/nova/+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.