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

Stop using `buildah mount` #57552

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
5 participants
@jordemort
Copy link

commented Jun 7, 2019

SUMMARY

This PR changes the buildah connection plugin to no longer use or rely on buildah mount - instead, it uses buildah copy for put_file, and runs dd inside of the container for fetch_file. This was mostly a matter of copying and pasting a bunch of code from the chroot connection plugin.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

plugins/connection/buildah.py

ADDITIONAL INFORMATION

This works around containers/buildah#1509 - buildah mount doesn't mount a container's volumes, only its root filesystem, so any paths located on volumes end up being inaccessible to Ansible. As discussed in ansible-community/ansible-bender#139 - cc @jamescassell and @TomasTomecek

@jamescassell

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Does this offer any benefits over just fixing buildah?

@jordemort

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

@jamescassell It saves the overhead of having to run buildah mount before and buildah unmount after every command

@jordemort

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

And of course buildah mount leaves state behind so if Ansible somehow died badly without getting to run buildah unmount it would leave that behind as well; this makes things fully stateless, at least as far as interaction with buildah is concerned.

@jordemort

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

Here's some shots of it in action. First, put_file:

18:09:26.044 utils.py          DEBUG  <dockto-local-base-20190607-180255094322-cont> THIS IS A BUILDAH CONTAINER
18:09:26.044 utils.py          DEBUG  <dockto-local-base-20190607-180255094322-cont> RUN [b'buildah', b'run', b'--', b'dockto-local-base-20190607-180255094322-cont', b'/bin/sh', b'-c', b'( umask 77 && mkdir -p "` echo /tmp/ansible-tmp-1559930966.0434167-222837254419318 `" && echo ansible-tmp-1559930966.0434167-222837254419318="` echo /tmp/ansible-tmp-1559930966.0434167-222837254419318 `" ) && sleep 0']
18:09:27.187 utils.py          DEBUG  Using module file /opt/dockto/lib/python3.7/site-packages/ansible/modules/system/debconf.py
18:09:27.187 utils.py          DEBUG  <dockto-local-base-20190607-180255094322-cont> PUT /root/.ansible/tmp/ansible-local-649oeixg8bc/tmp61k_51vc TO /tmp/ansible-tmp-1559930966.0434167-222837254419318/AnsiballZ_debconf.py
18:09:27.188 utils.py          DEBUG  <dockto-local-base-20190607-180255094322-cont> RUN [b'buildah', b'copy', b'--', b'dockto-local-base-20190607-180255094322-cont', b'/root/.ansible/tmp/ansible-local-649oeixg8bc/tmp61k_51vc', b'/tmp/ansible-tmp-1559930966.0434167-222837254419318/AnsiballZ_debconf.py']
18:09:27.696 utils.py          DEBUG  <dockto-local-base-20190607-180255094322-cont> RUN [b'buildah', b'run', b'--', b'dockto-local-base-20190607-180255094322-cont', b'/bin/sh', b'-c', b'chmod u+x /tmp/ansible-tmp-1559930966.0434167-222837254419318/ /tmp/ansible-tmp-1559930966.0434167-222837254419318/AnsiballZ_debconf.py && sleep 0']
18:09:28.804 utils.py          DEBUG  <dockto-local-base-20190607-180255094322-cont> RUN [b'buildah', b'run', b'--', b'dockto-local-base-20190607-180255094322-cont', b'/bin/sh', b'-c', b'/opt/dockto/bin/python3.7m /tmp/ansible-tmp-1559930966.0434167-222837254419318/AnsiballZ_debconf.py && sleep 0']
18:09:30.401 utils.py          DEBUG  <dockto-local-base-20190607-180255094322-cont> RUN [b'buildah', b'run', b'--', b'dockto-local-base-20190607-180255094322-cont', b'/bin/sh', b'-c', b'rm -f -r /tmp/ansible-tmp-1559930966.0434167-222837254419318/ > /dev/null 2>&1 && sleep 0']

...and then fetch_file:

18:09:12.809 utils.py          DEBUG  <dockto-local-base-20190607-180255094322-cont> FETCH /etc/passwd TO /out/base.passwd/dockto-local-base-20190607-180255094322-cont/etc/passwd
18:09:12.809 utils.py          DEBUG  <dockto-local-base-20190607-180255094322-cont> RUN [b'buildah', b'run', b'--', b'dockto-local-base-20190607-180255094322-cont', b'dd', b'if=/etc/passwd', b'bs=65536']
18:09:14.042 utils.py          DEBUG  <dockto-local-base-20190607-180255094322-cont> RUN [b'buildah', b'run', b'--', b'dockto-local-base-20190607-180255094322-cont', b'/bin/sh', b'-c', b'rm -f -r /tmp/ansible-tmp-1559930948.473485-32430100487254/ > /dev/null 2>&1 && sleep 0']

Going to take it out of draft status now that I've verified it works

@jordemort jordemort marked this pull request as ready for review Jun 7, 2019

@jamescassell

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Sounds good to me, though I haven't tested yet

Stop using `buildah mount`
Instead, use `buildah copy` for `put_file` and `buildah run dd` for `fetch_file`
Also, fix pipelining

Borrows a bunch of code from chroot.py

@jordemort jordemort force-pushed the jordemort:buildah-copy branch from 31a8841 to 1d1172e Jun 8, 2019

@jordemort

This comment has been minimized.

Copy link
Author

commented Jun 8, 2019

Found the missing in_data that was keeping pipelining from working. With this (and a buildah with containers/buildah#1653 applied), ansible-bender can be run with pipelining turned on, which sped up one of my container builds from 12m to 7m30s.

@TomasTomecek

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

SUMMARY

This PR changes the buildah connection plugin to no longer use or rely on buildah mount - instead, it uses buildah copy for put_file, and runs dd inside of the container for fetch_file.

dd seems like a good solution since it's part of coreutils

and yeah, buildah mount is flaky, especially rootless: the last big issue was when I had to wrap buildah mount with buildah unshare. So using buildah copy and dd could be more stable in the future.

@jamescassell It saves the overhead of having to run buildah mount before and buildah unmount after every command

Well, we could also run mount/unmount before/after a playbook run to save resources.

which sped up one of my container builds from 12m to 7m30s.

wow, awesome!

to_bytes(out_path, errors='surrogate_or_strict')
)
in_path = shlex_quote(self._prefix_login_path(in_path))
p = self._buffered_buildah("run", ['dd', 'if=%s' % in_path, 'bs=%s' % BUFSIZE])

This comment has been minimized.

Copy link
@TomasTomecek

TomasTomecek Jun 8, 2019

Contributor

It would be awesome if buildah supported copying from a container.

This comment has been minimized.

Copy link
@jordemort

jordemort Jun 8, 2019

Author

I played around with the possibility of using podman cp here if podman was available but it doesn't seem like podman can see buildah's working containers; as far as I can tell, they only share storage.

@TomasTomecek
Copy link
Contributor

left a comment

I actually debate if we could leave the former behaviour, make the new one default, and via an env var, be able to change back to the old one: once one solution stops working, have an ability to switch to the other one. Just an idea.

Edit: Also, since we now have podman conn plugin in 2.8, I should finally implement it into bender so there are more options, not just buildah.
Edit2: typo

@jordemort

This comment has been minimized.

Copy link
Author

commented Jun 8, 2019

dd seems like a good solution since it's part of coreutils

I stole the idea to use dd, along with most of the code I added here, from the chroot connection. I put very little original thought into this, even the put_file implementation is just the alternate one you had commented out with an exception handler tacked on.

I actually debate if we could leave to former behaviour, make the new one default, and via an env var, be able to change back to the old one: once one solution stops working, have an ability to switch to the other one. Just an idea.

I would be willing to add that, but as a user I'm having a hard time picturing when I'd use that environment variable. It would also necessitate adding tracking of the mount state back in; I guess we could define completely different versions of _connect based on the environment? From a developer perspective, Git will remember the old implementation, if it ever needs to be restored.

@TomasTomecek

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

I would be willing to add that, but as a user I'm having a hard time picturing when I'd use that environment variable.

I imagine something like this:

$ ANSIBLE_CONNECTION_BUILDAH_COPY_MODE=mount ansible-playbook -c buildah -i inv playbook...

It would also necessitate adding tracking of the mount state back in; I guess we could define completely different versions of _connect based on the environment?

Yup, sounds good.

But I'm not sure if that's really worth of doing. So let's leave this as it is now. We can always change in future.

@jordemort

This comment has been minimized.

Copy link
Author

commented Jun 14, 2019

So, we might want to put a hold on this because it seems like this suffers from the same problem that buildah mount does; i.e. can only copy files into the container rootfs, not the volumes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.