-
Notifications
You must be signed in to change notification settings - Fork 23.8k
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
add connection plugin for buildah #26170
add connection plugin for buildah #26170
Conversation
The test
The test
The test
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nit. Outside of that, looks great!
|
||
@ensure_connect | ||
def exec_command(self, cmd, in_data=None, sudoable=False): | ||
""" Run a command on the docker host """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is misleading. It's actually running a command inside a buildah container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to update all the comments, these come from the docker connection plugin.
This is a thought, and not a blocker... Not being able to copy a file from the container is interesting, and problematic potentially from an Ansible Container standpoint. During a build, Ansible Container mounts I notice there is a |
Chris, that is a very good point -- how would we utilize conductor with buildah? I'm assuming that mount operation should be sufficient for the I will try to finish the PR today. |
cmd_bytes = to_bytes(cmd, errors='surrogate_or_strict') | ||
cmd_args_list = shlex.split(cmd_bytes) | ||
|
||
local_cmd = ['buildah', 'run', '--', self._container_id] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assumes that buildah is in path, i would check first and return a fatal exception if this is missing informing the user the need for it to exist.
stderr = to_bytes(stderr, errors='surrogate_or_strict') | ||
display.vvvvv("STDOUT %s STDERR %s" % (stdout, stderr)) | ||
|
||
def fetch_file(self, in_path, out_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see the 'dd' workaround we use in chroot and other 'local plugins' to get around this limitation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it with cat >$out_path
, is it okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that has many implications, starting by requiring specific shells and issues with encodings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how shells have anything to do with this: it's not even invoked via shell. The sample I posted above is a shortcut to:
local_cmd = ['buildah', 'run', '--', self._container_id, 'cat',
to_bytes(in_path, errors='surrogate_or_strict')]
with open(to_bytes(out_path, errors='surrogate_or_strict'), 'wb') as out_file:
p = subprocess.Popen(local_cmd, shell=False, stdin=subprocess.PIPE,
stdout=out_file, stderr=subprocess.PIPE)
which I hoped would be obvious, sorry for not being precisely specific.
I understand that encodings might get messy. I'll take a look on the other workarounds. I fear that doing it with dd
would not be possible since dd
would get executed in the container and we need to get the file to host (some mounting magic could do it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>
is a redirection, which ONLY works inside shells, and some shells don't use it the same way.
|
||
def close(self): | ||
super(Connection, self).close() | ||
# TODO: we should probably get rid of ~/.ansible directory in the container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no to the TODO, as this assumes exclusive access, which is not guaranteed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I will remove the TODO then
|
||
def _connect(self): | ||
""" Create a container from specified container image, via host """ | ||
super(Connection, self)._connect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this check that the instance actually exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean the container exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
# | ||
# You should have received a copy of the GNU General Public License | ||
# along with Ansible. If not, see <http://www.gnu.org/licenses/>. | ||
from __future__ import (absolute_import, division, print_function) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a requirement, but it would be nice to have docs here, in 2.4 we are adding docs to connection plugins also (see ssh plugin)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will definitely provide documentation
# TODO: save actual user and print it | ||
|
||
def _connect(self): | ||
""" Create a container from specified container image, via host """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment does not match code ... also connections should not be creating hosts, just connecting to them
Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
8a67ea7
to
8b94eee
Compare
stderr = to_bytes(stderr, errors='surrogate_or_strict') | ||
display.vvvvv("STDOUT %s STDERR %s" % (stdout, stderr)) | ||
|
||
def fetch_file(self, in_path, out_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that has many implications, starting by requiring specific shells and issues with encodings
@bcoca thanks for comments, I will try to resolve all of them on Monday. I have one question: how should I implement integration tests? Originally I did it the same way other connection plugins were doing it -- after the rebase, I realized it's the old way so I turned it into a role by copying the playbook of generic connection test -- please review. |
@TomasTomecek You should be able to re-use the existing generic connection test without copying the playbook. Look at connection_lxc for an example. You'll need to test locally with |
Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
@mattclay thanks for the tip! Unfortunately I'm running into an issue which I have no idea how to resolve:
How come that coverage is in place when I haven't used |
@TomasTomecek You'll need to set This can be done in the same way the connection_ssh test sets environment vars while reusing the existing connection test. |
thanks @mattclay, that helped a lot Unfortunately I stumbled into another one: containers/buildah#179
|
The test
|
Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
Any update here? @bcoca |
Thanks! I'm glad you squashed the commits. |
@TomasTomecek thank you for your job! I'm very interesting to start using Ansible + Buildha but ... I don't know how can I start using that connection type? Should I use https://docs.ansible.com/ansible-container/ ? |
@it-praktyk Here are some brief instructions for testing: https://github.com/TomasTomecek/ansible/blob/8fd3b209a81255781efbfaf614bfbade828a9565/test/integration/targets/connection_buildah/test_connection.inventory Let's get through a list of commands how you can use it with ansible in practice, this is just from top of my head. You need to make sure that you created the buildah container before running the playbook and named the container same as the host you specified in the playbook.
This PR includes an integration test so you can check it out. In the meantime, I'm working on support for buildah in |
Thank you for explanation. I'll try track a progress. Please let me know if will you need someone for testing. |
I have finally pushed myself into writing a blog post how this PR can be utilized: https://blog.tomecek.net/post/building-containers-with-buildah-and-ansible/ |
Signed-off-by: Tomas Tomecek ttomecek@redhat.com
SUMMARY
This PR introduces a new connection plugin for buildah.
The connection plugin should be utilized in Ansible Container to perform builds of container images without the need to use a dedicated daemon (read, dockerd).
ISSUE TYPE
COMPONENT NAME
plugin/connection/buildah
ANSIBLE VERSION
ADDITIONAL INFORMATION
If you look at the code, you can see a bunch of
TODO
s -- should I resolve all of them? The biggest one is: copying files from the container -- buildah is expected to create new container images, hence it doesn't support copying files from the container now -- shall I request such feature?CC @chouseknecht @gregdek @j00bar @nalind