ec2_vol: added volume waiter and clarification on imports

Volume waiter is courtesy of @tremble
ansible-collections · Nov 16, 2020 · bd08fd2 · bd08fd2
1 parent ba73a4b
commit bd08fd2
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 34 deletions.
diff --git a/changelogs/fragments/53-ec2_vol-boto3-port.yml b/changelogs/fragments/53-ec2_vol-boto3-port.yml
@@ -1,2 +1,2 @@
 minor_changes:
-    - ec2_vol - ported ec2_vol to use boto3
+    - ec2_vol - ported ec2_vol to use boto3 (https://github.com/ansible-collections/amazon.aws/pull/53).
diff --git a/plugins/modules/ec2_vol.py b/plugins/modules/ec2_vol.py
@@ -228,6 +228,8 @@
     }
 '''
 
+import time
+
 from ..module_utils.core import AnsibleAWSModule
 from ..module_utils.ec2 import camel_dict_to_snake_dict
 from ..module_utils.ec2 import boto3_tag_list_to_ansible_dict
@@ -241,7 +243,7 @@
 try:
     import botocore
 except ImportError:
-    pass  # Taken care of by ec2.HAS_BOTO
+    pass  # Taken care of by AnsibleAWSModule
 
 
 def get_instance(module, ec2_conn, instance_id=None):
@@ -329,9 +331,9 @@ def delete_volume(module, ec2_conn, volume_id=None):
         try:
             ec2_conn.delete_volume(VolumeId=volume_id)
             changed = True
+        except is_boto3_error_code('InvalidVolume.NotFound'):
+            module.exit_json(changed=False)
         except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:  # pylint: disable=duplicate-except
-            if is_boto3_error_code('InvalidVolume.NotFound'):
-                module.exit_json(changed=False)
             module.fail_json_aws(e, msg='Error while deleting volume')
     return changed
 
@@ -427,8 +429,19 @@ def modify_dot_attribute(module, ec2_conn, instance_dict, device_name):
     delete_on_termination = module.params.get('delete_on_termination')
     changed = False
 
-    instance_dict = get_instance(module, ec2_conn=ec2_conn, instance_id=instance_dict['instance_id'])
-    mapped_block_device = get_mapped_block_device(instance_dict=instance_dict, device_name=device_name)
+    # volume_in_use can return *shortly* before it appears on the instance
+    # description
+    mapped_block_device = None
+    _attempt = 0
+    while mapped_block_device is None:
+        _attempt += 1
+        instance_dict = get_instance(module, ec2_conn=ec2_conn, instance_id=instance_dict['instance_id'])
+        mapped_block_device = get_mapped_block_device(instance_dict=instance_dict, device_name=device_name)
+        if mapped_block_device is None:
+            if _attempt > 2:
+                module.fail_json(msg='Unable to find device on instance',
+                                 device=device_name, instance=instance_dict)
+            time.sleep(1)
 
     if delete_on_termination != mapped_block_device['ebs'].get('delete_on_termination'):
         try:

diff --git a/tests/integration/targets/ec2_vol/tasks/main.yml b/tests/integration/targets/ec2_vol/tasks/main.yml
@@ -71,10 +71,12 @@
           - "'volume_id' in volume1"
           - "'volume_type' in volume1"
           - "'device' in volume1"
-          - "volume1.volume.status == 'available'"
-          - "volume1.volume_type == 'standard'"
-          - "'attachment_set' in volume1.volume and 'instance_id' in volume1.volume.attachment_set and not volume1.volume.attachment_set.instance_id"
-          - "not volume1.volume.encrypted"
+          - volume1.volume.status == 'available'
+          - volume1.volume_type == 'standard'
+          - "'attachment_set' in volume1.volume"
+          - "'instance_id' in volume1.volume.attachment_set"
+          - not volume1.volume.attachment_set.instance_id
+          - not volume1.volume.encrypted
 
     # no idempotency check needed here
 
@@ -98,11 +100,11 @@
           - "'volume_id' in volume2"
           - "'volume_type' in volume2"
           - "'device' in volume2"
-          - "volume2.volume.status == 'available'"
-          - "volume2.volume_type == 'io1'"
-          - "volume2.volume.iops == 101"
-          - "volume2.volume.size == 4"
-          - "volume2.volume.encrypted"
+          - volume2.volume.status == 'available'
+          - volume2.volume_type == 'io1'
+          - volume2.volume.iops == 101
+          - volume2.volume.size == 4
+          - volume2.volume.encrypted
 
     - name: create another volume (override module defaults) (idempotent)
       ec2_vol:
@@ -177,12 +179,12 @@
     - name: check task return attributes
       assert:
         that:
-          - "vol_attach_result.changed"
+          - vol_attach_result.changed
           - "'device' in vol_attach_result and vol_attach_result.device == '/dev/sdg'"
           - "'volume' in vol_attach_result"
-          - "vol_attach_result.volume.attachment_set.status == 'attached' or 'attaching'"
-          - "vol_attach_result.volume.attachment_set.instance_id == test_instance.instance_ids[0]"
-          - "vol_attach_result.volume.attachment_set.device == '/dev/sdg'"
+          - vol_attach_result.volume.attachment_set.status in ['attached', 'attaching']
+          - vol_attach_result.volume.attachment_set.instance_id == test_instance.instance_ids[0]
+          - vol_attach_result.volume.attachment_set.device == '/dev/sdg'
 
 #          Failing
 #          - "vol_attach_result.volume.attachment_set.deleteOnTermination"
@@ -214,12 +216,12 @@
     - name: check task return attributes
       assert:
         that:
-          - "new_vol_attach_result.changed"
+          - new_vol_attach_result.changed
           - "'device' in new_vol_attach_result and new_vol_attach_result.device == '/dev/sdh'"
           - "'volume' in new_vol_attach_result"
-          - "new_vol_attach_result.volume.attachment_set.status == 'attached' or 'attaching'"
-          - "new_vol_attach_result.volume.attachment_set.instance_id == test_instance.instance_ids[0]"
-          - "new_vol_attach_result.volume.attachment_set.device == '/dev/sdh'"
+          - new_vol_attach_result.volume.attachment_set.status in ['attached', 'attaching']
+          - new_vol_attach_result.volume.attachment_set.instance_id == test_instance.instance_ids[0]
+          - new_vol_attach_result.volume.attachment_set.device == '/dev/sdh'
 
     - name: attach a new volume to an instance (idempotent)
       ec2_vol:
@@ -250,11 +252,11 @@
     - name: check task return attributes
       assert:
         that:
-          - "attach_new_vol_from_snapshot_result.changed"
+          - attach_new_vol_from_snapshot_result.changed
           - "'device' in attach_new_vol_from_snapshot_result and attach_new_vol_from_snapshot_result.device == '/dev/sdi'"
           - "'volume' in attach_new_vol_from_snapshot_result"
-          - "attach_new_vol_from_snapshot_result.volume.attachment_set.status == 'attached' or 'attaching'"
-          - "attach_new_vol_from_snapshot_result.volume.attachment_set.instance_id == test_instance.instance_ids[0]"
+          - attach_new_vol_from_snapshot_result.volume.attachment_set.status in ['attached', 'attaching']
+          - attach_new_vol_from_snapshot_result.volume.attachment_set.instance_id == test_instance.instance_ids[0]
 
     - name: list volumes attached to instance
       ec2_vol:
@@ -265,9 +267,9 @@
     - name: check task return attributes
       assert:
         that:
-          - "not inst_vols.changed"
+          - not inst_vols.changed
           - "'volumes' in inst_vols"
-          - "inst_vols.volumes | length == 4"
+          - inst_vols.volumes | length == 4
 
     - name: get info on ebs volumes
       ec2_vol_info:
@@ -276,7 +278,7 @@
     - name: check task return attributes
       assert:
         that:
-          - "not ec2_vol_info.failed"
+          - not ec2_vol_info.failed
 
     - name: get info on ebs volumes
       ec2_vol_info:
@@ -287,7 +289,7 @@
     - name: check task return attributes
       assert:
         that:
-          - "{{ ec2_vol_info.volumes | length == 4 }}"
+          - ec2_vol_info.volumes | length == 4
 
     - name: detach volume from the instance
       ec2_vol:
@@ -298,8 +300,8 @@
     - name: check task return attributes
       assert:
         that:
-          - "new_vol_attach_result.changed"
-          - "new_vol_attach_result.volume.status == 'available'"
+          - new_vol_attach_result.changed
+          - new_vol_attach_result.volume.status == 'available'
 
     - name: detach volume from the instance (idempotent)
       ec2_vol:
@@ -310,7 +312,7 @@
     - name: check task return attributes
       assert:
         that:
-          - "not new_vol_attach_result_idem.changed"
+          - not new_vol_attach_result_idem.changed
 
     - name: delete volume
       ec2_vol:
@@ -332,12 +334,30 @@
     - name: check task return attributes
       assert:
         that:
-          - "not delete_volume_result_idem.changed"
+          - not delete_volume_result_idem.changed
           - '"Volume {{ volume2.volume_id }} does not exist" in delete_volume_result_idem.msg'
 
     # ==== Cleanup ============================================================
 
   always:
+    - name: Describe the volume before we delete it
+      ec2_vol_info:
+        id: "{{ item.volume_id }}"
+      ignore_errors: yes
+      register: pre_delete
+
+    - debug:
+        var: pre_delete
+
+    - name: Describe the instance before we delete it
+      ec2_instance_info:
+        instance_ids:
+          - "{{ test_instance.instance_ids[0] }}"
+      ignore_errors: yes
+      register: pre_delete
+
+    - debug:
+        var: pre_delete
 
     - name: delete test instance
       ec2_instance: