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

Add foreman callback plugin #17141

Merged
merged 1 commit into from Oct 3, 2016

Conversation

Projects
None yet
5 participants
@agx
Contributor

agx commented Aug 18, 2016

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

plugins/callback/foreman.py

ANSIBLE VERSION
ansible 2.2.0 (foreman-callback 6f2cfacca8) last updated 2016/08/18 20:03:16 (GMT +200)
  lib/ansible/modules/core: (detached HEAD 91a839f1e3) last updated 2016/08/18 20:06:19 (GMT +200)
  lib/ansible/modules/extras: (detached HEAD 1aeb9f8a8c) last updated 2016/08/18 20:06:19 (GMT +200)
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides
SUMMARY

Add a callback to report facts and reports to the foreman:

https://theforeman.org/
@bcoca

View changes

Show outdated Hide outdated lib/ansible/plugins/callback/foreman.py
@bcoca

View changes

Show outdated Hide outdated lib/ansible/plugins/callback/foreman.py
@bcoca

View changes

Show outdated Hide outdated lib/ansible/plugins/callback/foreman.py
@bcoca

View changes

Show outdated Hide outdated lib/ansible/plugins/callback/foreman.py
@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Aug 19, 2016

Contributor

@bcoca thanks for having a look at this PR. I think I addressed your concerns except for the python-requests removal. I'm happy to switch to open_url / fetch_url but I can't see how to get the current behaviour (allow to specify X509 certificate, key and CA) without reimplementing parts of requests myself or did I miss somthing in the *_url functions?

Contributor

agx commented Aug 19, 2016

@bcoca thanks for having a look at this PR. I think I addressed your concerns except for the python-requests removal. I'm happy to switch to open_url / fetch_url but I can't see how to get the current behaviour (allow to specify X509 certificate, key and CA) without reimplementing parts of requests myself or did I miss somthing in the *_url functions?

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Aug 26, 2016

Contributor

Hi, anything I can do to move this further? I'd like to stick to requests since we need client certificates for authentication against foreman but if it's a must have to use the *_url functions I can copy over the necessary code.

Contributor

agx commented Aug 26, 2016

Hi, anything I can do to move this further? I'd like to stick to requests since we need client certificates for authentication against foreman but if it's a must have to use the *_url functions I can copy over the necessary code.

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Aug 26, 2016

Member

if you insist on requests i would specify version and handle either it being missing or of insufficient/incompatible version. The API of that library has changed in backwards incompatible ways.

Member

bcoca commented Aug 26, 2016

if you insist on requests i would specify version and handle either it being missing or of insufficient/incompatible version. The API of that library has changed in backwards incompatible ways.

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Aug 29, 2016

Contributor

@bcoca thanks for your patience! I've added the check for requests. We're using very few parts of the requests API so I did not add any checks yet. I've tried several versions 2.10.0, 2.7.0, 2.4 and it worked there.

Contributor

agx commented Aug 29, 2016

@bcoca thanks for your patience! I've added the check for requests. We're using very few parts of the requests API so I did not add any checks yet. I've tried several versions 2.10.0, 2.7.0, 2.4 and it worked there.

@mattclay

This comment has been minimized.

Show comment
Hide comment
@mattclay

mattclay Aug 31, 2016

Member

Tests on shippable failed because this PR branch was created before shippable was enabled. If you rebase the branch the tests will pass, although in this case the tests will just no-op since there are no tests for this plugin.

Member

mattclay commented Aug 31, 2016

Tests on shippable failed because this PR branch was created before shippable was enabled. If you rebase the branch the tests will pass, although in this case the tests will just no-op since there are no tests for this plugin.

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Sep 1, 2016

Contributor

@mattclay I rebased but the tests still fail.

Contributor

agx commented Sep 1, 2016

@mattclay I rebased but the tests still fail.

@mattclay

This comment has been minimized.

Show comment
Hide comment
@mattclay

mattclay Sep 1, 2016

Member

@agx Look at the console log here for the test failure:

https://app.shippable.com/runs/57c7b815236da20f00dc7638/35/console

The error is:

2016-09-01 05:12:52 == Missing from future import (absolute_import, division, print_function) ==

Member

mattclay commented Sep 1, 2016

@agx Look at the console log here for the test failure:

https://app.shippable.com/runs/57c7b815236da20f00dc7638/35/console

The error is:

2016-09-01 05:12:52 == Missing from future import (absolute_import, division, print_function) ==

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Sep 2, 2016

Contributor

On Thu, Sep 01, 2016 at 12:10:31AM -0700, Matt Clay wrote:

@agx Look at the console log here for the test failure:

https://app.shippable.com/runs/57c7b815236da20f00dc7638/35/console

The error is:

2016-09-01 05:12:52 == Missing from future import (absolute_import, division, print_function) ==

Thanks, I also fixed another failure but the current one does not look
related:

https://app.shippable.com/runs/57c86e08640b890e00210509/1/console

Just repin?

Contributor

agx commented Sep 2, 2016

On Thu, Sep 01, 2016 at 12:10:31AM -0700, Matt Clay wrote:

@agx Look at the console log here for the test failure:

https://app.shippable.com/runs/57c7b815236da20f00dc7638/35/console

The error is:

2016-09-01 05:12:52 == Missing from future import (absolute_import, division, print_function) ==

Thanks, I also fixed another failure but the current one does not look
related:

https://app.shippable.com/runs/57c86e08640b890e00210509/1/console

Just repin?

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Sep 5, 2016

Contributor

@bcoca @mattclay Tests pass now, any chance this can be applied?

Contributor

agx commented Sep 5, 2016

@bcoca @mattclay Tests pass now, any chance this can be applied?

@bcoca bcoca removed the needs_revision label Sep 5, 2016

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Sep 5, 2016

Member

@agx, still, you need to specify requests versions, many machines come with <2.0 installed and I know it does not work because the API changed in incompatible ways.

Member

bcoca commented Sep 5, 2016

@agx, still, you need to specify requests versions, many machines come with <2.0 installed and I know it does not work because the API changed in incompatible ways.

@bcoca bcoca added the needs_revision label Sep 5, 2016

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Sep 6, 2016

Contributor

On Mon, Sep 05, 2016 at 04:20:02PM -0700, Brian Coca wrote:

@agx, still, you need to specify requests versions, many machines come with <2.0 installed and I know it does not work because the API changed in incompatible ways.

Fixed now. Cheking for >= 2.0.

Contributor

agx commented Sep 6, 2016

On Mon, Sep 05, 2016 at 04:20:02PM -0700, Brian Coca wrote:

@agx, still, you need to specify requests versions, many machines come with <2.0 installed and I know it does not work because the API changed in incompatible ways.

Fixed now. Cheking for >= 2.0.

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Sep 6, 2016

Contributor

@mattclay Tests are failing again but I don't think this is caused by the change:

2016-09-06 05:52:30 ERROR! Unexpected Exception: 'ascii' codec can't decode byte 0xc3 in position 95: ordinal not in range(128)

@bcoca Check for requests is now there (I noticed that no other code that uses requests checks for the version. Should we move this to common code and add checks as well in a follow up commit?)

Contributor

agx commented Sep 6, 2016

@mattclay Tests are failing again but I don't think this is caused by the change:

2016-09-06 05:52:30 ERROR! Unexpected Exception: 'ascii' codec can't decode byte 0xc3 in position 95: ordinal not in range(128)

@bcoca Check for requests is now there (I noticed that no other code that uses requests checks for the version. Should we move this to common code and add checks as well in a follow up commit?)

@mattclay

This comment has been minimized.

Show comment
Hide comment
@mattclay

mattclay Sep 6, 2016

Member

@agx The error is due to the non-ascii character in your name. Removing it eliminates the error.

Here's the full traceback:

Traceback (most recent call last):
  File "/root/ansible/bin/ansible-playbook", line 97, in <module>
    exit_code = cli.run()
  File "/root/ansible/lib/ansible/cli/playbook.py", line 154, in run
    results = pbex.run()
  File "/root/ansible/lib/ansible/executor/playbook_executor.py", line 82, in run
    self._tqm.load_callbacks()
  File "/root/ansible/lib/ansible/executor/task_queue_manager.py", line 277, in load_callbacks
    for callback_plugin in callback_loader.all(class_only=True):
  File "/root/ansible/lib/ansible/plugins/__init__.py", line 386, in all
    self._module_cache[path] = self._load_module_source(name, path)
  File "/root/ansible/lib/ansible/plugins/__init__.py", line 321, in _load_module_source
    module = imp.load_source(name, path, module_file)
  File "/usr/lib/python3.5/imp.py", line 172, in load_source
    module = _load(spec)
  File "<frozen importlib._bootstrap>", line 693, in _load
  File "<frozen importlib._bootstrap>", line 673, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 661, in exec_module
  File "<frozen importlib._bootstrap_external>", line 766, in get_code
  File "/usr/lib/python3.5/imp.py", line 156, in get_data
    return file.read()
  File "/usr/lib/python3.5/encodings/ascii.py", line 26, in decode
    return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 94: ordinal not in range(128)

The imp module is deprecated as of python 3.4 in favor of importlib, which is probably why this error only shows up when we test on python 3.

@abadger: This looks like a python 3 compatibility issue we'll need to address.

Member

mattclay commented Sep 6, 2016

@agx The error is due to the non-ascii character in your name. Removing it eliminates the error.

Here's the full traceback:

Traceback (most recent call last):
  File "/root/ansible/bin/ansible-playbook", line 97, in <module>
    exit_code = cli.run()
  File "/root/ansible/lib/ansible/cli/playbook.py", line 154, in run
    results = pbex.run()
  File "/root/ansible/lib/ansible/executor/playbook_executor.py", line 82, in run
    self._tqm.load_callbacks()
  File "/root/ansible/lib/ansible/executor/task_queue_manager.py", line 277, in load_callbacks
    for callback_plugin in callback_loader.all(class_only=True):
  File "/root/ansible/lib/ansible/plugins/__init__.py", line 386, in all
    self._module_cache[path] = self._load_module_source(name, path)
  File "/root/ansible/lib/ansible/plugins/__init__.py", line 321, in _load_module_source
    module = imp.load_source(name, path, module_file)
  File "/usr/lib/python3.5/imp.py", line 172, in load_source
    module = _load(spec)
  File "<frozen importlib._bootstrap>", line 693, in _load
  File "<frozen importlib._bootstrap>", line 673, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 661, in exec_module
  File "<frozen importlib._bootstrap_external>", line 766, in get_code
  File "/usr/lib/python3.5/imp.py", line 156, in get_data
    return file.read()
  File "/usr/lib/python3.5/encodings/ascii.py", line 26, in decode
    return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 94: ordinal not in range(128)

The imp module is deprecated as of python 3.4 in favor of importlib, which is probably why this error only shows up when we test on python 3.

@abadger: This looks like a python 3 compatibility issue we'll need to address.

@mattclay

View changes

Show outdated Hide outdated lib/ansible/plugins/callback/foreman.py
@mattclay

View changes

Show outdated Hide outdated lib/ansible/plugins/callback/foreman.py
@mattclay

View changes

Show outdated Hide outdated lib/ansible/plugins/callback/foreman.py
@abadger

This comment has been minimized.

Show comment
Hide comment
@abadger

abadger Sep 6, 2016

Member

imp on python3 issue should be fixed. Reading the file as bytes instead of text and passing it to imp allows imp to properly find the encoding. You'll need to rebase to pick up that change.

Member

abadger commented Sep 6, 2016

imp on python3 issue should be fixed. Reading the file as bytes instead of text and passing it to imp allows imp to properly find the encoding. You'll need to rebase to pick up that change.

@mattclay

This comment has been minimized.

Show comment
Hide comment
@mattclay

mattclay Sep 7, 2016

Member

@agx I reran the build after the python 3 fix was made by @abadger and all tests are now passing.

Member

mattclay commented Sep 7, 2016

@agx I reran the build after the python 3 fix was made by @abadger and all tests are now passing.

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Sep 7, 2016

Contributor

@mattclay Thanks for looking into the testsuite! I've addressed your comments. One thing I noticed:

As soon as I add any of the CALLBACK_* variables to the callback, the callback won't be invoked anymore whith:

 ansible -i localhost, -msetup -clocal localhost

while it still gets invoked fine with a playbook:

 ansible-playbook -i localhost, -l localhost play.yml -c local

Is this intentional?

Contributor

agx commented Sep 7, 2016

@mattclay Thanks for looking into the testsuite! I've addressed your comments. One thing I noticed:

As soon as I add any of the CALLBACK_* variables to the callback, the callback won't be invoked anymore whith:

 ansible -i localhost, -msetup -clocal localhost

while it still gets invoked fine with a playbook:

 ansible-playbook -i localhost, -l localhost play.yml -c local

Is this intentional?

@mattclay

This comment has been minimized.

Show comment
Hide comment
@mattclay

mattclay Sep 7, 2016

Member

@agx Yes. Callbacks included with Ansible must be enabled by setting the ANSIBLE_CALLBACK_WHITELIST environment variable.

For your plugin, you would use: ANSIBLE_CALLBACK_WHITELIST=foreman

Without this whitelisting, all callbacks shipped with Ansible would be used when running ansible-playbook.

Member

mattclay commented Sep 7, 2016

@agx Yes. Callbacks included with Ansible must be enabled by setting the ANSIBLE_CALLBACK_WHITELIST environment variable.

For your plugin, you would use: ANSIBLE_CALLBACK_WHITELIST=foreman

Without this whitelisting, all callbacks shipped with Ansible would be used when running ansible-playbook.

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Sep 7, 2016

Contributor

@mattclay I know I have to whitelist the plugin (and I do so via ansible.cfg) but it only gets run with ansible-playbook not with ansible -msetup ...

This isn't a bug in the plugin itself but looks rather like a behaviour change between plugins using CALLBACK_ and those that dont.

Contributor

agx commented Sep 7, 2016

@mattclay I know I have to whitelist the plugin (and I do so via ansible.cfg) but it only gets run with ansible-playbook not with ansible -msetup ...

This isn't a bug in the plugin itself but looks rather like a behaviour change between plugins using CALLBACK_ and those that dont.

@mattclay

This comment has been minimized.

Show comment
Hide comment
@mattclay

mattclay Sep 13, 2016

Member

@agx You need to set ANSIBLE_LOAD_CALLBACK_PLUGINS=true to use additional callbacks with ansible.

Member

mattclay commented Sep 13, 2016

@agx You need to set ANSIBLE_LOAD_CALLBACK_PLUGINS=true to use additional callbacks with ansible.

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Sep 16, 2016

Contributor

@mattclay ahh...thanks for the hint. Yes, with ANSIBLE_LOAD_CALLBACK_PLUGINS=true this works as expected.

Contributor

agx commented Sep 16, 2016

@mattclay ahh...thanks for the hint. Yes, with ANSIBLE_LOAD_CALLBACK_PLUGINS=true this works as expected.

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Sep 16, 2016

Contributor

@mattclay @bcoca just doublechecked that the plugin works with Foreman 1.12.x , would be great if it could be merged now.

Contributor

agx commented Sep 16, 2016

@mattclay @bcoca just doublechecked that the plugin works with Foreman 1.12.x , would be great if it could be merged now.

Show outdated Hide outdated lib/ansible/plugins/callback/foreman.py
Show outdated Hide outdated lib/ansible/plugins/callback/foreman.py
@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Sep 17, 2016

Contributor

On Fri, Sep 16, 2016 at 02:21:11PM -0700, Matt Clay wrote:

mattclay commented on this pull request.

  •                              self.FOREMAN_URL)
    
  •        verify = False
    
  •    else:  # Set ta a CA bundle:
    
  •        verify = self.FOREMAN_SSL_VERIFY
    
  •    return verify
    
  • def send_facts(self, host, data):
  •    """
    
  •    Sends facts to Foreman, to be parsed by foreman_ansible fact
    
  •    parser.  The default fact importer should import these facts
    
  •    properly.
    
  •    """
    
  •    data["_type"] = "ansible"
    
  •    data["_timestamp"] = datetime.now().strftime(self.TIME_FORMAT)
    
  •    data = json.dumps(data)
    
  •    facts_json = self.FACTS_FORMAT % dict(host=host, data=data)
    

Why not use json.dumps to create facts_json?

I think the idea here was to have the format facts should be posted
(FACTS_FORMAT) separated from the (json encoded) facts data. This way
once ca see at one glance what the format should look like. I can change
this to s.th. like:

 json.dumps({
    "name": ...
    "data": {
            "_type": "ansible"
            ...
    }       

but I think the current form is better readable.

  •    get a report json that Foreman can handle without writing
    
  •    another report importer.
    
  •    """
    
  •    status = defaultdict(lambda: 0)
    
  •    metrics = {}
    
  •    for host in stats.processed.keys():
    
  •        sum = stats.summarize(host)
    
  •        status["applied"] = sum['changed']
    
  •        status["failed"] = sum['failures'] + sum['unreachable']
    
  •        status["skipped"] = sum['skipped']
    
  •        log = self._build_log(self.items[host])
    
  •        metrics["time"] = {"total": int(time.time()) - self.start_time}
    
  •        self.items[host] = []
    
  •        report_json = self.REPORT_FORMAT % dict(host=host,
    

Why not use json.dumps to create report_json?

Same here.

Contributor

agx commented Sep 17, 2016

On Fri, Sep 16, 2016 at 02:21:11PM -0700, Matt Clay wrote:

mattclay commented on this pull request.

  •                              self.FOREMAN_URL)
    
  •        verify = False
    
  •    else:  # Set ta a CA bundle:
    
  •        verify = self.FOREMAN_SSL_VERIFY
    
  •    return verify
    
  • def send_facts(self, host, data):
  •    """
    
  •    Sends facts to Foreman, to be parsed by foreman_ansible fact
    
  •    parser.  The default fact importer should import these facts
    
  •    properly.
    
  •    """
    
  •    data["_type"] = "ansible"
    
  •    data["_timestamp"] = datetime.now().strftime(self.TIME_FORMAT)
    
  •    data = json.dumps(data)
    
  •    facts_json = self.FACTS_FORMAT % dict(host=host, data=data)
    

Why not use json.dumps to create facts_json?

I think the idea here was to have the format facts should be posted
(FACTS_FORMAT) separated from the (json encoded) facts data. This way
once ca see at one glance what the format should look like. I can change
this to s.th. like:

 json.dumps({
    "name": ...
    "data": {
            "_type": "ansible"
            ...
    }       

but I think the current form is better readable.

  •    get a report json that Foreman can handle without writing
    
  •    another report importer.
    
  •    """
    
  •    status = defaultdict(lambda: 0)
    
  •    metrics = {}
    
  •    for host in stats.processed.keys():
    
  •        sum = stats.summarize(host)
    
  •        status["applied"] = sum['changed']
    
  •        status["failed"] = sum['failures'] + sum['unreachable']
    
  •        status["skipped"] = sum['skipped']
    
  •        log = self._build_log(self.items[host])
    
  •        metrics["time"] = {"total": int(time.time()) - self.start_time}
    
  •        self.items[host] = []
    
  •        report_json = self.REPORT_FORMAT % dict(host=host,
    

Why not use json.dumps to create report_json?

Same here.

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Oct 2, 2016

Contributor

@mattclay I removed the hand built json making the code more compact. Anything else I can do to get this merged?

Contributor

agx commented Oct 2, 2016

@mattclay I removed the hand built json making the code more compact. Anything else I can do to get this merged?

@bcoca bcoca added this to the 2.3.0 milestone Oct 3, 2016

@bcoca bcoca removed the needs_revision label Oct 3, 2016

@bcoca bcoca removed this from the 2.3.0 milestone Oct 3, 2016

@bcoca bcoca merged commit 14a9bd6 into ansible:devel Oct 3, 2016

1 check failed

Shippable Run 4007 status is FAILED.
Details
@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Oct 4, 2016

Contributor

On Sun, Oct 02, 2016 at 09:13:04PM -0700, Brian Coca wrote:

Merged #17141.

Thanks!
-- Guido

Contributor

agx commented Oct 4, 2016

On Sun, Oct 02, 2016 at 09:13:04PM -0700, Brian Coca wrote:

Merged #17141.

Thanks!
-- Guido

sereinity added a commit to sereinity/ansible that referenced this pull request Jan 25, 2017

@ansibot ansibot added feature and removed feature_pull_request labels Mar 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment