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

@@ -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="<network>",
+                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)

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

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(self.r, ignore_missing=False)
-            obj = client.find_network(network)
+        client.delete_network(obj)
-            client.delete_network(obj)
 
     def take_action_compute(self, client, parsed_args):
-        for network in parsed_args.network:
+        network = utils.find_resource(client.networks, self.r)
-            network = utils.find_resource(
+        client.networks.delete(network.id)
-                client.networks,
-                network,
-            )
-            client.networks.delete(network.id)
 
 
 class ListNetwork(common.NetworkAndComputeLister):

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."""

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):
 

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 <https://bugs.launchpad.net/python-openstackclient/+bug/1556719>`_]
+    [Bug `1537856 <https://bugs.launchpad.net/python-openstackclient/+bug/1537856>`_]