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.