Transition to inventory plugins #3266
Conversation
| @@ -1797,3 +1815,651 @@ class Meta: | |||
|
|
|||
| def get_absolute_url(self, request=None): | |||
| return reverse('api:inventory_script_detail', kwargs={'pk': self.pk}, request=request) | |||
|
|
|||
|
|
|||
| # TODO: move to awx/main/models/inventory/injectors.py | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| @@ -57,15 +57,13 @@ | |||
| import sys | |||
There was a problem hiding this comment.
I picked up this dependency update as I was doing the rest of the work.
The inventory plugin needs openstacksdk, and I just had problems having that work with the old dependencies of the script before the rename, so I just gave up and upgraded it so they're both on the new libraries.
There was a problem hiding this comment.
shade is dead, long live shade
There was a problem hiding this comment.
I could swear I remember removing shade from requirements. Maybe it got added back in due to a rebase, and I gave up.
Do we still need shade? Can we make an issue to get rid of it?
| f.close() | ||
| os.chmod(path, stat.S_IRUSR | stat.S_IWUSR) | ||
| env['GCE_CREDENTIALS_FILE_PATH'] = path | ||
| return path |
There was a problem hiding this comment.
I added a return value here, and this is what I use in the inventory source injector to reference the file name.
| # Also do not send websocket status updates | ||
| with mock.patch.object(UnifiedJob, 'websocket_emit_status', mock.Mock()): | ||
| # The point of this test is that we replace run_pexpect with assertions | ||
| with mock.patch('awx.main.expect.run.run_pexpect', substitute_run): |
There was a problem hiding this comment.
You don't have to quadruple-nest these context managers:
https://docs.python.org/3/library/contextlib.html#contextlib.nested
There was a problem hiding this comment.
There's a lot of unlearning I have to do from making things python2 compatible. But for these, I also don't like formatting the indents before the :. I just can't make it look good no matter what I do.
There was a problem hiding this comment.
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
| handle, path = tempfile.mkstemp(dir=private_data_dir) | ||
| f = os.fdopen(handle, 'w') | ||
| json.dump(json_cred, f) | ||
| json.dump(json_cred, f, indent=2) |
There was a problem hiding this comment.
yes, these kinds of tests are new, so looking at the files wasn't really a thing before.
| if len(client) and len(tenant): | ||
| env['AZURE_CLIENT_ID'] = client | ||
| env['AZURE_TENANT'] = tenant | ||
| env['AZURE_SECRET'] = cred.get_input('secret', default='') | ||
| env['AZURE_SUBSCRIPTION_ID'] = cred.get_input('subscription', default='') |
|
|
||
|
|
||
| for cls in PluginFileInjector.__subclasses__(): | ||
| InventorySourceOptions.injectors[cls.__name__] = cls |
There was a problem hiding this comment.
I really like this pattern. Long term, I think an awesome thing to do here would be to unit test these injection implementations and lose all the crazy mock-based tests in tests/unit/test_tasks.
There was a problem hiding this comment.
I really like this pattern.
well, it is a pattern copied from credentials. The semantics can be implemented several ways, but that's easy to change. For native credential types you added them to the tracking dict in __init__ method (the PR you just merged), which might be the only other option I liked better than the __subclasses__ method here.
I think an awesome thing to do here would be to unit test these injection implementations and lose all the crazy mock-based tests in
tests/unit/test_tasks.
I've been talking with @chrismeyersfsu a lot about the build_private_data and build_private_data_files methods. The way we pass data between the two is complicated and confusing, but I really like the idea of first collecting what we're going to write to disk, and then having a final step where we create the directory & files. In both tests_tasks.py and the new content here, we have to write things to the filesystem, then pull them back out, rearrange them, and make assertions. Having the boundary between generating the content and writing it to disk could make that much more enjoyable.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
AlanCoding
force-pushed
the
inventory_plugins
branch
from
f62cf8e
to
94388eb
Compare
|
@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. |
AlanCoding
force-pushed
the
inventory_plugins
branch
from
51e8022
to
eafb9ca
Compare
There was a problem hiding this comment.
As I read this, openstack will start using the plugin if ansible 2.7.8+ is present, and none of the other ones will run no matter what because they do not have an initial_version defined.
I'm "ok" with that, but I guess what I'm getting at with this flag idea is that I'd like the option to try out the plugin if I reaaaaly want to, but for it always to use the script otherwise
| @property | ||
| def filename(self): | ||
| """Inventory filename for using the inventory plugin | ||
| This is created dynamically, but the auto plugin requires this exact naming |
| return bool( | ||
| self.plugin_name and self.initial_version and | ||
| Version(self.ansible_version) >= Version(self.initial_version) | ||
| ) |
There was a problem hiding this comment.
@ryanpetrello This is where I am wondering if we can add some other flag to let us use the plugin, and set the initial version to 2.9 on these things, since ansible 2.8 will be coming out soon
There was a problem hiding this comment.
I would really like to get 2.7 for some of the major sources. In particular, right now we have a valid shot at gce, we just need one image variable, and people have been making shots at this (still nothing working quite yet, but we know the general API endpoints that new requests have to be added for).
But core has a rule that features are not backported. That makes me think that 2.8 might be the furthest back we can get. That could apply to both the gce image variable and the group naming issue (or neither of them, I can't predict).
If we're going to push back any major source to 2.9, we need a good reason for doing so, and resources dedicated to doing it. And I would still be rather unhappy about it.
awx/main/models/inventory.py
Outdated
| # FIXME: put in correct version when Ansible core fixes are in | ||
| # 2.8 does not have all needed hostvars https://github.com/ansible/ansible/issues/51864 | ||
| # also affected by unsafe group names issue | ||
| # initial_version = '2.8' |
There was a problem hiding this comment.
so right now it does not have an initial version --> that means that should_use_plugin() will always returnFalse at this point, correct?
There was a problem hiding this comment.
yes.
So all plugin logic like that is non-functional now - they are only used by the unit tests. But we're still talking about the flag for enablement, which could change that.
ok, sorry, slow on the uptake. |
| private_ip: private_ipv4_addresses | json_query("[0]") | ||
| provisioning_state: provisioning_state | title | ||
| type: resource_type | ||
| keyed_groups: |
There was a problem hiding this comment.
In general, for compatibility keyed_groups and vars, it would be good to have both behind a "compatibility with prior inventory scripts" flag that is set at a per-source level, which then causes this compatibility info to be templated into the plugin config.
Yes, it's more work, but it allows us to migrate off of them later in a sane way.
There was a problem hiding this comment.
That's where we're trying to go, yes. The question for me isn't "if", but "when".
What you're suggesting is going to result in substantial UI impact and rework. I kind of have an idea of how they might do this, but there could be some significant back-and-fourth with the API, and we would need to meet and start talking about design.
If you look at the foreman plugin, I made a PR to Ansible so that the credential secrets can be provided from environment variables. By doing this, I'm pushing all of the secret information out of the inventory file, so that I can (at a later point), surface this inventory file through the API, so that we can give it to the user in the way that you're talking about.
There was a problem hiding this comment.
How is this substantial UI work? It's a checkbox, on by default, that does nothing when using a script, and causes a "{{ ... }}" section to be dumped into the plugin configuration yml when set.
There was a problem hiding this comment.
Because there is no UI element for the inventory plugin configuration now, and if they added a box for this, they would need to hide / disable a bunch of other fields when it becomes active.
requirements/requirements_ansible.in
Outdated
| @@ -32,6 +32,7 @@ azure-graphrbac==0.40.0 | |||
| # AWS | |||
| boto==2.47.0 # last which does not break ec2 scripts | |||
| boto3==1.6.2 | |||
| google-auth==1.6.2 # needed for gce inventory imports | |||
There was a problem hiding this comment.
comment in same format as others, if possible
AlanCoding
force-pushed
the
inventory_plugins
branch
from
9214f52
to
b2a74e1
Compare
|
@ryanpetrello @AlanCoding one rub for me is I want to try out using the plugins but I can only do this by hacking I wonder if we can go ahead and define the initial version as Open to other solutions, but do need some way to turn these on if we are going to use them |
|
For the ec2 plugin to plausibly pass testing, we need:
merged into Ansible Openstack is the same status is ec2 Azure needs ansible/ansible#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
force-pushed
the
inventory_plugins
branch
2 times, most recently
from
d54af00
to
a47f644
Compare
AlanCoding
changed the title
|
Build succeeded (gate pipeline).
|
|
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. |
|
regate |
supporting and related changes - Fix inconsistency between can_update / can_start - Avoid creating inventory file twice unnecessarily - Non-functional consolidation in Azure injection logic - Inject GCE creds as indented JSON for readability - Create new injector class structure, add gce - Reduce management command overrides of runtime environment
This new batch of tests assures that the injector logic for inventory source in their old script version remains untouched with the refactoring underway. Plugins are also tested by the same means of comparing to reference files, these will be used to assure that all parameters that used to be respected are still respected in the plugin system.
Initialize some inventory plugin test data files Implement openstack inventory plugin This may be removed later: - port non-JSON line strip method from core Dupliate effort with AWX mainline devel - Produce ansible_version related to venv Refactor some of injector management, moving more of this overhead into tasks.py, when it comes to managing injector kwargs Upgrade and move openstack inventory script sync up parameters Add extremely detailed logic to inventory file creation for ec2, Azure, and gce so that they are closer to a genuine superset of what the contrib script used to give.
Bump keystone auth to resolve problem with openstack script Clarify code path, routing to template vs. managed injector behavior is also now reflected in test data files Refactor test data layout for inventory injector logic Add developer docs for inventory plugins transition Memoize only get_ansible_version with no parameters Make inventory plugin injector enablement a separate concept from the initial_version switch tests to look for plugin_name as well Add plugin injectors for tower and foreman. Add jinja2 native types compat feature move tower source license compare logic to management command introduce inventory source compat mode pin jinja2 for native Ansible types Add parent group keys, and additional translations manual dash sanitization for un-region-like ec2 groups nest zones under regions using Ansible core feature just merged implement conditionally only with BOTH group_by options Make compat mode default be true in API models, UI add and edit controllers Add several additional hostvars to translation Add Azure tags null case translation Make Azure group_by key off source_vars to be consistent with the script support top-level ec2 boto_profile setting
Disable use of azure_rm inventory plugin Disable use of ec2 inventory plugin due to compatibility issues that are unresolved Fix conflicts with ansible runner integration Add additional content enabled by Ansible core changes
fix minor bug where credential not shown in API
AlanCoding
force-pushed
the
inventory_plugins
branch
from
45189f4
to
d39b3b3
Compare
|
Build succeeded.
|
|
recheck |
|
Build succeeded (gate pipeline).
|
|
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. |
|
Build succeeded.
|
|
Build succeeded (gate pipeline).
|
SUMMARY
Connect #171
ISSUE TYPE
COMPONENT NAME
AWX VERSION
(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:
ansible/ansible#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.