Modify volume mount behavior when source does not exist

[demonpig]

ansible-runner will ignore adding the mountpoint to the '-v` argument list if the source path does not exist. Instead it should respect whatever the user is passing to the '--container-volume-mount' option. The exception would be if the user tried to pass a file as the source path; ansible-runner should resolve the file's parent directory.

[demonpig]

I'm not entirely sure what to do about the failing unit tests. Please advise.

[Shrews]

I had to research this behavior a bit, but it appears that docker and podman handle these cases differently, which is curious to me since I thought they tried to mostly be 1-to-1 compatible.

• The docker run help page indicates that when the source volume mount does not exist, it will be created for you.
• On the other hand, the podman run help page indicates that podman will throw an error when the source volume does not exist and users "must pre-create the source files or directories".

Given the differing behavior, I'm not in favor of this change as it is since it would suddenly cause users who may be using podman to get unexpected errors during a different/later phase of ansible-runner. If you were to modify the change to consider which container engine is in use (i.e., ignore the existence check if using docker), I'd be more in favor of it. This also might clear up some of the unit test failures, but likely not all of them.

[Shrews]

@demonpig After some discussion with others around this, I'm starting to come around to the idea of just keeping this as-is and letting the container engine do whatever it's going to do. I haven't had a chance to explore your unit test failures, but you should check how your changes affect the output that is expected when comparing the generated run command.

[demonpig]

@Shrews Thanks for the updates. I'll take a look at them and see what I can do to fix them.

[demonpig]

Howdy! Was wondering if anyone could share their opinions on how to approach the failing tests. My initial thoughts are to redo them in order to get them to pass. I have been trying to figure out how to change the code within the _update_volume_mount_paths function to get it to pass these tests but am not able to come up with anything.

I'm trying to find balance between the following items:

• Preserve the volume mounts that the user passes into ansible-runner
• Preserve some of the safety mechanisms ansible-runner has in place such as making sure not to mount in /home, /, and /usr.

Any thoughts / ideas would be appreciated.

[sivel]

I believe a fair amount of tests will probably need to be updated, as they were expecting missing paths to be removed.

I haven't evaluated the changes after line 400. Do we need those extra changes? Or is it sufficient enough to just not return early?

[demonpig]

Do we need those extra changes? Or is it sufficient enough to just not return early?

I'll check right now.

[sivel]

Ok, I've checked and basically all of the other "is this a dir" and such fail, and we modify things we shouldn't be. Because we assume if it's not a dir, it's a file and need to do basename on it. Which further messes with the paths when it shouldn't.

I'm thinking we need to wrap all of the manipulations in 'if os.path.exists:`. And just skip all of the path manipulations if the file doesn't.

Although this is imprecise. I think what we really need to know is whether we are running in a streaming setup, and skip all of the manipulations then. Otherwise we're making a lot of assumptions that can be wrong, because we don't know the operating intent. However, I don't believe the config knows anything about whether it is operating in this manner.

I'm kind of "talking out loud" because I'm not sure how to make this do what it really needs in the scenarios it needs them.

[demonpig]

So "it works" but not to what I was expecting. The ./mounts2 does not exist on the filesystem and I wanted it mounted on /var/test/test. Instead I got /tmp/tmp.y978aJsoNg/:/var/test/.

ansible-runner command:

$ ansible-runner run . --playbook playbook.yml -i inventory.yml --process-isolation --container-image registry.redhat.io/ansible-automation-platform-24/ee-minimal-rhel8:latest -vv --debug --container-volume-mount ./mounts2:/var/test/test | grep 'command:'

Translated podman command:

command: podman run --rm --tty --interactive --workdir /runner/project -v /tmp/tmp.y978aJsoNg/:/runner/:Z -v **/tmp/tmp.y978aJsoNg/:/var/test/** --env-file /tmp/tmp.y978aJsoNg/artifacts/inventory.yml/env.list --quiet --name ansible_runner_inventory_yml registry.redhat.io/ansible-automation-platform-24/ee-minimal-rhel8:latest ansible-playbook -i /runner/inventory -e @/runner/env/extravars -vv playbook.yml

[sivel]

After "talking" that through, I feel like some changes being made in #1397 could help, but I'm wondering if we move streamer into the config.

That would then let us know we are in streaming mode, and could then skip path checking and manipulation.

[demonpig]

@sivel Sounds good. I'm not too familiar with #1397 but if that is the easier route to go, then I'm all for it. Essentially, I think the only way to fix the issue with the containerized offering of AAP is to fix this one issue. I was able to trace the issue down to ansible-runner "changing" the mount options when the source directory didn't exist.

[sivel]

We're talking this over more. We'll try to get back to you soon.

[Shrews]

Ok, so bear with me here as I brain dump some things I found.

I mapped out the surface area of the impact of this change (calls to the _update_volume_mount_paths() method) because I was unclear on its usage. Here's what I found:

Call location	Purpose	Other existence check	Note
_handle_ansible_cmd_options_bind_mounts()	playbook file path	No	with run_command() API and containerized
_handle_ansible_cmd_options_bind_mounts()	inventory file path	No	with run_command() API and containerized
_handle_ansible_cmd_options_bind_mounts()	vault password file path	No	with run_command() API and containerized
_handle_ansible_cmd_options_bind_mounts()	private key file path	No	with run_command() API and containerized
_handle_ansible_cmd_options_bind_mounts()	host CWD	Yes	with run_command() API and containerized
wrap_args_for_containerization()	host artifacts directory	Yes	when containerized
wrap_args_for_containerization()	private data directory	Yes	when containerized
wrap_args_for_containerization()	container volume mounts	No	when containerized
_handle_automounts()	$SSH_AUTH_SOCK file	No	with run_command() API and containerized
_handle_automounts()	$HOME/.ssh	Yes	with run_command() API and containerized
_handle_automounts()	/etc/ssh/ssh_known_hosts	Yes	with run_command() API and containerized

• The first 4 examples seem to totally break the run_command() API whenever the options to supply those files are given.
• The last 3 do some crazy thing where they mount the parent directory instead of the file for some unknown reason. This is already a known issue we've experienced in the past, and a recent issue was just opened on it, too.
• The others seem mostly valid, and this PR is changing one of them: "container volume mounts"

I'm of the opinion that trying to modify a function that is so totally broken, and does so many things wrong, is going to be pointless to attempt. I'm leaning towards just directly adding the mounts at the call point for this method for the container volume mounts (i.e., inside wrap_args_for_containerization() itself).

Anyone else have thoughts about that suggestion?

[sivel]

Inside wrap_args_for_containerization seems like the place to move it. Just took me a few to understand when that was actually called.

[demonpig]

Reverted my original changes and adjusted the wrap_args_for_containerization according to the above discussion.

[Shrews]

I feel like this current iteration might be the way to go, albeit some tests need adjusting, but I also feel it is significant enough of a change that we need to have a porting guide entry for it. The porting guide is being added in PR #1397, so it would need to be committed after that one. We'd also want to consult with the Controller team on the changes since these differences are not insignificant.

Some potential things we'd might want to mention in the guide, and to be considered in the interaction with Controller, and across the board:

• Under docker, a non-existing source path would be created for you, and always as a directory.
• Under podman, a non-existing source path would now cause an error later in the process.
• For both docker and podman, a non-path like source (does not begin with . or /) would now create a volume with that name instead of erroring. The implications of this worry me a bit, tbh.

AIUI, the first point is your main reasoning for the change, but it's the implications of the other points that we should think about a bit. The main thing we don't want to do is break anything in Controller.

[demonpig]

@Shrews Did you want the tests updated within this PR or in another?

[Shrews]

@Shrews Did you want the tests updated within this PR or in another?

Probably not much point in updating tests yet until we know for sure if this change negatively affects Controller or not.