From f4ff926ab161297d2281394689ea25a7ef10cbb1 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 10 Aug 2016 14:46:37 +0200 Subject: [PATCH] Fix updating port MAC address for active nodes Current update fails with HTTP 500 when trying to update a bound Neutron port. Remove the binding before update and restore it during update. For nodes in "active" state (or with instance_uuid assigned) only allow such update in maintenance mode. This is not a breaking change, as previously it did not work at all. Change-Id: I356fc0eb3702eb16b7f8e675ea416473c65af7e3 Closes-Bug: #1611744 --- ironic/conductor/manager.py | 11 ++++ ironic/dhcp/neutron.py | 24 +++++++ ironic/tests/unit/conductor/test_manager.py | 66 ++++++++++++++++++- ironic/tests/unit/dhcp/test_neutron.py | 48 +++++++++++++- .../update-live-port-ee3fa9b77f5d0cf7.yaml | 6 ++ 5 files changed, 149 insertions(+), 6 deletions(-) create mode 100644 releasenotes/notes/update-live-port-ee3fa9b77f5d0cf7.yaml diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 45a384715c..c75b5b6093 100644 --- a/ironic/conductor/manager.py +++ b/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 diff --git a/ironic/dhcp/neutron.py b/ironic/dhcp/neutron.py index 0ebe00042c..021af95b7d 100644 --- a/ironic/dhcp/neutron.py +++ b/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: diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 75c194751d..724cd26eb2 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -2707,7 +2707,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'}) @@ -2720,7 +2722,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'}) @@ -2738,7 +2742,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' @@ -2747,6 +2753,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) diff --git a/ironic/tests/unit/dhcp/test_neutron.py b/ironic/tests/unit/dhcp/test_neutron.py index ca93feb66e..d6c2e4b1b3 100644 --- a/ironic/tests/unit/dhcp/test_neutron.py +++ b/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, - mock_update_port): + def test_update_port_address_with_binding(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, + '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 = ( diff --git a/releasenotes/notes/update-live-port-ee3fa9b77f5d0cf7.yaml b/releasenotes/notes/update-live-port-ee3fa9b77f5d0cf7.yaml new file mode 100644 index 0000000000..7abd8c1c2c --- /dev/null +++ b/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).