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

feat: support changing stdout callback plugin and its options #1367

Closed
wants to merge 1 commit into from

Conversation

kurokobo
Copy link

@kurokobo kurokobo commented May 22, 2024

Summary

This PR adds (maybe) full support of changing stdout callback plugin and its options.

  • To change stdout callback plugin, specify ANSIBLE_STDOUT_CALLBACK environment variable (stdout_callback in ansible.cfg is ignored)
  • To change its options, specify desired environment variables, or keys in ansible.cfg

Closes 1322, Closes #994

Design

  • To minimize side effects, I did not significantly change the current implementation
  • I avoided the method suggested by Support new display options in ansible-core #994 that dynamically loading the DOCUMENTATION because it would make ansible-doc not work

Architecture

  • The problem with the current implementation is that although ANSIBLE_STDOUT_CALLBACK allows any plugin to be specified, only default_callback is specified in the extends_documentation_fragment, which means that we cannot specify any options at all except for the options in default_callback. This prevents many of the callback plugins from loading their own options, resulting in an error.
  • Therefore, we should allow awx_display to have the options specified for the originally specified plugin
  • Currently, all plugin options are loaded by self.set_options, but the options to be loaded are filtered using self._load_name (source)
  • Since self._load_name is the name of the plugin, if this is faked only when self.set_options is executed, the original plugin options can be set to awx_display
  • This allows any callback plugin to work properly with any options
  • The implementation for loading options using self._load_name has not changed since Ansible 2.5

Additional Information

@sivel @AlanCoding @nitzmahone @bcoca
I think it would be useful to be able to use any callback plugins with a small modification, what do you think of this implementation? I know it is never best to fake the name, but I think it would be great if there is a small improvement with a small (and not best) change in a short term, since a major renovation takes lots of time to complete designing and implemantation.

I could make it possible to control faking _load_name with a feature flag ALLOW_CUSTOM_CALLBACK_PLUGIN or something similar, but first I made a draft PR with minimal changes.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@bcoca
Copy link
Member

bcoca commented May 23, 2024

This just makes the options of the plugin conform to the default, which is not what #1322 tries to cover.

The 'fix' for that would be to change the 'awx_callback' to an 'logging' plugin and have runner ignore/pass-through the screen output and use it's the awx plugin to capture the required events independently. Then runner would also have to 'add' the plugin to configuration, which means querying the currently configured plugins (ansible-config can provide that) and then adding this on top, most likely via an environment variable as it does now, just a different one.

@kurokobo
Copy link
Author

Thank you for your comment!
I should not have linked #1332, indeed my PR is not at all what #1332 is aiming for. I removed it from my OP.

I just want to be able to use any callback plugin with any options with Ansible Runner anyway. Even a very simple modification like this PR will give the desired result.

I know it will take time to complete a major modification like #1332, so I want to show a suggestion to make this kind of small modification as something like time-limited solution until then.

I have confirmed that by using the EE image containing this PR, I can use non-default callback plugin with non-default options for AWX jobs (community,general.timestamp with timezone, format_string, and result_format).

image
image

If #1322 is going to be resolved in short term, I would not be particularly concerned about this PR,

@kurokobo
Copy link
Author

kurokobo commented May 23, 2024

For those who are insterested in this, I've published EE images that contain this PR:

Dockerfiles for above images:

FROM quay.io/ansible/awx-ee:24.4.0
USER root
RUN curl -Lo /usr/local/lib/python3.9/site-packages/ansible_runner/display_callback/callback/awx_display.py https://raw.githubusercontent.com/ansible/ansible-runner/9e35e4da04c80bdaea52c1449d5f35a256e51471/src/ansible_runner/display_callback/callback/awx_display.py
RUN pip3 install --no-cache-dir tzdata
RUN ansible-galaxy collection install -p /usr/share/ansible/collections/ansible_collections community.general==9.0.0
USER 1000
FROM ghcr.io/ansible-community/community-ee-base:2.16.7-1
USER root
RUN rm /usr/local/lib/python3.12/site-packages/ansible_runner/display_callback/callback/__pycache__/awx_display.cpython-312.pyc
RUN curl -Lo /usr/local/lib/python3.12/site-packages/ansible_runner/display_callback/callback/awx_display.py https://raw.githubusercontent.com/ansible/ansible-runner/9e35e4da04c80bdaea52c1449d5f35a256e51471/src/ansible_runner/display_callback/callback/awx_display.py
RUN pip3 install --no-cache-dir tzdata
RUN ansible-galaxy collection install -p /usr/share/ansible/collections/ansible_collections community.general==9.0.0
USER 1000

@bcoca
Copy link
Member

bcoca commented May 23, 2024

In this case the timestamp plugin is just a minor modification of default, while your patch will work for this simple case, I doubt that other plugins that depart more from 'default' or are not based on it will work well as the underlying code for awx_display is too tightly coupled to the 'default' callback plugin.

@nitzmahone
Copy link
Member

nitzmahone commented May 23, 2024

I haven't had time to consider this approach as an interim solution, but decoupling the callback/display integration required by AWX is a pretty high priority item that's been blocking a lot of work on ansible-core for a long time. The way we're looking at solving it would make awx_display a supplemental callback that consults event/task-correlated context collected by core during the run to yield the same event stream it does today, but without needing to be the stdout callback (and eliminating most of the core-internal monkeypatching, locking, and "display steganography" that breaks a bunch of planned core worker model changes). A side effect of that change would allow any stdout callback to be used by runner/AWX.

@kurokobo
Copy link
Author

Thanks for your comments again!

the underlying code for awx_display is too tightly coupled to the 'default' callback plugin

Ah makes sense. I'm not at all confident that I can guarantee that this PR will work without any issues for all stdout plugins in the world😄

So I would be looking forward to #1322 to be implemented! I'm also interested in #1322, but it will probably be an extremely steep road for me to implement it as I don't know much about awx_display. You are all by far the best maintainers for this than me.

Anyway thanks for taking your time!

@kurokobo kurokobo closed this May 24, 2024
@kurokobo kurokobo deleted the callback branch May 24, 2024 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support new display options in ansible-core
3 participants