ec2_vol: Fix iops setting and enforce iops/throughput parameters usage (

ansible-collections#371)

ec2_vol: Fix iops setting and enforce iops/throughput parameters usage

Reviewed-by: https://github.com/apps/ansible-zuul

changelogs/fragments/334-ec2_vol-iops-and-throughput-issues.yaml

@@ -0,0 +1,2 @@
+bugfixes:
+- ec2_vol - fix iops setting and enforce iops/throughput parameters usage (https://github.com/ansible-collections/amazon.aws/pull/334)

plugins/modules/ec2_vol.py

@@ -43,7 +43,6 @@
   iops:
     description:
       - The provisioned IOPs you want to associate with this volume (integer).
-      - By default AWS will set this to 100.
     type: int
   encrypted:
     description:
@@ -357,22 +356,40 @@ def update_volume(module, ec2_conn, volume):
     req_obj = {'VolumeId': volume['volume_id']}
 
     if module.params.get('modify_volume'):
+        target_type = module.params.get('volume_type')
+        original_type = None
+        type_changed = False
+        if target_type:
+            original_type = volume['volume_type']
+            if target_type != original_type:
+                type_changed = True
+                req_obj['VolumeType'] = target_type
+
         iops_changed = False
-        if volume['volume_type'] != 'standard':
-            target_iops = module.params.get('iops')
-            if target_iops:
-                original_iops = volume['iops']
-                if target_iops != original_iops:
+        target_iops = module.params.get('iops')
+        if target_iops:
+            original_iops = volume['iops']
+            if target_iops != original_iops:
+                iops_changed = True
+                req_obj['Iops'] = target_iops
+        else:
+            # If no IOPS value is specified and there was a volume_type update to gp3,
+            # the existing value is retained, unless a volume type is modified that supports different values,
+            # otherwise, the default iops value is applied.
+            if type_changed and target_type == 'gp3':
+                if (
+                    (volume['iops'] and (int(volume['iops']) < 3000 or int(volume['iops']) > 16000)) or not volume['iops']
+                ):
+                    req_obj['Iops'] = 3000
                     iops_changed = True
-                    req_obj['iops'] = target_iops
 
         target_size = module.params.get('volume_size')
         size_changed = False
         if target_size:
             original_size = volume['size']
             if target_size != original_size:
                 size_changed = True
-                req_obj['size'] = target_size
+                req_obj['Size'] = target_size
 
         target_type = module.params.get('volume_type')
         original_type = None
@@ -385,12 +402,11 @@ def update_volume(module, ec2_conn, volume):
 
         target_throughput = module.params.get('throughput')
         throughput_changed = False
-        if 'gp3' in [target_type, original_type]:
-            if target_throughput:
-                original_throughput = volume.get('throughput')
-                if target_throughput != original_throughput:
-                    throughput_changed = True
-                    req_obj['Throughput'] = target_throughput
+        if target_throughput:
+            original_throughput = volume.get('throughput')
+            if target_throughput != original_throughput:
+                throughput_changed = True
+                req_obj['Throughput'] = target_throughput
 
         changed = iops_changed or size_changed or type_changed or throughput_changed
 
@@ -415,9 +431,6 @@ def create_volume(module, ec2_conn, zone):
     volume_type = module.params.get('volume_type')
     snapshot = module.params.get('snapshot')
     throughput = module.params.get('throughput')
-    # If custom iops is defined we use volume_type "io1" rather than the default of "standard"
-    if iops:
-        volume_type = 'io1'
 
     volume = get_volume(module, ec2_conn)
 
@@ -439,6 +452,10 @@ def create_volume(module, ec2_conn, zone):
             if iops:
                 additional_params['Iops'] = int(iops)
 
+            # Use the default value if any iops has been specified when volume_type=gp3
+            if volume_type == 'gp3' and not iops:
+                additional_params['Iops'] = 3000
+
             if throughput:
                 additional_params['Throughput'] = int(throughput)
 
@@ -493,6 +510,7 @@ def attach_volume(module, ec2_conn, volume_dict, instance_dict, device_name):
     modify_dot_attribute(module, ec2_conn, instance_dict, device_name)
 
     volume = get_volume(module, ec2_conn, vol_id=volume_dict['volume_id'])
+
     return volume, changed
 
 
@@ -643,7 +661,13 @@ def main():
         purge_tags=dict(type='bool', default=False),
     )
 
-    module = AnsibleAWSModule(argument_spec=argument_spec)
+    module = AnsibleAWSModule(
+        argument_spec=argument_spec,
+        required_if=[
+            ['volume_type', 'io1', ['iops']],
+            ['volume_type', 'io2', ['iops']],
+        ],
+    )
 
     param_id = module.params.get('id')
     name = module.params.get('name')
@@ -654,6 +678,9 @@ def main():
     snapshot = module.params.get('snapshot')
     state = module.params.get('state')
     tags = module.params.get('tags')
+    iops = module.params.get('iops')
+    volume_type = module.params.get('volume_type')
+    throughput = module.params.get('throughput')
 
     if state == 'list':
         module.deprecate(
@@ -674,6 +701,22 @@ def main():
     else:
         detach_vol_flag = False
 
+    if iops:
+        if volume_type in ('gp2', 'st1', 'sc1', 'standard'):
+            module.fail_json(msg='IOPS is not supported for gp2, st1, sc1, or standard volumes.')
+
+        if volume_type == 'gp3' and (int(iops) < 3000 or int(iops) > 16000):
+            module.fail_json(msg='For a gp3 volume type, IOPS values must be between 3000 and 16000.')
+
+        if volume_type in ('io1', 'io2') and (int(iops) < 100 or int(iops) > 64000):
+            module.fail_json(msg='For io1 and io2 volume types, IOPS values must be between 100 and 64000.')
+
+    if throughput:
+        if volume_type != 'gp3':
+            module.fail_json(msg='Throughput is only supported for gp3 volume.')
+        if throughput < 125 or throughput > 1000:
+            module.fail_json(msg='Throughput values must be between 125 and 1000.')
+
     # Set changed flag
     changed = False
 

tests/integration/targets/ec2_vol/tasks/tests.yml

@@ -6,9 +6,10 @@
       aws_secret_key: '{{ aws_secret_key | default(omit) }}'
       security_token: '{{ security_token | default(omit) }}'
       region: '{{ aws_region | default(omit) }}'
-
+  
   collections:
     - amazon.aws
+    - community.aws
 
   block:
     - name: list available AZs
@@ -203,7 +204,7 @@
       assert:
         that:
           - "not vol_attach_result.changed"
-          - "vol_attach_result.volume.attachment_set.status == 'attached'"
+          - vol_attach_result.volume.attachment_set.status in ['attached', 'attaching']
 
     - name: attach a new volume to an instance
       ec2_vol:
@@ -271,6 +272,17 @@
       assert:
         that:
           - new_vol_attach_result.changed
+          - "'volume_id' in new_vol_attach_result"
+          - new_vol_attach_result.volume_id == "{{ new_vol_attach_result.volume_id }}"
+          - "'attachment_set' in new_vol_attach_result.volume"
+          - "'create_time' in new_vol_attach_result.volume"
+          - "'id' in new_vol_attach_result.volume"
+          - "'size' in new_vol_attach_result.volume"
+          - new_vol_attach_result.volume.size == 1
+          - "'volume_type' in new_vol_attach_result"
+          - new_vol_attach_result.volume_type == 'gp2'
+          - "'tags' in new_vol_attach_result.volume"
+          - (new_vol_attach_result.volume.tags | length) == 6
           - new_vol_attach_result.volume.tags["lowercase spaced"] == 'hello cruel world ❤️'
           - new_vol_attach_result.volume.tags["Title Case"] == 'Hello Cruel World ❤️'
           - new_vol_attach_result.volume.tags["CamelCase"] == 'SimpleCamelCase ❤️'
@@ -376,13 +388,26 @@
     - name: check that volume_type has changed
       assert:
         that:
-          - changed_gp3_volume.volume_type == 'gp3'
           - changed_gp3_volume.changed
+          - "'volume_id' in changed_gp3_volume"
+          - changed_gp3_volume.volume_id == "{{ new_vol_attach_result.volume_id }}"
+          - "'attachment_set' in changed_gp3_volume.volume"
+          - "'create_time' in changed_gp3_volume.volume"
+          - "'id' in changed_gp3_volume.volume"
+          - "'size' in changed_gp3_volume.volume"
+          - "'volume_type' in changed_gp3_volume"
+          - changed_gp3_volume.volume_type == 'gp3'
+          - "'iops' in changed_gp3_volume.volume"
+          - changed_gp3_volume.volume.iops == 3000
           # Ensure our tags are still here
+          - "'tags' in changed_gp3_volume.volume"
+          - (changed_gp3_volume.volume.tags | length) == 6
           - new_vol_attach_result.volume.tags["lowercase spaced"] == 'hello cruel world ❤️'
           - new_vol_attach_result.volume.tags["Title Case"] == 'Hello Cruel World ❤️'
           - new_vol_attach_result.volume.tags["CamelCase"] == 'SimpleCamelCase ❤️'
           - new_vol_attach_result.volume.tags["snake_case"] == 'simple_snake_case ❤️'
+          - new_vol_attach_result.volume.tags["ResourcePrefix"] == resource_prefix
+          - new_vol_attach_result.volume.tags["Name"] == '{{ resource_prefix }} - sdh'
 
     - name: volume must be from type gp3 (idempotent)
       ec2_vol:
@@ -399,8 +424,26 @@
     - name: must not changed (idempotent)
       assert:
         that:
-          - changed_gp3_volume.volume_type == 'gp3'
           - not changed_gp3_volume.changed
+          - "'volume_id' in changed_gp3_volume"
+          - changed_gp3_volume.volume_id == "{{ new_vol_attach_result.volume_id }}"
+          - "'attachment_set' in changed_gp3_volume.volume"
+          - "'create_time' in changed_gp3_volume.volume"
+          - "'id' in changed_gp3_volume.volume"
+          - "'size' in changed_gp3_volume.volume"
+          - "'volume_type' in changed_gp3_volume"
+          - changed_gp3_volume.volume_type == 'gp3'
+          - "'iops' in changed_gp3_volume.volume"
+          - changed_gp3_volume.volume.iops == 3000
+          - "'throughput' in changed_gp3_volume.volume"
+          - "'tags' in changed_gp3_volume.volume"
+          - (changed_gp3_volume.volume.tags | length) == 6
+          - new_vol_attach_result.volume.tags["lowercase spaced"] == 'hello cruel world ❤️'
+          - new_vol_attach_result.volume.tags["Title Case"] == 'Hello Cruel World ❤️'
+          - new_vol_attach_result.volume.tags["CamelCase"] == 'SimpleCamelCase ❤️'
+          - new_vol_attach_result.volume.tags["snake_case"] == 'simple_snake_case ❤️'
+          - new_vol_attach_result.volume.tags["ResourcePrefix"] == resource_prefix
+          - new_vol_attach_result.volume.tags["Name"] == '{{ resource_prefix }} - sdh'
 
     - name: re-read volume information to validate new volume_type
       ec2_vol_info:
@@ -446,11 +489,32 @@
         volume_size: 4
         name: "{{ resource_prefix }}_delete_on_terminate"
         device_name: /dev/sdj
+        volume_type: io1
         iops: 100
         tags:
             Tag Name with Space-and-dash: Tag Value with Space-and-dash
         delete_on_termination: yes
       register: dot_volume
+
+    - name: check task return attributes
+      assert:
+        that:
+          - dot_volume.changed
+          - "'attachment_set' in dot_volume.volume"
+          - "'deleteOnTermination' in dot_volume.volume.attachment_set"
+          - "dot_volume.volume.attachment_set.deleteOnTermination is defined"
+          - "'create_time' in dot_volume.volume"
+          - "'id' in dot_volume.volume"
+          - "'size' in dot_volume.volume"
+          - dot_volume.volume.size == 4
+          - "'volume_type' in dot_volume"
+          - dot_volume.volume_type == 'io1'
+          - "'iops' in dot_volume.volume"
+          - dot_volume.volume.iops == 100
+          - "'tags' in dot_volume.volume"
+          - (dot_volume.volume.tags | length ) == 2
+          - dot_volume.volume.tags["Name"] == "{{ resource_prefix }}_delete_on_terminate"
+          - dot_volume.volume.tags["Tag Name with Space-and-dash"] == 'Tag Value with Space-and-dash'
 
     - name: Gather volume info without any filters
       ec2_vol_info:
@@ -502,37 +566,69 @@
 
     - name: test create a new gp3 volume
       ec2_vol:
-        volume_size: 1
-        volume_type: gp3
+        volume_size: 7
         zone: "{{ availability_zone }}"
+        volume_type: gp3
         throughput: 130
+        iops: 3001
+        name: "GP3-TEST-{{ resource_prefix }}"
         tags:
           ResourcePrefix: "{{ resource_prefix }}"
       register: gp3_volume
 
     - name: check that volume_type is gp3
       assert:
         that:
-          - gp3_volume.volume_type == 'gp3'
           - gp3_volume.changed
+          - "'attachment_set' in gp3_volume.volume"
+          - "'deleteOnTermination' in gp3_volume.volume.attachment_set"
+          - gp3_volume.volume.attachment_set.deleteOnTermination == none
+          - "'create_time' in gp3_volume.volume"
+          - "'id' in gp3_volume.volume"
+          - "'size' in gp3_volume.volume"
+          - gp3_volume.volume.size == 7
+          - "'volume_type' in gp3_volume"
+          - gp3_volume.volume_type == 'gp3'
+          - "'iops' in gp3_volume.volume"
+          - gp3_volume.volume.iops == 3001
+          - "'throughput' in gp3_volume.volume"
           - gp3_volume.volume.throughput == 130
+          - "'tags' in gp3_volume.volume"
+          - (gp3_volume.volume.tags | length ) == 2
+          - gp3_volume.volume.tags["ResourcePrefix"] == "{{ resource_prefix }}"
 
     - name: increase throughput
       ec2_vol:
-        volume_size: 1
-        volume_type: gp3
+        volume_size: 7
         zone: "{{ availability_zone }}"
+        volume_type: gp3
         throughput: 131
+        modify_volume: yes
+        name: "GP3-TEST-{{ resource_prefix }}"
         tags:
           ResourcePrefix: "{{ resource_prefix }}"
       register: gp3_volume
 
     - name: check that throughput has changed
       assert:
         that:
-          - gp3_volume.volume_type == 'gp3'
           - gp3_volume.changed
+          - "'attachment_set' in gp3_volume.volume"
+          - "'deleteOnTermination' in gp3_volume.volume.attachment_set"
+          - gp3_volume.volume.attachment_set.deleteOnTermination == none
+          - "'create_time' in gp3_volume.volume"
+          - "'id' in gp3_volume.volume"
+          - "'size' in gp3_volume.volume"
+          - gp3_volume.volume.size == 7
+          - "'volume_type' in gp3_volume"
+          - gp3_volume.volume_type == 'gp3'
+          - "'iops' in gp3_volume.volume"
+          - gp3_volume.volume.iops == 3001
+          - "'throughput' in gp3_volume.volume"
           - gp3_volume.volume.throughput == 131
+          - "'tags' in gp3_volume.volume"
+          - (gp3_volume.volume.tags | length ) == 2
+          - gp3_volume.volume.tags["ResourcePrefix"] == "{{ resource_prefix }}"
 
 
     # ==== Cleanup ============================================================