Read vif port information in bulk
During startup, the agent was making many calls per port to read information about the current VLAN, external ID, etc. This resulted in hundreds of calls just to read information about a relatively small number of ports. This patch addresses that by converting a few key functions to lookup information for all of the ports at once. Performance improvement on dev laptop for 250 ports from agent start to port ACTIVE status: before: 1m21s after: 1m06s Closes-Bug: #1460233 Change-Id: Ic80c85a07fee3e5651dc19819c6cebdc2048dda7
This commit is contained in:
Kevin Benton
parent
49b64ca6bc
commit
4dc68ea88b
|
|
@ -141,6 +141,12 @@ class BaseOVS(object):
|
|||
return self.ovsdb.db_get(table, record, column).execute(
|
||||
check_error=check_error, log_errors=log_errors)
|
||||
|
||||
def db_list(self, table, records=None, columns=None,
|
||||
check_error=True, log_errors=True, if_exists=False):
|
||||
return (self.ovsdb.db_list(table, records=records, columns=columns,
|
||||
if_exists=if_exists).
|
||||
execute(check_error=check_error, log_errors=log_errors))
|
||||
|
||||
|
||||
class OVSBridge(BaseOVS):
|
||||
def __init__(self, br_name):
|
||||
|
|
@ -319,11 +325,12 @@ class OVSBridge(BaseOVS):
|
|||
def get_vif_ports(self):
|
||||
edge_ports = []
|
||||
port_names = self.get_port_name_list()
|
||||
port_info = self.db_list(
|
||||
'Interface', columns=['name', 'external_ids', 'ofport'])
|
||||
by_name = {x['name']: x for x in port_info}
|
||||
for name in port_names:
|
||||
external_ids = self.db_get_val("Interface", name, "external_ids",
|
||||
check_error=True)
|
||||
ofport = self.db_get_val("Interface", name, "ofport",
|
||||
check_error=True)
|
||||
external_ids = by_name[name]['external_ids']
|
||||
ofport = by_name[name]['ofport']
|
||||
if "iface-id" in external_ids and "attached-mac" in external_ids:
|
||||
p = VifPort(name, ofport, external_ids["iface-id"],
|
||||
external_ids["attached-mac"], self)
|
||||
|
|
@ -341,10 +348,9 @@ class OVSBridge(BaseOVS):
|
|||
|
||||
def get_vif_port_to_ofport_map(self):
|
||||
port_names = self.get_port_name_list()
|
||||
cmd = self.ovsdb.db_list(
|
||||
'Interface', port_names,
|
||||
columns=['name', 'external_ids', 'ofport'], if_exists=True)
|
||||
results = cmd.execute(check_error=True)
|
||||
results = self.db_list(
|
||||
'Interface', port_names, ['name', 'external_ids', 'ofport'],
|
||||
if_exists=True)
|
||||
port_map = {}
|
||||
for r in results:
|
||||
# fall back to basic interface name
|
||||
|
|
@ -359,10 +365,9 @@ class OVSBridge(BaseOVS):
|
|||
def get_vif_port_set(self):
|
||||
edge_ports = set()
|
||||
port_names = self.get_port_name_list()
|
||||
cmd = self.ovsdb.db_list(
|
||||
'Interface', port_names,
|
||||
columns=['name', 'external_ids', 'ofport'], if_exists=True)
|
||||
results = cmd.execute(check_error=True)
|
||||
results = self.db_list(
|
||||
'Interface', port_names, ['name', 'external_ids', 'ofport'],
|
||||
if_exists=True)
|
||||
for result in results:
|
||||
if result['ofport'] == UNASSIGNED_OFPORT:
|
||||
LOG.warn(_LW("Found not yet ready openvswitch port: %s"),
|
||||
|
|
@ -400,11 +405,42 @@ class OVSBridge(BaseOVS):
|
|||
|
||||
"""
|
||||
port_names = self.get_port_name_list()
|
||||
cmd = self.ovsdb.db_list('Port', port_names, columns=['name', 'tag'],
|
||||
if_exists=True)
|
||||
results = cmd.execute(check_error=True)
|
||||
results = self.db_list('Port', port_names, ['name', 'tag'],
|
||||
if_exists=True)
|
||||
return {p['name']: p['tag'] for p in results}
|
||||
|
||||
def get_vifs_by_ids(self, port_ids):
|
||||
interface_info = self.db_list(
|
||||
"Interface", columns=["name", "external_ids", "ofport"])
|
||||
by_id = {x['external_ids'].get('iface-id'): x for x in interface_info}
|
||||
intfs_on_bridge = self.ovsdb.list_ports(self.br_name).execute(
|
||||
check_error=True)
|
||||
result = {}
|
||||
for port_id in port_ids:
|
||||
result[port_id] = None
|
||||
if (port_id not in by_id or
|
||||
by_id[port_id]['name'] not in intfs_on_bridge):
|
||||
LOG.info(_LI("Port %(port_id)s not present in bridge "
|
||||
"%(br_name)s"),
|
||||
{'port_id': port_id, 'br_name': self.br_name})
|
||||
continue
|
||||
pinfo = by_id[port_id]
|
||||
if not self._check_ofport(port_id, pinfo):
|
||||
continue
|
||||
mac = pinfo['external_ids'].get('attached-mac')
|
||||
result[port_id] = VifPort(pinfo['name'], pinfo['ofport'],
|
||||
port_id, mac, self)
|
||||
return result
|
||||
|
||||
@staticmethod
|
||||
def _check_ofport(port_id, port_info):
|
||||
if port_info['ofport'] in [UNASSIGNED_OFPORT, INVALID_OFPORT]:
|
||||
LOG.warn(_LW("ofport: %(ofport)s for VIF: %(vif)s is not a"
|
||||
" positive integer"),
|
||||
{'ofport': port_info['ofport'], 'vif': port_id})
|
||||
return False
|
||||
return True
|
||||
|
||||
def get_vif_port_by_id(self, port_id):
|
||||
ports = self.ovsdb.db_find(
|
||||
'Interface', ('external_ids', '=', {'iface-id': port_id}),
|
||||
|
|
@ -413,10 +449,7 @@ class OVSBridge(BaseOVS):
|
|||
for port in ports:
|
||||
if self.br_name != self.get_bridge_for_iface(port['name']):
|
||||
continue
|
||||
if port['ofport'] in [UNASSIGNED_OFPORT, INVALID_OFPORT]:
|
||||
LOG.warn(_LW("ofport: %(ofport)s for VIF: %(vif)s is not a"
|
||||
" positive integer"),
|
||||
{'ofport': port['ofport'], 'vif': port_id})
|
||||
if not self._check_ofport(port_id, port):
|
||||
continue
|
||||
mac = port['external_ids'].get('attached-mac')
|
||||
return VifPort(port['name'], port['ofport'], port_id, mac, self)
|
||||
|
|
|
|||
|
|
@ -312,10 +312,17 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
|
|||
|
||||
def _restore_local_vlan_map(self):
|
||||
cur_ports = self.int_br.get_vif_ports()
|
||||
port_info = self.int_br.db_list(
|
||||
"Port", columns=["name", "other_config", "tag"])
|
||||
by_name = {x['name']: x for x in port_info}
|
||||
for port in cur_ports:
|
||||
local_vlan_map = self.int_br.db_get_val("Port", port.port_name,
|
||||
"other_config")
|
||||
local_vlan = self.int_br.db_get_val("Port", port.port_name, "tag")
|
||||
# if a port was deleted between get_vif_ports and db_lists, we
|
||||
# will get a KeyError
|
||||
try:
|
||||
local_vlan_map = by_name[port.port_name]['other_config']
|
||||
local_vlan = by_name[port.port_name]['tag']
|
||||
except KeyError:
|
||||
continue
|
||||
if not local_vlan:
|
||||
continue
|
||||
net_uuid = local_vlan_map.get('net_uuid')
|
||||
|
|
@ -730,6 +737,9 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
|
|||
port_other_config)
|
||||
|
||||
def _bind_devices(self, need_binding_ports):
|
||||
port_info = self.int_br.db_list(
|
||||
"Port", columns=["name", "tag"])
|
||||
tags_by_name = {x['name']: x['tag'] for x in port_info}
|
||||
for port_detail in need_binding_ports:
|
||||
lvm = self.local_vlan_map.get(port_detail['network_id'])
|
||||
if not lvm:
|
||||
|
|
@ -739,7 +749,7 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
|
|||
port = port_detail['vif_port']
|
||||
device = port_detail['device']
|
||||
# Do not bind a port if it's already bound
|
||||
cur_tag = self.int_br.db_get_val("Port", port.port_name, "tag")
|
||||
cur_tag = tags_by_name.get(port.port_name)
|
||||
if cur_tag != lvm.vlan:
|
||||
self.int_br.set_db_attribute(
|
||||
"Port", port.port_name, "tag", lvm.vlan)
|
||||
|
|
@ -1196,10 +1206,12 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
|
|||
self.conf.host)
|
||||
except Exception as e:
|
||||
raise DeviceListRetrievalError(devices=devices, error=e)
|
||||
vif_by_id = self.int_br.get_vifs_by_ids(
|
||||
[vif['device'] for vif in devices_details_list])
|
||||
for details in devices_details_list:
|
||||
device = details['device']
|
||||
LOG.debug("Processing port: %s", device)
|
||||
port = self.int_br.get_vif_port_by_id(device)
|
||||
port = vif_by_id.get(device)
|
||||
if not port:
|
||||
# The port disappeared and cannot be processed
|
||||
LOG.info(_LI("Port %s was not found on the integration bridge "
|
||||
|
|
|
|||
|
|
@ -209,6 +209,15 @@ class OVSBridgeTestCase(OVSBridgeTestBase):
|
|||
self.assertEqual(self.br.get_vif_port_by_id(vif.vif_id).vif_id,
|
||||
vif.vif_id)
|
||||
|
||||
def test_get_vifs_by_ids(self):
|
||||
for i in range(2):
|
||||
self.create_ovs_port()
|
||||
vif_ports = [self.create_ovs_vif_port() for i in range(3)]
|
||||
by_id = self.br.get_vifs_by_ids([v.vif_id for v in vif_ports])
|
||||
# convert to str for comparison of VifPorts
|
||||
by_id = {vid: str(vport) for vid, vport in by_id.items()}
|
||||
self.assertEqual({v.vif_id: str(v) for v in vif_ports}, by_id)
|
||||
|
||||
def test_delete_ports(self):
|
||||
# TODO(twilson) I intensely dislike the current delete_ports function
|
||||
# as the default behavior is really delete_vif_ports(), then it acts
|
||||
|
|
|
|||
|
|
@ -470,23 +470,22 @@ class OVS_Lib_Test(base.BaseTestCase):
|
|||
def _test_get_vif_ports(self, is_xen=False):
|
||||
pname = "tap99"
|
||||
ofport = 6
|
||||
ofport_data = self._encode_ovs_json(['ofport'], [[ofport]])
|
||||
vif_id = uuidutils.generate_uuid()
|
||||
mac = "ca:fe:de:ad:be:ef"
|
||||
id_field = 'xs-vif-uuid' if is_xen else 'iface-id'
|
||||
external_ids = ('{"data":[[["map",[["attached-mac","%(mac)s"],'
|
||||
'["%(id_field)s","%(vif)s"],'
|
||||
'["iface-status","active"]]]]],'
|
||||
'"headings":["external_ids"]}' % {
|
||||
'mac': mac, 'vif': vif_id, 'id_field': id_field})
|
||||
'["iface-status","active"]]], '
|
||||
'"%(name)s", %(ofport)s]],'
|
||||
'"headings":["external_ids", "name", "ofport"]}' % {
|
||||
'mac': mac, 'vif': vif_id, 'id_field': id_field,
|
||||
'name': pname, 'ofport': ofport})
|
||||
|
||||
# Each element is a tuple of (expected mock call, return_value)
|
||||
expected_calls_and_values = [
|
||||
(self._vsctl_mock("list-ports", self.BR_NAME), "%s\n" % pname),
|
||||
(self._vsctl_mock("--columns=external_ids", "list",
|
||||
"Interface", pname), external_ids),
|
||||
(self._vsctl_mock("--columns=ofport", "list", "Interface", pname),
|
||||
ofport_data),
|
||||
(self._vsctl_mock("--columns=name,external_ids,ofport", "list",
|
||||
"Interface"), external_ids),
|
||||
]
|
||||
if is_xen:
|
||||
expected_calls_and_values.append(
|
||||
|
|
@ -724,6 +723,35 @@ class OVS_Lib_Test(base.BaseTestCase):
|
|||
with testtools.ExpectedException(Exception):
|
||||
self.br.get_local_port_mac()
|
||||
|
||||
def test_get_vifs_by_ids(self):
|
||||
db_list_res = [
|
||||
{'name': 'qvo1', 'ofport': 1,
|
||||
'external_ids': {'iface-id': 'pid1', 'attached-mac': '11'}},
|
||||
{'name': 'qvo2', 'ofport': 2,
|
||||
'external_ids': {'iface-id': 'pid2', 'attached-mac': '22'}},
|
||||
{'name': 'qvo3', 'ofport': 3,
|
||||
'external_ids': {'iface-id': 'pid3', 'attached-mac': '33'}},
|
||||
{'name': 'qvo4', 'ofport': -1,
|
||||
'external_ids': {'iface-id': 'pid4', 'attached-mac': '44'}},
|
||||
]
|
||||
self.br.db_list = mock.Mock(return_value=db_list_res)
|
||||
self.br.ovsdb = mock.Mock()
|
||||
self.br.ovsdb.list_ports.return_value.execute.return_value = [
|
||||
'qvo1', 'qvo2', 'qvo4']
|
||||
by_id = self.br.get_vifs_by_ids(['pid1', 'pid2', 'pid3',
|
||||
'pid4', 'pid5'])
|
||||
# pid3 isn't on bridge and pid4 doesn't have a valid ofport and pid5
|
||||
# isn't present in the db
|
||||
self.assertIsNone(by_id['pid3'])
|
||||
self.assertIsNone(by_id['pid4'])
|
||||
self.assertIsNone(by_id['pid5'])
|
||||
self.assertEqual('pid1', by_id['pid1'].vif_id)
|
||||
self.assertEqual('qvo1', by_id['pid1'].port_name)
|
||||
self.assertEqual(1, by_id['pid1'].ofport)
|
||||
self.assertEqual('pid2', by_id['pid2'].vif_id)
|
||||
self.assertEqual('qvo2', by_id['pid2'].port_name)
|
||||
self.assertEqual(2, by_id['pid2'].ofport)
|
||||
|
||||
def _test_get_vif_port_by_id(self, iface_id, data, br_name=None,
|
||||
extra_calls_and_values=None):
|
||||
headings = ['external_ids', 'name', 'ofport']
|
||||
|
|
|
|||
|
|
@ -114,6 +114,8 @@ class TestOvsNeutronAgent(object):
|
|||
cfg.CONF.set_default('quitting_rpc_timeout', 10, 'AGENT')
|
||||
cfg.CONF.set_default('prevent_arp_spoofing', False, 'AGENT')
|
||||
kwargs = self.mod_agent.create_agent_config_map(cfg.CONF)
|
||||
mock.patch('neutron.agent.common.ovs_lib.OVSBridge.db_list',
|
||||
return_value=[]).start()
|
||||
|
||||
with mock.patch.object(self.mod_agent.OVSNeutronAgent,
|
||||
'setup_integration_br'),\
|
||||
|
|
@ -172,7 +174,10 @@ class TestOvsNeutronAgent(object):
|
|||
mock.patch.object(self.agent, 'provision_local_vlan') as \
|
||||
provision_local_vlan:
|
||||
int_br.get_vif_ports.return_value = [port]
|
||||
int_br.db_get_val.side_effect = [local_vlan_map, tag]
|
||||
int_br.db_list.return_value = [{
|
||||
'name': port.port_name, 'other_config': local_vlan_map,
|
||||
'tag': tag
|
||||
}]
|
||||
self.agent._restore_local_vlan_map()
|
||||
if tag:
|
||||
self.assertTrue(provision_local_vlan.called)
|
||||
|
|
@ -341,8 +346,8 @@ class TestOvsNeutronAgent(object):
|
|||
'get_devices_details_list',
|
||||
return_value=[details]),\
|
||||
mock.patch.object(self.agent.int_br,
|
||||
'get_vif_port_by_id',
|
||||
return_value=port),\
|
||||
'get_vifs_by_ids',
|
||||
return_value={details['device']: port}),\
|
||||
mock.patch.object(self.agent, func_name) as func:
|
||||
skip_devs, need_bound_devices = (
|
||||
self.agent.treat_devices_added_or_updated([{}], False))
|
||||
|
|
@ -383,8 +388,8 @@ class TestOvsNeutronAgent(object):
|
|||
'get_devices_details_list',
|
||||
return_value=[dev_mock]),\
|
||||
mock.patch.object(self.agent.int_br,
|
||||
'get_vif_port_by_id',
|
||||
return_value=None),\
|
||||
'get_vifs_by_ids',
|
||||
return_value={}),\
|
||||
mock.patch.object(self.agent,
|
||||
'treat_vif_port') as treat_vif_port:
|
||||
skip_devs = self.agent.treat_devices_added_or_updated([{}], False)
|
||||
|
|
@ -410,8 +415,8 @@ class TestOvsNeutronAgent(object):
|
|||
'get_devices_details_list',
|
||||
return_value=[fake_details_dict]),\
|
||||
mock.patch.object(self.agent.int_br,
|
||||
'get_vif_port_by_id',
|
||||
return_value=mock.MagicMock()),\
|
||||
'get_vifs_by_ids',
|
||||
return_value={'xxx': mock.MagicMock()}),\
|
||||
mock.patch.object(self.agent,
|
||||
'treat_vif_port') as treat_vif_port:
|
||||
skip_devs, need_bound_devices = (
|
||||
|
|
@ -449,6 +454,8 @@ class TestOvsNeutronAgent(object):
|
|||
self.agent,
|
||||
"treat_devices_added_or_updated",
|
||||
return_value=([], [])) as device_added_updated,\
|
||||
mock.patch.object(self.agent.int_br, "db_list",
|
||||
return_value=[]),\
|
||||
mock.patch.object(self.agent,
|
||||
"treat_devices_removed",
|
||||
return_value=False) as device_removed:
|
||||
|
|
@ -1179,6 +1186,9 @@ class AncillaryBridgesTest(object):
|
|||
mock.patch('neutron.agent.common.ovs_lib.BaseOVS.'
|
||||
'get_bridge_external_bridge_id',
|
||||
side_effect=pullup_side_effect),\
|
||||
mock.patch(
|
||||
'neutron.agent.common.ovs_lib.OVSBridge.' 'db_list',
|
||||
return_value=[]),\
|
||||
mock.patch(
|
||||
'neutron.agent.common.ovs_lib.OVSBridge.' 'get_vif_ports',
|
||||
return_value=[]):
|
||||
|
|
@ -1268,6 +1278,9 @@ class TestOvsDvrNeutronAgent(object):
|
|||
mock.patch('neutron.openstack.common.loopingcall.'
|
||||
'FixedIntervalLoopingCall',
|
||||
new=MockFixedIntervalLoopingCall),\
|
||||
mock.patch(
|
||||
'neutron.agent.common.ovs_lib.OVSBridge.' 'db_list',
|
||||
return_value=[]),\
|
||||
mock.patch(
|
||||
'neutron.agent.common.ovs_lib.OVSBridge.' 'get_vif_ports',
|
||||
return_value=[]):
|
||||
|
|
|
|||
|
|
@ -121,6 +121,7 @@ class TunnelTest(object):
|
|||
self.mock_int_bridge.add_patch_port.side_effect = (
|
||||
lambda tap, peer: self.ovs_int_ofports[tap])
|
||||
self.mock_int_bridge.get_vif_ports.return_value = []
|
||||
self.mock_int_bridge.db_list.return_value = []
|
||||
self.mock_int_bridge.db_get_val.return_value = {}
|
||||
|
||||
self.mock_map_tun_bridge = self.ovs_bridges[self.MAP_TUN_BRIDGE]
|
||||
|
|
@ -208,6 +209,7 @@ class TunnelTest(object):
|
|||
]
|
||||
self.mock_int_bridge_expected += [
|
||||
mock.call.get_vif_ports(),
|
||||
mock.call.db_list('Port', columns=['name', 'other_config', 'tag'])
|
||||
]
|
||||
|
||||
self.mock_tun_bridge_expected += [
|
||||
|
|
@ -246,7 +248,7 @@ class TunnelTest(object):
|
|||
|
||||
def _verify_mock_call(self, mock_obj, expected):
|
||||
mock_obj.assert_has_calls(expected)
|
||||
self.assertEqual(len(mock_obj.mock_calls), len(expected))
|
||||
self.assertEqual(expected, mock_obj.mock_calls)
|
||||
|
||||
def _verify_mock_calls(self):
|
||||
self._verify_mock_call(self.mock_int_bridge_cls,
|
||||
|
|
@ -599,6 +601,7 @@ class TunnelTestUseVethInterco(TunnelTest):
|
|||
]
|
||||
self.mock_int_bridge_expected += [
|
||||
mock.call.get_vif_ports(),
|
||||
mock.call.db_list('Port', columns=['name', 'other_config', 'tag'])
|
||||
]
|
||||
self.mock_tun_bridge_expected += [
|
||||
mock.call.delete_flows(),
|
||||
|
|
|
|||
Loading…
Reference in New Issue