ports & networks gather should validate existance
_gather_port_ids_and_networks assumes that the input networks variable doesn't contain the networks in ifaces. This is a wrong assumption ever since the introduction of the "refresh_cache-<instance id>" locks to the process in the Liberty cycle (see related bugs) and the "Refactor network API 'get_instance_nw_info'" patchset (https://review.openstack.org/#/c/146036/). The fix validates that the networks stated in ifaces doen't exist in the gotten networks list. Duplicate networks were observed at the following closed/related bugs. Change-Id: I8c2c9e3c89babbe5e48c5129b9854013690b38f6 Closes-Bug: #1522112 Related-Bug: #1467581 Related-Bug: #1501735
This commit is contained in:
parent
b8804e68eb
commit
3031adb857
|
@ -253,6 +253,23 @@ def get_client(context, admin=False):
|
||||||
region_name=CONF.neutron.region_name)
|
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):
|
class API(base_api.NetworkAPI):
|
||||||
"""API for interacting with the neutron 2.x API."""
|
"""API for interacting with the neutron 2.x API."""
|
||||||
|
|
||||||
|
@ -955,16 +972,32 @@ class API(base_api.NetworkAPI):
|
||||||
net_ids)
|
net_ids)
|
||||||
# an interface was added/removed from instance.
|
# an interface was added/removed from instance.
|
||||||
else:
|
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 + [
|
networks = networks + [
|
||||||
{'id': iface['network']['id'],
|
{'id': iface['network']['id'],
|
||||||
'name': iface['network']['label'],
|
'name': iface['network']['label'],
|
||||||
'tenant_id': iface['network']['meta']['tenant_id']}
|
'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.
|
# 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
|
return networks, port_ids
|
||||||
|
|
||||||
|
|
|
@ -2846,6 +2846,58 @@ class TestNeutronv2WithMock(test.TestCase):
|
||||||
api.get_instance_nw_info, 'context', instance)
|
api.get_instance_nw_info, 'context', instance)
|
||||||
mock_lock.assert_called_once_with('refresh_cache-%s' % instance.uuid)
|
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('oslo_concurrency.lockutils.lock')
|
||||||
@mock.patch.object(neutronapi.API, '_get_instance_nw_info')
|
@mock.patch.object(neutronapi.API, '_get_instance_nw_info')
|
||||||
@mock.patch('nova.network.base_api.update_instance_cache_with_nw_info')
|
@mock.patch('nova.network.base_api.update_instance_cache_with_nw_info')
|
||||||
|
|
Loading…
Reference in New Issue