diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index 2136cbaba753..c77aafb6f7b7 100644 --- a/nova/network/neutronv2/api.py +++ b/nova/network/neutronv2/api.py @@ -170,6 +170,23 @@ def get_client(context, admin=False): region_name=CONF.neutron.region_name) +def _is_not_duplicate(item, items, items_list_name, instance): + present = item in items + + # The expectation from this function's perspective is that the + # item is not part of the items list so if it is part of it + # we should at least log it as a warning + if present: + LOG.warning(_LW("%(item)s already exists in list: %(list_name)s " + "containing: %(items)s. ignoring it"), + {'item': item, + 'list_name': items_list_name, + 'items': items}, + instance=instance) + + return not present + + class API(base_api.NetworkAPI): """API for interacting with the neutron 2.x API.""" @@ -875,16 +892,32 @@ class API(base_api.NetworkAPI): net_ids) # an interface was added/removed from instance. else: - # Since networks does not contain the existing networks on the - # instance we use their values from the cache and add it. + + # Prepare the network ids list for validation purposes + networks_ids = [network['id'] for network in networks] + + # Validate that interface networks doesn't exist in networks. + # Though this issue can and should be solved in methods + # that prepare the networks list, this method should have this + # ignore-duplicate-networks/port-ids mechanism to reduce the + # probability of failing to boot the VM. networks = networks + [ {'id': iface['network']['id'], 'name': iface['network']['label'], 'tenant_id': iface['network']['meta']['tenant_id']} - for iface in ifaces] + for iface in ifaces + if _is_not_duplicate(iface['network']['id'], + networks_ids, + "networks", + instance)] # Include existing interfaces so they are not removed from the db. - port_ids = [iface['id'] for iface in ifaces] + port_ids + # Validate that the interface id is not in the port_ids + port_ids = [iface['id'] for iface in ifaces + if _is_not_duplicate(iface['id'], + port_ids, + "port_ids", + instance)] + port_ids return networks, port_ids diff --git a/nova/tests/unit/network/test_neutronv2.py b/nova/tests/unit/network/test_neutronv2.py index ad5080f3024e..e5bfcdf731a0 100644 --- a/nova/tests/unit/network/test_neutronv2.py +++ b/nova/tests/unit/network/test_neutronv2.py @@ -2850,6 +2850,58 @@ class TestNeutronv2WithMock(test.TestCase): api.get_instance_nw_info, 'context', instance) mock_lock.assert_called_once_with('refresh_cache-%s' % instance.uuid) + @mock.patch('nova.network.neutronv2.api.LOG') + def test_get_instance_nw_info_verify_duplicates_ignored(self, mock_log): + """test that the returned networks & port_ids from + _gather_port_ids_and_networks doesn't contain any duplicates + + The test fakes an instance with two ports connected to two networks. + The _gather_port_ids_and_networks method will be called with the + instance and a list of port ids of which one port id is configured + already to the instance (== duplicate #1) and a list of + networks that already contains a network to which an instance port + is connected (== duplicate #2). + + All-in-all, we expect the resulting port ids list to contain 3 items + (["instance_port_1", "port_1", "port_2"]) and the resulting networks + list to contain 3 items (["net_1", "net_2", "instance_network_1"]) + while the warning message for duplicate items was executed twice + (due to "duplicate #1" & "duplicate #2") + """ + + networks = [model.Network(id="net_1"), + model.Network(id="net_2")] + port_ids = ["port_1", "port_2"] + + instance_networks = [{"id": "instance_network_1", + "name": "fake_network", + "tenant_id": "fake_tenant_id"}] + instance_port_ids = ["instance_port_1"] + + network_info = model.NetworkInfo( + [{'id': port_ids[0], + 'network': networks[0]}, + {'id': instance_port_ids[0], + 'network': model.Network( + id=instance_networks[0]["id"], + label=instance_networks[0]["name"], + meta={"tenant_id": instance_networks[0]["tenant_id"]})}] + ) + + instance_uuid = uuid.uuid4() + instance = objects.Instance(uuid=instance_uuid, + info_cache=objects.InstanceInfoCache( + context=self.context, + instance_uuid=instance_uuid, + network_info=network_info)) + + new_networks, new_port_ids = self.api._gather_port_ids_and_networks( + self.context, instance, networks, port_ids) + + self.assertEqual(new_networks, networks + instance_networks) + self.assertEqual(new_port_ids, instance_port_ids + port_ids) + self.assertEqual(2, mock_log.warning.call_count) + @mock.patch('oslo_concurrency.lockutils.lock') @mock.patch.object(neutronapi.API, '_get_instance_nw_info') @mock.patch('nova.network.base_api.update_instance_cache_with_nw_info')