Repository: ansible/ansible
Pull request: ec2: fixes #19521, fixes #29456 - create instance-store AMI instances with correct shutdown behavior
##### SUMMARY
Termination is the required shutdown behavior of instance-store AMIs. The ec2 module option for the shutdown behavior defaults to 'stop' so I added a try/except to account for non-EBS-backed images and allow the default to be corrected. Added a note to the module option. Fixes #19521

##### ISSUE TYPE
 - Bugfix Pull Request

##### COMPONENT NAME
lib/ansible/modules/cloud/amazon/ec2.py

##### ANSIBLE VERSION
```
2.4.0
```


Problem statement:
ec2 module: instance_initiated_shutdown_behavior is invalid for instance-store backed AMIs
<!---
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release, and master branch are affected too.
-->

##### ISSUE TYPE
<!--- Pick one below and delete the rest: -->
 - Bug Report

##### COMPONENT NAME
ec2

##### ANSIBLE VERSION
<!--- Paste verbatim output from “ansible --version” between quotes below -->
```
ansible 2.2.0.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/vlg/ansible-playbooks/common/library']
```

##### CONFIGURATION
<!---
Mention any settings you have changed/added/removed in ansible.cfg
(or using the ANSIBLE_* environment variables).
-->
N/A
##### OS / ENVIRONMENT
<!---
Mention the OS you are running Ansible from, and the OS you are
managing, or say “N/A” for anything that is not platform-specific.
-->
N/A
##### SUMMARY
<!--- Explain the problem briefly -->
After upgrading Ansible to 2.2.0 our playbooks fatal out due to AWS rejecting the setting of a flag on certain AMI types (instance-store backed AMIs)

##### STEPS TO REPRODUCE
<!---
For bugs, show exactly how to reproduce the problem, using a minimal test-case.
For new features, show how the feature would be used.
-->

<!--- Paste example playbooks or commands between quotes below -->
```yaml
  - name: Spinning up 2 '{{ app }}' servers
    ec2:
      key_name: keynameishere
      instance_type: c3.xlarge
      image: '{{ image }}'
      wait: yes
      group_id: '{{ group }}'
      vpc_subnet_id: '{{ subnet }}'
      region: us-east-1
      count: 2
      user_data: |
                 #!/bin/bash
                 bootstrap script is here (redacted)
      assign_public_ip: '{{ use_public_ip }}'
      instance_profile_name: '{{ instance_profile }}'
      instance_tags:
        Platform: 'PlatformName'
        Environment: '{{ env }}'
        Application: '{{ app_type }}'
        Canary: 'Yes'
    register: ec2
```
relevant group_vars
```yaml
dl_image: ami-amiID
dl_group: sg-groupid
dl_subnet: subnet-subnetid
dl_elb: elbid
dl_instance_profile: IAMinstanceprofile
dl_use_public_ip: no
```
<!--- You can also paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- What did you expect to happen when running the steps above? -->
Instance should be spun up.

##### ACTUAL RESULTS
<!--- What actually happened? If possible run with extra verbosity (-vvvv) -->

Instance spin up fails because current Amazon Linux AMI is instance-store rather than EBS backed. instance_initiated_shutdown_behavior default value of 'stop' is invalid for an instance-store backed AMI, which we are using. 

<!--- Paste verbatim command output between quotes below -->
```
build	19-Dec-2016 10:14:25	fatal: [localhost]: FAILED! => {"changed": false, "failed": true, "msg": "Instance creation failed => InvalidParameterCombination: The attribute instanceInitiatedShutdownBehavior can only be used for EBS-backed images."}
```

Hints from issue discussion:
+1   Is there a workaround for this issue, a flag I can explicitly set to get past the error?
Actually found in the docs and module: instance_initiated_shutdown_behavior is the param - default is stop. I think terminate is the one we want for instance-store instances
Much appreciated, looks like it was added in 2.2 according to docs, which is why I didn't see it in there. Updated and working now.
Unfortunately I definitely don't want terminate as I don't want those instances deleted when they're shutdown.
@rentiak If I'm not mistaken, you can stop the instance, but any data on an "instance-store" is erased.  If you need your volume data to persist, your only option is an EBS based instance.  http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Stop_Start.html
@oucil You're right about instance store destroying the ephemeral storage, however that variable is not talking about terminating storage on stop. It appears to specifically be whether the instance itself should be terminated on stop

Module doc: "Set whether AWS will Stop or Terminate an instance on shutdown"

AWS doc: "By default, when you initiate a shutdown from an Amazon EBS-backed instance (using a command such as shutdown, halt, or poweroff), the instance stops. You can change this behavior using the InstanceInitiatedShutdownBehavior attribute for the instance so that it terminates instead."
@rentiak Our use case is as a simple "dumb" app server which is part of an ASG anyway so termination is the natural fit.  I'm coming up short thinking of a situation where you want to keep an instance in a "stopped" state that loses it's storage, more so since I'm assuming you've bootstrapped it in Ansible anyway. Not saying there isn't a case of course, just drawing a blank.  Are you running your OS on EBS and using the instance-store volume for something else then?
@rentiak - the option is only for for the root volume itself. For instance-store root instances you cannot actually "stop" them anyways via AWS (take a look in the console) - you can only terminate or reboot an instance-store backed root instance.

So it makes sense for the default option to be shutdown (takes care of EBS root instances) and the other option which is available in 2.2 to set terminate (which you can set for the above scenario).
That's valid. I guess then the docs probably need to be updated to have instance_initiated_shutdown_behavior marked as required for instance store-backed instances. It would be nice if it was more intelligent and handled that, which is documented here: https://github.com/ansible/ansible-modules-core/issues/5759
cc @lwade @tgerla
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: Create instance-store AMI instances with 'terminate' as the shutdown behavior since it is required.

Patch to review:
diff --git a/lib/ansible/modules/cloud/amazon/ec2.py b/lib/ansible/modules/cloud/amazon/ec2.py
index b64abd3a46ea80..7aa7983776ea99 100644
--- a/lib/ansible/modules/cloud/amazon/ec2.py
+++ b/lib/ansible/modules/cloud/amazon/ec2.py
@@ -227,7 +227,8 @@
   instance_initiated_shutdown_behavior:
     version_added: "2.2"
     description:
-    - Set whether AWS will Stop or Terminate an instance on shutdown
+    - Set whether AWS will Stop or Terminate an instance on shutdown. This parameter is ignored when using instance-store
+      images (which require termination on shutdown).
     required: false
     default: 'stop'
     choices: [ "stop", "terminate" ]
@@ -1193,7 +1194,15 @@ def create_instances(module, ec2, vpc, override_count=None):
                 # (the default) or 'terminate' here.
                 params['instance_initiated_shutdown_behavior'] = instance_initiated_shutdown_behavior or 'stop'
 
-                res = ec2.run_instances(**params)
+                try:
+                    res = ec2.run_instances(**params)
+                except boto.exception.EC2ResponseError as e:
+                    if "The attribute instanceInitiatedShutdownBehavior can only be used for EBS-backed images." in e.message:
+                        params['instance_initiated_shutdown_behavior'] = 'terminate'
+                        res = ec2.run_instances(**params)
+                    else:
+                        raise
+
                 instids = [i.id for i in res.instances]
                 while True:
                     try:


Code review comments:
diff_hunk: @@ -1193,7 +1194,15 @@ def create_instances(module, ec2, vpc, override_count=None):
                 # (the default) or 'terminate' here.
                 params['instance_initiated_shutdown_behavior'] = instance_initiated_shutdown_behavior or 'stop'
 
-                res = ec2.run_instances(**params)
+                try:
+                    res = ec2.run_instances(**params)
+                except boto.exception.EC2ResponseError as e:
+                    if "The attribute instanceInitiatedShutdownBehavior can only be used for EBS-backed images." in e.message:
path: lib/ansible/modules/cloud/amazon/ec2.py
text: @user1:
See if it's possible to use `e.error_code` instead of matching on the message.