ec2_instance should allow setting and changing PlacementGroup and PartitionNumber #1264
Conversation
|
@daniel-trnk this PR contains the following merge commits: Please rebase your branch to remove these commits. |
ansibullbot
added
feature
There was a problem hiding this comment.
Thanks for taking the time to submit this PR. Some comments inline.
It looks like our tests threw out a few errors and can be found in the logs (click on the link for each test, that'll take you to the build, from there you can click on the logs tab). Please address these.
Additionally:
- Please add some integration tests (see also tests/integration/targets/ec2_instance*) to test the code works as expected - https://github.com/ansible-collections/amazon.aws/blob/main/docs/docsite/rst/dev_guidelines.rst#integration-tests-for-aws-modules
- Please add a changelog - https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to
| @@ -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]), | |||
There was a problem hiding this comment.
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.
| partition_number: | ||
| description: | ||
| - The number of the partition within the placement group - only valid with partition typle placement groups. |
There was a problem hiding this comment.
| 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 |
| if (( desired_placement['GroupName'] != current_placement['GroupName']) or | ||
| ( 'PartitionNumber' in desired_placement and ( desired_placement['PartitionNumber'] != current_placement['PartitionNumber']))): |
There was a problem hiding this comment.
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.
| module.fail_json_aws(e, msg="Could not change placement instance {0} to {1}".format(instance['InstanceId'], | ||
| desired_placement_group,desired_partition_number)) |
There was a problem hiding this comment.
There are 3 args passed and only 2 used in the format...
| @@ -1497,6 +1502,33 @@ def change_network_attachments(instance, params): | |||
| return False | |||
|
|
|||
|
|
|||
| def change_placement(instance, params): | |||
There was a problem hiding this comment.
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.
| @@ -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]), | |||
There was a problem hiding this comment.
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
…y resolution? Error was: Provided candidate <amazon.aws:6.0.0-dev0 ... <amazon.aws:>=4.0.0 ,,,
Rename SES modules SUMMARY In line with what I understood to be the consensus on ansible-collections#881 and ansible-collections#610 Rename ses modules to remove the aws_ prefix. ISSUE TYPE Feature Pull Request COMPONENT NAME plugins/modules/aws_ses_identity.py plugins/modules/aws_ses_identity_policy.py plugins/modules/aws_ses_rule_set.py plugins/modules/ses_identity.py plugins/modules/ses_identity_policy.py plugins/modules/ses_rule_set.py ADDITIONAL INFORMATION Reviewed-by: Alina Buzachis <None>
Rename SES modules SUMMARY In line with what I understood to be the consensus on ansible-collections#881 and ansible-collections#610 Rename ses modules to remove the aws_ prefix. ISSUE TYPE Feature Pull Request COMPONENT NAME plugins/modules/aws_ses_identity.py plugins/modules/aws_ses_identity_policy.py plugins/modules/aws_ses_rule_set.py plugins/modules/ses_identity.py plugins/modules/ses_identity_policy.py plugins/modules/ses_rule_set.py ADDITIONAL INFORMATION Reviewed-by: Alina Buzachis <None>
Rename SES modules SUMMARY In line with what I understood to be the consensus on ansible-collections#881 and ansible-collections#610 Rename ses modules to remove the aws_ prefix. ISSUE TYPE Feature Pull Request COMPONENT NAME plugins/modules/aws_ses_identity.py plugins/modules/aws_ses_identity_policy.py plugins/modules/aws_ses_rule_set.py plugins/modules/ses_identity.py plugins/modules/ses_identity_policy.py plugins/modules/ses_rule_set.py ADDITIONAL INFORMATION Reviewed-by: Alina Buzachis <None>
|
#1825 handles placement group name and partition id |
|
Thanks for taking the time to open this PR. Unfortunately this PR has now bit rotted and as mentioned by @mfortin there's an alternative PR which is being actively worked on.. As such I'm going to close this PR. Please feel free to submit PRs in future. |
SUMMARY
I needed the ability to set PlacementGroup and PartitionNumber for the Placement of a new instance as well as changing them for a stopped existing instance. This is an attempt to implement that for your review.
Fixes #1263
ISSUE TYPE
COMPONENT NAME
ec2_instance
ADDITIONAL INFORMATION