From 5cb1720b6e1f2f799a3282828c317f1ef29b2189 Mon Sep 17 00:00:00 2001 From: Vasyl Saienko Date: Fri, 8 Sep 2017 09:32:50 +0300 Subject: [PATCH] Update vif_attach from NeutronVIFPortIDMixin This patch updates vif_attach from NeutronVIFPortIDMixin to rely on presence of Neutron. Drop incorrect logic that skips mac_address update when show_port() to neutron failed as NeutronVIFPortIDMixin is used only in flat and neutron network interfaces which require neutron presence. Update api tests to use real neutron port. Change-Id: Iffaf3569aa296b4466729cf7f62c92995b6b3723 --- ironic/drivers/modules/network/common.py | 33 ++++------------ .../drivers/modules/network/test_common.py | 38 ------------------- .../tests/api/admin/test_nodes.py | 34 +++++++++++------ 3 files changed, 30 insertions(+), 75 deletions(-) diff --git a/ironic/drivers/modules/network/common.py b/ironic/drivers/modules/network/common.py index db4df58388..7fe3527314 100644 --- a/ironic/drivers/modules/network/common.py +++ b/ironic/drivers/modules/network/common.py @@ -559,32 +559,15 @@ class NeutronVIFPortIDMixin(VIFPortIDMixin): # Address is optional for portgroups if port_like_obj.address: - # Check if the requested vif_id is a neutron port. If it is - # then attempt to update the port's MAC address. try: - client.show_port(vif_id) - except neutron_exceptions.NeutronClientException: - # TODO(mgoddard): Remove this except clause and handle errors - # properly. We can do this once a strategy has been determined - # for handling the tempest VIF tests in an environment that - # may not support neutron. - # NOTE(sambetts): If a client error occurs this is because - # either neutron doesn't exist because we're running in - # standalone environment or we can't find a matching neutron - # port which means a user might be requesting a non-neutron - # port. So skip trying to update the neutron port MAC address - # in these cases. - pass - else: - try: - neutron.update_port_address(vif_id, port_like_obj.address) - except exception.FailedToUpdateMacOnPort: - raise exception.NetworkError(_( - "Unable to attach VIF %(vif)s because Ironic can not " - "update Neutron port %(port)s MAC address to match " - "physical MAC address %(mac)s") % { - 'vif': vif_id, 'port': vif_id, - 'mac': port_like_obj.address}) + neutron.update_port_address(vif_id, port_like_obj.address) + except exception.FailedToUpdateMacOnPort: + raise exception.NetworkError(_( + "Unable to attach VIF %(vif)s because Ironic can not " + "update Neutron port %(port)s MAC address to match " + "physical MAC address %(mac)s") % { + 'vif': vif_id, 'port': vif_id, + 'mac': port_like_obj.address}) self._save_vif_to_port_like_obj(port_like_obj, vif_id) diff --git a/ironic/tests/unit/drivers/modules/network/test_common.py b/ironic/tests/unit/drivers/modules/network/test_common.py index af6d2451ce..35b7a8a94b 100644 --- a/ironic/tests/unit/drivers/modules/network/test_common.py +++ b/ironic/tests/unit/drivers/modules/network/test_common.py @@ -11,7 +11,6 @@ # under the License. import mock -from neutronclient.common import exceptions as neutron_exceptions from oslo_config import cfg from oslo_utils import uuidutils @@ -684,8 +683,6 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): mock_client.assert_called_once_with() self.assertFalse(mock_gpbpi.called) mock_gfp.assert_called_once_with(task, 'fake_vif_id', set()) - mock_client.return_value.show_port.assert_called_once_with( - 'fake_vif_id') mock_upa.assert_called_once_with("fake_vif_id", self.port.address) mock_save.assert_called_once_with(self.port, "fake_vif_id") @@ -724,8 +721,6 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): mock_gpbpi.assert_called_once_with(mock_client.return_value, 'fake_vif_id') mock_gfp.assert_called_once_with(task, 'fake_vif_id', {'physnet1'}) - mock_client.return_value.show_port.assert_called_once_with( - 'fake_vif_id') mock_upa.assert_called_once_with("fake_vif_id", self.port.address) mock_save.assert_called_once_with(self.port, "fake_vif_id") @@ -747,10 +742,7 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): mock_client.assert_called_once_with() self.assertFalse(mock_gpbpi.called) mock_gfp.assert_called_once_with(task, 'fake_vif_id', set()) - mock_client.return_value.show_port.assert_called_once_with( - 'fake_vif_id') mock_upa.assert_called_once_with("fake_vif_id", self.port.address) - mock_save.assert_called_once_with(self.port, "fake_vif_id") mock_plug.assert_called_once_with(task, self.port, mock.ANY) @mock.patch.object(common.VIFPortIDMixin, '_save_vif_to_port_like_obj') @@ -774,8 +766,6 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): mock_client.assert_called_once_with() self.assertFalse(mock_gpbpi.called) mock_gfp.assert_called_once_with(task, 'fake_vif_id', set()) - mock_client.return_value.show_port.assert_called_once_with( - 'fake_vif_id') mock_upa.assert_called_once_with("fake_vif_id", self.port.address) mock_save.assert_called_once_with(self.port, "fake_vif_id") mock_plug.assert_called_once_with(task, self.port, mock.ANY) @@ -822,36 +812,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): mock_client.assert_called_once_with() mock_gpbpi.assert_called_once_with(mock_client.return_value, 'fake_vif_id') - mock_client.return_value.show_port.assert_called_once_with( - 'fake_vif_id') self.assertFalse(mock_save.called) - @mock.patch.object(common.VIFPortIDMixin, '_save_vif_to_port_like_obj') - @mock.patch.object(common, 'get_free_port_like_object', autospec=True) - @mock.patch.object(neutron_common, 'get_client', autospec=True) - @mock.patch.object(neutron_common, 'update_port_address') - @mock.patch.object(neutron_common, 'get_physnets_by_port_uuid', - autospec=True) - def test_vif_attach_neutron_absent(self, mock_gpbpi, mock_upa, - mock_client, mock_gfp, mock_save): - self.port.physical_network = 'physnet1' - self.port.save() - vif = {'id': "fake_vif_id"} - mock_gfp.return_value = self.port - mock_client.return_value.show_port.side_effect = ( - neutron_exceptions.NeutronClientException) - mock_gpbpi.side_effect = exception.NetworkError - with task_manager.acquire(self.context, self.node.id) as task: - self.interface.vif_attach(task, vif) - mock_client.assert_called_once_with() - mock_gpbpi.assert_called_once_with(mock_client.return_value, - 'fake_vif_id') - mock_gfp.assert_called_once_with(task, 'fake_vif_id', set()) - mock_client.return_value.show_port.assert_called_once_with( - 'fake_vif_id') - self.assertFalse(mock_upa.called) - self.assertTrue(mock_save.called) - @mock.patch.object(common.VIFPortIDMixin, '_save_vif_to_port_like_obj') @mock.patch.object(common, 'get_free_port_like_object', autospec=True) @mock.patch.object(neutron_common, 'get_client') diff --git a/ironic_tempest_plugin/tests/api/admin/test_nodes.py b/ironic_tempest_plugin/tests/api/admin/test_nodes.py index 3e6f1f9efb..0500f4f0b8 100644 --- a/ironic_tempest_plugin/tests/api/admin/test_nodes.py +++ b/ironic_tempest_plugin/tests/api/admin/test_nodes.py @@ -299,6 +299,16 @@ class TestNodesVif(base.BaseBaremetalTest): _, self.chassis = self.create_chassis() _, self.node = self.create_node(self.chassis['uuid']) + self.net = self.admin_manager.networks_client.create_network() + + self.nport_id = self.admin_manager.ports_client.create_port( + network_id=self.net['network']['id'])['port']['id'] + + def tearDown(self): + super(TestNodesVif, self).tearDown() + self.admin_manager.ports_client.delete_port(self.nport_id) + self.admin_manager.networks_client.delete_network( + self.net['network']['id']) @decorators.idempotent_id('a3d319d0-cacb-4e55-a3dc-3fa8b74880f1') def test_vif_on_port(self): @@ -316,13 +326,13 @@ class TestNodesVif(base.BaseBaremetalTest): api_microversion_fixture.APIMicroversionFixture('1.28')) _, self.port = self.create_port(self.node['uuid'], data_utils.rand_mac_address()) - self.client.vif_attach(self.node['uuid'], 'test-vif') + self.client.vif_attach(self.node['uuid'], self.nport_id) _, body = self.client.vif_list(self.node['uuid']) - self.assertEqual({'vifs': [{'id': 'test-vif'}]}, body) + self.assertEqual({'vifs': [{'id': self.nport_id}]}, body) _, port = self.client.show_port(self.port['uuid']) - self.assertEqual('test-vif', + self.assertEqual(self.nport_id, port['internal_info']['tenant_vif_port_id']) - self.client.vif_detach(self.node['uuid'], 'test-vif') + self.client.vif_detach(self.node['uuid'], self.nport_id) _, body = self.client.vif_list(self.node['uuid']) self.assertEqual({'vifs': []}, body) _, port = self.client.show_port(self.port['uuid']) @@ -355,17 +365,17 @@ class TestNodesVif(base.BaseBaremetalTest): 'value': self.portgroup['uuid']}] self.client.update_port(self.port['uuid'], patch) - self.client.vif_attach(self.node['uuid'], 'test-vif') + self.client.vif_attach(self.node['uuid'], self.nport_id) _, body = self.client.vif_list(self.node['uuid']) - self.assertEqual({'vifs': [{'id': 'test-vif'}]}, body) + self.assertEqual({'vifs': [{'id': self.nport_id}]}, body) _, port = self.client.show_port(self.port['uuid']) self.assertNotIn('tenant_vif_port_id', port['internal_info']) _, portgroup = self.client.show_portgroup(self.portgroup['uuid']) - self.assertEqual('test-vif', + self.assertEqual(self.nport_id, portgroup['internal_info']['tenant_vif_port_id']) - self.client.vif_detach(self.node['uuid'], 'test-vif') + self.client.vif_detach(self.node['uuid'], self.nport_id) _, body = self.client.vif_list(self.node['uuid']) self.assertEqual({'vifs': []}, body) _, portgroup = self.client.show_portgroup(self.portgroup['uuid']) @@ -379,13 +389,13 @@ class TestNodesVif(base.BaseBaremetalTest): data_utils.rand_mac_address()) patch = [{'path': '/extra/vif_port_id', 'op': 'add', - 'value': 'test-vif'}] + 'value': self.nport_id}] self.client.update_port(self.port['uuid'], patch) _, body = self.client.vif_list(self.node['uuid']) - self.assertEqual({'vifs': [{'id': 'test-vif'}]}, body) + self.assertEqual({'vifs': [{'id': self.nport_id}]}, body) self.assertRaises(lib_exc.Conflict, self.client.vif_attach, - self.node['uuid'], 'test-vif') + self.node['uuid'], self.nport_id) - self.client.vif_detach(self.node['uuid'], 'test-vif') + self.client.vif_detach(self.node['uuid'], self.nport_id)