Repository: ansible/ansible
Pull request: add wait_for_running option in azure_rm_rediscache module
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
add wait_for_running option to poll wait redis provisioning_state in azure_rm_rediscache module
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
Fixes https://github.com/ansible/ansible/issues/54932

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request


##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->

##### ADDITIONAL INFORMATION
<!--- Include additional information to help people understand the change here -->
<!--- A step-by-step reproduction of the problem is helpful if there is no related issue -->

<!--- Paste verbatim command output below, e.g. before and after your change -->
```paste below

```


Problem statement:
azure_rm_rediscache needs to block by default on provisioning ops
##### SUMMARY
Currently, the `azure_rm_rediscache` module returns when the Create or Update operation completes, however this means that in general, the cache is not in a usable state, so further attempts to modify or use it will fail. This is even evident in the tests, most of which require polling the facts module to wait for the ProvisioningState to be Succeeded.

Should probably have a new `wait_for_provisioning` arg that defaults to `yes`, so that anyone using the module will not need to manually poll for status before further interacting with the cache. The tests can conditionally set the new arg to false where necessary to keep a "fast path" and "slow path".

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
azure_rm_rediscache

##### ANSIBLE VERSION
2.8.0 a1

##### CONFIGURATION
NA

##### OS / ENVIRONMENT
NA

##### STEPS TO REPRODUCE
Create new Redis Cache via azure_rm_rediscache, then try any update method or to actually use the cache. Also applicable to scale, update, delete operations.


##### EXPECTED RESULTS
A usable cache when the module returns (unless opted out).


##### ACTUAL RESULTS
A cache that is still provisioning

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/cloud/azure/azure_rm_rediscache.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/azure/azure_rm_rediscache.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @haroldwongms @trstringer @xscript @yungezz @yuwzho @zikalino
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
wait_for_provisioning makes sense from user perspective. will add it.

Commit message: add wait_for_running

Patch to review:
diff --git a/lib/ansible/modules/cloud/azure/azure_rm_rediscache.py b/lib/ansible/modules/cloud/azure/azure_rm_rediscache.py
index 308189a3e86eb4..f46146f760173e 100644
--- a/lib/ansible/modules/cloud/azure/azure_rm_rediscache.py
+++ b/lib/ansible/modules/cloud/azure/azure_rm_rediscache.py
@@ -131,6 +131,13 @@
                 choices:
                     - primary
                     - secondary
+    wait_for_running:
+        description:
+            - Wait till the Azure Cache for Redis instance provisioning_state is Succeeded.
+            - It takes several minutes for Azure Cache for Redis to be ready for use after creating/updating/rebooting.
+            - Set this option to true to wait for provisioning_state. Set to false if you don't care about provisioning_state.
+            - Poll wait timeout is 60 minutes.
+        type: bool
     state:
       description:
         - Assert the state of the Azure Cache for Redis.
@@ -209,8 +216,8 @@
 
 try:
     from msrestazure.azure_exceptions import CloudError
-    from msrest.polling import LROPoller
     from msrestazure.azure_operation import AzureOperationPoller
+    from msrest.polling import LROPoller
     from msrest.serialization import Model
     from azure.mgmt.redis import RedisManagementClient
     from azure.mgmt.redis.models import (RedisCreateParameters, RedisUpdateParameters, Sku)
@@ -366,6 +373,9 @@ def __init__(self):
             regenerate_key=dict(
                 type='dict',
                 options=regenerate_key_spec
+            ),
+            wait_for_running=dict(
+                type='bool'
             )
         )
 
@@ -386,6 +396,10 @@ def __init__(self):
         self.reboot = None
         self.regenerate_key = None
 
+        self.wait_for_running = None
+        self.wait_for_running_polling_interval_in_seconds = 30
+        self.wait_for_running_polling_times = 120
+
         self.tags = None
 
         self.results = dict(
@@ -562,8 +576,11 @@ def create_rediscache(self):
             response = self._client.redis.create(resource_group_name=self.resource_group,
                                                  name=self.name,
                                                  parameters=params)
-            if isinstance(response, LROPoller) or isinstance(response, AzureOperationPoller):
+            if isinstance(response, AzureOperationPoller) or isinstance(response, LROPoller):
                 response = self.get_poller_result(response)
+            
+            if self.wait_for_running:
+                self.wait_for_redis_running()
 
         except CloudError as exc:
             self.log('Error attempting to create the Azure Cache for Redis instance.')
@@ -598,9 +615,12 @@ def update_rediscache(self):
             response = self._client.redis.update(resource_group_name=self.resource_group,
                                                  name=self.name,
                                                  parameters=params)
-            if isinstance(response, LROPoller) or isinstance(response, AzureOperationPoller):
+            if isinstance(response, AzureOperationPoller) or isinstance(response, LROPoller):
                 response = self.get_poller_result(response)
 
+            if self.wait_for_running:
+                self.wait_for_redis_running()
+
         except CloudError as exc:
             self.log('Error attempting to update the Azure Cache for Redis instance.')
             self.fail(
@@ -659,6 +679,11 @@ def force_reboot_rediscache(self):
                                                        name=self.name,
                                                        reboot_type=self.reboot['reboot_type'],
                                                        shard_id=self.reboot.get('shard_id'))
+            if isinstance(response, AzureOperationPoller) or isinstance(response, LROPoller):
+                response = self.get_poller_result(response)
+
+            if self.wait_for_running:
+                self.wait_for_redis_running()
         except CloudError as e:
             self.log('Error attempting to force reboot the redis cache instance.')
             self.fail(
@@ -718,6 +743,23 @@ def parse_subnet(self):
             subnet_id = self.subnet
         return subnet_id
 
+    def wait_for_redis_running(self):
+        try:
+            response = self._client.redis.get(resource_group_name=self.resource_group, name=self.name)
+            status = response.provisioning_state
+            polling_times = 0
+
+            while polling_times < self.wait_for_running_polling_times:
+                if status.lower() != "succeeded":
+                    polling_times += 1
+                    time.sleep(self.wait_for_running_polling_interval_in_seconds)
+                    response = self._client.redis.get(resource_group_name=self.resource_group, name=self.name)
+                    status = response.provisioning_state
+                else:
+                    return True
+            self.fail("Azure Cache for Redis is not running after 60 mins.")
+        except CloudError as e:
+            self.fail("Failed to get Azure Cache for Redis: {0}".format(str(e)))
 
 def main():
     """Main execution"""
diff --git a/test/integration/targets/azure_rm_rediscache/tasks/main.yml b/test/integration/targets/azure_rm_rediscache/tasks/main.yml
index 72d46f7ad0d033..c35ab6ef34b6de 100644
--- a/test/integration/targets/azure_rm_rediscache/tasks/main.yml
+++ b/test/integration/targets/azure_rm_rediscache/tasks/main.yml
@@ -92,6 +92,7 @@
       enable_non_ssl_port: true
       tags:
         testing: foo
+      wait_for_running: True
     register: output
 
   - name: assert output changed
@@ -126,21 +127,13 @@
         size: C1
       tags:
         testing: foo
+      wait_for_running: True
     register: output
 
   - assert:
       that:
         - output.changed
 
-  - name: Wait for Redis provisioning to complete
-    azure_rm_rediscache_facts:
-        resource_group: "{{ resource_group }}"
-        name: "{{ redis_name }}"
-    register: facts
-    until: facts.rediscaches[0]['provisioning_state'] == 'Succeeded'
-    retries: 30
-    delay: 60
-
   - name: Force reboot redis cache
     azure_rm_rediscache:
       resource_group: "{{ resource_group }}"


Code review comments:
diff_hunk: @@ -131,6 +131,13 @@
                 choices:
                     - primary
                     - secondary
+    wait_for_running:
path: lib/ansible/modules/cloud/azure/azure_rm_rediscache.py
text: @user1:
maybe rename it to "wait_for_completion" or "wait_until_ready"? I think wait_for_running is not really clear.

@user1:
or "wait_until_provisioning_complete", sounds a kind of long but is the most clear.....

@user1:
also i think default should be "yes" as it's the case for most of the resources


@author:
i don't think default should be yes. this is long running operation, it's not good to default ask user waiting long time.

@author:
or as Matt suggested `wait_for_provisioning`?