Foreman: add options to support backwards compatibility with script #67070

jladdjr · 2020-02-04T01:29:18Z

SUMMARY

Introduces two new options - legacy_groups and legacy_hostvars - to the foreman plugin. When both options are enabled, the plugin returns the same groups and hostvars returned by the foreman script.

Introducing this backwards compatibility in anticipation of awx adopting the foreman plugin (need a way to switch over to the plugin while still preserving the inventory structure that was present with the old script).

c.f. ansible/awx#3509

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

foreman inventory plugin

ADDITIONAL INFORMATION

Overview of changes:

legacy_groups:

adds support for groups based on environment, location, organization hostvars
adds support for groups based on content_facet_attributes (i.e. lifecycle_environment and content_view)

legacy_hostvars:

by default, places hostvars under foreman key
moves foreman parameters under foreman_params key
existing behavior for foreman_facts already compatible with foreman script - left this unchanged

ansibot · 2020-02-04T01:32:07Z

cc @ares @ehelms @ekohl @xprazak2
click here for bot help

ansibot · 2020-02-04T01:42:51Z

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/plugins/inventory/foreman.py:221:4: dangerous-default-value: Dangerous default value [] as argument

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

lib/ansible/plugins/inventory/foreman.py:73:161: E501: line too long (170 > 160 characters)
lib/ansible/plugins/inventory/foreman.py:78:161: E501: line too long (197 > 160 characters)
lib/ansible/plugins/inventory/foreman.py:280:17: E303: too many blank lines (2)

click here for bot help

kdelee

shipit

jladdjr · 2020-02-04T20:43:59Z

@AlanCoding, @jainnikhil30 and I have been chatting about this PR. If I understand @AlanCoding and @jainnikhil30 correctly, it sounds like they are proposing that we use keyed_groups to get backwards compatibility with groups.

Here's my attempt at using keyed_groups to create the groups found in the script:

Groups returned by the script:

Groups returned by the plugin:

Comparing the two sets of groups:

foreman_environment_production looks great ✅
foreman_location_Default_Location’s name isn’t quite right b/c of capitalization and the added underscore ❌
foreman_organization__Default_Organization has these same issues ❌

As far as I can tell, keyed_groups gets us close, but not all the way there. Does anyone know of a way for us to get backwards compatibility with groups using keyed_groups (or anything else)?

jladdjr · 2020-02-04T20:46:54Z

Backwards compatibility with groups was one part of this PR, hostvars was the other part. As far as I can tell, there isn't a way to 'construct' the hostvars in a backwards compatible way without using custom logic like I've added here.

bcoca · 2020-02-04T21:00:00Z

@jladdjr you can apply lower() to the key, jinja2 filter and python builtins are still available for you to get the same names. Also the extra underscore is the default separator, just use '' instead.

As for variables, look at the compose system, from a quick reading of the code you should be able to do the same, set the 'foreman' prefix and use the other options to get all the same parameters/variables.

jladdjr · 2020-02-13T20:50:59Z

@bcoca - awesome, thanks for the tips, giving those a shot

jladdjr · 2020-02-14T00:16:28Z

@bcoca (cc @AlanCoding) took a look at the compose system (this is the related code, afaict) and the TLDR is that I'm not seeing a way to re-create what the script returns. Details below.

To go into a bit more detail about what kind of parity issues I'm seeing..

Script hostvars look like this:

plugin hostvars look like:

The script maps all hostvars to three keys: foreman, foreman_facts, and foreman_params (only foreman is shown in my screenshot).
The plugin doesn't file hostvars underneath any keys; all the hostvars are at the root level

Looking at the compose system, I'm not seeing a way to build the top-level dictionary I mentioned:

{
  "_meta": {
    "hostvars": {
      "myhost": {
        "foreman": {        <-- need to store hostvars under here
          <hostvars>
        },
        "foreman_facts": {  <-- .. and here
          <hostvars>
        },
        "foreman_params": { <-- .. and here
          <hostvars>
        },

If I try to use something like this in foreman.yml:

compose:
  foreman:
    architecture_id: foreman_architecture_id
    architecture_name: foreman_architecture_name

.. the compose system just creates a static dictionary mirroring what I have under compose. In order to build hostvars that form a dictionary, the compose system would need to do some recursion (which I don't see present right now).

Sorry for being a bit verbose - wanted to make sure I captured the scenario I'm trying to reproduce. Am I missing something here? Is compose able to fill out a dictionary of hostvars?

jladdjr · 2020-02-14T00:54:03Z

Meanwhile.. working on using keyed groups to get parity with group names.

Tried (1) setting separator field to empty string and (2) piping keys to lower. The group names still aren't quite lining up. Digging in now to see what's up.

example of changes I'm trying out:

jladdjr · 2020-02-15T01:31:32Z

Okay, I found a way to build the foreman groups using keyed_groups:

keyed_groups:
  - key: foreman_environment_name | lower | regex_replace(' ', '') | regex_replace('[^A-Za-z0-9\_]', '_') | regex_replace('none', '')
    prefix: foreman_environment_
    separator: ""
  - key: foreman_location_name | lower | regex_replace(' ', '') | regex_replace('[^A-Za-z0-9\_]', '_')
    prefix: foreman_location_
    separator: ""
  - key: foreman_organization_name | lower | regex_replace(' ', '') | regex_replace('[^A-Za-z0-9\_]', '_')
    prefix: foreman_organization_
    separator: ""
  - key: foreman_content_facet_attributes['lifecycle_environment_name'] | lower | regex_replace(' ', '') | regex_replace('[^A-Za-z0-9\_]', '_')
    prefix: foreman_lifecycle_environment_
    separator: ""
  - key: foreman_content_facet_attributes['content_view_name'] | lower | regex_replace(' ', '') | regex_replace('[^A-Za-z0-9\_]', '_')
    prefix: foreman_content_view
    separator: ""

So we can drop the changes here that build up groups. It still seems like the hostvar related changes are needed, though.

AlanCoding · 2020-02-18T19:22:47Z

lib/ansible/plugins/inventory/foreman.py

+            - Places hostvars in a dictionary with keys `foreman`, `foreman_facts`, and `foreman_params`
+        type: boolean
+        default: False
+        version_added: '2.10'


maybe you could add the deprecated flag to this data?

You mean, we should basically say in advance something like, "in 2.13, we'll drop support for this (and officially break with the old script)" - is that what you mean?

@wenottingham - do we have a general policy about when awx can officially break ties with the old inv. scripts? Guess the answer to that may vary for each inventory source.

why would it be deprecated? is it that complex to maintain it as an option?

It's a breaking change regardless of when it happens.

@AlanCoding per @wenottingham's note, it seems like this option is something that we'll need to maintain for a while unless (1) there's some other way we plan on providing compatibility in the future (I can't think of any) or (2) we plan on actually transitioning the awx foreman plugin towards its vanilla behavior (but that would be a breaking change for awx users and I'm not sure we plan on ever doing that). In short, doesn't seem like we are planning on deprecating support for the new legacy_hostvars option.

jladdjr · 2020-02-18T21:04:09Z

@bcoca @ares @ehelms @ekohl @xprazak2 - this is ready for review again

AlanCoding · 2020-02-18T21:20:46Z

This doesn't have the changes to get the legacy groups anymore, so that should make this simpler. +1 I absolutely advocate for merging this.

jladdjr · 2020-02-19T00:09:23Z

Even if the keyed_groups seem to work for now, it's hard to be sure it's really durable.

The jinja syntax (while a bit unwieldy) follows the old script's approach to creating groups pretty closely. I'm actually feeling pretty good about using keyed_groups (but if we did end up wanting to put some custom code in the plugin to handle this, I wouldn't be opposed to that either; they both effectively do the same thing).

Here's the final version of the plugin arguments that I came up with, btw:

plugin: foreman                                                                                                                                                                                                           user: my_user
password: my_pass
url: my_server
keyed_groups:                                                                                                                                                                                                               - key: foreman['environment_name'] | lower | regex_replace(' ', '') | regex_replace('[^A-Za-z0-9\_]', '_') | regex_replace('none', '')
    prefix: foreman_environment_                                                                                                                                                                                              separator: ""
  - key: foreman['location_name'] | lower | regex_replace(' ', '') | regex_replace('[^A-Za-z0-9\_]', '_')
    prefix: foreman_location_
    separator: ""
  - key: foreman['organization_name'] | lower | regex_replace(' ', '') | regex_replace('[^A-Za-z0-9\_]', '_')
    prefix: foreman_organization_
    separator: ""
  - key: foreman['content_facet_attributes']['lifecycle_environment_name'] | lower | regex_replace(' ', '') | regex_replace('[^A-Za-z0-9\_]', '_')
    prefix: foreman_lifecycle_environment_
    separator: ""
  - key: foreman['content_facet_attributes']['content_view_name'] | lower | regex_replace(' ', '') | regex_replace('[^A-Za-z0-9\_]', '_')
    prefix: foreman_content_view_
    separator: ""
legacy_hostvars: True
want_facts: True
want_params: True

When I pair the changes made to the plugin here, with these changes in awx -- which basically mirror the above plugin config snippet -- I confirmed that I'm able build the exact same inventory in awx when I run an inventory import with the script and plugin. Think we should ship it!

jladdjr · 2020-02-21T19:53:09Z

@ares @ehelms @ekohl @xprazak2 - do you all need anything in order to review this? Believe everything here is good-to-go.

ares · 2020-02-24T10:17:21Z

@sjha4 thoughts?

jladdjr · 2020-02-27T18:42:13Z

What's needed in order to move this forward? The awx team is trying to get this in so that the foreman plugin can be adopted for Tower 3.7.

cc @ryanpetrello @blomquisg

jladdjr · 2020-02-27T18:44:27Z

ready_for_review

sjha4 · 2020-02-27T19:01:11Z

From the foreman inventory api perspective, this looks fine to me since all of the data massaging is happening post the inventory call to foreman and outside foreman. If the pre-change and post-change o/p are the same, the change looks good to me..

sjha4 · 2020-02-27T19:07:27Z

From the foreman inventory api perspective, this looks fine to me since all of the data massaging is happening post the inventory call to foreman and outside foreman. If the pre-change and post-change o/p are the same, the change looks good to me..

Well..did not realize this was the plugin script and the larger change is to move awx to use this as the source..in which case the plugin script needs to use the foreman inventory report api like the https://github.com/ansible/ansible/blob/devel/contrib/inventory/foreman.py script does..

jladdjr · 2020-02-27T20:35:17Z

the plugin script needs to use the foreman inventory report api like the https://github.com/ansible/ansible/blob/devel/contrib/inventory/foreman.py script does..

@sjahl sounds like a good idea, but not a blocker for getting this in, right? Would vote for handling that separately.

The plugin currently breaks backwards compatibility w/out this change. We can move forward on other breaking changes / things needed for parity after getting this in. Making sure hostvars have parity is a pretty fundamental first step towards overall parity.

sjha4 · 2020-02-27T22:30:57Z

@jladdjr : sounds good.. 👍

jladdjr · 2020-02-27T23:25:00Z

sweet, in that case.. motion made, seconded, ship_it?

ares · 2020-03-02T09:14:24Z

shipit

jladdjr · 2020-03-06T21:53:38Z

superseded by theforeman/foreman-ansible-modules#591

jladdjr mentioned this pull request Feb 4, 2020

RFE enable sat6/foreman inventory plugin for ansible 2.9+ ansible/awx#3509

Closed

jladdjr force-pushed the foreman_plugin_compatibility branch 2 times, most recently from aa22c3d to 8bdbbc8 Compare February 4, 2020 01:54

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 4, 2020

kdelee approved these changes Feb 4, 2020

View reviewed changes

ansibot removed the needs_triage Needs a first human triage before being processed. label Feb 4, 2020

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 12, 2020

foreman plugin: add options to support compatibility with script

2047c19

drop legacy_groups option

4861aad

jladdjr force-pushed the foreman_plugin_compatibility branch from 8bdbbc8 to 4861aad Compare February 18, 2020 01:24

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 18, 2020

AlanCoding reviewed Feb 18, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 27, 2020

jladdjr mentioned this pull request Mar 4, 2020

add legacy_hostvars option willtome/foreman-ansible-modules#2

Merged

jladdjr closed this Mar 6, 2020

ansible locked and limited conversation to collaborators Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Foreman: add options to support backwards compatibility with script #67070

Foreman: add options to support backwards compatibility with script #67070

jladdjr commented Feb 4, 2020

ansibot commented Feb 4, 2020

ansibot commented Feb 4, 2020

kdelee left a comment

jladdjr commented Feb 4, 2020 •

edited

jladdjr commented Feb 4, 2020

bcoca commented Feb 4, 2020 •

edited

jladdjr commented Feb 13, 2020

jladdjr commented Feb 14, 2020

jladdjr commented Feb 14, 2020 •

edited

jladdjr commented Feb 15, 2020

AlanCoding Feb 18, 2020

jladdjr Feb 19, 2020 •

edited

wenottingham Feb 19, 2020

jladdjr Feb 21, 2020 •

edited

This comment has been minimized.

jladdjr commented Feb 18, 2020

AlanCoding commented Feb 18, 2020

jladdjr commented Feb 19, 2020 •

edited

jladdjr commented Feb 21, 2020

ares commented Feb 24, 2020

jladdjr commented Feb 27, 2020

jladdjr commented Feb 27, 2020

sjha4 commented Feb 27, 2020

sjha4 commented Feb 27, 2020

jladdjr commented Feb 27, 2020 •

edited

sjha4 commented Feb 27, 2020

jladdjr commented Feb 27, 2020

ares commented Mar 2, 2020

jladdjr commented Mar 6, 2020

Foreman: add options to support backwards compatibility with script #67070

Foreman: add options to support backwards compatibility with script #67070

Conversation

jladdjr commented Feb 4, 2020

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibot commented Feb 4, 2020

ansibot commented Feb 4, 2020

kdelee left a comment

Choose a reason for hiding this comment

jladdjr commented Feb 4, 2020 • edited

jladdjr commented Feb 4, 2020

bcoca commented Feb 4, 2020 • edited

jladdjr commented Feb 13, 2020

jladdjr commented Feb 14, 2020

jladdjr commented Feb 14, 2020 • edited

jladdjr commented Feb 15, 2020

AlanCoding Feb 18, 2020

Choose a reason for hiding this comment

jladdjr Feb 19, 2020 • edited

Choose a reason for hiding this comment

wenottingham Feb 19, 2020

Choose a reason for hiding this comment

jladdjr Feb 21, 2020 • edited

Choose a reason for hiding this comment

This comment has been minimized.

jladdjr commented Feb 18, 2020

AlanCoding commented Feb 18, 2020

jladdjr commented Feb 19, 2020 • edited

jladdjr commented Feb 21, 2020

ares commented Feb 24, 2020

jladdjr commented Feb 27, 2020

jladdjr commented Feb 27, 2020

sjha4 commented Feb 27, 2020

sjha4 commented Feb 27, 2020

jladdjr commented Feb 27, 2020 • edited

sjha4 commented Feb 27, 2020

jladdjr commented Feb 27, 2020

ares commented Mar 2, 2020

jladdjr commented Mar 6, 2020

jladdjr commented Feb 4, 2020 •

edited

bcoca commented Feb 4, 2020 •

edited

jladdjr commented Feb 14, 2020 •

edited

jladdjr Feb 19, 2020 •

edited

jladdjr Feb 21, 2020 •

edited

jladdjr commented Feb 19, 2020 •

edited

jladdjr commented Feb 27, 2020 •

edited