From 0fbafd91548385b1e0f04349fa58edbc2f5ed4dc Mon Sep 17 00:00:00 2001 From: Alina Buzachis Date: Wed, 21 Apr 2021 16:21:07 +0200 Subject: [PATCH] ec2_vol: Fix iops setting and enforce iops/throughput parameters usage Signed-off-by: Alina Buzachis --- plugins/modules/ec2_vol.py | 79 +++++++++--- .../targets/ec2_vol/tasks/tests.yml | 113 ++++++++++++++++-- 2 files changed, 163 insertions(+), 29 deletions(-) diff --git a/plugins/modules/ec2_vol.py b/plugins/modules/ec2_vol.py index 321b0084f31..4cb601b731a 100644 --- a/plugins/modules/ec2_vol.py +++ b/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,14 +355,32 @@ 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 @@ -371,7 +388,7 @@ def update_volume(module, ec2_conn, volume): 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 diff --git a/tests/integration/targets/ec2_vol/tasks/tests.yml b/tests/integration/targets/ec2_vol/tasks/tests.yml index 3fa64d5547c..74449b16cb0 100644 --- a/tests/integration/targets/ec2_vol/tasks/tests.yml +++ b/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