Avoid processing ports which are not yet ready

This patch changes get_vif_port_set in order to not return
OVS ports for which the ofport is not yet assigned, thus avoiding
a regex match failure in get_vif_port_by_id.
Because of this failure, treat_vif_port is unable to wire
the port.
As get_vif_port_by_id is also used elsewhere in the agent, it has
been enhanced in order to tolerate situations in which ofport might
have not yet been assigned.

The ofport field is added to the list of those monitored by the
SimpleInterfaceMonitor. This will guarantee an event is generated
when the ofport is assigned to a port. Otherwise there is a risk
a port would be never processed if it was not yet ready the first
time is was detected. This change won't trigger any extra processing
on the agent side.

Finally, this patch avoids fetching device details from the plugin
for ports which have disappeared from the OVS bridge. This is a
little optimization which might be beneficial for short lived ports.

Change-Id: Icf7f0c7d6fe5239a358567cc9dc9db8ec11c15be
Partial-Bug: #1253896

neutron/agent/linux/ovs_lib.py

@@ -118,7 +118,7 @@ class OVSBridge(BaseOVS):
         mac = 'attached-mac="(?P<vif_mac>([a-fA-F\d]{2}:){5}([a-fA-F\d]{2}))"'
         iface = 'iface-id="(?P<vif_id>[^"]+)"'
         name = 'name\s*:\s"(?P<port_name>[^"]*)"'
-        port = 'ofport\s*:\s(?P<ofport>-?\d+)'
+        port = 'ofport\s*:\s(?P<ofport>(-?\d+|\[\]))'
         _re = ('%(external)s:\s{ ( %(mac)s,? | %(iface)s,? | . )* }'
                ' \s+ %(name)s \s+ %(port)s' % {'external': external,
                                                'mac': mac,
@@ -356,7 +356,7 @@ class OVSBridge(BaseOVS):
     def get_vif_port_set(self):
         port_names = self.get_port_name_list()
         edge_ports = set()
-        args = ['--format=json', '--', '--columns=name,external_ids',
+        args = ['--format=json', '--', '--columns=name,external_ids,ofport',
                 'list', 'Interface']
         result = self.run_vsctl(args, check_error=True)
         if not result:
@@ -366,14 +366,29 @@ class OVSBridge(BaseOVS):
             if name not in port_names:
                 continue
             external_ids = dict(row[1][1])
-            if "iface-id" in external_ids and "attached-mac" in external_ids:
+            # Do not consider VIFs which aren't yet ready
+            # This can happen when ofport values are either [] or ["set", []]
+            # We will therefore consider only integer values for ofport
+            ofport = row[2]
+            try:
+                int_ofport = int(ofport)
+            except (ValueError, TypeError):
+                LOG.warn(_("Found not yet ready openvswitch port: %s"), row)
+            else:
+                if int_ofport > 0:
+                    if ("iface-id" in external_ids and
+                        "attached-mac" in external_ids):
                         edge_ports.add(external_ids['iface-id'])
                     elif ("xs-vif-uuid" in external_ids and
                           "attached-mac" in external_ids):
-                # if this is a xenserver and iface-id is not automatically
+                        # if this is a xenserver and iface-id is not
-                # synced to OVS from XAPI, we grab it from XAPI directly
+                        # automatically synced to OVS from XAPI, we grab it
-                iface_id = self.get_xapi_iface_id(external_ids["xs-vif-uuid"])
+                        # from XAPI directly
+                        iface_id = self.get_xapi_iface_id(
+                            external_ids["xs-vif-uuid"])
                         edge_ports.add(iface_id)
+                else:
+                    LOG.warn(_("Found failed openvswitch port: %s"), row)
         return edge_ports
 
     def get_vif_port_by_id(self, port_id):
@@ -383,12 +398,24 @@ class OVSBridge(BaseOVS):
         result = self.run_vsctl(args)
         if not result:
             return
+        # TODO(salv-orlando): consider whether it would be possible to use
+        # JSON formatting rather than doing regex parsing.
         match = self.re_id.search(result)
         try:
             vif_mac = match.group('vif_mac')
             vif_id = match.group('vif_id')
             port_name = match.group('port_name')
+            # Tolerate ports which might not have an ofport as they are not
+            # ready yet
+            # NOTE(salv-orlando): Consider returning None when ofport is not
+            # available.
+            try:
                 ofport = int(match.group('ofport'))
+            except ValueError:
+                LOG.warn(_("ofport for vif: %s is not a valid integer. "
+                           "The port has not yet been configured by OVS"),
+                         vif_id)
+                ofport = None
             return VifPort(port_name, ofport, vif_id, vif_mac, self)
         except Exception as e:
             LOG.info(_("Unable to parse regex results. Exception: %s"), e)

neutron/agent/linux/ovsdb_monitor.py

@@ -72,7 +72,7 @@ class SimpleInterfaceMonitor(OvsdbMonitor):
     def __init__(self, root_helper=None, respawn_interval=None):
         super(SimpleInterfaceMonitor, self).__init__(
             'Interface',
-            columns=['name'],
+            columns=['name', 'ofport'],
             format='json',
             root_helper=root_helper,
             respawn_interval=respawn_interval,

neutron/plugins/openvswitch/agent/ovs_neutron_agent.py

@@ -599,7 +599,7 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
         if cur_tag != str(lvm.vlan):
             self.int_br.set_db_attribute("Port", port.port_name, "tag",
                                          str(lvm.vlan))
-            if int(port.ofport) != -1:
+            if port.ofport != -1:
                 self.int_br.delete_flows(in_port=port.ofport)
 
     def port_unbound(self, vif_id, net_uuid=None):
@@ -854,6 +854,13 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
 
     def treat_vif_port(self, vif_port, port_id, network_id, network_type,
                        physical_network, segmentation_id, admin_state_up):
+        # When this function is called for a port, the port should have
+        # an OVS ofport configured, as only these ports were considered
+        # for being treated. If that does not happen, it is a potential
+        # error condition of which operators should be aware
+        if not vif_port.ofport:
+            LOG.warn(_("VIF port: %s has no ofport configured, and might not "
+                       "be able to transmit"), vif_port.vif_id)
         if vif_port:
             if admin_state_up:
                 self.port_bound(vif_port, network_id, network_type,
@@ -918,7 +925,15 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
     def treat_devices_added_or_updated(self, devices):
         resync = False
         for device in devices:
-            LOG.debug(_("Processing port:%s"), device)
+            LOG.debug(_("Processing port %s"), device)
+            port = self.int_br.get_vif_port_by_id(device)
+            if not port:
+                # The port has disappeared and should not be processed
+                # There is no need to put the port DOWN in the plugin as
+                # it never went up in the first place
+                LOG.info(_("Port %s was not found on the integration bridge "
+                           "and will therefore not be processed"), device)
+                continue
             try:
                 # TODO(salv-orlando): Provide bulk API for retrieving
                 # details for all devices in one call
@@ -931,7 +946,6 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
                           {'device': device, 'e': e})
                 resync = True
                 continue
-            port = self.int_br.get_vif_port_by_id(details['device'])
             if 'port_id' in details:
                 LOG.info(_("Port %(device)s updated. Details: %(details)s"),
                          {'device': device, 'details': details})
@@ -950,9 +964,10 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
                     LOG.debug(_("Setting status for %s to DOWN"), device)
                     self.plugin_rpc.update_device_down(
                         self.context, device, self.agent_id, cfg.CONF.host)
+                LOG.info(_("Configuration for device %s completed."), device)
             else:
-                LOG.debug(_("Device %s not defined on plugin"), device)
+                LOG.warn(_("Device %s not defined on plugin"), device)
-                if (port and int(port.ofport) != -1):
+                if (port and port.ofport != -1):
                     self.port_dead(port)
         return resync
 

neutron/tests/unit/openvswitch/test_ovs_lib.py

@@ -408,12 +408,13 @@ class OVS_Lib_Test(base.BaseTestCase):
             ovs_row = []
             r["data"].append(ovs_row)
             for cell in row:
-                if isinstance(cell, str):
+                if isinstance(cell, (str, int, list)):
                     ovs_row.append(cell)
                 elif isinstance(cell, dict):
                     ovs_row.append(["map", cell.items()])
                 else:
-                    raise TypeError('%r not str or dict' % type(cell))
+                    raise TypeError('%r not int, str, list or dict' %
+                                    type(cell))
         return jsonutils.dumps(r)
 
     def _test_get_vif_port_set(self, is_xen):
@@ -425,11 +426,17 @@ class OVS_Lib_Test(base.BaseTestCase):
         headings = ['name', 'external_ids']
         data = [
             # A vif port on this bridge:
-            ['tap99', {id_key: 'tap99id', 'attached-mac': 'tap99mac'}],
+            ['tap99', {id_key: 'tap99id', 'attached-mac': 'tap99mac'}, 1],
+            # A vif port on this bridge not yet configured
+            ['tap98', {id_key: 'tap98id', 'attached-mac': 'tap98mac'}, []],
+            # Another vif port on this bridge not yet configured
+            ['tap97', {id_key: 'tap97id', 'attached-mac': 'tap97mac'},
+             ['set', []]],
+
             # A vif port on another bridge:
-            ['tap88', {id_key: 'tap88id', 'attached-mac': 'tap88id'}],
+            ['tap88', {id_key: 'tap88id', 'attached-mac': 'tap88id'}, 1],
             # Non-vif port on this bridge:
-            ['tun22', {}],
+            ['tun22', {}, 2],
         ]
 
         # Each element is a tuple of (expected mock call, return_value)
@@ -438,7 +445,7 @@ class OVS_Lib_Test(base.BaseTestCase):
                        root_helper=self.root_helper),
              'tap99\ntun22'),
             (mock.call(["ovs-vsctl", self.TO, "--format=json",
-                        "--", "--columns=name,external_ids",
+                        "--", "--columns=name,external_ids,ofport",
                         "list", "Interface"],
                        root_helper=self.root_helper),
              self._encode_ovs_json(headings, data)),
@@ -494,7 +501,7 @@ class OVS_Lib_Test(base.BaseTestCase):
                        root_helper=self.root_helper),
              'tap99\n'),
             (mock.call(["ovs-vsctl", self.TO, "--format=json",
-                        "--", "--columns=name,external_ids",
+                        "--", "--columns=name,external_ids,ofport",
                         "list", "Interface"],
                        root_helper=self.root_helper),
              RuntimeError()),
@@ -615,3 +622,36 @@ class OVS_Lib_Test(base.BaseTestCase):
                         return_value=mock.Mock(address=None)):
             with testtools.ExpectedException(Exception):
                 self.br.get_local_port_mac()
+
+    def _test_get_vif_port_by_id(self, ofport=None):
+        expected_output = ('external_ids : {attached-mac="aa:bb:cc:dd:ee:ff", '
+                           'iface-id="tap99id",'
+                           'iface-status=active, '
+                           'vm-uuid="tap99vm"}'
+                           '\nname : "tap99"\nofport : %(ofport)s\n')
+
+        # Each element is a tuple of (expected mock call, return_value)
+        expected_calls_and_values = [
+            (mock.call(["ovs-vsctl", self.TO,
+                        "--", "--columns=external_ids,name,ofport",
+                        "find", "Interface",
+                        'external_ids:iface-id="tap99id"'],
+                       root_helper=self.root_helper),
+             expected_output % {'ofport': ofport or '[]'}),
+        ]
+        tools.setup_mock_calls(self.execute, expected_calls_and_values)
+
+        vif_port = self.br.get_vif_port_by_id('tap99id')
+        self.assertEqual(vif_port.vif_id, 'tap99id')
+        self.assertEqual(vif_port.vif_mac, 'aa:bb:cc:dd:ee:ff')
+        self.assertEqual(vif_port.port_name, 'tap99')
+        tools.verify_mock_calls(self.execute, expected_calls_and_values)
+        return vif_port
+
+    def test_get_vif_by_port_id_with_ofport(self):
+        vif_port = self._test_get_vif_port_by_id('1')
+        self.assertEqual(vif_port.ofport, 1)
+
+    def test_get_vif_by_port_id_without_ofport(self):
+        vif_port = self._test_get_vif_port_by_id()
+        self.assertIsNone(vif_port.ofport)

neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py

@@ -248,8 +248,11 @@ class TestOvsNeutronAgent(base.BaseTestCase):
         self.assertEqual(expected, actual)
 
     def test_treat_devices_added_returns_true_for_missing_device(self):
-        with mock.patch.object(self.agent.plugin_rpc, 'get_device_details',
+        with contextlib.nested(
-                               side_effect=Exception()):
+            mock.patch.object(self.agent.plugin_rpc, 'get_device_details',
+                              side_effect=Exception()),
+            mock.patch.object(self.agent.int_br, 'get_vif_port_by_id',
+                              return_value=mock.Mock())):
             self.assertTrue(self.agent.treat_devices_added_or_updated([{}]))
 
     def _mock_treat_devices_added_updated(self, details, port, func_name):
@@ -284,7 +287,15 @@ class TestOvsNeutronAgent(base.BaseTestCase):
         self.assertTrue(self._mock_treat_devices_added_updated(
             mock.MagicMock(), port, 'port_dead'))
 
-    def test_treat_devices_added_updated_updates_known_port(self):
+    def test_treat_devices_added_does_not_process_missing_port(self):
+        with contextlib.nested(
+            mock.patch.object(self.agent.plugin_rpc, 'get_device_details'),
+            mock.patch.object(self.agent.int_br, 'get_vif_port_by_id',
+                              return_value=None)
+        ) as (get_dev_fn, get_vif_func):
+            self.assertFalse(get_dev_fn.called)
+
+    def test_treat_devices_added__updated_updates_known_port(self):
         details = mock.MagicMock()
         details.__contains__.side_effect = lambda x: True
         self.assertTrue(self._mock_treat_devices_added_updated(