From 56f9227063cb86594600ccc80c661101f0f0c2c8 Mon Sep 17 00:00:00 2001 From: Tang Chen Date: Mon, 14 Mar 2016 14:03:04 +0800 Subject: [PATCH] Enhance exception handling for "network delete" command This patch rework "network delete" command following the rules in doc/source/command-errors.rst. In "network delete" command, there are multiple REST API calls, and we should make as many of them as possible. And log error for each one, give a better error message. Also return a non-zero exit code. Change-Id: I39ae087dd7bd08d049d513abfa6c5cab2bd13b2b Partial-Bug: #1556719 --- doc/source/command-errors.rst | 42 ++++- openstackclient/network/common.py | 37 +++++ openstackclient/network/v2/network.py | 20 +-- openstackclient/tests/compute/v2/fakes.py | 19 +++ .../tests/network/v2/test_network.py | 153 +++++++++++++++--- .../notes/bug-1556719-d2dcf61acf87e856.yaml | 6 + 6 files changed, 247 insertions(+), 30 deletions(-) create mode 100644 releasenotes/notes/bug-1556719-d2dcf61acf87e856.yaml diff --git a/doc/source/command-errors.rst b/doc/source/command-errors.rst index 35d7f9d07..a24dc317b 100644 --- a/doc/source/command-errors.rst +++ b/doc/source/command-errors.rst @@ -99,8 +99,8 @@ to be handled by having the proper options in a set command available to allow recovery in the case where the primary resource has been created but the subsequent calls did not complete. -Example -~~~~~~~ +Example 1 +~~~~~~~~~ This example is taken from the ``volume snapshot set`` command where ``--property`` arguments are set using the volume manager's ``set_metadata()`` method, @@ -161,3 +161,41 @@ remaining arguments are set using the ``update()`` method. # without aborting prematurely if result > 0: raise SomeNonFatalException + +Example 2 +~~~~~~~~~ + +This example is taken from the ``network delete`` command which takes multiple +networks to delete. All networks will be delete in a loop, which makes multiple +``delete_network()`` calls. + +.. code-block:: python + + class DeleteNetwork(common.NetworkAndComputeCommand): + """Delete network(s)""" + + def update_parser_common(self, parser): + parser.add_argument( + 'network', + metavar="", + nargs="+", + help=("Network(s) to delete (name or ID)") + ) + return parser + + def take_action(self, client, parsed_args): + ret = 0 + + for network in parsed_args.network: + try: + obj = client.find_network(network, ignore_missing=False) + client.delete_network(obj) + except Exception: + self.app.log.error("Failed to delete network with name " + "or ID %s." % network) + ret += 1 + + if ret > 0: + total = len(parsed_args.network) + msg = "Failed to delete %s of %s networks." % (ret, total) + raise exceptions.CommandError(msg) diff --git a/openstackclient/network/common.py b/openstackclient/network/common.py index 1e2c4cce9..a3047d84b 100644 --- a/openstackclient/network/common.py +++ b/openstackclient/network/common.py @@ -15,6 +15,7 @@ import abc import six from openstackclient.common import command +from openstackclient.common import exceptions @six.add_metaclass(abc.ABCMeta) @@ -68,6 +69,42 @@ class NetworkAndComputeCommand(command.Command): pass +@six.add_metaclass(abc.ABCMeta) +class NetworkAndComputeDelete(NetworkAndComputeCommand): + """Network and Compute Delete + + Delete class for commands that support implementation via + the network or compute endpoint. Such commands have different + implementations for take_action() and may even have different + arguments. This class supports bulk deletion, and error handling + following the rules in doc/source/command-errors.rst. + """ + + def take_action(self, parsed_args): + ret = 0 + resources = getattr(parsed_args, self.resource, []) + + for r in resources: + self.r = r + try: + if self.app.client_manager.is_network_endpoint_enabled(): + self.take_action_network(self.app.client_manager.network, + parsed_args) + else: + self.take_action_compute(self.app.client_manager.compute, + parsed_args) + except Exception as e: + self.app.log.error("Failed to delete %s with name or ID " + "'%s': %s" % (self.resource, r, e)) + ret += 1 + + if ret: + total = len(resources) + msg = "%s of %s %ss failed to delete." % (ret, total, + self.resource) + raise exceptions.CommandError(msg) + + @six.add_metaclass(abc.ABCMeta) class NetworkAndComputeLister(command.Lister): """Network and Compute Lister diff --git a/openstackclient/network/v2/network.py b/openstackclient/network/v2/network.py index 20d943ed6..fcbe77c7b 100644 --- a/openstackclient/network/v2/network.py +++ b/openstackclient/network/v2/network.py @@ -224,9 +224,13 @@ class CreateNetwork(common.NetworkAndComputeShowOne): return (columns, data) -class DeleteNetwork(common.NetworkAndComputeCommand): +class DeleteNetwork(common.NetworkAndComputeDelete): """Delete network(s)""" + # Used by base class to find resources in parsed_args. + resource = 'network' + r = None + def update_parser_common(self, parser): parser.add_argument( 'network', @@ -234,20 +238,16 @@ class DeleteNetwork(common.NetworkAndComputeCommand): nargs="+", help=("Network(s) to delete (name or ID)") ) + return parser def take_action_network(self, client, parsed_args): - for network in parsed_args.network: - obj = client.find_network(network) - client.delete_network(obj) + obj = client.find_network(self.r, ignore_missing=False) + client.delete_network(obj) def take_action_compute(self, client, parsed_args): - for network in parsed_args.network: - network = utils.find_resource( - client.networks, - network, - ) - client.networks.delete(network.id) + network = utils.find_resource(client.networks, self.r) + client.networks.delete(network.id) class ListNetwork(common.NetworkAndComputeLister): diff --git a/openstackclient/tests/compute/v2/fakes.py b/openstackclient/tests/compute/v2/fakes.py index 6c67c470b..c88ef85b8 100644 --- a/openstackclient/tests/compute/v2/fakes.py +++ b/openstackclient/tests/compute/v2/fakes.py @@ -910,6 +910,25 @@ class FakeNetwork(object): return networks + @staticmethod + def get_networks(networks=None, count=2): + """Get an iterable MagicMock object with a list of faked networks. + + If networks list is provided, then initialize the Mock object with the + list. Otherwise create one. + + :param List networks: + A list of FakeResource objects faking networks + :param int count: + The number of networks to fake + :return: + An iterable Mock object with side_effect set to a list of faked + networks + """ + if networks is None: + networks = FakeNetwork.create_networks(count=count) + return mock.Mock(side_effect=networks) + class FakeHost(object): """Fake one host.""" diff --git a/openstackclient/tests/network/v2/test_network.py b/openstackclient/tests/network/v2/test_network.py index 8a75101b5..c8a6d0820 100644 --- a/openstackclient/tests/network/v2/test_network.py +++ b/openstackclient/tests/network/v2/test_network.py @@ -14,6 +14,7 @@ import copy import mock +from mock import call from openstackclient.common import exceptions from openstackclient.common import utils from openstackclient.network.v2 import network @@ -321,33 +322,88 @@ class TestCreateNetworkIdentityV2(TestNetwork): class TestDeleteNetwork(TestNetwork): - # The network to delete. - _network = network_fakes.FakeNetwork.create_one_network() - def setUp(self): super(TestDeleteNetwork, self).setUp() + # The networks to delete + self._networks = network_fakes.FakeNetwork.create_networks(count=3) + self.network.delete_network = mock.Mock(return_value=None) - self.network.find_network = mock.Mock(return_value=self._network) + self.network.find_network = network_fakes.FakeNetwork.get_networks( + networks=self._networks) # Get the command object to test self.cmd = network.DeleteNetwork(self.app, self.namespace) - def test_delete(self): + def test_delete_one_network(self): arglist = [ - self._network.name, + self._networks[0].name, ] verifylist = [ - ('network', [self._network.name]), + ('network', [self._networks[0].name]), ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) + result = self.cmd.take_action(parsed_args) - self.network.delete_network.assert_called_once_with(self._network) + self.network.delete_network.assert_called_once_with(self._networks[0]) self.assertIsNone(result) + def test_delete_multiple_networks(self): + arglist = [] + for n in self._networks: + arglist.append(n.id) + verifylist = [ + ('network', arglist), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + result = self.cmd.take_action(parsed_args) + + calls = [] + for n in self._networks: + calls.append(call(n)) + self.network.delete_network.assert_has_calls(calls) + self.assertIsNone(result) + + def test_delete_multiple_networks_exception(self): + arglist = [ + self._networks[0].id, + 'xxxx-yyyy-zzzz', + self._networks[1].id, + ] + verifylist = [ + ('network', arglist), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # Fake exception in find_network() + ret_find = [ + self._networks[0], + exceptions.NotFound('404'), + self._networks[1], + ] + self.network.find_network = mock.Mock(side_effect=ret_find) + + # Fake exception in delete_network() + ret_delete = [ + None, + exceptions.NotFound('404'), + ] + self.network.delete_network = mock.Mock(side_effect=ret_delete) + + self.assertRaises(exceptions.CommandError, self.cmd.take_action, + parsed_args) + + # The second call of find_network() should fail. So delete_network() + # was only called twice. + calls = [ + call(self._networks[0]), + call(self._networks[1]), + ] + self.network.delete_network.assert_has_calls(calls) + class TestListNetwork(TestNetwork): @@ -730,36 +786,97 @@ class TestCreateNetworkCompute(TestNetworkCompute): class TestDeleteNetworkCompute(TestNetworkCompute): - # The network to delete. - _network = compute_fakes.FakeNetwork.create_one_network() - def setUp(self): super(TestDeleteNetworkCompute, self).setUp() self.app.client_manager.network_endpoint_enabled = False + # The networks to delete + self._networks = compute_fakes.FakeNetwork.create_networks(count=3) + self.compute.networks.delete.return_value = None # Return value of utils.find_resource() - self.compute.networks.get.return_value = self._network + self.compute.networks.get = \ + compute_fakes.FakeNetwork.get_networks(networks=self._networks) # Get the command object to test self.cmd = network.DeleteNetwork(self.app, None) - def test_network_delete(self): + def test_delete_one_network(self): arglist = [ - self._network.label, + self._networks[0].label, ] verifylist = [ - ('network', [self._network.label]), + ('network', [self._networks[0].label]), ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) + result = self.cmd.take_action(parsed_args) - self.compute.networks.delete.assert_called_once_with(self._network.id) + self.compute.networks.delete.assert_called_once_with( + self._networks[0].id) self.assertIsNone(result) + def test_delete_multiple_networks(self): + arglist = [] + for n in self._networks: + arglist.append(n.label) + verifylist = [ + ('network', arglist), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + result = self.cmd.take_action(parsed_args) + + calls = [] + for n in self._networks: + calls.append(call(n.id)) + self.compute.networks.delete.assert_has_calls(calls) + self.assertIsNone(result) + + def test_delete_multiple_networks_exception(self): + arglist = [ + self._networks[0].id, + 'xxxx-yyyy-zzzz', + self._networks[1].id, + ] + verifylist = [ + ('network', arglist), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # Fake exception in utils.find_resource() + # In compute v2, we use utils.find_resource() to find a network. + # It calls get() several times, but find() only one time. So we + # choose to fake get() always raise exception, then pass through. + # And fake find() to find the real network or not. + self.compute.networks.get.side_effect = Exception() + ret_find = [ + self._networks[0], + Exception(), + self._networks[1], + ] + self.compute.networks.find.side_effect = ret_find + + # Fake exception in delete() + ret_delete = [ + None, + Exception(), + ] + self.compute.networks.delete = mock.Mock(side_effect=ret_delete) + + self.assertRaises(exceptions.CommandError, self.cmd.take_action, + parsed_args) + + # The second call of utils.find_resource() should fail. So delete() + # was only called twice. + calls = [ + call(self._networks[0].id), + call(self._networks[1].id), + ] + self.compute.networks.delete.assert_has_calls(calls) + class TestListNetworkCompute(TestNetworkCompute): diff --git a/releasenotes/notes/bug-1556719-d2dcf61acf87e856.yaml b/releasenotes/notes/bug-1556719-d2dcf61acf87e856.yaml new file mode 100644 index 000000000..7c8e5c062 --- /dev/null +++ b/releasenotes/notes/bug-1556719-d2dcf61acf87e856.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - Command ``network delete`` will delete as many networks as possible, log + and report failures in the end. + [Bug `1556719 `_] + [Bug `1537856 `_]