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.

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

Adds termination support to the ec2 module #3293

Merged
merged 3 commits into from
Jun 30, 2013
Merged

Adds termination support to the ec2 module #3293

merged 3 commits into from
Jun 30, 2013

Conversation

jarv
Copy link
Contributor

@jarv jarv commented Jun 21, 2013

Currently using this to automate provisioning and tear-down with a jenkins job.

Pass in the instances output of the ec2 module
to terminate a list of instances that were previously provisioned.

Pass in the `instances` output of the ec2 module
to terminate a list of instances that were previously provisioned.

Useful for automated testing.
@zbal
Copy link
Contributor

zbal commented Jun 21, 2013

I was starting to implementing something similar, cool to see you already have something!

Do you think we could get with a state approach instead? state being present by default to maintain backward compatibility, but with extra values such as started/restarted/terminated/stopped.

Also could we use the existing id attribute instead of a terminate_list param?

@jarv
Copy link
Contributor Author

jarv commented Jun 21, 2013

So you think this is better?

ec2: state=absent id={{ item.id }}
with_items: {{ ec2.instances }}

I couldn't decide whether to pass in a single ID or the full ec2.instances
list (which would avoid having to add with_items)

Since the id parameter isn't used right now for instance id in the create
context I would hesitate to overload it.
Maybe "instance_id"?

-John

On Fri, Jun 21, 2013 at 10:26 AM, Vincent Viallet
notifications@github.comwrote:

I was starting to implementing something similar, cool to see you already
have something!

Do you think we could get with a state approach instead? state being
present by default to maintain backward compatibility, but with extra
values such as started/restarted/terminated/stopped.

Also could we use the existing id attribute instead of a terminate_list
param?


Reply to this email directly or view it on GitHubhttps://github.com//pull/3293#issuecomment-19818906
.

@zbal
Copy link
Contributor

zbal commented Jun 21, 2013

Well, I like the approach of the state as we find it in almost all of the modules.

For the id, I was more thinking about a comma separated list of ids to allow multi update at once (ex. id: i-12345,i-54321); moreover that would still allow your suggested approach using with_items.

I'm very confused by the use of id currently; it seems to be used only for client-token while documentation state this is the identifier for an instance or a set of instances... I'm not against moving away from that variable if it is meant for something else.

@jarv
Copy link
Contributor Author

jarv commented Jun 21, 2013

The client-token is an arbitrary string that you set that can be used for
idempotency.
If I set id to "FOO" then the next time I call it with the same id "FOO" it
won't bring up instances if they already exist with that ID.
The token has a limited lifespan however and is definitely confusing here
since it has nothing at all to do with the instance ID.

Maybe to make things a little cleaner we could add a new parameter to the
ec2 create output like "instance_list", this way to invoke the termination
handler it would be
ec2: state=absent instance_id={{ item http://item.id/ }}
with_items: {{ ec2.instance_list }}

or

module: ec2
state: absent
instance_ids: {{ec2.instance_list}}

In the end I want to make it very easy to pass in the instances for
termination from the output of the create.

-John

On Fri, Jun 21, 2013 at 10:57 AM, Vincent Viallet
notifications@github.comwrote:

Well, I like the approach of the state as we find it in almost all of the
modules.

For the id, I was more thinking about a comma separated list of ids to
allow multi update at once; moreover that would still allow your suggested
approach using with_items.

I'm very confused by the use of id currently; it seems to be used only
for client-token while documentation state this is the identifier for an
instance or a set of instances
...


Reply to this email directly or view it on GitHubhttps://github.com//pull/3293#issuecomment-19820864
.

@lwade
Copy link
Contributor

lwade commented Jun 21, 2013

+1 to this:

In the end I want to make it very easy to pass in the instances for
termination from the output of the create.

We're still talking about idempotency here in this thread so lets reference: #3299

I think this is a good solution for now and I'd go with instance_ids. I think we all agree with @zbal in that we're moving in the right direction towards a module with various idempotent options. Lots of this code going in can be re-used but ideally we do want to end up with a state parameter controlling everything, then with this terminate capability and an ability to choose the idempotency attribute.

/me starts thinking we need to have an AWS module session/breakout in IRC or at the next Ansible event. Do you think we can schedule something like this where we all meet to discuss?

@zbal
Copy link
Contributor

zbal commented Jun 21, 2013

+1 for simplicity and clarity !

I prefer the 2nd option:

module: ec2
state: absent
instance_ids: {{ec2.instance_list}}

I'm up for an AWS / Cloud modules discussion as well around idempotency and guidelines; I pushed the Digital Ocean (#3205) / Linode (#3298) platforms a few days ago, I'm pretty sure there is a whole bunch of other cloud platforms that will have to be covered in a next future.

@jarv
Copy link
Contributor Author

jarv commented Jun 21, 2013

OK I think this is what we want, @lwade @zbal look good?

Example playbook:

# Launch instances, runs some tasks
# and then terminate them


- name: Create a sandbox instance
  hosts: localhost
  gather_facts: False
  vars:
    keypair: my_keypair
    instance_type: m1.small
    security_group: my_securitygroup
    image: my_ami_id
    region: us-east-1
  tasks:
    - name: Launch instance
      local_action: ec2 keypair=$keypair group=$security_group instance_type=$instance_type image=$image wait=true region=$region
      register: ec2
    - name: Add new instance to host group
      local_action: add_host hostname=${item.public_ip} groupname=launched
      with_items: ${ec2.instances}
    - name: Wait for SSH to come up
      local_action: wait_for host=${item.public_dns_name} port=22 delay=60 timeout=320 state=started
      with_items: ${ec2.instances}

- name: Configure instance(s)
  hosts: launched
  sudo: True
  gather_facts: True
  roles:
    - my_awesome_role
    - my_awesome_test

- name: Terminate instances
  hosts: localhost
  connection: local
  tasks:
    - name: Terminate instances that were previously launched
      local_action:
        module: ec2
        state: 'absent'
        instance_ids: {{ec2.instance_ids}}


@zbal
Copy link
Contributor

zbal commented Jun 24, 2013

Looks good to me.

@lwade
Copy link
Contributor

lwade commented Jun 25, 2013

Likewise :)

On 24 June 2013 04:10, Vincent Viallet notifications@github.com wrote:

Looks good to me.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3293#issuecomment-19887110
.

@jarv
Copy link
Contributor Author

jarv commented Jun 26, 2013

@mpdehaan This is ready to merge

mpdehaan added a commit that referenced this pull request Jun 30, 2013
Adds termination support to the ec2 module
@mpdehaan mpdehaan merged commit fafb3c1 into ansible:devel Jun 30, 2013
@mpdehaan
Copy link
Contributor

merginated, thanks!

@mpdehaan
Copy link
Contributor

don't know if there is something to be resolved on #3299 relative to comments there about this, let's use that thread to discuss.

@ansible ansible locked and limited conversation to collaborators Apr 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants