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

Foreman facts uploading correctly merges facts #51546

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
7 participants
@ares
Copy link
Contributor

ares commented Jan 31, 2019

SUMMARY

Foreman facts callback does not work well when more facts are added from roles

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

foreman

ADDITIONAL INFORMATION

We upload facts for any "ok" event, given Foreman behavior, that removes all existing facts in Foreman of Ansible source. While setup phase upload all ansible_* modules, if next play introduces new fact, it deletes all facts from step one and adds just newly added facts. Also sending update for each play is not performance heavy, it's better to build the final set in memory and then send at once.

there's no visible change in fact
@ares

This comment has been minimized.

Copy link
Contributor Author

ares commented Jan 31, 2019

@ekohl mind to give it a quick read since you were debugging the issue with me? thanks

ansible_facts = {}
for _, result in results:
ansible_facts.update(result.get('ansible_facts', {}))
data = {}

This comment has been minimized.

@ekohl

ekohl Jan 31, 2019

Contributor

Probably cleaner to write something like:

facts = {
    "name": host,                     
    "facts": {            
        "ansible_facts": ansible_facts,                        
        "_type": "ansible",            
        "_timestamp": datetime.now().strftime(self.TIME_FORMAT),
    },                                                          
}
for host, results in self.items.items():
ansible_facts = {}
for _, result in results:
ansible_facts.update(result.get('ansible_facts', {}))

This comment has been minimized.

@ekohl

ekohl Jan 31, 2019

Contributor

I know I wrote this, but it can be easier to read (and perhaps slightly more efficient):

for _, result in results:
    if 'ansible_facts' in result:
        ansible_facts.update(result['ansible_facts'])
@bcoca
Copy link
Member

bcoca left a comment

This can end up creating a large memory baloon as it will be an additional cache of all host facts for each host in play. Since this seems to try to compensate the fact that there is no API call to append facts to a host in foreman I would suggest trying to use a fact cache plugin to populate this instead.

verify=self.ssl_verify)
r.raise_for_status()
except requests.exceptions.RequestException as err:
print(str(err))

This comment has been minimized.

@bcoca

bcoca Jan 31, 2019

Member

no prints, no str , use display and to_text

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 31, 2019

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

lib/ansible/plugins/callback/foreman.py:250:0: trailing-newlines Trailing newlines

The test ansible-test sanity --test no-underscore-variable [explain] failed with 1 error:

lib/ansible/plugins/callback/foreman.py:145:17: use `dummy` instead of `_` for a variable name

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

lib/ansible/plugins/callback/foreman.py:250:1: W391 blank line at end of file

click here for bot help

@mattclay

This comment has been minimized.

Copy link
Member

mattclay commented Feb 2, 2019

The sanity test failures are valid. The integration test failures are unrelated and can be ignored.

@dagwieers dagwieers added the foreman label Feb 8, 2019

@ansibot ansibot added the stale_ci label Feb 16, 2019

@ares ares force-pushed the ares:fix/facts_uploading branch from f96175d to 0801ca7 Feb 28, 2019

@ares

This comment has been minimized.

Copy link
Contributor Author

ares commented Feb 28, 2019

Thank you very much both for looking. I updated what you suggested. Regarding performance concern, it's valid but this PR doesn't make the existing behavior much worse. This covers the case when some tasks register additional facts, usually that's lower amount as for setup in the beginning. Changing the way how Foreman parses facts would be nice, but a lot of things we rely on today would no longer apply. I don't think we can modify it anytime soon.

I'll also look at sanity test failures.

@ares ares force-pushed the ares:fix/facts_uploading branch from 0801ca7 to f286306 Feb 28, 2019

@ares ares force-pushed the ares:fix/facts_uploading branch from f286306 to 30ce41e Feb 28, 2019

@ares

This comment has been minimized.

Copy link
Contributor Author

ares commented Feb 28, 2019

Would it be please possible to cherry-pick this to stable-2.7, it is quite important fix for use with Foreman
I think it's ready_for_review

@bcoca

bcoca approved these changes Feb 28, 2019

@ares

This comment has been minimized.

Copy link
Contributor Author

ares commented Feb 28, 2019

Thanks Brian! @ekohl, mind to take a look? As per bot help, your shipit should help :-)

@ekohl

ekohl approved these changes Feb 28, 2019

Copy link
Contributor

ekohl left a comment

:shipit:

@ekohl

This comment has been minimized.

Copy link
Contributor

ekohl commented Mar 4, 2019

shipit

@ekohl

ekohl approved these changes Mar 4, 2019

Copy link
Contributor

ekohl left a comment

shipit

@ansibot ansibot added the stale_ci label Mar 12, 2019

@ares

This comment has been minimized.

Copy link
Contributor Author

ares commented Mar 20, 2019

reopening because of stale_ci flag

@ares ares closed this Mar 20, 2019

@ares ares reopened this Mar 20, 2019

@ansibot ansibot removed the stale_ci label Mar 20, 2019

@iNecas

iNecas approved these changes Mar 20, 2019

Copy link

iNecas left a comment

Would be great to get this in, as without this, the callback is not very useful once you start using things like set_facts

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.