Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ec2_instance should allow setting and changing PlacementGroup and PartitionNumber #1264

Closed
wants to merge 4 commits into from
Closed

ec2_instance should allow setting and changing PlacementGroup and PartitionNumber #1264

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a1afc7
Adding ability to set and change PlacementGroup and PartitionNumber o…
llunved Nov 12, 2022
06a2544
Fixing syntax.
llunved Nov 12, 2022
4a36286
Merge branch 'ansible-collections:main' into main
daniel-trnk Nov 12, 2022
e274715
the versioning scheme with -dev0 may be breaking the galaxy dependenc…
daniel-trnk Dec 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion plugins/modules/ec2_instance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,9 @@
description:
- The placement group that needs to be assigned to the instance.
type: str
partition_number:
description:
- The number of the partition within the placement group - only valid with partition typle placement groups.
Comment on lines +325 to +327
Copy link
Contributor

@tremble tremble Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
partition_number:
description:
- The number of the partition within the placement group - only valid with partition typle placement groups.
partition_number:
description:
- The number of the partition within the placement group - only valid with partition typle placement groups.
type: int
version_added: 5.3.0

metadata_options:
description:
- Modify the metadata options for the instance.
Expand Down Expand Up @@ -1259,6 +1262,8 @@ def build_top_level_options(params):
spec['Placement']['GroupName'] = str(params.get('placement_group'))
else:
spec.setdefault('Placement', {'GroupName': str(params.get('placement_group'))})
if params.get('partition_number'):
spec['Placement']['PartitionNumber'] = int(params.get('partition_number'))
if params.get('ebs_optimized') is not None:
spec['EbsOptimized'] = params.get('ebs_optimized')
if params.get('instance_initiated_shutdown_behavior'):
Expand Down Expand Up @@ -1468,7 +1473,7 @@ def value_wrapper(v):
InstanceId=instance['InstanceId'],
SourceDestCheck={'Value': check},
))

return changes_to_apply


Expand Down Expand Up @@ -1497,6 +1502,33 @@ def change_network_attachments(instance, params):
return False


def change_placement(instance, params):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will currently result in the placement being updated while running in "check" mode.

What I'd suggest is something like the following.

def change_placement(instance, params):
    current_placement = instance.get("Placement", {}) 
    current_group = current_placement.get("GroupName")
    current_partition = current_placement.get("PartitionNumber")
    desired_group = params.get("placement_group")
    desired_partition = params.get("partition_number")

    changed = False

    if desired_group is not None and current_group != desired_group:
        changed = True
    if desired_partition is not None and current_partition != desired_partition:
        changed = True

    if module.check_mode or changed is False:
        return changed

    desired_placement = {"GroupName": desired_group or current_group}
    if desired_partition:
        desired_placement["PartitionNumber"] = desired_partition

    try:
        client.modify_instance_placement(
            aws_retry=True, InstanceId=instance["InstanceId"], **desired_placement
        )   
    except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e:
        module.fail_json_aws(
            e,  
            msg="Could not change placement of instance {0} to {1}".format(
                instance["InstanceId"], desired_placement
            ),  
        )   

    return True

By simply returning early if we're in check mode or don't have a change to make we can avoid the extra level of nesting.

By pulling the desired/current state out into temporary variables, it's much easier to follow (and debug) the "has something changed" logic.

current_placement = instance['Placement']
desired_placement = dict()
if params.get('placement_group'):
desired_placement.update({'GroupName':params.get('placement_group')})
else:
desired_placement.update({'GroupName':current_placement['GroupName']})

if params.get('partition_number'):
desired_placement.update({'PartitionNumber':params.get('partition_number')})

if (( desired_placement['GroupName'] != current_placement['GroupName']) or
( 'PartitionNumber' in desired_placement and ( desired_placement['PartitionNumber'] != current_placement['PartitionNumber']))):
Comment on lines +1516 to +1517
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is going to throw a KeyError exception if someone tries to assign a partition number while using a cluster placement group. While the API would still return an error, the API error message is cleanly caught and likely has a helpful error message.


try:
client.modify_instance_placement(
aws_retry=True,
InstanceId=instance['InstanceId'],
**desired_placement
)
except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e:
module.fail_json_aws(e, msg="Could not change placement instance {0} to {1}".format(instance['InstanceId'],
desired_placement_group,desired_partition_number))
Comment on lines +1526 to +1527
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 3 args passed and only 2 used in the format...

return True
return False


def find_instances(ids=None, filters=None):
sanitized_filters = dict()

Expand Down Expand Up @@ -1796,6 +1828,7 @@ def handle_existing(existing_matches, state, filters):
changed |= bool(changes)
changed |= add_or_update_instance_profile(existing_matches[0], module.params.get('iam_instance_profile'))
changed |= change_network_attachments(existing_matches[0], module.params)
changed |= change_placement(existing_matches[0], module.params)

altered = find_instances(ids=[i['InstanceId'] for i in existing_matches])
alter_config_result = dict(
Expand Down Expand Up @@ -2016,6 +2049,7 @@ def main():
)),
tenancy=dict(type='str', choices=['dedicated', 'default']),
placement_group=dict(type='str'),
partition_number=dict(type='int', choices=[1,2,3,4,5,6,7]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While there's currently a limit of 7 partitions per placement group, I'd recommend against hardcoding this limit using choices. With a hard coded limit, should Amazon increase the limit we need a new release to support the new partitions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to require that the placement group is set if someone sets the partition number?
This would cause things to fail early with an obvious error message if someone tried to set the partition number when still using the default placement, rather than potentially performing half an update.

This could be achieved using "required_by" to the module creation (we already have things like mutually_exclusive)
https://docs.ansible.com/ansible/latest/dev_guide/developing_program_flow_modules.html#argument-spec-dependencies

instance_initiated_shutdown_behavior=dict(type='str', choices=['stop', 'terminate']),
termination_protection=dict(type='bool'),
hibernation_options=dict(type='bool', default=False),
Expand Down