From 115cf068a6d48cdf8b0d20a3c5a779bb8120aa9b Mon Sep 17 00:00:00 2001 From: Kevin_Zheng Date: Mon, 28 Nov 2016 16:12:51 +0800 Subject: [PATCH] Don't delete neutron port when attach failed Currently, when attaching neutron pre-existing port to an instance, if the attach failed, it will also be deleted in Neutron side due to bad judgement of the who created the port by reading not up-to-date info_cache. The workflow starts at: https://github.com/openstack/nova/blob/9ed0d6114/nova/network/neutronv2/api.py#L881 ordered_ports and preexisting_port_ids are the same when attaching a preexisting port to an instance and it calls https://github.com/openstack/nova/blob/9ed0d6114/nova/network/base_api.py#L246 which calls back into the neutronv2 api code https://github.com/openstack/nova/blob/9ed0d6114/nova/network/neutronv2/api.py#L1274 and at this point, compute_utils.refresh_info_cache_for_instance(context, instance) won't have the newly attached port in it(see debug log: http://paste.openstack.org/show/613232/) because _build_network_info_model() is going to process it. The instance obj in memoryt with old info_cache will be used at rollback process and causing the miss-judging. This patch fixed it by updating instance.info_cache to the new ic after it is created. Co-Authored-By: huangtianhua@huawei.com Change-Id: Ib323b74d4ea1e874b476ab5addfc6bc79cb7c751 closes-bug: #1645175 --- .../instance-shutdown-end.json | 15 +----------- nova/network/base_api.py | 1 + nova/tests/unit/network/test_api.py | 24 ++++++++++++++----- .../notes/bug-1645175-b1ef3ad9a3e44ed6.yaml | 11 +++++++++ 4 files changed, 31 insertions(+), 20 deletions(-) create mode 100644 releasenotes/notes/bug-1645175-b1ef3ad9a3e44ed6.yaml diff --git a/doc/notification_samples/instance-shutdown-end.json b/doc/notification_samples/instance-shutdown-end.json index ea36c1c6a1..78f2aee61f 100644 --- a/doc/notification_samples/instance-shutdown-end.json +++ b/doc/notification_samples/instance-shutdown-end.json @@ -11,21 +11,8 @@ "fault":null, "host":"compute", "host_name":"some-server", - "ip_addresses": [{ - "nova_object.name": "IpPayload", - "nova_object.namespace": "nova", - "nova_object.version": "1.0", - "nova_object.data": { - "mac": "fa:16:3e:4c:2c:30", - "address": "192.168.1.3", - "port_uuid": "ce531f90-199f-48c0-816c-13e38010b442", - "meta": {}, - "version": 4, - "label": "private-network", - "device_name": "tapce531f90-19" - } - }], "key_name": "my-key", + "ip_addresses": [], "kernel_id":"", "launched_at":"2012-10-29T13:42:11Z", "image_uuid": "155d900f-4e14-4e4c-a73d-069cbf4541e6", diff --git a/nova/network/base_api.py b/nova/network/base_api.py index ccfa153094..12bef29949 100644 --- a/nova/network/base_api.py +++ b/nova/network/base_api.py @@ -48,6 +48,7 @@ def update_instance_cache_with_nw_info(impl, context, instance, ic = objects.InstanceInfoCache.new(context, instance.uuid) ic.network_info = nw_info ic.save(update_cells=update_cells) + instance.info_cache = ic except Exception: with excutils.save_and_reraise_exception(): LOG.exception(_LE('Failed storing info cache'), instance=instance) diff --git a/nova/tests/unit/network/test_api.py b/nova/tests/unit/network/test_api.py index 9cac18843e..a632c7e867 100644 --- a/nova/tests/unit/network/test_api.py +++ b/nova/tests/unit/network/test_api.py @@ -15,6 +15,7 @@ """Tests for network API.""" +import copy import itertools import uuid @@ -559,7 +560,7 @@ class ApiTestCase(test.TestCase): @mock.patch('nova.network.api.API') -@mock.patch('nova.db.instance_info_cache_update', return_value=fake_info_cache) +@mock.patch('nova.db.instance_info_cache_update') class TestUpdateInstanceCache(test.NoDBTestCase): def setUp(self): super(TestUpdateInstanceCache, self).setUp() @@ -572,32 +573,41 @@ class TestUpdateInstanceCache(test.NoDBTestCase): def test_update_nw_info_none(self, db_mock, api_mock): api_mock._get_instance_nw_info.return_value = self.nw_info - + info_cache = copy.deepcopy(fake_info_cache) + info_cache.update({'network_info': self.nw_json}) + db_mock.return_value = info_cache base_api.update_instance_cache_with_nw_info(api_mock, self.context, self.instance, None) api_mock._get_instance_nw_info.assert_called_once_with(self.context, self.instance) db_mock.assert_called_once_with(self.context, self.instance.uuid, {'network_info': self.nw_json}) + self.assertEqual(self.nw_info, self.instance.info_cache.network_info) def test_update_nw_info_one_network(self, db_mock, api_mock): - api_mock._get_instance_nw_info.return_value = self.nw_info + info_cache = copy.deepcopy(fake_info_cache) + info_cache.update({'network_info': self.nw_json}) + db_mock.return_value = info_cache base_api.update_instance_cache_with_nw_info(api_mock, self.context, self.instance, self.nw_info) self.assertFalse(api_mock._get_instance_nw_info.called) db_mock.assert_called_once_with(self.context, self.instance.uuid, {'network_info': self.nw_json}) + self.assertEqual(self.nw_info, self.instance.info_cache.network_info) def test_update_nw_info_empty_list(self, db_mock, api_mock): - api_mock._get_instance_nw_info.return_value = self.nw_info + new_nw_info = network_model.NetworkInfo([]) + db_mock.return_value = fake_info_cache base_api.update_instance_cache_with_nw_info(api_mock, self.context, - self.instance, - network_model.NetworkInfo([])) + self.instance, new_nw_info) self.assertFalse(api_mock._get_instance_nw_info.called) db_mock.assert_called_once_with(self.context, self.instance.uuid, {'network_info': '[]'}) + self.assertEqual(new_nw_info, self.instance.info_cache.network_info) def test_decorator_return_object(self, db_mock, api_mock): + db_mock.return_value = fake_info_cache + @base_api.refresh_cache def func(self, context, instance): return network_model.NetworkInfo([]) @@ -607,6 +617,8 @@ class TestUpdateInstanceCache(test.NoDBTestCase): {'network_info': '[]'}) def test_decorator_return_none(self, db_mock, api_mock): + db_mock.return_value = fake_info_cache + @base_api.refresh_cache def func(self, context, instance): pass diff --git a/releasenotes/notes/bug-1645175-b1ef3ad9a3e44ed6.yaml b/releasenotes/notes/bug-1645175-b1ef3ad9a3e44ed6.yaml new file mode 100644 index 0000000000..f5453bcbd7 --- /dev/null +++ b/releasenotes/notes/bug-1645175-b1ef3ad9a3e44ed6.yaml @@ -0,0 +1,11 @@ +--- +other: + - | + ``instance.shutdown.end`` versioned notification will + have an empty ``ip_addresses`` field since the network + resources associated with the instance are deallocated + before this notification is sent, which is actually + more accurate. Consumers should rely on the + instance.shutdown.start notification if they need the + network information for the instance when it is being + deleted.