From 4bf9c6e21c3b101649dd2d99882dc2041e3f98ab Mon Sep 17 00:00:00 2001 From: Matt 'Archer' Vaughn Date: Wed, 10 Aug 2022 16:47:03 -0400 Subject: [PATCH 1/7] fix incomplete enforce_count() return values This is the Ansible way. Since a call to enforce_count() requires that a call to find_instances() has already been made, the information required to populate the `instances` return key is already held by the variable `existing_matches`. There are zero use cases where it makes sense not to output this information; if I want exactly one such instance and an existing instance matches the filter, I definitely always want to know its details. --- plugins/modules/ec2_instance.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/modules/ec2_instance.py b/plugins/modules/ec2_instance.py index 4026869577b..bebae465cc9 100644 --- a/plugins/modules/ec2_instance.py +++ b/plugins/modules/ec2_instance.py @@ -1812,6 +1812,8 @@ def enforce_count(existing_matches, module, desired_module_state): if current_count == exact_count: module.exit_json( changed=False, + instances=[pretty_instance(i) for i in existing_matches], + instance_ids=[i['InstanceId'] for i in existing_matches], msg='{0} instances already running, nothing to do.'.format(exact_count) ) From da28d7dc573a98ab5299ecba9f9b66529d13290d Mon Sep 17 00:00:00 2001 From: "M. Raymond Vaughn" Date: Wed, 10 Aug 2022 20:37:05 -0400 Subject: [PATCH 2/7] Changelog: - populate `instance_ids` in all cases where `existing_matches` are present, even in check mode - populate `instances` in all non-check mode cases where `existing_matches` are present - the `instance_ids` and `instances` return values are always a union of any `existing_matches` and instances launched/terminated by the module - the only time `instance_ids` is absent is in the case that check mode is true and `existing_matches` is the empty set When `exact_count` is in use: - if a call to `ensure_present()` would launch instances and `existing_matches` are present, the return key `changed_ids` will indicate only the launched instances; the return key `instance_ids` will include both existing and launched instances. This allows for selection of either subset of the cohort. - if a call to `enforce_count()` would terminate instances and `existing matches` are present, the return key `terminated_ids` will still indicate the terminated instances; the return key `instance_ids` will indicate all instances that matched the filter query prior to any terminations as a final means of preserving the metadata of all instances in the cohort. --- plugins/modules/ec2_instance.py | 91 ++++++++++++++++++++++++++------- 1 file changed, 73 insertions(+), 18 deletions(-) diff --git a/plugins/modules/ec2_instance.py b/plugins/modules/ec2_instance.py index bebae465cc9..0b78a6ab934 100644 --- a/plugins/modules/ec2_instance.py +++ b/plugins/modules/ec2_instance.py @@ -533,9 +533,24 @@ """ RETURN = r""" +instance_ids: + description: a list of ec2 instance IDs matching the provided specification and filters + returned: always + type: list + sample: ["i-0123456789abcdef0", "i-0123456789abcdef1"] +changed_ids: + description: a list of the set of ec2 instance IDs changed by the module action + returned: when instances that must be present are launched + type: list + sample: ["i-0123456789abcdef0"] +terminated_ids: + description: a list of the set of ec2 instance IDs terminated by the module action + returned: when instances that must be absent are terminated + type: list + sample: ["i-0123456789abcdef1"] instances: description: a list of ec2 instances - returned: when wait == true + returned: when wait == true or when matching instances already exist type: complex contains: ami_launch_index: @@ -1831,7 +1846,12 @@ def enforce_count(existing_matches, module, desired_module_state): all_instance_ids = [x['InstanceId'] for x in existing_matches] terminate_ids = all_instance_ids[0:to_terminate] if module.check_mode: - module.exit_json(changed=True, msg='Would have terminated following instances if not in check mode {0}'.format(terminate_ids)) + module.exit_json( + changed=True, + terminated_ids=terminate_ids, + instance_ids=all_instance_ids, + msg='Would have terminated following instances if not in check mode {0}'.format(terminate_ids) + ) # terminate instances try: client.terminate_instances(aws_retry=True, InstanceIds=terminate_ids) @@ -1840,10 +1860,14 @@ def enforce_count(existing_matches, module, desired_module_state): pass except botocore.exceptions.ClientError as e: # pylint: disable=duplicate-except module.fail_json(e, msg='Unable to terminate instances') + # include data for all matched instances in addition to the list of terminations + # allowing for recovery of metadata from the destructive operation module.exit_json( changed=True, msg='Successfully terminated instances.', terminated_ids=terminate_ids, + instance_ids=all_instance_ids, + instances=existing_matches, ) except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: @@ -1860,11 +1884,21 @@ def ensure_present(existing_matches, desired_module_state, current_count=None): instance_spec = build_run_instance_spec(module.params, current_count) # If check mode is enabled,suspend 'ensure function'. if module.check_mode: - module.exit_json( - changed=True, - spec=instance_spec, - msg='Would have launched instances if not in check_mode.', - ) + if existing_matches: + instance_ids = [x['InstanceId'] for x in existing_matches] + module.exit_json( + changed=True, + instance_ids=instance_ids, + instances=existing_matches, + spec=instance_spec, + msg='Would have launched instances if not in check_mode.', + ) + else: + module.exit_json( + changed=True, + spec=instance_spec, + msg='Would have launched instances if not in check_mode.', + ) instance_response = run_instances(**instance_spec) instances = instance_response['Instances'] instance_ids = [i['InstanceId'] for i in instances] @@ -1891,23 +1925,44 @@ def ensure_present(existing_matches, desired_module_state, current_count=None): try: client.modify_instance_attribute(aws_retry=True, **c) except botocore.exceptions.ClientError as e: - module.fail_json_aws(e, msg="Could not apply change {0} to new instance.".format(str(c))) - + module.fail_json_aws(e, msg="Could not apply change {0} to new instance.".format(str(c))) + if existing_matches: + # If we came from enforce_count, create a second list to distinguish + # between existing and new instances when returning the entire cohort + all_instance_ids = [x['InstanceId'] for x in existing_matches] + instance_ids if not module.params.get('wait'): + if existing_matches: + module.exit_json( + changed=True, + changed_ids=instance_ids, + instance_ids=all_instance_ids, + spec=instance_spec, + ) + else: + module.exit_json( + changed=True, + instance_ids=instance_ids, + spec=instance_spec, + ) + await_instances(instance_ids, desired_module_state=desired_module_state) + instances = find_instances(ids=instance_ids) + + if existing_matches: + all_instances = existing_matches + instances + module.exit_json( + changed=True, + changed_ids=instance_ids, + instance_ids=all_instance_ids, + instances=[pretty_instance(i) for i in all_instances], + spec=instance_spec, + ) + else: module.exit_json( changed=True, instance_ids=instance_ids, + instances=[pretty_instance(i) for i in instances], spec=instance_spec, ) - await_instances(instance_ids, desired_module_state=desired_module_state) - instances = find_instances(ids=instance_ids) - - module.exit_json( - changed=True, - instances=[pretty_instance(i) for i in instances], - instance_ids=instance_ids, - spec=instance_spec, - ) except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: module.fail_json_aws(e, msg="Failed to create new EC2 instance") From 2fc61b9658403d1174761183bd2f7bcd3de9b416 Mon Sep 17 00:00:00 2001 From: "M. Raymond Vaughn" Date: Wed, 10 Aug 2022 20:53:31 -0400 Subject: [PATCH 3/7] Updated integration tests for return values: - instances - instance_ids - terminated_ids - changed_ids --- .../tasks/main.yml | 37 ++++++++++++++----- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/tests/integration/targets/ec2_instance_instance_multiple/tasks/main.yml b/tests/integration/targets/ec2_instance_instance_multiple/tasks/main.yml index 0c366244b95..4613d1d9e84 100644 --- a/tests/integration/targets/ec2_instance_instance_multiple/tasks/main.yml +++ b/tests/integration/targets/ec2_instance_instance_multiple/tasks/main.yml @@ -164,7 +164,8 @@ that: - create_multiple_instances is not failed - create_multiple_instances is not changed - - '"instance_ids" not in create_multiple_instances' + - '"instance_ids" in create_multiple_instances' + - create_multiple_instances.instance_ids | length == 5 - '"ec2:RunInstances" not in create_multiple_instances.resource_actions' - name: "Enforce instance count - launch 5 instances (Idempotency)" @@ -183,7 +184,8 @@ that: - create_multiple_instances is not failed - create_multiple_instances is not changed - - '"instance_ids" not in create_multiple_instances' + - '"instance_ids" in create_multiple_instances' + - create_multiple_instances.instance_ids | length == 5 - '"ec2:RunInstances" not in create_multiple_instances.resource_actions' - name: "Enforce instance count to 3 - Terminate 2 instances (check_mode)" @@ -202,7 +204,10 @@ that: - terminate_multiple_instances is not failed - terminate_multiple_instances is changed - - '"instance_ids" not in terminate_multiple_instances' + - '"instance_ids" in terminate_multiple_instances' + - terminate_multiple_instances.instance_ids | length == 5 + - '"terminated_ids" in terminate_multiple_instances' + - terminate_multiple_instances.terminated_ids | length == 2 - '"ec2:RunInstances" not in terminate_multiple_instances.resource_actions' - name: "Enforce instance count to 3 - Terminate 2 instances" @@ -221,6 +226,8 @@ that: - terminate_multiple_instances is not failed - terminate_multiple_instances is changed + - '"instance_ids" in terminate_multiple_instances' + - terminate_multiple_instances.instance_ids | length == 5 - '"terminated_ids" in terminate_multiple_instances' - terminate_multiple_instances.terminated_ids | length == 2 @@ -240,7 +247,9 @@ that: - terminate_multiple_instances is not failed - terminate_multiple_instances is not changed - - '"instance_ids" not in terminate_multiple_instances' + - '"instance_ids" in terminate_multiple_instances' + - terminate_multiple_instances.instance_ids | length == 3 + - '"terminated_ids" not in terminate_multiple_instances' - '"ec2:TerminateInstances" not in terminate_multiple_instances.resource_actions' - name: "Enforce instance count to 3 - Terminate 2 instances (Idempotency)" @@ -258,7 +267,9 @@ that: - terminate_multiple_instances is not failed - terminate_multiple_instances is not changed - - '"instance_ids" not in terminate_multiple_instances' + - '"instance_ids" in terminate_multiple_instances' + - terminate_multiple_instances.instance_ids | length == 3 + - '"terminated_ids" not in terminate_multiple_instances' - '"ec2:TerminateInstances" not in terminate_multiple_instances.resource_actions' - name: "Enforce instance count to 6 - Launch 3 more instances (check_mode)" @@ -278,7 +289,9 @@ that: - create_multiple_instances is not failed - create_multiple_instances is changed - - '"instance_ids" not in create_multiple_instances' + - '"instance_ids" in create_multiple_instances' + - create_multiple_instances.instance_ids | length == 3 + - '"changed_ids" not in create_multiple_instances' - '"ec2:RunInstances" not in create_multiple_instances.resource_actions' - name: "Enforce instance count to 6 - Launch 3 more instances" @@ -302,7 +315,9 @@ - create_multiple_instances is not failed - create_multiple_instances is changed - '"instance_ids" in create_multiple_instances' - - create_multiple_instances.instance_ids | length == 3 + - create_multiple_instances.instance_ids | length == 6 + - '"changed_ids" in create_multiple_instances' + - create_multiple_instances.changed_ids | length == 3 - name: "Enforce instance count to 6 - Launch 3 more instances (check_mode - Idempotency)" ec2_instance: @@ -321,7 +336,9 @@ that: - create_multiple_instances is not failed - create_multiple_instances is not changed - - '"instance_ids" not in create_multiple_instances' + - '"instance_ids" in create_multiple_instances' + - create_multiple_instances.instance_ids | length == 6 + - '"changed_ids" not in create_multiple_instances' - '"ec2:RunInstances" not in create_multiple_instances.resource_actions' - name: "Enforce instance count to 6 - Launch 3 more instances (Idempotency)" @@ -340,7 +357,9 @@ that: - create_multiple_instances is not failed - create_multiple_instances is not changed - - '"instance_ids" not in create_multiple_instances' + - '"instance_ids" in create_multiple_instances' + - create_multiple_instances.instance_ids | length == 6 + - '"changed_ids" not in create_multiple_instances' - '"ec2:RunInstances" not in create_multiple_instances.resource_actions' From 86c9963ade4892aaa0570836d51373ad2a8a4957 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Thu, 9 Feb 2023 11:10:19 +0100 Subject: [PATCH 4/7] lint --- plugins/modules/ec2_instance.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/modules/ec2_instance.py b/plugins/modules/ec2_instance.py index 0b78a6ab934..a350c38a19a 100644 --- a/plugins/modules/ec2_instance.py +++ b/plugins/modules/ec2_instance.py @@ -1925,7 +1925,7 @@ def ensure_present(existing_matches, desired_module_state, current_count=None): try: client.modify_instance_attribute(aws_retry=True, **c) except botocore.exceptions.ClientError as e: - module.fail_json_aws(e, msg="Could not apply change {0} to new instance.".format(str(c))) + module.fail_json_aws(e, msg="Could not apply change {0} to new instance.".format(str(c))) if existing_matches: # If we came from enforce_count, create a second list to distinguish # between existing and new instances when returning the entire cohort From 89bedf1925168724c311cfb49021af9948b2c26d Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Thu, 9 Feb 2023 11:13:44 +0100 Subject: [PATCH 5/7] version_added --- plugins/modules/ec2_instance.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/plugins/modules/ec2_instance.py b/plugins/modules/ec2_instance.py index a350c38a19a..0ed82a733d9 100644 --- a/plugins/modules/ec2_instance.py +++ b/plugins/modules/ec2_instance.py @@ -538,16 +538,19 @@ returned: always type: list sample: ["i-0123456789abcdef0", "i-0123456789abcdef1"] + version_added: 5.3.0 changed_ids: description: a list of the set of ec2 instance IDs changed by the module action returned: when instances that must be present are launched type: list sample: ["i-0123456789abcdef0"] + version_added: 5.3.0 terminated_ids: description: a list of the set of ec2 instance IDs terminated by the module action returned: when instances that must be absent are terminated type: list sample: ["i-0123456789abcdef1"] + version_added: 5.3.0 instances: description: a list of ec2 instances returned: when wait == true or when matching instances already exist From cb8db3a12e194f951aac9f9c3d8251eb40205894 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Thu, 9 Feb 2023 11:14:41 +0100 Subject: [PATCH 6/7] changelog --- changelogs/fragments/964-ec2_instance-return-instances.yml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelogs/fragments/964-ec2_instance-return-instances.yml diff --git a/changelogs/fragments/964-ec2_instance-return-instances.yml b/changelogs/fragments/964-ec2_instance-return-instances.yml new file mode 100644 index 00000000000..045d03202a3 --- /dev/null +++ b/changelogs/fragments/964-ec2_instance-return-instances.yml @@ -0,0 +1,2 @@ +minor_changes: +- ec2_instance - more consistently return ``instances`` information (https://github.com/ansible-collections/amazon.aws/pull/964). From 178d86f74052396570ecde0a3918d21bab26cb35 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Thu, 23 Feb 2023 09:33:49 +0100 Subject: [PATCH 7/7] black/lint --- plugins/modules/ec2_instance.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/plugins/modules/ec2_instance.py b/plugins/modules/ec2_instance.py index 0ed82a733d9..ea3f3060d4e 100644 --- a/plugins/modules/ec2_instance.py +++ b/plugins/modules/ec2_instance.py @@ -1831,8 +1831,8 @@ def enforce_count(existing_matches, module, desired_module_state): module.exit_json( changed=False, instances=[pretty_instance(i) for i in existing_matches], - instance_ids=[i['InstanceId'] for i in existing_matches], - msg='{0} instances already running, nothing to do.'.format(exact_count) + instance_ids=[i["InstanceId"] for i in existing_matches], + msg=f"{exact_count} instances already running, nothing to do.", ) elif current_count < exact_count: @@ -1853,7 +1853,7 @@ def enforce_count(existing_matches, module, desired_module_state): changed=True, terminated_ids=terminate_ids, instance_ids=all_instance_ids, - msg='Would have terminated following instances if not in check mode {0}'.format(terminate_ids) + msg=f"Would have terminated following instances if not in check mode {terminate_ids}", ) # terminate instances try: @@ -1888,19 +1888,19 @@ def ensure_present(existing_matches, desired_module_state, current_count=None): # If check mode is enabled,suspend 'ensure function'. if module.check_mode: if existing_matches: - instance_ids = [x['InstanceId'] for x in existing_matches] + instance_ids = [x["InstanceId"] for x in existing_matches] module.exit_json( changed=True, instance_ids=instance_ids, instances=existing_matches, spec=instance_spec, - msg='Would have launched instances if not in check_mode.', + msg="Would have launched instances if not in check_mode.", ) else: module.exit_json( changed=True, spec=instance_spec, - msg='Would have launched instances if not in check_mode.', + msg="Would have launched instances if not in check_mode.", ) instance_response = run_instances(**instance_spec) instances = instance_response['Instances'] @@ -1932,8 +1932,8 @@ def ensure_present(existing_matches, desired_module_state, current_count=None): if existing_matches: # If we came from enforce_count, create a second list to distinguish # between existing and new instances when returning the entire cohort - all_instance_ids = [x['InstanceId'] for x in existing_matches] + instance_ids - if not module.params.get('wait'): + all_instance_ids = [x["InstanceId"] for x in existing_matches] + instance_ids + if not module.params.get("wait"): if existing_matches: module.exit_json( changed=True,