Add a waiter to share create/delete

This option enables "create" and "delete"
commands to wait until the action has completed.
There are numerous asynchronous operations
and this behavior is desirable for all
of them, however, the feature will be implemented
in parts and tracked as various "low-hanging-fruit"
bugs for new contributors to get a hang of
the contributor process.

This patch introduces a framework for future
implementations, including a sample set of
unit and functional tests for create/delete
operations on shares with the v2 shell client.

Partially-implements: bp add-wait-to-async-commands
Closes-Bug: #1898304
Change-Id: I099ccf4bce383905aeefc770905bd4d06470eaf2
Signed-off-by: Goutham Pacha Ravi <gouthampravi@gmail.com>

manilaclient/exceptions.py

@@ -17,9 +17,31 @@
 Exception definitions.
 """
 
+from manilaclient.common._i18n import _
 from manilaclient.common.apiclient.exceptions import *  # noqa
 
 
+class ManilaclientException(Exception):
+    """A generic client error."""
+    message = _("An unexpected error occured.")
+
+    def __init__(self, message):
+        self.message = message or self.message
+
+    def __str__(self):
+        return self.message
+
+
+class ResourceInErrorState(ManilaclientException):
+    """A resource is in an unexpected 'error' state."""
+    message = _("Resource is in error state")
+
+
+class TimeoutException(ManilaclientException):
+    """A request has timed out"""
+    message = _("Request has timed out")
+
+
 class NoTokenLookupException(ClientException):  # noqa: F405
     """No support for looking up endpoints.
 

manilaclient/tests/functional/base.py

@@ -309,8 +309,9 @@ class BaseTestCase(base.ClientTestBase):
     def create_share(cls, share_protocol=None, size=None, share_network=None,
                      share_type=None, name=None, description=None,
                      public=False, snapshot=None, metadata=None,
-                     client=None, cleanup_in_class=False,
+                     client=None, use_wait_option=False,
-                     wait_for_creation=True, microversion=None):
+                     cleanup_in_class=False, wait_for_creation=True,
+                     microversion=None):
         client = client or cls.get_admin_client()
         data = {
             'share_protocol': share_protocol or client.share_protocol,
@@ -321,6 +322,7 @@ class BaseTestCase(base.ClientTestBase):
             'snapshot': snapshot,
             'metadata': metadata,
             'microversion': microversion,
+            'wait': use_wait_option,
         }
 
         share_type = share_type or CONF.share_type
@@ -340,11 +342,18 @@ class BaseTestCase(base.ClientTestBase):
             cls.class_resources.insert(0, resource)
         else:
             cls.method_resources.insert(0, resource)
-        if wait_for_creation:
+        if wait_for_creation and not use_wait_option:
             client.wait_for_resource_status(share['id'],
                                             constants.STATUS_AVAILABLE)
         return share
 
+    @classmethod
+    def delete_share(cls, shares_to_delete, share_group_id=None,
+                     wait=False, client=None, microversion=None):
+        client = client or cls.get_admin_client()
+        client.delete_share(shares_to_delete, share_group_id=share_group_id,
+                            wait=wait, microversion=microversion)
+
     @classmethod
     def _determine_share_network_to_use(cls, client, share_type,
                                         microversion=None):

manilaclient/tests/functional/client.py

@@ -712,7 +712,7 @@ class ManilaCLIClient(base.CLIClient):
 
     def create_share(self, share_protocol, size, share_network=None,
                      share_type=None, name=None, description=None,
-                     public=False, snapshot=None, metadata=None,
+                     public=False, snapshot=None, metadata=None, wait=False,
                      microversion=None):
         """Creates a share.
 
@@ -726,6 +726,7 @@ class ManilaCLIClient(base.CLIClient):
             Default is False.
         :param snapshot: str -- Name or ID of a snapshot to use as source.
         :param metadata: dict -- key-value data to provide with share creation.
+        :param wait: bool - the client must wait for "available" state
         :param microversion: str -- API microversion that should be used.
         """
         cmd = 'create %(share_protocol)s %(size)s ' % {
@@ -741,7 +742,7 @@ class ManilaCLIClient(base.CLIClient):
             description = data_utils.rand_name('autotest_share_description')
         cmd += '--description %s ' % description
         if public:
-            cmd += '--public'
+            cmd += '--public '
         if snapshot is not None:
             cmd += '--snapshot %s ' % snapshot
         if metadata:
@@ -750,6 +751,8 @@ class ManilaCLIClient(base.CLIClient):
                 metadata_cli += '%(k)s=%(v)s ' % {'k': k, 'v': v}
             if metadata_cli:
                 cmd += '--metadata %s ' % metadata_cli
+        if wait:
+            cmd += '--wait '
         share_raw = self.manila(cmd, microversion=microversion)
         share = output_parser.details(share_raw)
         return share
@@ -784,17 +787,25 @@ class ManilaCLIClient(base.CLIClient):
 
     @not_found_wrapper
     @forbidden_wrapper
-    def delete_share(self, shares, microversion=None):
+    def delete_share(self, shares, share_group_id=None, wait=False,
+                     microversion=None):
         """Deletes share[s] by Names or IDs.
 
         :param shares: either str or list of str that can be either Name
             or ID of a share(s) that should be deleted.
+        :param share_group_id: a common share group ID for the shares being
+          deleted
+        :param wait: bool -- whether to wait for the shares to be deleted
         """
         if not isinstance(shares, list):
             shares = [shares]
         cmd = 'delete '
         for share in shares:
             cmd += '%s ' % share
+        if share_group_id:
+            cmd += '--share-group-id %s ' % share_group_id
+        if wait:
+            cmd += '--wait '
         return self.manila(cmd, microversion=microversion)
 
     def list_shares(self, all_tenants=False, filters=None, columns=None,

manilaclient/tests/functional/test_shares.py

@@ -17,6 +17,7 @@ import ddt
 import testtools
 
 from tempest.lib.common.utils import data_utils
+from tempest.lib import exceptions
 
 from manilaclient.common import constants
 from manilaclient import config
@@ -26,6 +27,7 @@ from manilaclient.tests.functional import utils
 CONF = config.CONF
 
 
+@ddt.ddt
 class SharesReadWriteBase(base.BaseTestCase):
     protocol = None
 
@@ -90,6 +92,33 @@ class SharesReadWriteBase(base.BaseTestCase):
         self.assertEqual('1', get['size'])
         self.assertEqual(self.protocol.upper(), get['share_proto'])
 
+    @ddt.data(True, False)
+    def test_create_delete_with_wait(self, use_wait_option):
+        name = data_utils.rand_name('share-with-wait-%s')
+        description = data_utils.rand_name('we-wait-until-share-is-ready')
+
+        share_1, share_2 = (
+            self.create_share(self.protocol, name=(name % num),
+                              description=description,
+                              use_wait_option=use_wait_option,
+                              client=self.user_client)
+            for num in range(0, 2)
+        )
+
+        expected_status = "available" if use_wait_option else "creating"
+        self.assertEqual(expected_status, share_1['status'])
+        self.assertEqual(expected_status, share_2['status'])
+
+        if use_wait_option:
+            self.delete_share([share_1['id'], share_2['id']],
+                              wait=use_wait_option,
+                              client=self.user_client)
+
+            for share in (share_1, share_2):
+                self.assertRaises(
+                    exceptions.NotFound,
+                    self.user_client.get_share, share['id'])
+
 
 @ddt.ddt
 class SharesTestMigration(base.BaseTestCase):

manilaclient/tests/unit/v2/fakes.py

@@ -173,7 +173,13 @@ class FakeHTTPClient(fakes.FakeHTTPClient):
         return (200, {}, body)
 
     def get_shares_1234(self, **kw):
-        share = {'share': {'id': 1234, 'name': 'sharename'}}
+        share = {
+            'share': {
+                'id': 1234,
+                'name': 'sharename',
+                'status': 'available',
+            },
+        }
         return (200, {}, share)
 
     def get_share_servers_1234(self, **kw):
@@ -609,7 +615,7 @@ class FakeHTTPClient(fakes.FakeHTTPClient):
         return (202, {}, {'share_network_subnet': {}})
 
     def post_shares(self, **kwargs):
-        return (202, {}, {'share': {}})
+        return (202, {}, {'share': {'id': '1234', 'status': 'creating'}})
 
     def post_snapshots(self, **kwargs):
         return (202, {}, {'snapshot': {}})
@@ -617,6 +623,12 @@ class FakeHTTPClient(fakes.FakeHTTPClient):
     def delete_shares_1234(self, **kw):
         return (202, {}, None)
 
+    def delete_shares_share_abc(self, **kw):
+        return (202, {}, None)
+
+    def delete_shares_share_xyz(self, **kw):
+        return (202, {}, None)
+
     def delete_snapshots_1234(self, **kwargs):
         return (202, {}, None)
 

manilaclient/tests/unit/v2/test_shell.py

@@ -41,6 +41,7 @@ from manilaclient.v2 import share_networks
 from manilaclient.v2 import share_servers
 from manilaclient.v2 import share_snapshots
 from manilaclient.v2 import share_types
+from manilaclient.v2 import shares
 from manilaclient.v2 import shell as shell_v2
 
 
@@ -930,6 +931,31 @@ class ShellTest(test_utils.TestCase):
             'delete fake-not-found'
         )
 
+    @ddt.data(('share_xyz', ), ('share_abc', 'share_xyz'))
+    def test_delete_wait(self, shares_to_delete):
+        fake_shares = [
+            shares.Share('fake', {'id': share})
+            for share in shares_to_delete
+        ]
+        share_not_found_error = ("Delete for share %s failed: No share with "
+                                 "a name or ID of '%s' exists.")
+        shares_are_not_found_errors = [
+            exceptions.CommandError(share_not_found_error % (share, share))
+            for share in shares_to_delete
+        ]
+        self.mock_object(
+            shell_v2, '_find_share',
+            mock.Mock(side_effect=(fake_shares + shares_are_not_found_errors)))
+
+        self.run_command('delete %s --wait' % ' '.join(shares_to_delete))
+
+        shell_v2._find_share.assert_has_calls([
+            mock.call(self.shell.cs, share) for share in shares_to_delete
+        ])
+        for share in fake_shares:
+            uri = '/shares/%s' % share.id
+            self.assert_called_anytime('DELETE', uri, clear_callstack=False)
+
     def test_list_snapshots(self):
         self.run_command('snapshot-list')
         self.assert_called('GET', '/snapshots/detail')
@@ -1975,6 +2001,13 @@ class ShellTest(test_utils.TestCase):
         expected['share']['metadata'] = {"key1": "value1", "key2": "value2"}
         self.assert_called("POST", "/shares", body=expected)
 
+    def test_create_with_wait(self):
+        self.run_command("create nfs 1 --wait")
+        expected = self.create_share_body.copy()
+        self.assert_called_anytime(
+            "POST", "/shares", body=expected, clear_callstack=False)
+        self.assert_called("GET", "/shares/1234")
+
     def test_allow_access_cert(self):
         self.run_command("access-allow 1234 cert client.example.com")
 

manilaclient/v2/shell.py

@@ -16,6 +16,7 @@
 
 from operator import xor
 import os
+import re
 import sys
 import time
 
@@ -29,34 +30,84 @@ from manilaclient.common import constants
 from manilaclient import exceptions
 
 
-def _poll_for_status(poll_fn, obj_id, action, final_ok_states,
+def _wait_for_resource_status(cs,
-                     poll_period=5, show_progress=True):
+                              resource,
-    """Block while action is performed, periodically printing progress."""
+                              expected_status,
-    def print_progress(progress):
+                              resource_type='share',
-        if show_progress:
+                              status_attr='status',
-            msg = ('\rInstance %(action)s... %(progress)s%% complete'
+                              poll_timeout=900,
-                   % dict(action=action, progress=progress))
+                              poll_interval=2):
-        else:
+    """Waiter for resource status changes
-            msg = '\rInstance %(action)s...' % dict(action=action)
 
-        sys.stdout.write(msg)
+    :param cs: command shell control
-        sys.stdout.flush()
+    :param expected_status: a string or a list of strings containing expected
+       states to wait for
+    :param resource_type: 'share', 'snapshot', 'share_replica', 'share_group',
+       or 'share_group_snapshot'
+    :param status_attr: 'status', 'task_state', 'access_rules_status' or any
+       other status field that is expected to have the "expected_status"
+    :param poll_timeout: how long to wait for in seconds
+    :param poll_interval: how often to try in seconds
+    """
+    find_resource = {
+        'share': _find_share,
+        'snapshot': _find_share_snapshot,
+        'share_replica': _find_share_replica,
+        'share_group': _find_share_group,
+        'share_group_snapshot': _find_share_group_snapshot,
+    }
 
-    print()
+    print_resource = {
+        'share': _print_share,
+        'snapshot': _print_share_snapshot,
+        'share_replica': _print_share_replica,
+        'share_group': _print_share_group,
+        'share_group_snapshot': _print_share_group_snapshot,
+    }
+
+    expected_status = expected_status or ('available', )
+    if not isinstance(expected_status, (list, tuple, set)):
+        expected_status = (expected_status, )
+
+    time_elapsed = 0
+    timeout_message = ("%(resource_type)s %(resource)s did not reach "
+                       "%(expected_states)s within %(seconds)d seconds.")
+    error_message = ("%(resource_type)s %(resource)s has reached a failed "
+                     "state.")
+    deleted_message = ("%(resource_type)s %(resource)s has been successfully "
+                       "deleted.")
+    message_payload = {
+        'resource_type': resource_type.capitalize(),
+        'resource': resource.id,
+    }
+    not_found_regex = "no %s .* exists" % resource_type
     while True:
-        obj = poll_fn(obj_id)
+        if time_elapsed > poll_timeout:
-        status = obj.status.lower()
+            print_resource[resource_type](cs, resource)
-        progress = getattr(obj, 'progress', None) or 0
+            message_payload.update({'expected_states': expected_status,
-        if status in final_ok_states:
+                                    'seconds': poll_timeout})
-            print_progress(100)
+            raise exceptions.TimeoutException(
-            print("\nFinished")
+                message=timeout_message % message_payload)
+        try:
+            resource = find_resource[resource_type](cs, resource.id)
+        except exceptions.CommandError as e:
+            if (re.search(not_found_regex, str(e), flags=re.IGNORECASE)
+                    and 'deleted' in expected_status):
+                print(deleted_message % message_payload)
+                break
+            else:
+                raise e
+
+        if getattr(resource, status_attr) in expected_status:
             break
-        elif status == "error":
+        elif 'error' in getattr(resource, status_attr):
-            print("\nError %(action)s instance" % {'action': action})
+            print_resource[resource_type](cs, resource)
-            break
+            raise exceptions.ResourceInErrorState(
-        else:
+                message=error_message % message_payload)
-            print_progress(progress)
+        time.sleep(poll_interval)
-            time.sleep(poll_period)
+        time_elapsed += poll_interval
+
+    return resource
 
 
 def _find_share(cs, share):
@@ -131,6 +182,11 @@ def _print_share(cs, share):  # noqa
     cliutils.print_dict(info)
 
 
+def _wait_for_share_status(cs, share, expected_status='available'):
+    return _wait_for_resource_status(
+        cs, share, expected_status, resource_type='share')
+
+
 def _find_share_instance(cs, instance):
     """Get a share instance by ID."""
     return apiclient_utils.find_resource(cs.share_instances, instance)
@@ -809,6 +865,10 @@ def do_rate_limits(cs, args):
     help='Optional share group name or ID in which to create the share '
          '(Default=None).',
     default=None)
+@cliutils.arg(
+    '--wait',
+    action='store_true',
+    help='Wait for share creation')
 @cliutils.service_type('sharev2')
 def do_create(cs, args):
     """Creates a new share (NFS, CIFS, CephFS, GlusterFS, HDFS or MAPRFS)."""
@@ -832,6 +892,10 @@ def do_create(cs, args):
                              is_public=args.public,
                              availability_zone=args.availability_zone,
                              share_group_id=share_group)
+
+    if args.wait:
+        share = _wait_for_share_status(cs, share)
+
     _print_share(cs, share)
 
 
@@ -1581,19 +1645,25 @@ def do_revert_to_snapshot(cs, args):
     help='Optional share group name or ID which contains the share '
          '(Default=None).',
     default=None)
+@cliutils.arg(
+    '--wait',
+    action='store_true',
+    help='Wait for share deletion')
 @cliutils.service_type('sharev2')
 def do_delete(cs, args):
     """Remove one or more shares."""
     failure_count = 0
+    shares_to_delete = []
 
     for share in args.share:
         try:
             share_ref = _find_share(cs, share)
+            shares_to_delete.append(share_ref)
             if args.share_group:
                 share_group_id = _find_share_group(cs, args.share_group).id
-                share_ref.delete(share_group_id=share_group_id)
+                cs.shares.delete(share_ref, share_group_id=share_group_id)
             else:
-                share_ref.delete()
+                cs.shares.delete(share_ref)
         except Exception as e:
             failure_count += 1
             print("Delete for share %s failed: %s" % (share, e),
@@ -1603,6 +1673,13 @@ def do_delete(cs, args):
         raise exceptions.CommandError("Unable to delete any of the specified "
                                       "shares.")
 
+    if args.wait:
+        for share in shares_to_delete:
+            try:
+                _wait_for_share_status(cs, share, expected_status='deleted')
+            except exceptions.CommandError as e:
+                print(e, file=sys.stderr)
+
 
 @cliutils.arg(
     'share',

releasenotes/notes/bug-1898304-add-wait-to-share-create-delete-f121073f2f4402ff.yaml

@@ -0,0 +1,7 @@
+---
+features:
+  - |
+    The commands "manila create" and "manila delete" now
+    accept an optional "--wait" option that allows users
+    to let the client poll for the completion of the
+    operation.