openstack/neutron
Code Issues Proposed changes

Do not skip ports with ofport unset or invalid

Browse Source 
This change removes the "_check_ofport" function and its use form
the ovs_lib.py file.

By skipping ports without a unique ofport in the "get_vifs_by_ids"
and "get_vifs_by_id" functions, the OVS agent incorrectly treated
newly added port with an ofport of -1 as removed ports in the
"treat_devices_added_or_updated" function.

Co-Authored-By: Rodolfo Alonso Hernandez <ralonsoh@redhat.com>

Change-Id: I79158baafbb99bee99a1d687039313eb454d3a9b
Partial-Bug: #1734320
Partial-Bug: #1815989
This commit is contained in:
Sean Mooney 2019-03-01 04:43:20 +00:00 committed by Rodolfo Alonso Hernandez
parent 856cae4cf8
commit 7fd2725cb1
6 changed files with 45 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
neutron/agent/common/ovs_lib.py
View File

@ -632,22 +632,11 @@ class OVSBridge(BaseOVS):
{'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.warning("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}),
@ -656,8 +645,6 @@ class OVSBridge(BaseOVS):
for port in ports:
if self.br_name != self.get_bridge_for_iface(port['name']):
continue
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)
LOG.info("Port %(port_id)s not present in bridge %(br_name)s",

12
neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py
View File

@ -23,6 +23,7 @@ import oslo_messaging
from oslo_utils import excutils
from osprofiler import profiler
from neutron.agent.common import ovs_lib
from neutron.agent.linux.openvswitch_firewall import firewall as ovs_firewall
from neutron.common import utils as n_utils
from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants
@ -424,6 +425,13 @@ class OVSDVRNeutronAgent(object):
local_compute_ports)
vif_by_id = self.int_br.get_vifs_by_ids(
[local_port['id'] for local_port in local_compute_ports])
# A router port has an OVS interface with type internal. Once the
# interface is created, a valid ofport will be assigned.
vif_by_id = {k: v for k, v in vif_by_id.items()
if not v or v.ofport not in
(ovs_lib.INVALID_OFPORT, ovs_lib.UNASSIGNED_OFPORT)}
for local_port in local_compute_ports:
vif = vif_by_id.get(local_port['id'])
if not vif:
@ -605,6 +613,10 @@ class OVSDVRNeutronAgent(object):
"%(ofport)s, rebinding.",
{'vif': port.vif_id, 'ofport': port.ofport})
self.unbind_port_from_dvr(port, local_vlan_map)
if port.ofport in (ovs_lib.INVALID_OFPORT,
ovs_lib.UNASSIGNED_OFPORT):
return
if device_owner == n_const.DEVICE_OWNER_DVR_INTERFACE:
self._bind_distributed_router_interface_port(port,
local_vlan_map,

20
neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py
View File

@ -1911,6 +1911,7 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin,
devices = devices_details_list.get('devices')
vif_by_id = self.int_br.get_vifs_by_ids(
[vif['device'] for vif in devices])
devices_not_in_datapath = set()
for details in devices:
device = details['device']
LOG.debug("Processing port: %s", device)
@ -1923,6 +1924,9 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin,
skipped_devices.append(device)
continue
if not port.ofport or port.ofport == ovs_lib.INVALID_OFPORT:
devices_not_in_datapath.add(device)
if 'port_id' in details:
LOG.info("Port %(device)s updated. Details: %(details)s",
{'device': device, 'details': details})
@ -1942,7 +1946,9 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin,
details['network_id'])
if details['device'] in re_added:
self.ext_manager.delete_port(self.context, details)
self.ext_manager.handle_port(self.context, details)
if device not in devices_not_in_datapath:
self.ext_manager.handle_port(self.context, details)
else:
if n_const.NO_ACTIVE_BINDING in details:
# Port was added to the bridge, but its binding in this
@ -1958,7 +1964,7 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin,
if (port and port.ofport != -1):
self.port_dead(port)
return (skipped_devices, binding_no_activated_devices,
need_binding_devices, failed_devices)
need_binding_devices, failed_devices, devices_not_in_datapath)
def _update_port_network(self, port_id, network_id):
self._clean_network_ports(port_id)
@ -2051,10 +2057,12 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin,
need_binding_devices = []
skipped_devices = set()
binding_no_activated_devices = set()
devices_not_in_datapath = set()
start = time.time()
if devices_added_updated:
(skipped_devices, binding_no_activated_devices,
need_binding_devices, failed_devices['added']) = (
need_binding_devices, failed_devices['added'],
devices_not_in_datapath) = (
self.treat_devices_added_or_updated(
devices_added_updated, provisioning_needed, re_added))
LOG.info("process_network_ports - iteration:%(iter_num)d - "
@ -2079,11 +2087,11 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin,
added_ports = (port_info.get('added', set()) - skipped_devices -
binding_no_activated_devices)
self._add_port_tag_info(need_binding_devices)
self.process_install_ports_egress_flows(need_binding_devices)
self.sg_agent.setup_port_filters(added_ports,
added_to_datapath = added_ports - devices_not_in_datapath
self.sg_agent.setup_port_filters(added_to_datapath,
port_info.get('updated', set()))
LOG.info("process_network_ports - iteration:%(iter_num)d - "
"agent port security group processed in %(elapsed).3f",
{'iter_num': self.iter_num,

4
neutron/tests/unit/agent/common/test_ovs_lib.py
View File

@ -498,9 +498,9 @@ class OVS_Lib_Test(base.BaseTestCase):
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'])
# pid3 isn't on bridge and pid4 doesn't have a valid ofport
# pid3 isn't on bridge
self.assertIsNone(by_id['pid3'])
self.assertIsNone(by_id['pid4'])
self.assertEqual(-1, by_id['pid4'].ofport)
self.assertEqual('pid1', by_id['pid1'].vif_id)
self.assertEqual('qvo1', by_id['pid1'].port_name)
self.assertEqual(1, by_id['pid1'].ofport)

22
neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py
View File

@ -882,7 +882,7 @@ class TestOvsNeutronAgent(object):
'get_port_tag_dict',
return_value={}),\
mock.patch.object(self.agent, func_name) as func:
skip_devs, _, need_bound_devices, _ = (
skip_devs, _, need_bound_devices, _, _ = (
self.agent.treat_devices_added_or_updated([], False, set()))
# The function should not raise
self.assertFalse(skip_devs)
@ -900,7 +900,7 @@ class TestOvsNeutronAgent(object):
'get_vifs_by_ids',
return_value={details['device']: port}),\
mock.patch.object(self.agent, 'port_dead') as func:
skip_devs, binding_no_activated_devices, _, _ = (
skip_devs, binding_no_activated_devices, _, _, _ = (
self.agent.treat_devices_added_or_updated([], False, set()))
self.assertFalse(skip_devs)
self.assertTrue(func.called)
@ -977,7 +977,7 @@ class TestOvsNeutronAgent(object):
[], False, set())
# The function should return False for resync and no device
# processed
self.assertEqual((['the_skipped_one'], set(), [], set()),
self.assertEqual((['the_skipped_one'], set(), [], set(), set()),
skip_devs)
ext_mgr_delete_port.assert_called_once_with(
self.agent.context, {'port_id': 'the_skipped_one'})
@ -995,7 +995,7 @@ class TestOvsNeutronAgent(object):
mock.patch.object(self.agent,
'treat_vif_port') as treat_vif_port:
failed_devices = {'added': set(), 'removed': set()}
(_, _, _, failed_devices['added']) = (
(_, _, _, failed_devices['added'], _) = (
self.agent.treat_devices_added_or_updated([], False, set()))
# The function should return False for resync and no device
# processed
@ -1026,7 +1026,7 @@ class TestOvsNeutronAgent(object):
return_value={}),\
mock.patch.object(self.agent,
'treat_vif_port') as treat_vif_port:
skip_devs, _, need_bound_devices, _ = (
skip_devs, _, need_bound_devices, _, _ = (
self.agent.treat_devices_added_or_updated([], False, set()))
# The function should return False for resync
self.assertFalse(skip_devs)
@ -1135,7 +1135,8 @@ class TestOvsNeutronAgent(object):
self.agent, "treat_devices_added_or_updated",
return_value=(
skipped_devices, binding_no_activated_devices, [],
failed_devices['added'])) as device_added_updated,\
failed_devices['added'],
set())) as device_added_updated,\
mock.patch.object(self.agent.int_br, "get_ports_attributes",
return_value=[]),\
mock.patch.object(self.agent,
@ -3157,7 +3158,7 @@ class TestOvsDvrNeutronAgent(object):
),
] + self._expected_port_bound(self._port, lvid,
network_type=network_type)
int_br.assert_has_calls(expected_on_int_br)
int_br.assert_has_calls(expected_on_int_br, any_order=True)
tun_br.assert_not_called()
phys_br.assert_has_calls(expected_on_phys_br)
int_br.reset_mock()
@ -3241,7 +3242,7 @@ class TestOvsDvrNeutronAgent(object):
lvid=lvid,
ip_version=ip_version,
gateway_ip=gateway_ip)
int_br.assert_has_calls(expected_on_int_br)
int_br.assert_has_calls(expected_on_int_br, any_order=True)
tun_br.assert_has_calls(expected_on_tun_br)
phys_br.assert_not_called()
int_br.reset_mock()
@ -3489,7 +3490,8 @@ class TestOvsDvrNeutronAgent(object):
False)
lvid = self.agent.vlan_manager.get(self._net_uuid).vlan
int_br.assert_has_calls(
self._expected_port_bound(self._port, lvid))
self._expected_port_bound(self._port, lvid),
any_order=True)
expected_on_tun_br = [
mock.call.provision_local_vlan(network_type='vxlan',
lvid=lvid, segmentation_id=None, distributed=True),
@ -3594,7 +3596,7 @@ class TestOvsDvrNeutronAgent(object):
False)
lvid = self.agent.vlan_manager.get(self._net_uuid).vlan
int_br.assert_has_calls(
self._expected_port_bound(self._port, lvid))
self._expected_port_bound(self._port, lvid), any_order=True)
expected_on_tun_br = [
mock.call.provision_local_vlan(
network_type='vxlan',

7
neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py
View File

@ -14,6 +14,7 @@
# under the License.
#
import collections
import time
from unittest import mock
@ -30,6 +31,8 @@ from neutron.tests.unit.plugins.ml2.drivers.openvswitch.agent \
import test_vlanmanager
Switch = collections.namedtuple('Switch', ['br_name'])
# Useful global dummy variables.
NET_UUID = '3faeebfe-5d37-11e1-a64b-000c29d5f0a7'
LS_ID = 420
@ -38,8 +41,8 @@ LV_IDS = [42, 43]
VIF_ID = '404deaec-5d37-11e1-a64b-000c29d5f0a8'
VIF_MAC = '3c:09:24:1e:78:23'
OFPORT_NUM = 1
VIF_PORT = ovs_lib.VifPort('port', OFPORT_NUM,
VIF_ID, VIF_MAC, 'switch')
VIF_PORT = ovs_lib.VifPort('port', OFPORT_NUM, VIF_ID, VIF_MAC,
Switch(br_name='br_name'))
VIF_PORTS = {VIF_ID: VIF_PORT}
FIXED_IPS = [{'subnet_id': 'my-subnet-uuid',
'ip_address': '1.1.1.1'}]