From 5d07ec908cd7bee1c332ee3be6aee17e8386b9ea Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Mon, 13 Jul 2015 11:49:42 +0200 Subject: [PATCH] Tighten exception handler for import_object oslo_utils raise ImportError if import fails. We should propagate other failures to callers. Otherwise we may hide issues. Also report exact failure from import_object in case L3 agent fails to import interface_driver. As part of the job, consolidated code to load interface driver into common function. Also, stopped checking for specific log messages in dhcp and l3 agent unit tests: it's too fragile and actually not something we need a unit test for. Not to introduce more work for people who handle py3 porting effort, added the unit test into the list of those that are executed for py34 job until the whole suite is ready for python3. Change-Id: I10cdb8414c9fb4ad5cfd3f3b2630811f50ffb0c7 --- neutron/agent/common/utils.py | 23 ++++++++ neutron/agent/l3/agent.py | 12 +---- neutron/agent/linux/dhcp.py | 17 ++---- neutron/tests/unit/agent/common/test_utils.py | 53 +++++++++++++++++++ neutron/tests/unit/agent/dhcp/test_agent.py | 15 ++---- neutron/tests/unit/agent/l3/test_agent.py | 17 ++---- tox.ini | 1 + 7 files changed, 92 insertions(+), 46 deletions(-) create mode 100644 neutron/tests/unit/agent/common/test_utils.py diff --git a/neutron/agent/common/utils.py b/neutron/agent/common/utils.py index 2eddabd6db7..2b50da21704 100644 --- a/neutron/agent/common/utils.py +++ b/neutron/agent/common/utils.py @@ -15,10 +15,33 @@ import os +from oslo_log import log as logging +from oslo_utils import importutils + +from neutron.i18n import _LE + if os.name == 'nt': from neutron.agent.windows import utils else: from neutron.agent.linux import utils + +LOG = logging.getLogger(__name__) + + execute = utils.execute + + +def load_interface_driver(conf): + if not conf.interface_driver: + LOG.error(_LE('An interface driver must be specified')) + raise SystemExit(1) + try: + return importutils.import_object(conf.interface_driver, conf) + except ImportError as e: + LOG.error(_LE("Error importing interface driver " + "'%(driver)s': %(inner)s"), + {'driver': conf.interface_driver, + 'inner': e}) + raise SystemExit(1) diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py index e3379bfae65..23906bc3d73 100644 --- a/neutron/agent/l3/agent.py +++ b/neutron/agent/l3/agent.py @@ -21,9 +21,9 @@ import oslo_messaging from oslo_service import loopingcall from oslo_service import periodic_task from oslo_utils import excutils -from oslo_utils import importutils from oslo_utils import timeutils +from neutron.agent.common import utils as common_utils from neutron.agent.l3 import dvr from neutron.agent.l3 import dvr_edge_router as dvr_router from neutron.agent.l3 import dvr_local_router as dvr_local_router @@ -165,15 +165,7 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, config=self.conf, resource_type='router') - try: - self.driver = importutils.import_object( - self.conf.interface_driver, - self.conf - ) - except Exception: - LOG.error(_LE("Error importing interface driver " - "'%s'"), self.conf.interface_driver) - raise SystemExit(1) + self.driver = common_utils.load_interface_driver(self.conf) self.context = n_context.get_admin_context_without_session() self.plugin_rpc = L3PluginApi(topics.L3PLUGIN, host) diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py index d5d748dec4f..0a06259c1a0 100644 --- a/neutron/agent/linux/dhcp.py +++ b/neutron/agent/linux/dhcp.py @@ -23,10 +23,10 @@ import time import netaddr from oslo_config import cfg from oslo_log import log as logging -from oslo_utils import importutils from oslo_utils import uuidutils import six +from neutron.agent.common import utils as common_utils from neutron.agent.linux import external_process from neutron.agent.linux import ip_lib from neutron.agent.linux import iptables_manager @@ -36,7 +36,7 @@ from neutron.common import exceptions from neutron.common import ipv6_utils from neutron.common import utils as commonutils from neutron.extensions import extra_dhcp_opt as edo_ext -from neutron.i18n import _LE, _LI, _LW +from neutron.i18n import _LI, _LW LOG = logging.getLogger(__name__) @@ -919,18 +919,7 @@ class DeviceManager(object): def __init__(self, conf, plugin): self.conf = conf self.plugin = plugin - if not conf.interface_driver: - LOG.error(_LE('An interface driver must be specified')) - raise SystemExit(1) - try: - self.driver = importutils.import_object( - conf.interface_driver, conf) - except Exception as e: - LOG.error(_LE("Error importing interface driver '%(driver)s': " - "%(inner)s"), - {'driver': conf.interface_driver, - 'inner': e}) - raise SystemExit(1) + self.driver = common_utils.load_interface_driver(conf) def get_interface_name(self, network, port): """Return interface(device) name for use by the DHCP process.""" diff --git a/neutron/tests/unit/agent/common/test_utils.py b/neutron/tests/unit/agent/common/test_utils.py new file mode 100644 index 00000000000..7c89b1e2b5e --- /dev/null +++ b/neutron/tests/unit/agent/common/test_utils.py @@ -0,0 +1,53 @@ +# Copyright 2015 Red Hat, Inc. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import mock + +from neutron.agent.common import config +from neutron.agent.common import utils +from neutron.agent.linux import interface +from neutron.tests import base +from neutron.tests.unit import testlib_api + + +class TestLoadInterfaceDriver(base.BaseTestCase): + + def setUp(self): + super(TestLoadInterfaceDriver, self).setUp() + self.conf = config.setup_conf() + config.register_interface_driver_opts_helper(self.conf) + + def test_load_interface_driver_not_set(self): + with testlib_api.ExpectedException(SystemExit): + utils.load_interface_driver(self.conf) + + def test_load_interface_driver_wrong_driver(self): + self.conf.set_override('interface_driver', 'neutron.NonExistentDriver') + with testlib_api.ExpectedException(SystemExit): + utils.load_interface_driver(self.conf) + + def test_load_interface_driver_does_not_consume_irrelevant_errors(self): + self.conf.set_override('interface_driver', + 'neutron.agent.linux.interface.NullDriver') + with mock.patch('oslo_utils.importutils.import_object', + side_effect=RuntimeError()): + with testlib_api.ExpectedException(RuntimeError): + utils.load_interface_driver(self.conf) + + def test_load_interface_driver_success(self): + self.conf.set_override('interface_driver', + 'neutron.agent.linux.interface.NullDriver') + self.assertIsInstance(utils.load_interface_driver(self.conf), + interface.NullDriver) diff --git a/neutron/tests/unit/agent/dhcp/test_agent.py b/neutron/tests/unit/agent/dhcp/test_agent.py index 876bf8db424..96ddd988e1b 100644 --- a/neutron/tests/unit/agent/dhcp/test_agent.py +++ b/neutron/tests/unit/agent/dhcp/test_agent.py @@ -438,22 +438,17 @@ class TestDhcpAgent(base.BaseTestCase): def test_none_interface_driver(self): cfg.CONF.set_override('interface_driver', None) - with mock.patch.object(dhcp, 'LOG') as log: - self.assertRaises(SystemExit, dhcp.DeviceManager, - cfg.CONF, None) - msg = 'An interface driver must be specified' - log.error.assert_called_once_with(msg) + self.assertRaises(SystemExit, dhcp.DeviceManager, + cfg.CONF, None) def test_nonexistent_interface_driver(self): # Temporarily turn off mock, so could use the real import_class # to import interface_driver. self.driver_cls_p.stop() self.addCleanup(self.driver_cls_p.start) - cfg.CONF.set_override('interface_driver', 'foo') - with mock.patch.object(dhcp, 'LOG') as log: - self.assertRaises(SystemExit, dhcp.DeviceManager, - cfg.CONF, None) - self.assertEqual(log.error.call_count, 1) + cfg.CONF.set_override('interface_driver', 'foo.bar') + self.assertRaises(SystemExit, dhcp.DeviceManager, + cfg.CONF, None) class TestLogArgs(base.BaseTestCase): diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index b683727fdb5..6c3b7438074 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -44,7 +44,6 @@ from neutron.agent import rpc as agent_rpc from neutron.common import config as base_config from neutron.common import constants as l3_constants from neutron.common import exceptions as n_exc -from neutron.i18n import _LE from neutron.plugins.common import constants as p_const from neutron.tests import base from neutron.tests.common import l3_test_common @@ -1852,18 +1851,12 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): def test_nonexistent_interface_driver(self): self.conf.set_override('interface_driver', None) - with mock.patch.object(l3_agent, 'LOG') as log: - self.assertRaises(SystemExit, l3_agent.L3NATAgent, - HOSTNAME, self.conf) - msg = 'An interface driver must be specified' - log.error.assert_called_once_with(msg) + self.assertRaises(SystemExit, l3_agent.L3NATAgent, + HOSTNAME, self.conf) - self.conf.set_override('interface_driver', 'wrong_driver') - with mock.patch.object(l3_agent, 'LOG') as log: - self.assertRaises(SystemExit, l3_agent.L3NATAgent, - HOSTNAME, self.conf) - msg = _LE("Error importing interface driver '%s'") - log.error.assert_called_once_with(msg, 'wrong_driver') + self.conf.set_override('interface_driver', 'wrong.driver') + self.assertRaises(SystemExit, l3_agent.L3NATAgent, + HOSTNAME, self.conf) @mock.patch.object(namespaces.RouterNamespace, 'delete') @mock.patch.object(dvr_snat_ns.SnatNamespace, 'delete') diff --git a/tox.ini b/tox.ini index f473cd350a8..0f2431e30df 100644 --- a/tox.ini +++ b/tox.ini @@ -177,6 +177,7 @@ commands = python -m testtools.run \ neutron.tests.unit.agent.l3.test_dvr_fip_ns \ neutron.tests.unit.agent.common.test_config \ neutron.tests.unit.agent.common.test_polling \ + neutron.tests.unit.agent.common.test_utils \ neutron.tests.unit.agent.linux.test_ip_lib \ neutron.tests.unit.agent.linux.test_keepalived \ neutron.tests.unit.agent.linux.test_daemon \