Raise an error from ovs_lib list operations

Previously list operations in ovs_lib returns an empty list
if RuntimeError occurs and a caller cannot distinguish an error
from normal results. This commit changes ovs_lib list operations
(get_vif_port_set, get_vif_ports, get_bridges) to raise an
exception when RuntimeError occurs.

Note: callers of these commands are ovs/nec/ryu-agent and ovs_cleanup.
- plugin agents: these commands are inside in try/except clause
  in daemon loop and there is no need to change.
- ovs_cleanup: there is no error catch logic in main() at now
  and it calls commands other than ovs_lib, so it can be cleanup
  later if required.

It also fixes the code to use excutils.save_and_reraise_exception
when reraising an exception.

Change-Id: I2aa3b51b8661c75846cb588c08c8f8ee00c37004
Closes-Bug: #1277029

diff --git a/neutron/agent/linux/ovs_lib.py b/neutron/agent/linux/ovs_lib.py
index 45e672096e0dcaddf1f9689e0c6951a31c4d2aba..3993cf824dad3714e0e11a910a9d5315d092e6f8 100644 (file)
--- a/neutron/agent/linux/ovs_lib.py
+++ b/neutron/agent/linux/ovs_lib.py
@@ -24,6 +24,7 @@ from oslo.config import cfg
 
 from neutron.agent.linux import ip_lib
 from neutron.agent.linux import utils
+from neutron.openstack.common import excutils
 from neutron.openstack.common import jsonutils
 from neutron.openstack.common import log as logging
 from neutron.plugins.common import constants as p_const
@@ -68,10 +69,12 @@ class BaseOVS(object):
         try:
             return utils.execute(full_args, root_helper=self.root_helper)
         except Exception as e:
-            LOG.error(_("Unable to execute %(cmd)s. Exception: %(exception)s"),
-                      {'cmd': full_args, 'exception': e})
-            if check_error:
-                raise
+            with excutils.save_and_reraise_exception() as ctxt:
+                LOG.error(_("Unable to execute %(cmd)s. "
+                            "Exception: %(exception)s"),
+                          {'cmd': full_args, 'exception': e})
+                if not check_error:
+                    ctxt.reraise = False
 
     def add_bridge(self, bridge_name):
         self.run_vsctl(["--", "--may-exist", "add-br", bridge_name])
@@ -83,17 +86,19 @@ class BaseOVS(object):
         try:
             self.run_vsctl(['br-exists', bridge_name], check_error=True)
         except RuntimeError as e:
-            if 'Exit code: 2\n' in str(e):
-                return False
-            raise
+            with excutils.save_and_reraise_exception() as ctxt:
+                if 'Exit code: 2\n' in str(e):
+                    ctxt.reraise = False
+                    return False
         return True
 
     def get_bridge_name_for_port_name(self, port_name):
         try:
             return self.run_vsctl(['port-to-br', port_name], check_error=True)
         except RuntimeError as e:
-            if 'Exit code: 1\n' not in str(e):
-                raise
+            with excutils.save_and_reraise_exception() as ctxt:
+                if 'Exit code: 1\n' in str(e):
+                    ctxt.reraise = False
 
     def port_exists(self, port_name):
         return bool(self.get_bridge_name_for_port_name(port_name))
@@ -281,15 +286,15 @@ class OVSBridge(BaseOVS):
                         "type=patch", "options:peer=%s" % remote_name])
         return self.get_port_ofport(local_name)
 
-    def db_get_map(self, table, record, column):
-        output = self.run_vsctl(["get", table, record, column])
+    def db_get_map(self, table, record, column, check_error=False):
+        output = self.run_vsctl(["get", table, record, column], check_error)
         if output:
             output_str = output.rstrip("\n\r")
             return self.db_str_to_map(output_str)
         return {}
 
-    def db_get_val(self, table, record, column):
-        output = self.run_vsctl(["get", table, record, column])
+    def db_get_val(self, table, record, column, check_error=False):
+        output = self.run_vsctl(["get", table, record, column], check_error)
         if output:
             return output.rstrip("\n\r")
 
@@ -304,7 +309,7 @@ class OVSBridge(BaseOVS):
         return ret
 
     def get_port_name_list(self):
-        res = self.run_vsctl(["list-ports", self.br_name])
+        res = self.run_vsctl(["list-ports", self.br_name], check_error=True)
         if res:
             return res.strip().split("\n")
         return []
@@ -318,16 +323,20 @@ class OVSBridge(BaseOVS):
         try:
             return utils.execute(args, root_helper=self.root_helper).strip()
         except Exception as e:
-            LOG.error(_("Unable to execute %(cmd)s. Exception: %(exception)s"),
-                      {'cmd': args, 'exception': e})
+            with excutils.save_and_reraise_exception():
+                LOG.error(_("Unable to execute %(cmd)s. "
+                            "Exception: %(exception)s"),
+                          {'cmd': args, 'exception': e})
 
     # returns a VIF object for each VIF port
     def get_vif_ports(self):
         edge_ports = []
         port_names = self.get_port_name_list()
         for name in port_names:
-            external_ids = self.db_get_map("Interface", name, "external_ids")
-            ofport = self.db_get_val("Interface", name, "ofport")
+            external_ids = self.db_get_map("Interface", name, "external_ids",
+                                           check_error=True)
+            ofport = self.db_get_val("Interface", name, "ofport",
+                                     check_error=True)
             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)
@@ -348,7 +357,7 @@ class OVSBridge(BaseOVS):
         edge_ports = set()
         args = ['--format=json', '--', '--columns=name,external_ids',
                 'list', 'Interface']
-        result = self.run_vsctl(args)
+        result = self.run_vsctl(args, check_error=True)
         if not result:
             return edge_ports
         for row in jsonutils.loads(result)['data']:
@@ -419,8 +428,8 @@ def get_bridges(root_helper):
     try:
         return utils.execute(args, root_helper=root_helper).strip().split("\n")
     except Exception as e:
-        LOG.exception(_("Unable to retrieve bridges. Exception: %s"), e)
-        return []
+        with excutils.save_and_reraise_exception():
+            LOG.exception(_("Unable to retrieve bridges. Exception: %s"), e)
 
 
 def get_installed_ovs_usr_version(root_helper):

diff --git a/neutron/tests/unit/openvswitch/test_ovs_lib.py b/neutron/tests/unit/openvswitch/test_ovs_lib.py
index 4950d366481dfffaf6a9270ed356127fc51345c4..7f0e9f2378782bb83c08ac583d999b2097ef0373 100644 (file)
--- a/neutron/tests/unit/openvswitch/test_ovs_lib.py
+++ b/neutron/tests/unit/openvswitch/test_ovs_lib.py
@@ -454,10 +454,10 @@ class OVS_Lib_Test(base.BaseTestCase):
             get_xapi_iface_id.assert_called_once_with('tap99id')
 
     def test_get_vif_ports_nonxen(self):
-        self._test_get_vif_ports(False)
+        self._test_get_vif_ports(is_xen=False)
 
     def test_get_vif_ports_xen(self):
-        self._test_get_vif_ports(True)
+        self._test_get_vif_ports(is_xen=True)
 
     def test_get_vif_port_set_nonxen(self):
         self._test_get_vif_port_set(False)
@@ -465,19 +465,24 @@ class OVS_Lib_Test(base.BaseTestCase):
     def test_get_vif_port_set_xen(self):
         self._test_get_vif_port_set(True)
 
-    def test_get_vif_port_set_list_ports_error(self):
+    def test_get_vif_ports_list_ports_error(self):
         expected_calls_and_values = [
             (mock.call(["ovs-vsctl", self.TO, "list-ports", self.BR_NAME],
                        root_helper=self.root_helper),
              RuntimeError()),
-            (mock.call(["ovs-vsctl", self.TO, "--format=json",
-                        "--", "--columns=name,external_ids",
-                        "list", "Interface"],
+        ]
+        tools.setup_mock_calls(self.execute, expected_calls_and_values)
+        self.assertRaises(RuntimeError, self.br.get_vif_ports)
+        tools.verify_mock_calls(self.execute, expected_calls_and_values)
+
+    def test_get_vif_port_set_list_ports_error(self):
+        expected_calls_and_values = [
+            (mock.call(["ovs-vsctl", self.TO, "list-ports", self.BR_NAME],
                        root_helper=self.root_helper),
-             self._encode_ovs_json(['name', 'external_ids'], []))
+             RuntimeError()),
         ]
         tools.setup_mock_calls(self.execute, expected_calls_and_values)
-        self.assertEqual(set(), self.br.get_vif_port_set())
+        self.assertRaises(RuntimeError, self.br.get_vif_port_set)
         tools.verify_mock_calls(self.execute, expected_calls_and_values)
 
     def test_get_vif_port_set_list_interface_error(self):
@@ -492,7 +497,7 @@ class OVS_Lib_Test(base.BaseTestCase):
              RuntimeError()),
         ]
         tools.setup_mock_calls(self.execute, expected_calls_and_values)
-        self.assertEqual(set(), self.br.get_vif_port_set())
+        self.assertRaises(RuntimeError, self.br.get_vif_port_set)
         tools.verify_mock_calls(self.execute, expected_calls_and_values)
 
     def test_clear_db_attribute(self):
@@ -569,6 +574,16 @@ class OVS_Lib_Test(base.BaseTestCase):
             mock.call('tap5678')
         ])
 
+    def test_delete_neutron_ports_list_error(self):
+        expected_calls_and_values = [
+            (mock.call(["ovs-vsctl", self.TO, "list-ports", self.BR_NAME],
+                       root_helper=self.root_helper),
+             RuntimeError()),
+        ]
+        tools.setup_mock_calls(self.execute, expected_calls_and_values)
+        self.assertRaises(RuntimeError, self.br.delete_ports, all_ports=False)
+        tools.verify_mock_calls(self.execute, expected_calls_and_values)
+
     def _test_get_bridges(self, exp_timeout=None):
         bridges = ['br-int', 'br-ex']
         root_helper = 'sudo'