Merge "ports & networks gather should validate existance"

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
 

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')