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

Signed-off-by: Alina Buzachis <[email protected]>

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:
@@ -356,22 +355,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
@@ -384,12 +401,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
 
@@ -413,9 +429,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)
 
@@ -437,6 +450,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)
 
@@ -491,6 +508,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
 
 
@@ -688,7 +706,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')
@@ -699,6 +723,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(
@@ -715,6 +742,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='For a gp3 volume type, IOPS values must be between 3000 and 16000.')
+        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,9 @@
       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
@@ -156,7 +156,7 @@
           - "volume3.volume.snapshot_id ==  vol1_snapshot.snapshot_id"
 
     - name: create an ec2 instance
-      ec2_instance:
+      ec2:
         name: "{{ resource_prefix }}"
         vpc_subnet_id: "{{ testing_subnet.subnet.id }}"
         instance_type: t3.nano
@@ -203,7 +203,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 +271,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 +387,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 +423,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 +488,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:
@@ -503,8 +566,8 @@
     - name: test create a new gp3 volume
       ec2_vol:
         volume_size: 1
-        volume_type: gp3
         zone: "{{ availability_zone }}"
+        volume_type: gp3
         throughput: 130
         tags:
           ResourcePrefix: "{{ resource_prefix }}"
@@ -513,15 +576,29 @@
     - 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 == true
+          - "'create_time' in gp3_volume.volume"
+          - "'id' in gp3_volume.volume"
+          - "'size' in gp3_volume.volume"
+          - gp3_volume.volume.size == 1
+          - "'volume_type' in gp3_volume"
+          - gp3_volume.volume_type == 'gp3'
+          - "'iops' in gp3_volume.volume"
+          - gp3_volume.volume.iops == 3000
+          - "'throughput' in gp3_volume.volume"
           - gp3_volume.volume.throughput == 130
+          - "'tags' in gp3_volume.volume"
+          - (gp3_volume.tags | length ) == 1
+          - gp3_volume.volume.tags["ResourcePrefix"] == "{{ resource_prefix }}"
 
     - name: increase throughput
       ec2_vol:
         volume_size: 1
-        volume_type: gp3
         zone: "{{ availability_zone }}"
+        volume_type: gp3
         throughput: 131
         tags:
           ResourcePrefix: "{{ resource_prefix }}"
@@ -530,9 +607,23 @@
     - 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 == true
+          - "'create_time' in gp3_volume.volume"
+          - "'id' in gp3_volume.volume"
+          - "'size' in gp3_volume.volume"
+          - gp3_volume.volume.size == 1
+          - "'volume_type' in gp3_volume"
+          - gp3_volume.volume_type == 'gp3'
+          - "'iops' in gp3_volume.volume"
+          - gp3_volume.volume.iops == 100
+          - "'throughput' in gp3_volume.volume"
           - gp3_volume.volume.throughput == 131
+          - "'tags' in gp3_volume.volume"
+          - (gp3_volume.volume.tags | length ) == 1
+          - gp3_volume.volume.tags["ResourcePrefix"] == "{{ resource_prefix }}"
 
 
     # ==== Cleanup ============================================================
@@ -549,7 +640,7 @@
         var: pre_delete
 
     - name: delete test instance
-      ec2_instance:
+      ec2:
         instance_ids:
           - "{{ test_instance.instance_ids[0] }}"
         state: terminated