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

Extended foreman inventory and cache to work with redis #52800

Open
wants to merge 8 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@FutureTecSystems
Copy link

FutureTecSystems commented Feb 22, 2019

SUMMARY

With the change from Foreman inventory script to Ansible inventory plugin we discovered enourmous problems regarding our inventory and host parameters. Things like chopped strings, wrong data types and so on. For this reason we fixed the issue using the resolving function for parameters from the old script and used some improvements of the Foreman API. After this we recognized that caching inventories only worked with jsonfile. In this first step we made the Foreman inventory cache working with redis. We want to work on this topic but this PR is only the beginning. The code is functional and tested in a big environment of about 600 machines, speeding up playbooks around 40%.

The next step we need help integrating a clean solution on this topic.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

Inventory Module
Cache Module

ADDITIONAL INFORMATION

FutureTecSystems added some commits Feb 22, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 22, 2019

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

lib/ansible/plugins/cache/__init__.py:331:33: E127 continuation line over-indented for visual indent

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Feb 22, 2019

FutureTecSystems added some commits Feb 22, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 22, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/cloud/kubevirt/kubevirt_preset.py:0:0: E325 Argument 'resource_definition' in argument_spec defines type as <function list_dict_str at 0x7f74b783f598> but documentation defines type as 'str'
lib/ansible/modules/cloud/kubevirt/kubevirt_rs.py:0:0: E325 Argument 'resource_definition' in argument_spec defines type as <function list_dict_str at 0x7f74b63e07b8> but documentation defines type as 'str'

click here for bot help

@bcoca

This comment has been minimized.

Copy link
Member

bcoca commented Feb 22, 2019

related #50446

@@ -309,3 +309,61 @@ def _load(self, path):

def _dump(self, value, path):
return self._plugin._dump(value, path)


class InventoryRedisCacheModule(BaseFileCacheModule):

This comment has been minimized.

@bcoca

bcoca Feb 22, 2019

Member

see #50446, we are going in the direction of generalizing the interface for all cache plugins, we don't want to have to create inventory specific cache plugins for every type

@@ -49,6 +50,9 @@
description: Toggle, if true the inventory will retrieve 'all_parameters' information as host vars
type: boolean
default: False
inventory_cache_key:
description: Cache key used in InventoryRedisCacheModule
default: inventory

This comment has been minimized.

@bcoca

bcoca Feb 22, 2019

Member

missing version_added, also see previus comment on being 'generic'

# Try yaml
try:
value = yaml.load(param['value'])
except yaml.YAMLError:

This comment has been minimized.

@bcoca

bcoca Feb 22, 2019

Member

json is a subset of yaml so it is most likely to be loaded here even if it was json to begin with, but pyyaml parser can introduce some differences, you might want to reverse the order.

@FutureTecSystems

This comment has been minimized.

Copy link
Author

FutureTecSystems commented Feb 22, 2019

I know this PR (#50446), it gave me some hints but I needed a faster solution without being too generic in the first place. The fix in the inventory was mandatory and we had to make some improvements using "include=all_parameters" on the Foreman API to reduce running time in the environment this big here.

Is there a roadmap for finishing the generalizing so that I've just to add my improvements on top or does it still take some time? In the latter I could offer some help but first I need to understand your way and already executed work so far. From the amount of files already touched I was not able to identify the ideas behind that code.

@bcoca

This comment has been minimized.

Copy link
Member

bcoca commented Feb 22, 2019

So some history, initial 'inventory cache' implementation was made to only include 'file based caches', not just jsonfile, but that is most common one. This was due mostly to serialization issues we hit with some of the db based caches, but also because these tended to have much more latency, sometimes longer than the api service response they were caching.

Still users do seem to want to use any cache plugin so we started the work to generalize the interface and make it usable by both fact and inventory systems to use any cache plugin, that is what the PR above is aiming at and does have many moving parts and updates to make this possible.

As for the 'timing', it is being aimed for 2.8 and is very close to be merge. Even if it does not make it, I'm not inclined to merge this PR with its modifications to caching, I'm fine with the changes to the foreman plugin.

@jborean93

This comment has been minimized.

Copy link
Contributor

jborean93 commented Feb 25, 2019

CI test failures https://app.shippable.com/github/ansible/ansible/runs/109914/104/tests

{
    "changed": false, 
    "msg": "All items completed", 
    "results": [
        {
            "_ansible_item_label": "foreman_pgagne_sats", 
            "_ansible_item_result": true, 
            "_ansible_no_log": false, 
            "_ansible_verbose_always": true, 
            "assertion": "'foreman_pgagne_sats' in groups\n", 
            "changed": false, 
            "evaluated_to": false, 
            "failed": true, 
            "item": "foreman_pgagne_sats", 
            "msg": "Assertion failed"
        }, 
        {
            "_ansible_item_label": "foreman_base", 
            "_ansible_item_result": true, 
            "_ansible_no_log": false, 
            "_ansible_verbose_always": true, 
            "assertion": "'foreman_base' in groups\n", 
            "changed": false, 
            "evaluated_to": false, 
            "failed": true, 
            "item": "foreman_base", 
            "msg": "Assertion failed"
        }
    ]
}

@jborean93 jborean93 removed the needs_triage label Feb 25, 2019

if inventory is not None:
return inventory

params = {'page': 1, 'per_page': 1000}

This comment has been minimized.

@ekohl

ekohl Feb 26, 2019

Contributor

This limits you to 1000 hosts where previously it iterated all the pages.

This comment has been minimized.

@FutureTecSystems

FutureTecSystems Feb 27, 2019

Author

Yeah, this was a dirty workaround because we had a big showstopper here. It's on my ToDo to clean up the current state.

@FutureTecSystems

This comment has been minimized.

Copy link
Author

FutureTecSystems commented Feb 27, 2019

So some history, initial 'inventory cache' implementation was made to only include 'file based caches', not just jsonfile, but that is most common one. This was due mostly to serialization issues we hit with some of the db based caches, but also because these tended to have much more latency, sometimes longer than the api service response they were caching.

Still users do seem to want to use any cache plugin so we started the work to generalize the interface and make it usable by both fact and inventory systems to use any cache plugin, that is what the PR above is aiming at and does have many moving parts and updates to make this possible.

As for the 'timing', it is being aimed for 2.8 and is very close to be merge. Even if it does not make it, I'm not inclined to merge this PR with its modifications to caching, I'm fine with the changes to the foreman plugin.

I'm fine with the changes to the caching not being merged. So I'm gonna work on the improvements for the Foreman plugin based on the new caching layer and hope that we can merge them later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.