Improve state and config changes in ec2_instance #550
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| # Test that configuration changes, like security groups and instance attributes, | ||
| # are updated correctly when the instance has different states, and also when | ||
| # changing the state of an instance. | ||
| # https://github.com/ansible-collections/community.aws/issues/16 | ||
| - block: | ||
| - name: "Make instance with sg and termination protection enabled" | ||
| ec2_instance: | ||
| state: present | ||
| name: "{{ resource_prefix }}-test-state-param-changes" | ||
| image_id: "{{ ec2_ami_image }}" | ||
| tags: | ||
| TestId: "{{ ec2_instance_tag_TestId }}" | ||
| security_groups: "{{ sg.group_id }}" | ||
| vpc_subnet_id: "{{ testing_subnet_b.subnet.id }}" | ||
| termination_protection: False | ||
| instance_type: "{{ ec2_instance_type }}" | ||
| wait: True | ||
| register: create_result | ||
|
|
||
| - assert: | ||
| that: | ||
| - create_result is not failed | ||
| - create_result.changed | ||
| - '"instances" in create_result' | ||
| - '"instance_ids" in create_result' | ||
| - '"spec" in create_result' | ||
| - create_result.instances[0].security_groups[0].group_id == "{{ sg.group_id }}" | ||
| - create_result.spec.DisableApiTermination == False | ||
|
|
||
| - name: "Change sg and termination protection while instance is in state running" | ||
| ec2_instance: | ||
| state: running | ||
| name: "{{ resource_prefix }}-test-state-param-changes" | ||
| image_id: "{{ ec2_ami_image }}" | ||
| tags: | ||
| TestId: "{{ ec2_instance_tag_TestId }}" | ||
| security_groups: "{{ sg2.group_id }}" | ||
| vpc_subnet_id: "{{ testing_subnet_b.subnet.id }}" | ||
| termination_protection: True | ||
| instance_type: "{{ ec2_instance_type }}" | ||
| register: change_params_result | ||
|
|
||
| - assert: | ||
| that: | ||
| - change_params_result is not failed | ||
| - change_params_result.changed | ||
| - '"instances" in change_params_result' | ||
| - '"instance_ids" in change_params_result' | ||
| - '"changes" in change_params_result' | ||
| - change_params_result.instances[0].security_groups[0].group_id == "{{ sg2.group_id }}" | ||
| - change_params_result.changes[0].DisableApiTermination.Value == True | ||
| - change_params_result.changes[1].Groups[0] == "{{ sg2.group_id }}" # TODO fix this to be less fragile | ||
|
|
||
|
|
||
| - name: "Change instance state from running to stopped, and change sg and termination protection" | ||
| ec2_instance: | ||
| state: stopped | ||
| name: "{{ resource_prefix }}-test-state-param-changes" | ||
| image_id: "{{ ec2_ami_image }}" | ||
| tags: | ||
| TestId: "{{ ec2_instance_tag_TestId }}" | ||
| security_groups: "{{ sg.group_id }}" | ||
| vpc_subnet_id: "{{ testing_subnet_b.subnet.id }}" | ||
| termination_protection: False | ||
| instance_type: "{{ ec2_instance_type }}" | ||
| register: change_state_params_result | ||
|
|
||
| - assert: | ||
| that: | ||
| - change_state_params_result is not failed | ||
| - change_state_params_result.changed | ||
| - '"instances" in change_state_params_result' | ||
| - '"instance_ids" in change_state_params_result' | ||
| - '"changes" in change_state_params_result' | ||
| - '"stop_success" in change_state_params_result' | ||
| - '"stop_failed" in change_state_params_result' | ||
| - change_state_params_result.instances[0].security_groups[0].group_id == "{{ sg.group_id }}" | ||
| - change_state_params_result.changes[0].DisableApiTermination.Value == False | ||
|
|
||
| - name: "Change sg and termination protection while instance is in state stopped" | ||
| ec2_instance: | ||
| state: stopped | ||
| name: "{{ resource_prefix }}-test-state-param-changes" | ||
| image_id: "{{ ec2_ami_image }}" | ||
| tags: | ||
| TestId: "{{ ec2_instance_tag_TestId }}" | ||
| security_groups: "{{ sg2.group_id }}" | ||
| vpc_subnet_id: "{{ testing_subnet_b.subnet.id }}" | ||
| termination_protection: True | ||
| instance_type: "{{ ec2_instance_type }}" | ||
| register: change_params_stopped_result | ||
|
|
||
| - assert: | ||
| that: | ||
| - change_params_stopped_result is not failed | ||
| - change_params_stopped_result.changed | ||
| - '"instances" in change_params_stopped_result' | ||
| - '"instance_ids" in change_params_stopped_result' | ||
| - '"changes" in change_params_stopped_result' | ||
| - change_params_stopped_result.instances[0].security_groups[0].group_id == "{{ sg2.group_id }}" | ||
| - change_params_stopped_result.changes[0].DisableApiTermination.Value == True | ||
|
|
||
| - name: "Change instance state from stopped to running, and change sg and termination protection" | ||
| ec2_instance: | ||
| state: running | ||
| name: "{{ resource_prefix }}-test-state-param-changes" | ||
| image_id: "{{ ec2_ami_image }}" | ||
| tags: | ||
| TestId: "{{ ec2_instance_tag_TestId }}" | ||
| security_groups: "{{ sg.group_id }}" | ||
| vpc_subnet_id: "{{ testing_subnet_b.subnet.id }}" | ||
| termination_protection: False | ||
| instance_type: "{{ ec2_instance_type }}" | ||
| register: change_params_start_result | ||
|
|
||
| - assert: | ||
| that: | ||
| - change_params_start_result is not failed | ||
| - change_params_start_result.changed | ||
| - '"instances" in change_params_start_result' | ||
| - '"instance_ids" in change_params_start_result' | ||
| - '"changes" in change_params_start_result' | ||
| - '"reboot_success" in change_params_start_result' | ||
| - '"reboot_failed" in change_params_start_result' | ||
| - change_params_start_result.instances[0].security_groups[0].group_id == "{{ sg.group_id }}" | ||
| - change_params_start_result.changes[0].DisableApiTermination.Value == False | ||
|
|
||
| always: | ||
|
|
||
| - name: Set termination protection to false (so we can terminate instance) (cleanup) | ||
| ec2_instance: | ||
| filters: | ||
| tag:TestId: "{{ ec2_instance_tag_TestId }}" | ||
| termination_protection: False | ||
| ignore_errors: yes | ||
|
|
||
| - name: Terminate instance | ||
| ec2_instance: | ||
| filters: | ||
| tag:TestId: "{{ ec2_instance_tag_TestId }}" | ||
| state: absent | ||
| wait: False | ||
| ignore_errors: yes | ||
|
|
||
| - include_tasks: env_cleanup.yml |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be interesting if this behavior changed because stopping and starting an instance gives new underlying hardware, but a reboot call doesn't. That's an important distinction when there are hardware issues on the instance (if you run long running instances like I do, this is quite a common occurrence).
I hadn't realized that this is what the module did so in playbooks I have for recovering, I do actually call the module twice, once to stop it and once to start. I wonder if anyone has settled on using using the
restarted/rebootedstatuses in Ansible to recover, only to find that it won't work anymore if this changes. Probably a niche thing...But it does make me think.. a dedicated integrated "
stopthenstart" mode (whatrestartedactually does now) would be useful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My gut inclination was to change this to actually call `reboot' since it's not really doing what it advertises right now, and we'll have a narrow window before 2.0 where we can make non-backwards-compatible changes. But I don't want to overload this PR with too many changes.
Maybe @tremble can weigh in if it would be useful to make this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right, if it advertises reboot and a reboot call is available, that's what should be called. I'm only mentioning a case (albeit one that might be rare) where that change could be breaking. And that another option that explicitly does what the current behavior is might have uses. The new option value should be introduced before changing the behavior imo so that there's an overlap where people could make forward-compatible changes if they wanted to keep that behavior.
But I dunno maybe that's all overthinking.
(I also agree it's not really related to this PR)