(alternative proposal) update how callback plugin gets copied and added to job container #1093

TheRealHaoLiu · 2022-06-06T15:09:34Z

alternative proposal for issue #1088

when in containerized environment

always copy callback plugin to private_data_dir/artifacts/{job_id}/callback
appending to ANSIBLE_CALLBACK_PLUGINS with the runner callback plugin location (from private_data/artifacts)
remove utils.callback_mount due to unused method

Co-Authored-By: Alan Rominger arominge@redhat.com
Signed-off-by: Hao Liu haoli@redhat.com

shanemcd · 2022-06-06T15:13:42Z

ansible_runner/config/_base.py

@@ -267,6 +268,14 @@ def _prepare_env(self, runner_mode='pexpect'):
            if callback_dir is None:
                callback_dir = get_callback_dir()
            self.env['ANSIBLE_CALLBACK_PLUGINS'] = ':'.join(filter(None, (self.env.get('ANSIBLE_CALLBACK_PLUGINS'), callback_dir)))
+        else:


if not followed by an else is kinda breaking my brain. How about we just flip these conditions? if self.containerized ... else.

AlanCoding · 2022-06-06T17:39:20Z

ansible_runner/config/_base.py

+            # when containerized, copy the callback dir to $pirvate_data_dir/artifacts/<job_id>/callback
+            # than append to env['Ansible_CALLBACK_PLUGINS'] with the copyied location
+            callback_dir = os.path.join(self.artifact_dir, 'callback')
+            shutil.copytree(get_callback_dir(), callback_dir)


I tested this and saw:

drwxrwxrwx. 3 alancoding alancoding 4096 May 2 15:53 callback

These permissions are going to set off alarm bells. We probably need to pre-create the directory so we can set specific permissions. Practically, we'd probably copy from above in this same file

ansible-runner/ansible_runner/config/_base.py

Line 138 in 8174803

os.makedirs(self.artifact_dir, exist_ok=True, mode=0o700)

but os.mkdir might be preferable here.

discussed with @AlanCoding about the topic, the copytree inherited the file permission of the parent ansible-runner installation... so long as the permission for the original install is good the copied version would be good as well

AlanCoding · 2022-06-06T17:49:12Z

Great work! You knocked out the tests really fast. I ran the demo project, and wanted to paste this for illustration.

(env) [alancoding@alan-red-hat ansible-runner]$ tree demo/artifacts/
demo/artifacts/
└── 84f49826-4f12-4ae6-a6f1-228bea672e96
    ├── ansible_version.txt
    ├── callback
    │   ├── awx_display.py
    │   ├── __init__.py
    │   └── __pycache__
    │       ├── awx_display.cpython-310.pyc
    │       └── awx_display.cpython-38.pyc
    ├── command
    ├── env.list
    ├── fact_cache
    │   └── localhost
    ├── job_events
    │   ├── 10-1e52863e-d222-7606-33ef-000000000008.json
    │   ├── 11-262c7ff0-11b9-40df-9530-a1693ca2a7e1.json
    │   ├── 12-8f0623ab-18bb-4d15-981c-dda5113e370e.json
    │   ├── 13-01d48534-f9b4-4e80-95ff-e920de186441.json
    │   ├── 1-d15d3349-848d-4aaf-8694-044629cede74.json
    │   ├── 2-58e4bf62-44db-4116-b370-fe126162d433.json
    │   ├── 3-52d7c10a-8c6c-49d1-90d5-b1fce26ba649.json
    │   ├── 4-84ae27a4-3a7d-4b66-9dd0-67382aede5ce.json
    │   ├── 5-889ad32d-64e5-4dba-bcef-48b8a1a3b204.json
    │   ├── 6-1e52863e-d222-7606-33ef-000000000006.json
    │   ├── 7-1e52863e-d222-7606-33ef-00000000000c.json
    │   ├── 8-c5d488f2-d157-4958-a572-eeafbea5d564.json
    │   └── 9-c1174640-7275-432a-a4e9-8ecfcbd56ec6.json
    ├── rc
    ├── status
    ├── stderr
    └── stdout

This requires:

diff --git a/demo/env/settings b/demo/env/settings
index 693b1d3..32e7b82 100644
--- a/demo/env/settings
+++ b/demo/env/settings
@@ -2,3 +2,5 @@
 idle_timeout: 600
 job_timeout: 3600
 pexpect_timeout: 10
+process_isolation: true
+process_isolation_executable: podman

This has an aesthetic consequence for users, and we might need to consider adding a blurb in the docs to give them confidence that this is intended.

If we can accept this added folder, then I believe this is a more maintainable pattern moving forward. It makes sense to me that we would cut a copy for every run (every job ident), because we don't want to assume that every job is ran by the same ansible-runner install.

For testing - I think we may need to consider the possibility that someone re-runs the same private_data_dir & ident. If we try to re-copy, I'm unclear what will happen. To spitball an expectation - I would prefer that we delete the existing folder.

AlanCoding

Code-wise, this looks complete at the moment. We might want some docs changes, since this touches the artifacts folder. I would consider back-porting to release_2.2, because I anticipate others having the same problems you had.

Akasurde · 2022-06-14T06:14:45Z

ansible_runner/config/_base.py

@@ -262,7 +263,18 @@ def _prepare_env(self, runner_mode='pexpect'):
                self.fact_cache = os.path.join(self.artifact_dir, self.settings['fact_cache'])

        # Use local callback directory
-        if not self.containerized:
+        if self.containerized:
+            # when containerized, copy the callback dir to $pirvate_data_dir/artifacts/<job_id>/callback


Suggested change

# when containerized, copy the callback dir to $pirvate_data_dir/artifacts/<job_id>/callback

# when containerized, copy the callback dir to $private_data_dir/artifacts/<job_id>/callback

Akasurde · 2022-06-14T06:15:15Z

ansible_runner/config/_base.py

-        if not self.containerized:
+        if self.containerized:
+            # when containerized, copy the callback dir to $pirvate_data_dir/artifacts/<job_id>/callback
+            # than append to env['Ansible_CALLBACK_PLUGINS'] with the copyied location.


Suggested change

# than append to env['Ansible_CALLBACK_PLUGINS'] with the copyied location.

# than append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copied location.

Akasurde · 2022-06-14T06:17:35Z

Code-wise, this looks complete at the moment. We might want some docs changes, since this touches the artifacts folder. I would consider back-porting to release_2.2, because I anticipate others having the same problems you had.

same here.

eqrx

Looks good to me except for the typos @Akasurde mentioned. thank you for your work :)

TheRealHaoLiu · 2022-06-14T11:46:40Z

commit amended with fix to type

Akasurde · 2022-06-14T12:07:37Z

ansible_runner/config/_base.py

-        if not self.containerized:
+        if self.containerized:
+            # when containerized, copy the callback dir to $private_data_dir/artifacts/<job_id>/callback
+            # than append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copyied location.


Suggested change

# than append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copyied location.

# then append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copied location.

Akasurde · 2022-06-14T12:07:50Z

ansible_runner/config/_base.py

+            # when containerized, copy the callback dir to $private_data_dir/artifacts/<job_id>/callback
+            # than append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copyied location.
+            callback_dir = os.path.join(self.artifact_dir, 'callback')
+            # if callback dir already exist (on repeat execution with same ident), remove it first.


Suggested change

# if callback dir already exist (on repeat execution with same ident), remove it first.

# if callback dir already exists (on repeat execution with the same ident), remove it first.

eqrx

Only two typos left :) Code looks great, thank you!

TheRealHaoLiu · 2022-06-14T20:29:11Z

Only two typos left :) Code looks great, thank you!

I gotta go find me a good vscode plugin >.<

when in containerized environment - always copy callback plugin to private_data_dir/artifacts/{job_id}/callback - appending to `ANSIBLE_CALLBACK_PLUGINS` with the runner callback plugin location (from private_data/artifacts) - unused method `utils.callback_mount` - update unit tests Signed-off-by: Hao Liu <haoli@redhat.com> Co-Authored-By: Alan Rominger <arominge@redhat.com> Signed-off-by: Hao Liu <haoli@redhat.com> Signed-off-by: Hao Liu <the.real.hao.liu@gmail.com>

TheRealHaoLiu · 2022-06-15T01:38:27Z

commit amended with fix to grammatical error

eqrx · 2022-06-15T09:16:39Z

Only two typos left :) Code looks great, thank you!

I gotta go find me a good vscode plugin >.<

no worries .... I do that all the time :D Luckily shrews still puts up with me

…sible#1093) when in containerized environment - always copy callback plugin to private_data_dir/artifacts/{job_id}/callback - appending to `ANSIBLE_CALLBACK_PLUGINS` with the runner callback plugin location (from private_data/artifacts) - unused method `utils.callback_mount` - update unit tests Signed-off-by: Hao Liu <haoli@redhat.com> Co-Authored-By: Alan Rominger <arominge@redhat.com> Signed-off-by: Hao Liu <haoli@redhat.com> Signed-off-by: Hao Liu <the.real.hao.liu@gmail.com> Co-authored-by: Alan Rominger <arominge@redhat.com> (cherry picked from commit 3b74385)

…sible#1093) when in containerized environment - always copy callback plugin to private_data_dir/artifacts/{job_id}/callback - appending to `ANSIBLE_CALLBACK_PLUGINS` with the runner callback plugin location (from private_data/artifacts) - unused method `utils.callback_mount` - update unit tests Co-authored-by: Alan Rominger <arominge@redhat.com> (cherry picked from commit 3b74385) Signed-off-by: Hao Liu <the.real.hao.liu@gmail.com>

) (#1098) when in containerized environment - always copy callback plugin to private_data_dir/artifacts/{job_id}/callback - appending to `ANSIBLE_CALLBACK_PLUGINS` with the runner callback plugin location (from private_data/artifacts) - unused method `utils.callback_mount` - update unit tests Co-authored-by: Alan Rominger <arominge@redhat.com> (cherry picked from commit 3b74385) Signed-off-by: Hao Liu <the.real.hao.liu@gmail.com> Co-authored-by: Alan Rominger <arominge@redhat.com>

TheRealHaoLiu requested a review from a team as a code owner June 6, 2022 15:09

shanemcd reviewed Jun 6, 2022

View reviewed changes

TheRealHaoLiu force-pushed the callback-dir-mount-containerized branch 3 times, most recently from a08300a to 8174803 Compare June 6, 2022 17:22

TheRealHaoLiu mentioned this pull request Jun 6, 2022

update callback_dir copy and mounting behavior #1089

Closed

AlanCoding reviewed Jun 6, 2022

View reviewed changes

TheRealHaoLiu force-pushed the callback-dir-mount-containerized branch 2 times, most recently from 6c52498 to 33d09be Compare June 6, 2022 18:29

TheRealHaoLiu requested review from shanemcd and AlanCoding June 6, 2022 19:26

AlanCoding approved these changes Jun 6, 2022

View reviewed changes

Shrews added the needs_triage New item that needs to be triaged label Jun 7, 2022

Shrews linked an issue Jun 7, 2022 that may be closed by this pull request

fail to create container when ansible-runner is installed in /usr #1088

Closed

Akasurde reviewed Jun 14, 2022

View reviewed changes

eqrx suggested changes Jun 14, 2022

View reviewed changes

TheRealHaoLiu force-pushed the callback-dir-mount-containerized branch from 33d09be to a2503f5 Compare June 14, 2022 11:46

TheRealHaoLiu requested review from eqrx and Akasurde June 14, 2022 11:56

Akasurde reviewed Jun 14, 2022

View reviewed changes

eqrx removed the needs_triage New item that needs to be triaged label Jun 14, 2022

eqrx suggested changes Jun 14, 2022

View reviewed changes

TheRealHaoLiu force-pushed the callback-dir-mount-containerized branch from a2503f5 to d73fe9f Compare June 14, 2022 20:32

Akasurde approved these changes Jun 15, 2022

View reviewed changes

TheRealHaoLiu requested a review from eqrx June 15, 2022 01:38

eqrx approved these changes Jun 15, 2022

View reviewed changes

eqrx merged commit 3b74385 into ansible:devel Jun 15, 2022

TheRealHaoLiu mentioned this pull request Jun 15, 2022

[backport][2.2] update how callback plugin gets copied and added to job container #1097

Closed

TheRealHaoLiu mentioned this pull request Jun 15, 2022

[backport][2.2]update how callback plugin gets copied and added to job container #1098

Merged

TheRealHaoLiu deleted the callback-dir-mount-containerized branch June 15, 2022 17:18

MalfuncEddie mentioned this pull request Aug 28, 2022

Artifacts not being set/persisted from AWX job_template runs ansible/awx#12750

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(alternative proposal) update how callback plugin gets copied and added to job container #1093

(alternative proposal) update how callback plugin gets copied and added to job container #1093

TheRealHaoLiu commented Jun 6, 2022 •

edited

shanemcd Jun 6, 2022

TheRealHaoLiu Jun 6, 2022

AlanCoding Jun 6, 2022

TheRealHaoLiu Jun 6, 2022 •

edited

AlanCoding commented Jun 6, 2022

AlanCoding left a comment

Akasurde Jun 14, 2022

Akasurde Jun 14, 2022

Akasurde commented Jun 14, 2022

eqrx left a comment

TheRealHaoLiu commented Jun 14, 2022

Akasurde Jun 14, 2022

Akasurde Jun 14, 2022

eqrx left a comment •

edited

TheRealHaoLiu commented Jun 14, 2022

TheRealHaoLiu commented Jun 15, 2022

eqrx commented Jun 15, 2022

	# when containerized, copy the callback dir to $pirvate_data_dir/artifacts/<job_id>/callback
	# when containerized, copy the callback dir to $private_data_dir/artifacts/<job_id>/callback

	# than append to env['Ansible_CALLBACK_PLUGINS'] with the copyied location.
	# than append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copied location.

	# than append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copyied location.
	# then append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copied location.

	# if callback dir already exist (on repeat execution with same ident), remove it first.
	# if callback dir already exists (on repeat execution with the same ident), remove it first.

(alternative proposal) update how callback plugin gets copied and added to job container #1093

(alternative proposal) update how callback plugin gets copied and added to job container #1093

Conversation

TheRealHaoLiu commented Jun 6, 2022 • edited

shanemcd Jun 6, 2022

Choose a reason for hiding this comment

TheRealHaoLiu Jun 6, 2022

Choose a reason for hiding this comment

AlanCoding Jun 6, 2022

Choose a reason for hiding this comment

TheRealHaoLiu Jun 6, 2022 • edited

Choose a reason for hiding this comment

AlanCoding commented Jun 6, 2022

AlanCoding left a comment

Choose a reason for hiding this comment

Akasurde Jun 14, 2022

Choose a reason for hiding this comment

Akasurde Jun 14, 2022

Choose a reason for hiding this comment

Akasurde commented Jun 14, 2022

eqrx left a comment

Choose a reason for hiding this comment

TheRealHaoLiu commented Jun 14, 2022

Akasurde Jun 14, 2022

Choose a reason for hiding this comment

Akasurde Jun 14, 2022

Choose a reason for hiding this comment

eqrx left a comment • edited

Choose a reason for hiding this comment

TheRealHaoLiu commented Jun 14, 2022

TheRealHaoLiu commented Jun 15, 2022

eqrx commented Jun 15, 2022

TheRealHaoLiu commented Jun 6, 2022 •

edited

TheRealHaoLiu Jun 6, 2022 •

edited

eqrx left a comment •

edited