From ecef65b9b51fc1bd684ca9eb72a024ee44ca9347 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Wed, 23 Oct 2019 08:31:59 +0000 Subject: [PATCH] Do not use exceptions in get_link_id() to control the code flow Instead of using exceptions as control flow, check the Pyroute2 command result and only raise the Neutron exception if needed. This will also reduce the traceback log in case of raising NetworkInterfaceNotFound. Although in Python the use of exception for this is common, this is usually considered an antipattern. Change-Id: I0e8bb3b0f6a46f2bac75e38c6ac6cdd094247f89 Closes-Bug: #1849449 --- neutron/privileged/agent/linux/ip_lib.py | 8 +++---- .../privileged/agent/linux/test_ip_lib.py | 23 +++++++++++++++---- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/neutron/privileged/agent/linux/ip_lib.py b/neutron/privileged/agent/linux/ip_lib.py index fcf48899337..800cf35bf40 100644 --- a/neutron/privileged/agent/linux/ip_lib.py +++ b/neutron/privileged/agent/linux/ip_lib.py @@ -241,11 +241,11 @@ def _translate_ip_device_exception(e, device=None, namespace=None): def get_link_id(device, namespace): - try: - with get_iproute(namespace) as ip: - return ip.link_lookup(ifname=device)[0] - except IndexError: + with get_iproute(namespace) as ip: + link_id = ip.link_lookup(ifname=device) + if not link_id or len(link_id) < 1: raise NetworkInterfaceNotFound(device=device, namespace=namespace) + return link_id[0] def _run_iproute_link(command, device, namespace=None, **kwargs): diff --git a/neutron/tests/unit/privileged/agent/linux/test_ip_lib.py b/neutron/tests/unit/privileged/agent/linux/test_ip_lib.py index 2efb168051c..0531a5d31b9 100644 --- a/neutron/tests/unit/privileged/agent/linux/test_ip_lib.py +++ b/neutron/tests/unit/privileged/agent/linux/test_ip_lib.py @@ -47,11 +47,24 @@ class IpLibTestCase(base.BaseTestCase): def test_run_iproute_link_interface_not_exists(self): with mock.patch.object(pyroute2, "IPRoute") as iproute_mock: ip_mock = iproute_mock() - ip_mock.__enter__().link_lookup.return_value = [] - self.assertRaises( - priv_lib.NetworkInterfaceNotFound, - priv_lib._run_iproute_link, - "test_cmd", "eth0", None, test_param="test_value") + ret_values = [ + [], # No interface found. + None, # Unexpected output but also handled. + ] + for ret_val in ret_values: + ip_mock.__enter__().link_lookup.return_value = ret_val + self.assertRaises( + priv_lib.NetworkInterfaceNotFound, + priv_lib._run_iproute_link, + "test_cmd", "eth0", None, test_param="test_value") + + @mock.patch.object(priv_lib, 'get_iproute') + def test_get_link_id(self, mock_iproute): + mock_ip = mock.Mock() + mock_ip.link_lookup.return_value = ['interface_id'] + mock_iproute.return_value.__enter__.return_value = mock_ip + self.assertEqual('interface_id', + priv_lib.get_link_id('device', 'namespace')) def test_run_iproute_link_interface_removed_during_call(self): with mock.patch.object(pyroute2, "IPRoute") as iproute_mock: