Fix performance of Server List with Neutron for Admins
https://review.openstack.org/#/c/30048/ optimized the
SecurityGroupsOutputController for the case where the server
list only contains one server, but in all other cases the current
code calls the Neutron driver in a way that makes it retrieve
all ports and security groups visible to the tenant.
For users with a Neutron admin role this retrieves all ports
and all SecGroups in the system, which on a large system is a
major performance issue and often leads to client timeouts.
Normally these users have further qualified their query to a
specific tenant or host, or are maybe just trying to get their
own list of servers.
If instead we pass the pre-filtered list of servers to Neutron
it can query for ports associated with just the device IDs of
the servers. We then use that list of ports to build a
specific list of security groups IDs to query for.
This approach means that we can remove the initial
optimization for a single server case.
Closes-Bug: 1228384
(cherry picked from commit 18c3ac4a89
)
Conflicts:
nova/tests/api/openstack/compute/contrib/test_neutron_security_groups.py
Change-Id: I6f72533056e4f336f7578cc883fd1a125c2048a9
This commit is contained in:
parent
e5cb2de6a6
commit
19fdaa225a
@ -521,20 +521,15 @@ class SecurityGroupsOutputController(wsgi.Controller):
|
||||
# neutron security groups the requested security groups for the
|
||||
# instance are not in the db and have not been sent to neutron yet.
|
||||
if req.method != 'POST':
|
||||
if len(servers) == 1:
|
||||
group = (self.security_group_api
|
||||
.get_instance_security_groups(context,
|
||||
servers[0]['id']))
|
||||
if group:
|
||||
servers[0][key] = group
|
||||
else:
|
||||
sg_instance_bindings = (
|
||||
sg_instance_bindings = (
|
||||
self.security_group_api
|
||||
.get_instances_security_groups_bindings(context))
|
||||
for server in servers:
|
||||
groups = sg_instance_bindings.get(server['id'])
|
||||
if groups:
|
||||
server[key] = groups
|
||||
.get_instances_security_groups_bindings(context,
|
||||
servers))
|
||||
for server in servers:
|
||||
groups = sg_instance_bindings.get(server['id'])
|
||||
if groups:
|
||||
server[key] = groups
|
||||
|
||||
# In this section of code len(servers) == 1 as you can only POST
|
||||
# one server in an API request.
|
||||
else:
|
||||
|
@ -67,20 +67,15 @@ class SecurityGroupsOutputController(wsgi.Controller):
|
||||
# neutron security groups the requested security groups for the
|
||||
# instance are not in the db and have not been sent to neutron yet.
|
||||
if req.method != 'POST':
|
||||
if len(servers) == 1:
|
||||
group = (self.security_group_api
|
||||
.get_instance_security_groups(context,
|
||||
servers[0]['id']))
|
||||
if group:
|
||||
servers[0][key] = group
|
||||
else:
|
||||
sg_instance_bindings = (
|
||||
self.security_group_api
|
||||
.get_instances_security_groups_bindings(context))
|
||||
for server in servers:
|
||||
groups = sg_instance_bindings.get(server['id'])
|
||||
if groups:
|
||||
server[key] = groups
|
||||
sg_instance_bindings = (
|
||||
self.security_group_api
|
||||
.get_instances_security_groups_bindings(context,
|
||||
servers))
|
||||
for server in servers:
|
||||
groups = sg_instance_bindings.get(server['id'])
|
||||
if groups:
|
||||
server[key] = groups
|
||||
|
||||
# In this section of code len(servers) == 1 as you can only POST
|
||||
# one server in an API request.
|
||||
else:
|
||||
|
@ -42,6 +42,11 @@ wrap_check_security_groups_policy = compute_api.policy_decorator(
|
||||
CONF = cfg.CONF
|
||||
LOG = logging.getLogger(__name__)
|
||||
|
||||
# NOTE: Neutron client has a max URL length of 8192, so we have
|
||||
# to limit the number of IDs we include in any single search. Really
|
||||
# doesn't seem to be any point in making this a config value.
|
||||
MAX_SEARCH_IDS = 150
|
||||
|
||||
|
||||
class SecurityGroupAPI(security_group_base.SecurityGroupBase):
|
||||
|
||||
@ -279,35 +284,92 @@ class SecurityGroupAPI(security_group_base.SecurityGroupBase):
|
||||
raise exc_info[0], exc_info[1], exc_info[2]
|
||||
return self._convert_to_nova_security_group_rule_format(rule)
|
||||
|
||||
def get_instances_security_groups_bindings(self, context):
|
||||
def _get_ports_from_server_list(self, servers, neutron):
|
||||
"""Returns a list of ports used by the servers."""
|
||||
|
||||
def _chunk_by_ids(servers, limit):
|
||||
ids = []
|
||||
for server in servers:
|
||||
ids.append(server['id'])
|
||||
if len(ids) >= limit:
|
||||
yield ids
|
||||
ids = []
|
||||
if ids:
|
||||
yield ids
|
||||
|
||||
# Note: Have to split the query up as the search criteria
|
||||
# form part of the URL, which has a fixed max size
|
||||
ports = []
|
||||
for ids in _chunk_by_ids(servers, MAX_SEARCH_IDS):
|
||||
search_opts = {'device_id': ids}
|
||||
ports.extend(neutron.list_ports(**search_opts).get('ports'))
|
||||
|
||||
return ports
|
||||
|
||||
def _get_secgroups_from_port_list(self, ports, neutron):
|
||||
"""Returns a dict of security groups keyed by thier ids."""
|
||||
|
||||
def _chunk_by_ids(sg_ids, limit):
|
||||
sg_id_list = []
|
||||
for sg_id in sg_ids:
|
||||
sg_id_list.append(sg_id)
|
||||
if len(sg_id_list) >= limit:
|
||||
yield sg_id_list
|
||||
sg_id_list = []
|
||||
if sg_id_list:
|
||||
yield sg_id_list
|
||||
|
||||
# Find the set of unique SecGroup IDs to search for
|
||||
sg_ids = set()
|
||||
for port in ports:
|
||||
sg_ids.update(port.get('security_groups', []))
|
||||
|
||||
# Note: Have to split the query up as the search criteria
|
||||
# form part of the URL, which has a fixed max size
|
||||
security_groups = {}
|
||||
for sg_id_list in _chunk_by_ids(sg_ids, MAX_SEARCH_IDS):
|
||||
sg_search_opts = {'id': sg_id_list}
|
||||
search_results = neutron.list_security_groups(**sg_search_opts)
|
||||
for sg in search_results.get('security_groups'):
|
||||
security_groups[sg['id']] = sg
|
||||
|
||||
return security_groups
|
||||
|
||||
def get_instances_security_groups_bindings(self, context, servers,
|
||||
detailed=False):
|
||||
"""Returns a dict(instance_id, [security_groups]) to allow obtaining
|
||||
all of the instances and their security groups in one shot.
|
||||
"""
|
||||
neutron = neutronv2.get_client(context)
|
||||
ports = neutron.list_ports().get('ports')
|
||||
security_groups = neutron.list_security_groups().get('security_groups')
|
||||
security_group_lookup = {}
|
||||
instances_security_group_bindings = {}
|
||||
for security_group in security_groups:
|
||||
security_group_lookup[security_group['id']] = security_group
|
||||
|
||||
neutron = neutronv2.get_client(context)
|
||||
|
||||
ports = self._get_ports_from_server_list(servers, neutron)
|
||||
|
||||
security_groups = self._get_secgroups_from_port_list(ports, neutron)
|
||||
|
||||
instances_security_group_bindings = {}
|
||||
for port in ports:
|
||||
for port_security_group in port.get('security_groups', []):
|
||||
try:
|
||||
sg = security_group_lookup[port_security_group]
|
||||
# name is optional in neutron so if not specified return id
|
||||
if sg.get('name'):
|
||||
sg_entry = {'name': sg['name']}
|
||||
for port_sg_id in port.get('security_groups', []):
|
||||
|
||||
# Note: have to check we found port_sg as its possible
|
||||
# the port has an SG that this user doesn't have access to
|
||||
port_sg = security_groups.get(port_sg_id)
|
||||
if port_sg:
|
||||
if detailed:
|
||||
sg_entry = self._convert_to_nova_security_group_format(
|
||||
port_sg)
|
||||
instances_security_group_bindings.setdefault(
|
||||
port['device_id'], []).append(sg_entry)
|
||||
else:
|
||||
sg_entry = {'name': sg['id']}
|
||||
instances_security_group_bindings.setdefault(
|
||||
port['device_id'], []).append(sg_entry)
|
||||
except KeyError:
|
||||
# This should only happen due to a race condition
|
||||
# if the security group on a port was deleted after the
|
||||
# ports were returned. We pass since this security
|
||||
# group is no longer on the port.
|
||||
pass
|
||||
# name is optional in neutron so if not specified
|
||||
# return id
|
||||
name = port_sg.get('name')
|
||||
if not name:
|
||||
name = port_sg.get('id')
|
||||
sg_entry = {'name': name}
|
||||
instances_security_group_bindings.setdefault(
|
||||
port['device_id'], []).append(sg_entry)
|
||||
|
||||
return instances_security_group_bindings
|
||||
|
||||
def get_instance_security_groups(self, context, instance_uuid,
|
||||
@ -316,38 +378,10 @@ class SecurityGroupAPI(security_group_base.SecurityGroupBase):
|
||||
If detailed is True then it also returns the full details of the
|
||||
security groups associated with an instance.
|
||||
"""
|
||||
neutron = neutronv2.get_client(context)
|
||||
params = {'device_id': instance_uuid}
|
||||
ports = neutron.list_ports(**params)
|
||||
security_groups = neutron.list_security_groups().get('security_groups')
|
||||
|
||||
security_group_lookup = {}
|
||||
for security_group in security_groups:
|
||||
security_group_lookup[security_group['id']] = security_group
|
||||
|
||||
ret = []
|
||||
for port in ports['ports']:
|
||||
for security_group in port.get('security_groups', []):
|
||||
try:
|
||||
if detailed:
|
||||
ret.append(self._convert_to_nova_security_group_format(
|
||||
security_group_lookup[security_group]))
|
||||
else:
|
||||
name = security_group_lookup[security_group].get(
|
||||
'name')
|
||||
# Since the name is optional for
|
||||
# neutron security groups
|
||||
if not name:
|
||||
name = security_group
|
||||
ret.append({'name': name})
|
||||
except KeyError:
|
||||
# This should only happen due to a race condition
|
||||
# if the security group on a port was deleted after the
|
||||
# ports were returned. We pass since this security
|
||||
# group is no longer on the port.
|
||||
pass
|
||||
|
||||
return ret
|
||||
servers = [{'id': instance_uuid}]
|
||||
sg_bindings = self.get_instances_security_groups_bindings(
|
||||
context, servers, detailed)
|
||||
return sg_bindings.get(instance_uuid)
|
||||
|
||||
def _has_security_group_requirements(self, port):
|
||||
port_security_enabled = port.get('port_security_enabled', True)
|
||||
|
@ -270,6 +270,8 @@ class TestNeutronSecurityGroups(
|
||||
self.manager._removeSecurityGroup(req, '1', body)
|
||||
|
||||
def test_get_instances_security_groups_bindings(self):
|
||||
servers = [{'id': test_security_groups.FAKE_UUID1},
|
||||
{'id': test_security_groups.FAKE_UUID2}]
|
||||
sg1 = self._create_sg_template(name='test1').get('security_group')
|
||||
sg2 = self._create_sg_template(name='test2').get('security_group')
|
||||
# test name='' is replaced with id
|
||||
@ -290,8 +292,8 @@ class TestNeutronSecurityGroups(
|
||||
security_group_api = self.controller.security_group_api
|
||||
bindings = (
|
||||
security_group_api.get_instances_security_groups_bindings(
|
||||
context.get_admin_context()))
|
||||
self.assertEquals(bindings, expected)
|
||||
context.get_admin_context(), servers))
|
||||
self.assertEqual(bindings, expected)
|
||||
|
||||
def test_get_instance_security_groups(self):
|
||||
sg1 = self._create_sg_template(name='test1').get('security_group')
|
||||
@ -763,7 +765,7 @@ class MockClient(object):
|
||||
device_id = _params.get('device_id')
|
||||
for port in self._fake_ports.values():
|
||||
if device_id:
|
||||
if device_id == port['device_id']:
|
||||
if port['device_id'] in device_id:
|
||||
ret.append(port)
|
||||
else:
|
||||
ret.append(port)
|
||||
|
@ -1509,9 +1509,14 @@ def fake_compute_create(*args, **kwargs):
|
||||
return ([fake_compute_get(*args, **kwargs)], '')
|
||||
|
||||
|
||||
def fake_get_instances_security_groups_bindings(inst, context):
|
||||
return {UUID1: [{'name': 'fake-0-0'}, {'name': 'fake-0-1'}],
|
||||
UUID2: [{'name': 'fake-1-0'}, {'name': 'fake-1-1'}]}
|
||||
def fake_get_instances_security_groups_bindings(inst, context, servers):
|
||||
groups = {UUID1: [{'name': 'fake-0-0'}, {'name': 'fake-0-1'}],
|
||||
UUID2: [{'name': 'fake-1-0'}, {'name': 'fake-1-1'}],
|
||||
UUID3: [{'name': 'fake-2-0'}, {'name': 'fake-2-1'}]}
|
||||
result = {}
|
||||
for server in servers:
|
||||
result[server['id']] = groups.get(server['id'])
|
||||
return result
|
||||
|
||||
|
||||
class SecurityGroupsOutputTest(test.TestCase):
|
||||
|
@ -96,9 +96,14 @@ def fake_get_instance_security_groups(*args, **kwargs):
|
||||
return [{'name': 'fake'}]
|
||||
|
||||
|
||||
def fake_get_instances_security_groups_bindings(inst, context):
|
||||
return {UUID1: [{'name': 'fake-0-0'}, {'name': 'fake-0-1'}],
|
||||
UUID2: [{'name': 'fake-1-0'}, {'name': 'fake-1-1'}]}
|
||||
def fake_get_instances_security_groups_bindings(inst, context, servers):
|
||||
groups = {UUID1: [{'name': 'fake-0-0'}, {'name': 'fake-0-1'}],
|
||||
UUID2: [{'name': 'fake-1-0'}, {'name': 'fake-1-1'}],
|
||||
UUID3: [{'name': 'fake-2-0'}, {'name': 'fake-2-1'}]}
|
||||
result = {}
|
||||
for server in servers:
|
||||
result[server['id']] = groups.get(server['id'])
|
||||
return result
|
||||
|
||||
|
||||
class SecurityGroupsOutputTest(test.TestCase):
|
||||
|
@ -28,8 +28,11 @@ def fake_get(*args, **kwargs):
|
||||
return nova_group
|
||||
|
||||
|
||||
def fake_get_instance_security_groups(*args, **kwargs):
|
||||
return [{'name': 'test'}]
|
||||
def fake_get_instances_security_groups_bindings(self, context, servers):
|
||||
result = {}
|
||||
for s in servers:
|
||||
result[s.get('id')] = [{'name': 'test'}]
|
||||
return result
|
||||
|
||||
|
||||
class SecurityGroupsJsonTest(test_servers.ServersSampleBase):
|
||||
@ -40,8 +43,8 @@ class SecurityGroupsJsonTest(test_servers.ServersSampleBase):
|
||||
super(SecurityGroupsJsonTest, self).setUp()
|
||||
self.stubs.Set(neutron_driver.SecurityGroupAPI, 'get', fake_get)
|
||||
self.stubs.Set(neutron_driver.SecurityGroupAPI,
|
||||
'get_instance_security_groups',
|
||||
fake_get_instance_security_groups)
|
||||
'get_instances_security_groups_bindings',
|
||||
fake_get_instances_security_groups_bindings)
|
||||
|
||||
def test_server_create(self):
|
||||
self._post_server()
|
||||
|
@ -83,3 +83,99 @@ class TestNeutronDriver(test.NoDBTestCase):
|
||||
sg_api = security_groups.NativeNeutronSecurityGroupAPI()
|
||||
self.assertRaises(exception.SecurityGroupLimitExceeded,
|
||||
sg_api.add_rules, self.context, None, name, [vals])
|
||||
|
||||
def test_instances_security_group_bindings(self):
|
||||
server_id = 'c5a20e8d-c4b0-47cf-9dca-ebe4f758acb1'
|
||||
port1_id = '4c505aec-09aa-47bc-bcc0-940477e84dc0'
|
||||
port2_id = 'b3b31a53-6e29-479f-ae5c-00b7b71a6d44'
|
||||
sg1_id = '2f7ce969-1a73-4ef9-bbd6-c9a91780ecd4'
|
||||
sg2_id = '20c89ce5-9388-4046-896e-64ffbd3eb584'
|
||||
servers = [{'id': server_id}]
|
||||
ports = [{'id': port1_id, 'device_id': server_id,
|
||||
'security_groups': [sg1_id]},
|
||||
{'id': port2_id, 'device_id': server_id,
|
||||
'security_groups': [sg2_id]}]
|
||||
port_list = {'ports': ports}
|
||||
sg1 = {'id': sg1_id, 'name': 'wol'}
|
||||
sg2 = {'id': sg2_id, 'name': 'eor'}
|
||||
security_groups_list = {'security_groups': [sg1, sg2]}
|
||||
|
||||
sg_bindings = {server_id: [{'name': 'wol'}, {'name': 'eor'}]}
|
||||
|
||||
self.moxed_client.list_ports(device_id=[server_id]).AndReturn(
|
||||
port_list)
|
||||
self.moxed_client.list_security_groups(id=[sg2_id, sg1_id]).AndReturn(
|
||||
security_groups_list)
|
||||
self.mox.ReplayAll()
|
||||
|
||||
sg_api = neutron_driver.SecurityGroupAPI()
|
||||
result = sg_api.get_instances_security_groups_bindings(
|
||||
self.context, servers)
|
||||
self.assertEquals(result, sg_bindings)
|
||||
|
||||
def _test_instances_security_group_bindings_scale(self, num_servers):
|
||||
max_query = 150
|
||||
sg1_id = '2f7ce969-1a73-4ef9-bbd6-c9a91780ecd4'
|
||||
sg2_id = '20c89ce5-9388-4046-896e-64ffbd3eb584'
|
||||
sg1 = {'id': sg1_id, 'name': 'wol'}
|
||||
sg2 = {'id': sg2_id, 'name': 'eor'}
|
||||
security_groups_list = {'security_groups': [sg1, sg2]}
|
||||
servers = []
|
||||
device_ids = []
|
||||
ports = []
|
||||
sg_bindings = {}
|
||||
for i in xrange(0, num_servers):
|
||||
server_id = "server-%d" % i
|
||||
port_id = "port-%d" % i
|
||||
servers.append({'id': server_id})
|
||||
device_ids.append(server_id)
|
||||
ports.append({'id': port_id,
|
||||
'device_id': server_id,
|
||||
'security_groups': [sg1_id, sg2_id]})
|
||||
sg_bindings[server_id] = [{'name': 'wol'}, {'name': 'eor'}]
|
||||
|
||||
for x in xrange(0, num_servers, max_query):
|
||||
self.moxed_client.list_ports(
|
||||
device_id=device_ids[x:x + max_query]).\
|
||||
AndReturn({'ports': ports[x:x + max_query]})
|
||||
|
||||
self.moxed_client.list_security_groups(id=[sg2_id, sg1_id]).AndReturn(
|
||||
security_groups_list)
|
||||
self.mox.ReplayAll()
|
||||
|
||||
sg_api = neutron_driver.SecurityGroupAPI()
|
||||
result = sg_api.get_instances_security_groups_bindings(
|
||||
self.context, servers)
|
||||
self.assertEquals(result, sg_bindings)
|
||||
|
||||
def test_instances_security_group_bindings_less_than_max(self):
|
||||
self._test_instances_security_group_bindings_scale(100)
|
||||
|
||||
def test_instances_security_group_bindings_max(self):
|
||||
self._test_instances_security_group_bindings_scale(150)
|
||||
|
||||
def test_instances_security_group_bindings_more_then_max(self):
|
||||
self._test_instances_security_group_bindings_scale(300)
|
||||
|
||||
def test_instances_security_group_bindings_with_hidden_sg(self):
|
||||
servers = [{'id': 'server_1'}]
|
||||
ports = [{'id': '1', 'device_id': 'dev_1', 'security_groups': ['1']},
|
||||
{'id': '2', 'device_id': 'dev_1', 'security_groups': ['2']}]
|
||||
port_list = {'ports': ports}
|
||||
sg1 = {'id': '1', 'name': 'wol'}
|
||||
sg2 = {'id': '2', 'name': 'eor'}
|
||||
# User doesn't have access to sg2
|
||||
security_groups_list = {'security_groups': [sg1]}
|
||||
|
||||
sg_bindings = {'dev_1': [{'name': 'wol'}]}
|
||||
|
||||
self.moxed_client.list_ports(device_id=['server_1']).AndReturn(
|
||||
port_list)
|
||||
self.moxed_client.list_security_groups(id=['1', '2']).AndReturn(
|
||||
security_groups_list)
|
||||
self.mox.ReplayAll()
|
||||
|
||||
sg_api = neutron_driver.SecurityGroupAPI()
|
||||
result = sg_api.get_instances_security_groups_bindings(
|
||||
self.context, servers)
|
||||
self.assertEquals(result, sg_bindings)
|
||||
|
Loading…
Reference in New Issue
Block a user