Merge "Fix updating port MAC address for active nodes"

ironic/conductor/manager.py

@@ -1652,6 +1652,17 @@ class ConductorManager(base_manager.BaseConductorManager):
                                   purpose='port update') as task:
             node = task.node
 
+            # Only allow updating MAC addresses for active nodes if maintenance
+            # mode is on.
+            if ((node.provision_state == states.ACTIVE or node.instance_uuid)
+                and 'address' in port_obj.obj_what_changed() and
+                not node.maintenance):
+                    action = _("Cannot update hardware address for port "
+                               "%(port)s as node %(node)s is active or has "
+                               "instance UUID assigned")
+                    raise exception.InvalidState(action % {'node': node.uuid,
+                                                           'port': port_uuid})
+
             # If port update is modifying the portgroup membership of the port
             # or modifying the local_link_connection or pxe_enabled flags then
             # node should be in MANAGEABLE/INSPECTING/ENROLL provisioning state

ironic/dhcp/neutron.py

@@ -70,6 +70,16 @@ class NeutronDHCPApi(base.BaseDHCP):
             LOG.exception(_LE("Failed to update Neutron port %s."), port_id)
             raise exception.FailedToUpdateDHCPOptOnPort(port_id=port_id)
 
+    def _get_binding(self, client, port_id):
+        """Get binding:host_id property from Neutron."""
+        try:
+            return client.show_port(port_id).get('port', {}).get(
+                'binding:host_id')
+        except neutron_client_exc.NeutronClientException:
+            LOG.exception(_LE('Failed to get the current binding on Neutron '
+                              'port %s.'), port_id)
+            raise exception.FailedToUpdateMacOnPort(port_id=port_id)
+
     def update_port_address(self, port_id, address, token=None):
         """Update a port's mac address.
 
@@ -78,7 +88,21 @@ class NeutronDHCPApi(base.BaseDHCP):
         :param token: optional auth token.
         :raises: FailedToUpdateMacOnPort
         """
+        client = neutron.get_client(token)
         port_req_body = {'port': {'mac_address': address}}
+
+        current_binding = self._get_binding(client, port_id)
+        if current_binding:
+            binding_clean_body = {'port': {'binding:host_id': ''}}
+            try:
+                client.update_port(port_id, binding_clean_body)
+            except neutron_client_exc.NeutronClientException:
+                LOG.exception(_LE("Failed to remove the current binding from "
+                                  "Neutron port %s."), port_id)
+                raise exception.FailedToUpdateMacOnPort(port_id=port_id)
+
+            port_req_body['port']['binding:host_id'] = current_binding
+
         try:
             neutron.get_client(token).update_port(port_id, port_req_body)
         except neutron_client_exc.NeutronClientException:

ironic/tests/unit/conductor/test_manager.py

@@ -2706,7 +2706,9 @@ class UpdatePortTestCase(mgr_utils.ServiceSetUpMixin,
 
     @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_address')
     def test_update_port_address(self, mac_update_mock):
-        node = obj_utils.create_test_node(self.context, driver='fake')
+        node = obj_utils.create_test_node(self.context, driver='fake',
+                                          instance_uuid=None,
+                                          provision_state='available')
         port = obj_utils.create_test_port(self.context,
                                           node_id=node.id,
                                           extra={'vif_port_id': 'fake-id'})
@@ -2719,7 +2721,9 @@ class UpdatePortTestCase(mgr_utils.ServiceSetUpMixin,
 
     @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_address')
     def test_update_port_address_fail(self, mac_update_mock):
-        node = obj_utils.create_test_node(self.context, driver='fake')
+        node = obj_utils.create_test_node(self.context, driver='fake',
+                                          instance_uuid=None,
+                                          provision_state='available')
         port = obj_utils.create_test_port(self.context,
                                           node_id=node.id,
                                           extra={'vif_port_id': 'fake-id'})
@@ -2737,7 +2741,9 @@ class UpdatePortTestCase(mgr_utils.ServiceSetUpMixin,
 
     @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_address')
     def test_update_port_address_no_vif_id(self, mac_update_mock):
-        node = obj_utils.create_test_node(self.context, driver='fake')
+        node = obj_utils.create_test_node(self.context, driver='fake',
+                                          instance_uuid=None,
+                                          provision_state='available')
         port = obj_utils.create_test_port(self.context, node_id=node.id)
 
         new_address = '11:22:33:44:55:bb'
@@ -2746,6 +2752,60 @@ class UpdatePortTestCase(mgr_utils.ServiceSetUpMixin,
         self.assertEqual(new_address, res.address)
         self.assertFalse(mac_update_mock.called)
 
+    @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_address')
+    def test_update_port_address_active_node(self, mac_update_mock):
+        node = obj_utils.create_test_node(self.context, driver='fake',
+                                          instance_uuid=None,
+                                          provision_state='active')
+        port = obj_utils.create_test_port(self.context,
+                                          node_id=node.id,
+                                          extra={'vif_port_id': 'fake-id'})
+        old_address = port.address
+        new_address = '11:22:33:44:55:bb'
+        port.address = new_address
+        exc = self.assertRaises(messaging.rpc.ExpectedException,
+                                self.service.update_port,
+                                self.context, port)
+        # Compare true exception hidden by @messaging.expected_exceptions
+        self.assertEqual(exception.InvalidState, exc.exc_info[0])
+        port.refresh()
+        self.assertEqual(old_address, port.address)
+
+    @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_address')
+    def test_update_port_address_instance_uuid(self, mac_update_mock):
+        node = obj_utils.create_test_node(self.context, driver='fake',
+                                          instance_uuid='uuid',
+                                          provision_state='error')
+        port = obj_utils.create_test_port(self.context,
+                                          node_id=node.id,
+                                          extra={'vif_port_id': 'fake-id'})
+        old_address = port.address
+        new_address = '11:22:33:44:55:bb'
+        port.address = new_address
+        exc = self.assertRaises(messaging.rpc.ExpectedException,
+                                self.service.update_port,
+                                self.context, port)
+        # Compare true exception hidden by @messaging.expected_exceptions
+        self.assertEqual(exception.InvalidState, exc.exc_info[0])
+        port.refresh()
+        self.assertEqual(old_address, port.address)
+
+    @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_address')
+    def test_update_port_address_maintenance(self, mac_update_mock):
+        node = obj_utils.create_test_node(self.context, driver='fake',
+                                          instance_uuid='uuid',
+                                          provision_state='active',
+                                          maintenance=True)
+        port = obj_utils.create_test_port(self.context,
+                                          node_id=node.id,
+                                          extra={'vif_port_id': 'fake-id'})
+        new_address = '11:22:33:44:55:bb'
+        port.address = new_address
+        res = self.service.update_port(self.context, port)
+        self.assertEqual(new_address, res.address)
+        mac_update_mock.assert_called_once_with('fake-id', new_address,
+                                                token=self.context.auth_token)
+
     def test_update_port_node_deleting_state(self):
         node = obj_utils.create_test_node(self.context, driver='fake',
                                           provision_state=states.DELETING)

ironic/tests/unit/dhcp/test_neutron.py

@@ -98,25 +98,67 @@ class TestNeutron(db_base.DbTestCase):
             api.provider.update_port_dhcp_opts,
             port_id, opts)
 
+    @mock.patch.object(client.Client, 'show_port')
     @mock.patch.object(client.Client, 'update_port')
     @mock.patch.object(client.Client, '__init__')
-    def test_update_port_address(self, mock_client_init, mock_update_port):
+    def test_update_port_address(self, mock_client_init, mock_update_port,
+                                 mock_show_port):
         address = 'fe:54:00:77:07:d9'
         port_id = 'fake-port-id'
         expected = {'port': {'mac_address': address}}
         mock_client_init.return_value = None
+        mock_show_port.return_value = {}
 
         api = dhcp_factory.DHCPFactory()
         api.provider.update_port_address(port_id, address)
         mock_update_port.assert_called_once_with(port_id, expected)
 
+    @mock.patch.object(client.Client, 'show_port')
     @mock.patch.object(client.Client, 'update_port')
     @mock.patch.object(client.Client, '__init__')
-    def test_update_port_address_with_exception(self, mock_client_init,
+    def test_update_port_address_with_binding(self, mock_client_init,
-                                                mock_update_port):
+                                              mock_update_port,
+                                              mock_show_port):
+        address = 'fe:54:00:77:07:d9'
+        port_id = 'fake-port-id'
+        expected = {'port': {'mac_address': address,
+                             'binding:host_id': 'host'}}
+        mock_client_init.return_value = None
+        mock_show_port.return_value = {'port': {'binding:host_id': 'host'}}
+
+        api = dhcp_factory.DHCPFactory()
+        api.provider.update_port_address(port_id, address)
+        mock_update_port.assert_any_call(port_id,
+                                         {'port': {'binding:host_id': ''}})
+        mock_update_port.assert_any_call(port_id, expected)
+
+    @mock.patch.object(client.Client, 'show_port')
+    @mock.patch.object(client.Client, 'update_port')
+    @mock.patch.object(client.Client, '__init__')
+    def test_update_port_address_show_failed(self, mock_client_init,
+                                             mock_update_port,
+                                             mock_show_port):
         address = 'fe:54:00:77:07:d9'
         port_id = 'fake-port-id'
         mock_client_init.return_value = None
+        mock_show_port.side_effect = (
+            neutron_client_exc.NeutronClientException())
+
+        api = dhcp_factory.DHCPFactory()
+        self.assertRaises(exception.FailedToUpdateMacOnPort,
+                          api.provider.update_port_address, port_id, address)
+        self.assertFalse(mock_update_port.called)
+
+    @mock.patch.object(client.Client, 'show_port')
+    @mock.patch.object(client.Client, 'update_port')
+    @mock.patch.object(client.Client, '__init__')
+    def test_update_port_address_with_exception(self, mock_client_init,
+                                                mock_update_port,
+                                                mock_show_port):
+        address = 'fe:54:00:77:07:d9'
+        port_id = 'fake-port-id'
+        mock_client_init.return_value = None
+        mock_show_port.return_value = {}
 
         api = dhcp_factory.DHCPFactory()
         mock_update_port.side_effect = (

releasenotes/notes/update-live-port-ee3fa9b77f5d0cf7.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - Fixed updating a MAC on a port for active instances in maintenance mode
+    (used to return HTTP 500 previously).
+  - Return HTTP 400 for requests to update a MAC on a port for an active
+    instance without maintenance mode set (used to return HTTP 500 previously).