search

ansible / awx

AWX provides a web-based user interface, REST API, and task engine built on top of Ansible. It is one of the upstream projects for Red Hat Ansible Automation Platform.
Other
13.54k stars 3.35k forks source link

Transition to inventory plugins #3266

Closed AlanCoding closed 5 years ago

AlanCoding commented 5 years ago
SUMMARY

Connect https://github.com/ansible/awx/issues/171

ISSUE TYPE
COMPONENT NAME
AWX VERSION
3.0.1

(I assumed this would be released in 4.0.0, but could be wrong)

ADDITIONAL INFORMATION

This only runs openstack under the inventory plugin.

The "big 3" of ec2, gce, and azure are kind of in a holding pattern right now. We have issues:

Right now those are deemed to be blockers. There's also another issue:

https://github.com/ansible/ansible/pull/50035

This is a common blocker for more than 1 of them.

There's a lot more baggage that I'm sure to be forgetting right now.

With this PR...

A notable omission is that we discussed a flag for the user to force using or not using the plugin. Such a flag is not implemented here, because I don't quite know what it will look like, and it won't be completely trivial in implementation.

So right now, you control whether the plugin is used by the version of Ansible in your custom venv, and that's it.

The tests, and the test data, here are brutal, but there's a reason behind those. If I'm showing 10 keyed_groups in the list, that's because we don't have a "group_by" filter, which was a pre-existing shortcoming. If you don't like seeing all those in the test data, then you shouldn't like that we load way more groups than what the user would intentionally choose to include. For the compose variables, this will always be our most comprehensive library of compatibility parameters.

Some testing is duplicated with awx/main/tests/unit/test_tasks.py, and we could reduce that.

AlanCoding commented 5 years ago

Instead of addressing this inside this PR, I wanted to see if I could take on this change via a rebase:

https://github.com/ansible/awx/pull/3275

If not, I could do cache.clear() before running any of these new tests.

ryanpetrello commented 5 years ago

Instead of addressing this inside this PR, I wanted to see if I could take on this change via a rebase:

I'm fine with that if it addresses your test failures ✅

AlanCoding commented 5 years ago

@kdelee you might want to take a look at my last commit, and let me talk out what I'm doing here.

I have added plugin injectors for the tower and satelitte6 cases. Both of these need some extra work, but basically we have a switch which we can switch to turn on. The tests show that the code is running and I produce the files shown in the test data. So the question then comes down to "will an import ran with these files run?"

I haven't yet put in a parameter to force it to use the plugin. Even if we did that, I think we would have to disallow using tower, due to some weird reasons.

kdelee commented 5 years ago

I haven't yet put in a parameter to force it to use the plugin. Even if we did that, I think we would have to disallow using tower, due to some weird reasons.

ok, sorry, slow on the uptake. should_use_plugin is the switch, but we still need to decide on user-set flag

kdelee commented 5 years ago

@ryanpetrello @AlanCoding one rub for me is I want to try out using the plugins but I can only do this by hacking should_use_plugin locally. Need some nob to turn.

I wonder if we can go ahead and define the initial version as 2.8 on all ones that are "usable-ish" e.g. do not explode in flames, and define some setting like settings.ENABLE_ANSIBLE_INVENTORY_PLUGINS that will default to True, which would give people a way out for the time being if the roof does fall on their head, but help us scare up some dragons by default if folks are using 2.8.

Open to other solutions, but do need some way to turn these on if we are going to use them

AlanCoding commented 5 years ago

For the ec2 plugin to plausibly pass testing, we need:

merged into Ansible devel branch. Both of these require a corresponding change in this PR. I am not 100% sure that the contract for those has stabilized.

Openstack is the same status is ec2

Azure needs https://github.com/ansible/ansible/pull/53046, in addition to the 2 above.

gce has no real viable work-in-progress yet, but have a plausible mechanism, but it needs some serious work.

Since we are sure that are going to work in Ansible 2.8, then we can put that version in here? Then if you run with Ansible devel, you'll have the plugins running.

I don't want to jump into a new toggle for this, because we're just going to be chasing our tails. It is on the agenda to introduce a "compat mode" toggle, and this is totally separate from the issue of enablement of plugins.

AlanCoding commented 5 years ago

screen shot 2019-02-28 at 2 10 04 pm

@mabashian @wenottingham

wenottingham commented 5 years ago

Should that be 'host varaibles and host groups"?

AlanCoding commented 5 years ago

Well, technically yes, but given scrutiny I would rather just say "it's different, read the docs".

Groups come from hostvars, but that's actually not a very fair defense of the wording, because although groups derive from different hostvars they should have the same value. As a completely separate item, novel sanitization of group names is being done in the inventory plugins, which you will get and should get if you turn off compatibility mode. So yes, group names will change when you turn that off for this reason.

wenottingham commented 5 years ago

But we are also creating groups as a compatibility item , as there are no groups out of most plugins out of the box.

AlanCoding commented 5 years ago

But we are also creating groups as a compatibility item , as there are no groups out of most plugins out of the box.

I don't know if you found this out by reading the stuff in the docs/ folder, but you are 100% correct.

I don't yet know how I would word that message. If anyone wants to give me text I'll put it in. We should seek to be very non-comital. One thing @kdelee mentioned is that we should suggest setting overwrite to true. If not, https://github.com/ansible/awx/issues/3141 will be very hazardous.

I want to do #3141 (or get someone else to), but I need this finished first.

AlanCoding commented 5 years ago

recheck

softwarefactory-project-zuul[bot] commented 5 years ago

Build succeeded.

AlanCoding commented 5 years ago

Also can do something like move it up to the decorator: https://github.com/ansible/ansible-runner/blob/33c7c7689388c9c3c0f68a81c298dcc08fbc1dcf/test/unit/test_runner_config.py#L410-L413

Would you prefer that?

I didn't love the multi-line hanging indent, but I'd rather go with @ryanpetrello's suggestion of with A, B, C: than the decorators

AlanCoding commented 5 years ago

rebase will be coming soon, the history is going to get pretty messy.

rooftopcellist commented 5 years ago

recheck

AlanCoding commented 5 years ago

Current pop-up help text is

If checked, additional duplicate host variables will be added to obtain compatibility with the old inventory scripts. When not checked and running as inventory plugins, only modern variable names will be used.

Trying to address comments...

If checked, configuration options are used to return content similar to deprecated inventory scripts. When running Ansible 2.8 or higher, inventory plugins may be used. Inventory plugins name host, groups, and hostvars differently and no group-by options are enabled by default.

There is a universe of details hinted at in those words, but these words are not wrong.

You might say that relationships are different, but I've detailed these in painful detail with ongoing patches in core, and from a user-oriented perspective I think we can call it a solved problem, and the added connections are still provided with compat mode off if group_by is on.

I'd love to see if I can get approval or alternative suggestions for this @wenottingham @tvo318 @kdelee

AlanCoding commented 5 years ago

revised text:

If checked, configuration options are used to return content similar to deprecated inventory scripts. Using compatibility mode allows you to continue using the legacy names of hosts, groups, and host variables. Refer to Ansible Tower User Guide for more details.

softwarefactory-project-zuul[bot] commented 5 years ago

Build succeeded.

softwarefactory-project-zuul[bot] commented 5 years ago

Build succeeded.

AlanCoding commented 5 years ago

@wenottingham @tvo318 @matburt

My last set of proposed revisions to the tooltip were here:

https://github.com/ansible/awx/pull/3266#issuecomment-473305292

Outdated conversation here

Based on feedback for the docs, a functional change was made to not do any different group_by behavior. By making that change, we allow ourselves to say that compat mode is only about naming of things.

Revised proposed tooltip text

If checked, hostvars, groups, and some hosts are returned with legacy names. The hostvars will have content duplicated, and groups can contain special characters with this enabled.

New naming of hostvars and hosts is due to transitioning to inventory plugins, and new naming of groups is due to name sanitization.

Refer to Ansible Tower User Guide for more details.[insert link]

kdelee commented 5 years ago

Newest proposal for the name of the "compatiblity_mode"

@tvo318 suggested compatibility_naming=contemporary/legacy would be drop down choice, may need devel + qe from @marshmalien and @dsesami

wenottingham commented 5 years ago

Based on feedback for the docs, a functional change was made to not do any different group_by behavior. By making that change, we allow ourselves to say that compat mode is only about naming of things.

Can you rephrase this?

compatibility_naming=contemporary/legacy would be drop down choice, may need devel + qe from @marshmalien and @dsesami

Wait, why would we ever expose multi-choice knobs here?

AlanCoding commented 5 years ago

Wait, why would we ever expose multi-choice knobs here?

I read this as more of a UI-centered argument, that it's easier for the user to understand looking at the word "legacy" than it is to look at the word "true". It's possible that the API could do some sort of a mapped choice as well.

Can you rephrase this?

This is concerned with the case of blank group_by field, in other words, group_by=""

In the current form of this PR, you get the same number of groups with or without compatibility mode on (although they can be named differently). If that sounds reasonable to you, then we are good.

wenottingham commented 5 years ago

I read this as more of a UI-centered argument, that it's easier for the user to understand looking at the word "legacy" than it is to look at the word "true". It's possible that the API could do some sort of a mapped choice as well.

If the choice is surfaced in the UI as "enable compatible groups and variables" (or similar), I think that's reasonably clear, and an clear expansion of an API "compatibility_mode" boolean. Tooltip would just be "Replicate groups and variables from legacy inventory scripts from "

wenottingham commented 5 years ago

In the current form of this PR, you get the same number of groups with or without compatibility mode on (although they can be named differently). If that sounds reasonable to you, then we are good.

Just to be explicit as possible:

Is this a correct understanding?

AlanCoding commented 5 years ago

@wenottingham

no group_by: We produce the same set of functional groups that we had previously. When compatibility_mode=False, the groups may have slightly different names (in terms of keys), and the names will be sanitized. When compatibility_mode=True, it will replicate as much as possible the prior group structure verbatim.

Correct.

AlanCoding commented 5 years ago

@s-hertel @maxamillion The last commit I pushed is intended to address the feedback you gave me about the syntax. Thank you for that. In the related PR https://github.com/ansible/ansible/pull/53435 I see bracket notion used, but from discussing I got that using dict.subdict would be better. I'm pretty much agnostic to that now, and the latter does actually make the quotes easier to manage.

I've finished running this through my testing, so I'm happy with it now.

softwarefactory-project-zuul[bot] commented 5 years ago

Build succeeded.

AlanCoding commented 5 years ago

Rebased branch because of conflict introduced by https://github.com/ansible/awx/pull/3481

AlanCoding commented 5 years ago

Sorry for the noise, I rebased just to reduce the number of commits from the last series of changes.

softwarefactory-project-zuul[bot] commented 5 years ago

Build succeeded.

softwarefactory-project-zuul[bot] commented 5 years ago

Build succeeded (gate pipeline).

softwarefactory-project-zuul[bot] commented 5 years ago

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

ryanpetrello commented 5 years ago

regate

softwarefactory-project-zuul[bot] commented 5 years ago

Build succeeded.

AlanCoding commented 5 years ago

recheck

softwarefactory-project-zuul[bot] commented 5 years ago

Build succeeded (gate pipeline).

softwarefactory-project-zuul[bot] commented 5 years ago

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

softwarefactory-project-zuul[bot] commented 5 years ago

Build succeeded.

softwarefactory-project-zuul[bot] commented 5 years ago

Build succeeded (gate pipeline).