diff --git a/neutron/extensions/_port_mac_address_override.py b/neutron/extensions/_port_mac_address_override.py new file mode 100644 index 00000000000..1b7ba184f9b --- /dev/null +++ b/neutron/extensions/_port_mac_address_override.py @@ -0,0 +1,33 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +# TODO(gibi): remove this when the neutron-lib change +# https://review.opendev.org/c/openstack/neutron-lib/+/831935 merged and +# released + +NAME = 'Neutron Port MAC address override' +ALIAS = 'port-mac-override' +DESCRIPTION = ( + "Allow overriding the MAC address of a direct-physical Port via the " + "active binding profile") + +UPDATED_TIMESTAMP = "2022-03-18T10:00:00-00:00" + +RESOURCE_ATTRIBUTE_MAP = {} + +IS_SHIM_EXTENSION = True +IS_STANDARD_ATTR_EXTENSION = False +SUB_RESOURCE_ATTRIBUTE_MAP = {} +ACTION_MAP = {} +REQUIRED_EXTENSIONS = [] +OPTIONAL_EXTENSIONS = [] +ACTION_STATUS = {} diff --git a/neutron/extensions/port_mac_address_override.py b/neutron/extensions/port_mac_address_override.py new file mode 100644 index 00000000000..77b5afcffa4 --- /dev/null +++ b/neutron/extensions/port_mac_address_override.py @@ -0,0 +1,27 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# TODO(gibi): replace the import with +# from neutron_lib.api.definitions import port_mac_address_override +# once https://review.opendev.org/c/openstack/neutron-lib/+/831935 merged and +# neutron-lib is released +from neutron.extensions import ( + _port_mac_address_override as port_mac_address_override +) +from neutron_lib.api import extensions as api_extensions + + +class Port_mac_address_override(api_extensions.APIExtensionDescriptor): + """Extension to support port MAC address override""" + + api_definition = port_mac_address_override diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py index 8bb30a52e36..88a9d39d73f 100644 --- a/neutron/plugins/ml2/plugin.py +++ b/neutron/plugins/ml2/plugin.py @@ -119,6 +119,13 @@ from neutron.db import securitygroups_rpc_base as sg_db_rpc from neutron.db import segments_db from neutron.db import subnet_service_type_mixin from neutron.db import vlantransparent_db +# TODO(gibi): replace the import with +# from neutron_lib.api.definitions import port_mac_address_override +# once https://review.opendev.org/c/openstack/neutron-lib/+/831935 merged and +# neutron-lib is released +from neutron.extensions import ( + _port_mac_address_override as port_mac_address_override +) from neutron.extensions import dhcpagentscheduler as dhcp_ext from neutron.extensions import filter_validation from neutron.extensions import security_groups_shared_filtering_lib @@ -231,6 +238,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, pnap_def.ALIAS, pdp_def.ALIAS, quota_check_limit.ALIAS, + port_mac_address_override.ALIAS, ] # List of agent types for which all binding_failed ports should try to be @@ -751,6 +759,37 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, # updated binding state. self._update_port_dict_binding(port, cur_context_binding) + # In general the MAC address information flows fom the neutron + # port to the device in the backend. Except for direct-physical + # ports. In that case the MAC address flows from the physical + # device, the PF, to the neutron port. So when such a port is + # being bound to a host the port's MAC address needs to be + # updated. + # Nova puts the new MAC into the binding profile when + # requesting the binding either by creating it or by activating + # currently inactive one. + if ( + cur_context_binding.vnic_type == + portbindings.VNIC_DIRECT_PHYSICAL + ): + profile = jsonutils.loads(cur_context_binding.profile) + new_mac = profile.get('device_mac_address') + # When the port is being unbound or the client removes the + # MAC from the binding profile then the MAC of the port + # needs to be reset. It is necessary to avoid the + # duplicated MAC address issue when an unbound port is + # still using the MAC of a physical device and another + # port is being bound to that physical device. + if not new_mac: + new_mac = self._generate_macs()[0] + old_mac = port_db.mac_address + port_db.mac_address = new_mac + LOG.debug( + 'The MAC address of the Port %s is updated from %s to ' + '%s due to committing a new port binding.', + port_id, old_mac, new_mac + ) + # Update the port status if requested by the bound driver. if (bind_context._binding_levels and bind_context._new_port_status): diff --git a/neutron/tests/unit/plugins/ml2/test_plugin.py b/neutron/tests/unit/plugins/ml2/test_plugin.py index 9e94dde6702..0e4a9f77c8f 100644 --- a/neutron/tests/unit/plugins/ml2/test_plugin.py +++ b/neutron/tests/unit/plugins/ml2/test_plugin.py @@ -1952,7 +1952,7 @@ class TestMl2PluginOnly(Ml2PluginV2TestCase): plugin = directory.get_plugin() port = {'device_id': '123', 'device_owner': 'compute:nova'} new_attrs = ({'device_id': '', 'device_owner': ''} if unbinding else - {'name': 'new'}) + {'name': 'new'}) binding = mock.Mock() binding.vnic_type = ( portbindings.VNIC_DIRECT_PHYSICAL if direct_physical else diff --git a/neutron/tests/unit/plugins/ml2/test_port_binding.py b/neutron/tests/unit/plugins/ml2/test_port_binding.py index 1b90b7912a5..bc1f0dc05e2 100644 --- a/neutron/tests/unit/plugins/ml2/test_port_binding.py +++ b/neutron/tests/unit/plugins/ml2/test_port_binding.py @@ -15,6 +15,7 @@ from unittest import mock +from neutron_lib.api.definitions import port as port_def from neutron_lib.api.definitions import portbindings from neutron_lib.api.definitions import portbindings_extended as pbe_ext from neutron_lib import constants as const @@ -662,3 +663,179 @@ class ExtendedPortBindingTestCase(test_plugin.NeutronDbPluginV2TestCase): response.status_int) self.assertTrue(exceptions.PortBindingError.__name__ in response.text) + + def _create_unbound_port(self): + with self.port() as port: + return port['port'] + + def _get_port_mac(self, port_id): + port = self._show('ports', port_id)['port'] + return port[port_def.PORT_MAC_ADDRESS] + + def _update_port_with_binding( + self, port_id, host, vnic_type, binding_profile_mac + ): + device_owner = f'{const.DEVICE_OWNER_COMPUTE_PREFIX}nova' + update_body = { + 'port': { + 'device_owner': device_owner, + 'device_id': 'some-nova-instance', + 'binding:vnic_type': vnic_type, + 'binding:host_id': host, + 'binding:profile': {}, + } + } + if binding_profile_mac: + update_body['port']['binding:profile'] = { + 'device_mac_address': binding_profile_mac + } + + with mock.patch.object( + mechanism_test.TestMechanismDriver, '_check_port_context' + ): + req = self.new_update_request('ports', update_body, port_id) + self.assertEqual(200, req.get_response(self.api).status_int) + + def test_bind_non_pf_port_with_mac_port_not_updated(self): + new_mac = 'b4:96:91:34:f4:aa' + port = self._create_unbound_port() + # Neutron generates a mac for each port created + generated_mac = port['mac_address'] + + # provide a new MAC in the binding for a normal port + self._update_port_with_binding( + port['id'], + self.host, + vnic_type=portbindings.VNIC_NORMAL, + binding_profile_mac=new_mac + ) + + # Neutron ignores the MAC in the binding for non PF ports so the + # generated MAC remains + self.assertEqual(generated_mac, self._get_port_mac(port['id'])) + + def test_bind_pf_port_with_mac_port_updated(self): + host1 = self.host + host_1_pf_mac = 'b4:96:91:34:f4:aa' + # this hostname is also accepted by the TestMechanismDriver + host2 = 'host-ovs-filter-active' + host_2_pf_mac = 'b4:96:91:34:f4:bb' + port = self._create_unbound_port() + # neutron generates a MAC for each port created + generated_mac = port[port_def.PORT_MAC_ADDRESS] + + # Now lets bind the port as Nova to host1 + # Nova, when binds a PF port, provides the current MAC in the + # binding profile + self._update_port_with_binding( + port['id'], + host1, + vnic_type=portbindings.VNIC_DIRECT_PHYSICAL, + binding_profile_mac=host_1_pf_mac + ) + + current_mac = self._get_port_mac(port['id']) + self.assertNotEqual(generated_mac, current_mac) + # we expect that Neutron updates the port's MAC based on the + # binding + self.assertEqual(host_1_pf_mac, current_mac) + + # Now create a new, inactive binding on a different host with a + # different MAC + profile = {'device_mac_address': host_2_pf_mac} + kwargs = { + pbe_ext.PROFILE: profile, + pbe_ext.VNIC_TYPE: portbindings.VNIC_DIRECT_PHYSICAL, + } + self._make_port_binding(self.fmt, port['id'], host2, **kwargs) + # this should not change the MAC on the port as the new binding is + # inactive + self.assertEqual(host_1_pf_mac, self._get_port_mac(port['id'])) + + # Now activate the binding on host2 + with mock.patch.object( + mechanism_test.TestMechanismDriver, '_check_port_context' + ): + self._activate_port_binding(port['id'], host2) + + # Neutron should update the port MAC to the MAC in the host2 binding + self.assertEqual(host_2_pf_mac, self._get_port_mac(port['id'])) + + # Now activate the binding on host1 again + with mock.patch.object( + mechanism_test.TestMechanismDriver, '_check_port_context' + ): + self._activate_port_binding(port['id'], host1) + + # Neutron should update the port MAC to the MAC in the host1 binding + self.assertEqual(host_1_pf_mac, self._get_port_mac(port['id'])) + + # Now delete the inactive binding + response = self._delete_port_binding(port['id'], host2) + self.assertEqual(webob.exc.HTTPNoContent.code, response.status_int) + # the MAC should not change + self.assertEqual(host_1_pf_mac, self._get_port_mac(port['id'])) + + # Now remove the MAC from the active binding profile + self._update_port_with_binding( + port['id'], + host1, + portbindings.VNIC_DIRECT_PHYSICAL, + binding_profile_mac=None, + ) + # so the port should have a generated mac + generated_mac = self._get_port_mac(port['id']) + self.assertNotIn( + generated_mac, + {host_1_pf_mac, host_2_pf_mac} + ) + + # delete the remaining binding + response = self._delete_port_binding(port['id'], host1) + self.assertEqual(webob.exc.HTTPNoContent.code, response.status_int) + # the MAC should not change as it is already the generated MAC + self.assertEqual(generated_mac, self._get_port_mac(port['id'])) + + def test_pf_port_unbound_mac_reset(self): + host1 = self.host + host_1_pf_mac = 'b4:96:91:34:f4:aa' + + port = self._create_unbound_port() + # neutron generates a MAC for each port created + generated_mac = port[port_def.PORT_MAC_ADDRESS] + + # Now lets bind the port as Nova to host1 + # Nova, when binds a PF port, provides the current MAC in the + # binding profile + self._update_port_with_binding( + port['id'], + host1, + vnic_type=portbindings.VNIC_DIRECT_PHYSICAL, + binding_profile_mac=host_1_pf_mac + ) + + current_mac = self._get_port_mac(port['id']) + self.assertNotEqual(generated_mac, current_mac) + # we expect that Neutron updates the port's MAC based on the + # binding + self.assertEqual(host_1_pf_mac, current_mac) + + # Now unbound the port + update_body = { + 'port': { + 'device_owner': '', + 'device_id': '', + 'binding:host_id': None, + 'binding:profile': {}, + } + } + + with mock.patch.object( + mechanism_test.TestMechanismDriver, '_check_port_context' + ): + req = self.new_update_request('ports', update_body, port['id']) + self.assertEqual(200, req.get_response(self.api).status_int) + + # Neutron expected to reset the MAC to a generated one so that the + # physical function on host1 is now usable for other ports + self.assertNotEqual(host_1_pf_mac, self._get_port_mac(port['id'])) diff --git a/releasenotes/notes/bug-1942329-7687504f9b177f80.yaml b/releasenotes/notes/bug-1942329-7687504f9b177f80.yaml new file mode 100644 index 00000000000..707e6aba198 --- /dev/null +++ b/releasenotes/notes/bug-1942329-7687504f9b177f80.yaml @@ -0,0 +1,14 @@ +--- +fixes: + - | + `1942329 `_ + Port binding logic for direct-physical ports has been extended to allow + providing the MAC address of the physical device via the binding profile. + If it is provided then Neutron overwrites the value of the + ``device_mac_address`` field of the port object in the database with the + value from the active binding profile. + If there are ports bound before `the nova side of this fix is depolyed + `_ then the VM using + the port needs to be moved or the port needs to be detached and re-attached + to force nova to provide the MAC address of the direct-physical port in the + port binding.