Bringing cohesion to ec2_facts and EC2 inventory plugin

[Ches Martin]

Hi folks,

I dug around the list to see if anyone has raised this point before and didn't find it addressed directly, excuse me if I missed something.

Michael has stated before that it would be nice if the ec2_facts module provided tag data. I concur. And yes, it's been pointed out that the EC2 inventory plugin provides tags as hostvars—along with other facts—which is very useful, most of the time more convenient than using the ec2_tag module when you just want to fetch a value.

I am reaching an itching point with the disconnect between these two sources of EC2 facts, though. They're both officially supported but it feels obvious that they originated from different contributions. There is redundant data under different variable names. Different semantics for what might/can be in-memory. My team gets confused about what vars are coming from which source, and when they should use one versus the other. Searching the mailing list seems to suggest that it confuses others too.

Usually we prefer info from the inventory plugin when possible (primarily because of the readily available tags, as aforementioned), but it falls apart for provisioning plays, for instance:

1. Someone writes a play that launches EC2 instances.
   
2. They add a follow-up play that creates DNS records for the instances, using ec2_public_dns_name (a variable from the inventory plugin, which they've become accustomed to using).
3. It doesn't work, because this host variable isn't available in-memory yet after the instance launch play.
4. Upon a quick googling, they think an ec2_facts play before the DNS play must be the answer based on what they quickly read.
   
5. It isn't. ansible_ec2_public_hostname is the answer, and that ec2_facts play.

This sounds like a lot of incidental complexity to me.

And there's still the matter of tags. Getting tag data into memory for instances launched in the same run proves to be pretty ugly/cumbersome, currently. Wouldn't it be nice if all you needed was the ec2_facts refresh play to load new tags?

So what can we do about this? Ideally, I'd like to use the EC2 inventory plugin, and have ec2_facts work to refresh values of the same host var keys in-memory. Inherent in that is that I'd like to have consistent variable names throughout my code base. Of course some people may want to occasionally use ec2_facts without using the inventory plugin (I guess?). That should be fine, but they could work together seamlessly.

I'm willing to take a stab at working on this, but wanted to bring up whether it would be welcomed, what major concerns there may be, etc.

Thanks for reading.

-Ches

[Michael Peters]

This is something I've thought about too. The variable naming mismatch
has caused me some headaches as well as the issue of trying to load
all of the information that the EC2 dynamic inventory pulls for newly
provisioned machines. Would be nice if I could add_host and ec2_facts
without having to worry about what the vars are called.

But renaming the variables in either one would cause problems for
existing playbooks, so it would need to keep the old names around for
existing uses. This could lead to some confusion as the hostvars will
be polluted with duplicate entries and there's no real way to
encourage people to use the "right" names for the vars.

IMO the ec2.py script should be changed to match the ec2_facts module
as the latter is an official part of ansible and the former is often
referred to as a "starting point" for your own inventory scripts.

> --
> You received this message because you are subscribed to the Google Groups
> "Ansible Project" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to ansible-proje...@googlegroups.com.
> To post to this group, send email to ansible...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/ansible-project/9d5e2341-a517-4e18-9a58-f0fb3d29c747%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Ches Martin]

On Sunday, September 14, 2014 2:39:27 AM UTC+7, Michael Peters wrote:

But renaming the variables in either one would cause problems for
existing playbooks, so it would need to keep the old names around for
existing uses. This could lead to some confusion as the hostvars will
be polluted with duplicate entries and there's no real way to
encourage people to use the "right" names for the vars.

There should be a deprecation period where names to be phased out are still supported, indeed. On the bright side, maybe, is that I imagine most people vendor a copy of ec2.py, it's awkward to try to symlink it or something from the Ansible distribution on different working environments and production control hosts. So in that case, upgrading the inventory plugin is a very conscious move.

 

IMO the ec2.py script should be changed to match the ec2_facts module
as the latter is an official part of ansible and the former is often
referred to as a "starting point" for your own inventory scripts.

That's probably the way to go, given the above assertion about ec2.py being a conscious upgrade decision for most, and also that it doesn't use the ansible_ namespace convention expected of official stuff.

-Ches

[Michael DeHaan]

Random things:

- I've heard from some that the EC2 metadata service doesn't provide tag data.   This seems to be true, which means we can't really make it provide tag data.

- I'm open to expanding this module to use additional APIs.

- Of the two modules, ec2_facts is rarely used, and dynamic inventory is very widely used

- It seems best if ec2_facts conform to inventory, rather than vice versa

- We don't want to break anyone, but this may be unavoidable.  As such, I'd suggest a return_mode='v1', defaulting to something like 'v2', and making sure common code from module_utils is used so that functions from the inventory script return the same data

- I don't understand some statements in the above, like I can't parse " ansible_ec2_public_hostname is the answer, and that ec2_facts play." what that means.

It might be useful to call out specific examples other than just the tags being missing, such as which variable names you don't see standardized.

On Sat, Sep 13, 2014 at 5:28 AM, Ches Martin <ch...@nausicaamedia.com> wrote:

--

[Michael DeHaan]

IMO the ec2.py script should be changed to match the ec2_facts module
as the latter is an official part of ansible and the former is often
referred to as a "starting point" for your own inventory scripts.

No, it should not.

ec2.py is much more widely used, and both are parts of Ansible. 

We don't ship inventory scripts in the distro - but we probably should.   They are most definitely more widely utilized than the facts module.

 

To view this discussion on the web visit https://groups.google.com/d/msgid/ansible-project/CAJQqANdqu7TkyX3Vwsu7fND36r9djjHmvGa1r690yu6CP8sJUw%40mail.gmail.com.

[Ches Martin]

Thanks Michael, that's helpful, especially about giving favor to the inventory plugin. I'll try to drop in IRC when I can start hacking on it (probably a week from now), I'm sure I'll have some questions about internals like what you're saying about module_utils, but a few remarks:

On Sunday, September 14, 2014 8:35:16 PM UTC+7, Michael DeHaan wrote:

- It seems best if ec2_facts conform to inventory, rather than vice versa

It might be useful to call out specific examples other than just the tags being missing, such as which variable names you don't see standardized.

Well... basically all of them: ec2_facts uses ansible_ec2_ as a prefix, the inventory plugin uses ec2_. It's not only prefix, the public hostname thing is an example, which I'll get to in a sec. When I'm sitting at a working environment I'll gist some censored output from ec2.py --list and ansible -m ec2_facts for sake of reference in discussion. The inventory plugin has this comment FWIW:

These variables are pulled out of a boto.ec2.instance object. There is a lack of
consistency with variable spellings (camelCase and underscores) since this
just loops through all variables the object exposes. It is preferred to use the
ones with underscores when multiple exist.

Setting aside the issue of which names are kept and backwards compat for now (I think it's pretty clear that we'll want to preserve it, for awhile at least), would you prefer to standardize on ansible_ec2_ as var prefix?

- We don't want to break anyone, but this may be unavoidable.  As such, I'd suggest a return_mode='v1', defaulting to something like 'v2', and making sure common code from module_utils is used so that functions from the inventory script return the same data

I assume the return_mode refers to a new option for the facts module, that sounds reasonable. What do you think about the inventory plugin? I actually hadn't realized they weren't in the distro until you just pointed it out. I have seen, like, one blog post where someone shows an example that git clones ansible, uses the hacking setup, and symlinks ec2.py into their playbooks repo. I tend to think/hope few people do that in "real life". I have vendored a copy of the script in our repo, so upgrading Ansible wouldn't break me unexpectedly, but that's just me.

- I don't understand some statements in the above, like I can't parse " ansible_ec2_public_hostname is the answer, and that ec2_facts play." what that means.

 

Sorry, my anecdote was a little contrived, because the use case of "launch new instances and then create DNS records with their public names" can be solved just by using a register on the launch play, the necessary info is available in the result and ec2_facts isn't needed. The point I was trying to make is that the inventory plugin calls it ec2_public_dns_name and there's no convenient way to get that specific var name associated with newly launched hosts in memory during the run (okay, you could do it manually with add_host and the values from registered ec2_result.instances, but I said "convenient").

Imagine a more complex case where you add_host new instances to an already-existing group, and do more with the group later in the run. So you loop over the group and try to do hostvars[host].ec2_public_dns_name. This will break on the newly launched hosts. What will work is to run ec2_facts for the group, and then use hostvars[host].ansible_ec2_public_hostname. I hope that makes sense now.

My team favors the inventory plugin most of the time but cases like this trip them up. Particularly because we use tags a good deal, we've become conditioned to first looking to ec2.py --list to find EC2 vars to use, hence there's a bunch of ec2_public_dns_name and its ilk used throughout our playbooks. People are confused when they write a provisioning playbook and these fail. I've had to explain this discrepancy multiple times.

-Ches

[Michael DeHaan]

On Sun, Sep 14, 2014 at 12:40 PM, Ches Martin <ch...@nausicaamedia.com> wrote:

Thanks Michael, that's helpful, especially about giving favor to the inventory plugin. I'll try to drop in IRC when I can start hacking on it (probably a week from now), I'm sure I'll have some questions about internals like what you're saying about module_utils, but a few remarks:

On Sunday, September 14, 2014 8:35:16 PM UTC+7, Michael DeHaan wrote:

- It seems best if ec2_facts conform to inventory, rather than vice versa

It might be useful to call out specific examples other than just the tags being missing, such as which variable names you don't see standardized.

Well... basically all of them: ec2_facts uses ansible_ec2_ as a prefix, the inventory plugin uses ec2_. It's not only prefix, the public hostname thing is an example, which I'll get to in a sec. When I'm sitting at a working environment I'll gist some censored output from ec2.py --list and ansible -m ec2_facts for sake of reference in discussion. The inventory plugin has this comment FWIW:

These variables are pulled out of a boto.ec2.instance object. There is a lack of
consistency with variable spellings (camelCase and underscores) since this
just loops through all variables the object exposes. It is preferred to use the
ones with underscores when multiple exist.

There's a pull request to add an option to standardize the boto facts, that I believe we merged, though I agree it's a little all over the place.

Unfortunate result of what upstream is being used - and having a lot of an existing userbase - and not wanting to force a change.

I think we need to start considering it though.

Setting aside the issue of which names are kept and backwards compat for now (I think it's pretty clear that we'll want to preserve it, for awhile at least), would you prefer to standardize on ansible_ec2_ as var prefix?

I'd prefer to standardize on what the inventory script does it, since that's more widely used, but to be safe, the easiest thing may be to return *everything*.

Though I agree that's also ugly, I'm worried about the upgrade case a little.

Maybe too much.

Need to think about this some.

  

My team favors the inventory plugin most of the time but cases like this trip them up. Particularly because we use tags a good deal, we've become conditioned to first looking to ec2.py --list to find EC2 vars to use, hence there's a bunch of ec2_public_dns_name and its ilk used throughout our playbooks. People are confused when they write a provisioning playbook and these fail. I've had to explain this discrepancy multiple times.

*nod*.   That makes sense to me.

Would this be solved short term by just adding "ec2_public_dns_name" as a variable coming out of ec2_facts and ignore some of the other variable issues for a while?  

(Serious question...)