Merge "Fixes the issue that instance bond port can't get IP address" into stable/victoria

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,6 +127,7 @@ 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.
+        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:

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,
+                  'binding:host_id': node.uuid}
-                  'mac_address': port_like_obj.address}
+    # 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

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

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'

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()

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.