update callback_dir copy and mounting behavior #1089
Conversation
- enhance callback_mount copy_if_needed check to include unmountable location `/usr` - add option to specify location when copying callback plugin - if callback dir copy is needed copy to artifact directory rather than /tmp Signed-off-by: Hao Liu <haoli@redhat.com>
TheRealHaoLiu
force-pushed
the
enhance-callback-plugin-mount
branch
from
eb3a409
to
39107af
Compare
| tmp_path = tempfile.mkdtemp(prefix='ansible_runner_plugins_') | ||
| register_for_cleanup(tmp_path) | ||
| host_path = os.path.join(tmp_path, 'callback') | ||
| if callback_dir.startswith('/usr') or not is_dir_owner(callback_dir): |
There was a problem hiding this comment.
add check for if the source is from an unmountable dir (/usr)
There was a problem hiding this comment.
I'm curious if we could have some more descriptive criteria than .startswith. What is it about the /usr folder that makes it un-mountable, and can we check that criteria more directly?
| if copy_dir is None: | ||
| tmp_path = tempfile.mkdtemp(prefix='ansible_runner_plugins_') | ||
| register_for_cleanup(tmp_path) | ||
| host_path = os.path.join(tmp_path, 'callback') | ||
| else: | ||
| host_path = os.path.join(copy_dir, 'callback') |
There was a problem hiding this comment.
does not change how callback_mount when call like before
callback_mount(copy_if_needed=False) will still behave like before
| @@ -505,7 +505,7 @@ def wrap_args_for_containerization(self, args, execution_mode, cmdline_args): | |||
| self._update_volume_mount_paths(new_args, "{}".format(self.private_data_dir), dst_mount_path="/runner", labels=":Z") | |||
|
|
|||
| # Mount the stdout callback plugin from the ansible-runner code base | |||
| mount_paths = callback_mount(copy_if_needed=True) | |||
| mount_paths = callback_mount(copy_if_needed=True, copy_dir="{}/artifacts".format(self.private_data_dir)) | |||
There was a problem hiding this comment.
$private_data_dir/artifacts is a good candidate for copying the callback plugin to for the following reasons
- clean up after run like tmpdir so no accumulation of garbage
- have to be (and is therefore always) mountable by the container that are created
| host_path = os.path.join(tmp_path, 'callback') | ||
| if callback_dir.startswith('/usr') or not is_dir_owner(callback_dir): | ||
| if copy_dir is None: | ||
| tmp_path = tempfile.mkdtemp(prefix='ansible_runner_plugins_') |
There was a problem hiding this comment.
Probably better to get rid of this entirely. You have a good case that the artifacts dir is a better location, and if it doesn't cause any problems with testing, then we should use that location in all cases, and I'd say copy_dir shouldn't be an argument here.
There was a problem hiding this comment.
what is the best way to get the correct private_data_dir from the context of this function?
reason why i preserve the previous method signature is because i do not know what else is relying on this method (internal or imported by other project)
pull change from - ansible#1084 - ansible#1089
|
#1093 alternative fix proposal |
|
closed due to alternative proposal #1093 being accepted |
#1088
/usrSigned-off-by: Hao Liu haoli@redhat.com