From f93003d968acdd8e4ecda05c52cfacc8de3c9888 Mon Sep 17 00:00:00 2001 From: Kaifeng Wang Date: Tue, 3 Nov 2020 21:22:03 +0800 Subject: [PATCH] Fixes the issue that instance bond port can't get IP address The issue is that when a port group doesn't have a mac address assigned by operators, and during provisioning we unbind/bind tenant port with None which causes the mac address to be regenerated twice and differs from the originally one allocated by nova or users which was packed into config drive. The end result is that, bond port has different mac address configured and can't the IP address from neutron. Change-Id: I92ed5d17239216324d6a69e0ed8771fd6948d6ec Story: 2008300 Task: 41185 (cherry picked from commit fe01ddb2bce12dc1693d64a30e5b1b822e7b839e) --- ironic/common/neutron.py | 6 +- ironic/drivers/modules/network/common.py | 6 +- ironic/drivers/modules/network/neutron.py | 6 +- ironic/tests/unit/common/test_neutron.py | 8 ++ .../drivers/modules/network/test_neutron.py | 90 ++++++++++++++++++- .../notes/portgroup-mac-649ed31c3525e4f0.yaml | 7 ++ 6 files changed, 116 insertions(+), 7 deletions(-) create mode 100644 releasenotes/notes/portgroup-mac-649ed31c3525e4f0.yaml diff --git a/ironic/common/neutron.py b/ironic/common/neutron.py index c6c84f3df3..e81f360aae 100644 --- a/ironic/common/neutron.py +++ b/ironic/common/neutron.py @@ -104,7 +104,7 @@ def update_neutron_port(context, port_id, attrs, client=None): return client.update_port(port_id, **attrs) -def unbind_neutron_port(port_id, client=None, context=None): +def unbind_neutron_port(port_id, client=None, context=None, reset_mac=True): """Unbind a neutron port Remove a neutron port's binding profile and host ID so that it returns to @@ -113,6 +113,7 @@ def unbind_neutron_port(port_id, client=None, context=None): :param port_id: Neutron port ID. :param client: Optional a Neutron client object. :param context: request context + :param reset_mac: reset mac address :type context: ironic.common.context.RequestContext :raises: NetworkError """ @@ -126,7 +127,8 @@ def unbind_neutron_port(port_id, client=None, context=None): # Exception PortBound will be raised by neutron as it refuses to # update the mac address of a bound port if we attempt to unbind and # reset the mac in the same call. - update_neutron_port(context, port_id, attrs_reset_mac, client) + if reset_mac: + update_neutron_port(context, port_id, attrs_reset_mac, client) # NOTE(vsaienko): Ignore if port was deleted before calling vif detach. except openstack_exc.ResourceNotFound: LOG.info('Port %s was not found while unbinding.', port_id) diff --git a/ironic/drivers/modules/network/common.py b/ironic/drivers/modules/network/common.py index 3798c2bd49..f10089d265 100644 --- a/ironic/drivers/modules/network/common.py +++ b/ironic/drivers/modules/network/common.py @@ -268,8 +268,10 @@ def plug_port_to_tenant_network(task, port_like_obj, client=None): # because other port attributes may have been set by the user or # nova. port_attrs = {'binding:vnic_type': neutron.VNIC_BAREMETAL, - 'binding:host_id': node.uuid, - 'mac_address': port_like_obj.address} + 'binding:host_id': node.uuid} + # NOTE(kaifeng) Only update mac address when it's available + if port_like_obj.address: + port_attrs['mac_address'] = port_like_obj.address binding_profile = {'local_link_information': local_link_info} if local_group_info: binding_profile['local_group_information'] = local_group_info diff --git a/ironic/drivers/modules/network/neutron.py b/ironic/drivers/modules/network/neutron.py index 379be9e5cb..68520cbf16 100644 --- a/ironic/drivers/modules/network/neutron.py +++ b/ironic/drivers/modules/network/neutron.py @@ -235,7 +235,11 @@ class NeutronNetwork(common.NeutronVIFPortIDMixin, link_info = port_like_obj.local_link_connection neutron.wait_for_host_agent(client, link_info['hostname']) - neutron.unbind_neutron_port(vif_port_id, context=task.context) + # NOTE(kaifeng) address is optional for port group, avoid to + # regenerate mac when the address is absent. + reset_mac = bool(port_like_obj.address) + neutron.unbind_neutron_port(vif_port_id, context=task.context, + reset_mac=reset_mac) def need_power_on(self, task): """Check if the node has any Smart NIC ports diff --git a/ironic/tests/unit/common/test_neutron.py b/ironic/tests/unit/common/test_neutron.py index 07dcb290ec..2263dd2c7c 100644 --- a/ironic/tests/unit/common/test_neutron.py +++ b/ironic/tests/unit/common/test_neutron.py @@ -1207,6 +1207,14 @@ class TestUnbindPort(base.TestCase): neutron.unbind_neutron_port(port_id, context=self.context) mock_unp.assert_has_calls(update_calls) + def test_unbind_neutron_port_not_reset_mac(self, mock_unp): + port_id = 'fake-port-id' + attr_unbind = {'binding:host_id': '', 'binding:profile': {}} + neutron.unbind_neutron_port(port_id, context=self.context, + reset_mac=False) + mock_unp.assert_called_once_with(self.context, port_id, attr_unbind, + None) + @mock.patch.object(neutron, 'LOG', autospec=True) def test_unbind_neutron_port_not_found(self, mock_log, mock_unp): port_id = 'fake-port-id' diff --git a/ironic/tests/unit/drivers/modules/network/test_neutron.py b/ironic/tests/unit/drivers/modules/network/test_neutron.py index bee21422cd..895c8ec5aa 100644 --- a/ironic/tests/unit/drivers/modules/network/test_neutron.py +++ b/ironic/tests/unit/drivers/modules/network/test_neutron.py @@ -533,7 +533,8 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.id) as task: self.interface.unconfigure_tenant_networks(task) mock_unbind_port.assert_called_once_with( - self.port.extra['vif_port_id'], context=task.context) + self.port.extra['vif_port_id'], context=task.context, + reset_mac=True) @mock.patch.object(neutron_common, 'get_client', autospec=True) @mock.patch.object(neutron_common, 'wait_for_host_agent', autospec=True) @@ -550,9 +551,36 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.id) as task: self.interface.unconfigure_tenant_networks(task) mock_unbind_port.assert_called_once_with( - self.port.extra['vif_port_id'], context=task.context) + self.port.extra['vif_port_id'], context=task.context, + reset_mac=True) wait_agent_mock.assert_called_once_with(nclient, 'hostname') + @mock.patch.object(neutron_common, 'unbind_neutron_port', autospec=True) + def test_unconfigure_tenant_networks_portgroup_1(self, mock_unbind_port): + pg = utils.create_test_portgroup( + self.context, node_id=self.node.id, address='ff:54:00:cf:2d:32', + internal_info={'tenant_vif_port_id': uuidutils.generate_uuid()}) + with task_manager.acquire(self.context, self.node.id) as task: + self.interface.unconfigure_tenant_networks(task) + mock_unbind_port.assert_has_calls([ + mock.call(self.port.extra['vif_port_id'], context=task.context, + reset_mac=True), + mock.call(pg.internal_info['tenant_vif_port_id'], + context=task.context, reset_mac=True)]) + + @mock.patch.object(neutron_common, 'unbind_neutron_port', autospec=True) + def test_unconfigure_tenant_networks_portgroup_2(self, mock_unbind_port): + pg = utils.create_test_portgroup( + self.context, node_id=self.node.id, address=None, + internal_info={'tenant_vif_port_id': uuidutils.generate_uuid()}) + with task_manager.acquire(self.context, self.node.id) as task: + self.interface.unconfigure_tenant_networks(task) + mock_unbind_port.assert_has_calls([ + mock.call(self.port.extra['vif_port_id'], context=task.context, + reset_mac=True), + mock.call(pg.internal_info['tenant_vif_port_id'], + context=task.context, reset_mac=False)]) + def test_configure_tenant_networks_no_ports_for_node(self): n = utils.create_test_node(self.context, network_interface='neutron', uuid=uuidutils.generate_uuid()) @@ -755,6 +783,64 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): call2_attrs)] ) + @mock.patch.object(neutron_common, 'get_neutron_port_data', autospec=True) + @mock.patch.object(neutron_common, 'wait_for_host_agent', autospec=True) + @mock.patch.object(neutron_common, 'update_neutron_port', autospec=True) + @mock.patch.object(neutron_common, 'get_client', autospec=True) + @mock.patch.object(neutron_common, 'get_local_group_information', + autospec=True) + def test_configure_tenant_networks_with_portgroups_no_address( + self, glgi_mock, client_mock, update_mock, wait_agent_mock, + port_data_mock): + pg = utils.create_test_portgroup( + self.context, node_id=self.node.id, address=None, + extra={'vif_port_id': uuidutils.generate_uuid()}) + port1 = utils.create_test_port( + self.context, node_id=self.node.id, address='ff:54:00:cf:2d:33', + uuid=uuidutils.generate_uuid(), + portgroup_id=pg.id, + local_link_connection={'switch_id': '0a:1b:2c:3d:4e:ff', + 'port_id': 'Ethernet1/1', + 'switch_info': 'switch2'} + ) + port2 = utils.create_test_port( + self.context, node_id=self.node.id, address='ff:54:00:cf:2d:34', + uuid=uuidutils.generate_uuid(), + portgroup_id=pg.id, + local_link_connection={'switch_id': '0a:1b:2c:3d:4e:ff', + 'port_id': 'Ethernet1/2', + 'switch_info': 'switch2'} + ) + local_group_info = {'a': 'b'} + glgi_mock.return_value = local_group_info + expected_attrs = {'binding:vnic_type': 'baremetal', + 'binding:host_id': self.node.uuid} + call1_attrs = copy.deepcopy(expected_attrs) + call1_attrs['binding:profile'] = { + 'local_link_information': [self.port.local_link_connection] + } + call1_attrs['mac_address'] = '52:54:00:cf:2d:32' + call2_attrs = copy.deepcopy(expected_attrs) + call2_attrs['binding:profile'] = { + 'local_link_information': [port1.local_link_connection, + port2.local_link_connection], + 'local_group_information': local_group_info + } + with task_manager.acquire(self.context, self.node.id) as task: + # Override task.portgroups here, to have ability to check + # that mocked get_local_group_information was called with + # this portgroup object. + task.portgroups = [pg] + self.interface.configure_tenant_networks(task) + client_mock.assert_called_once_with(context=task.context) + glgi_mock.assert_called_once_with(task, pg) + update_mock.assert_has_calls( + [mock.call(self.context, self.port.extra['vif_port_id'], + call1_attrs), + mock.call(self.context, pg.extra['vif_port_id'], + call2_attrs)] + ) + def test_need_power_on_true(self): self.port.is_smartnic = True self.port.save() diff --git a/releasenotes/notes/portgroup-mac-649ed31c3525e4f0.yaml b/releasenotes/notes/portgroup-mac-649ed31c3525e4f0.yaml new file mode 100644 index 0000000000..2859fd695e --- /dev/null +++ b/releasenotes/notes/portgroup-mac-649ed31c3525e4f0.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixes the issue that when the MAC address of a port group is not set and + been attached to instance, the landed bond port cannot get IP address + due to inconsistent MAC address between the tenant port and the initially + allocated one in the config drive.