-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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 a guestfs connexion plugin #13612
Conversation
d90746c
to
84457a6
Compare
|
||
self.guestfs.add_drive_opts(self.disk) | ||
|
||
def _find_python(self): |
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.
Shouldn't this use the value of ansible_python_interpreter? There is a mechanism (winrm.py uses it) to pass in some host variables to connection plugins, so this should probably implement that.
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 didn't knew about set_host_overrides, I will take a look.
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.
So, we need autodetection for Fedora and Ubuntu (since they come with python 3 pre installed, so I need to detect them for running raw to install python2). Now, I am not sure how should the plugin react if the user give a explicit python_interpreter, and if we can assume to be installed if given (especially for something like raw module, which should work without python).
In fact, I wonder if I shouldn't also have a wrapper in shell, since you can't use it on freebsd image with the current system of using a python wrapper if python is not installed (which is the case on freebsd among others). And I might need it soon for netbsd images.
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.
connection plugins should not have their own python logic. If distros are using different python/missing python it is up to user to add (plenty of examples with raw and ansible_python_interpreter).
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.
The python detection is used by the connexion plugin for internal matter, ie be able to run the script whose code is in the EXECUTION_SCRIPT constant, which is a wrapper that do encode stdin/stdout as json, because guestfish do not return the 2 streams separate.
This is separate and not related to the fact that ansible also use python. If perl was installed by default everywhere, I would use it, or if I had a good reason to believe that I could write a portable shell script doing the same without losing sanity points, I would do it.
I guess I could replace that part by a script that do execute the binary, dump stdout and stderr in 2 separate file and then get them, but this look more ugly than the current way (albeit more portable). And if I remember correctly, the python detection is needed so I can run raw or the various shell fragment in the first place. I can take a look again, because that code is not so fresh, but I am confident that if I could have avoided that, I would.
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.
this is why we have shell plugins .... but I do agree on the sanity part ...
@mscherer: Could you add this to the integration tests? Specifically, add a section to test_connection.inventory for guestfs and then make sure the tests pass by running:
|
Seems to work for me, I was able to point it to a qcow2 image and run ansible playbook against it. I only tested fedora images, so didn't try windows/ntfs/etc.
Would this include specifying a (local) libvirt/qemu url? ie, user vs system ? |
|
||
import os | ||
import os.path | ||
import guestfs |
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 should raise AnsibleError
when guestfs
python module isn't available.
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.
Good point, fixed in the pushed branch
af1d478
to
15521fc
Compare
@mattclay I added a first attempt at the test, I am not sure how to run them the correct way (and I rather avoid running disruptive test on my work laptop). It requires to download a cloud image to test, so I guess I should commit that file in git :) |
@alikins Yeah, I think so. It should be able to do all that you can do with guestfs command line. So we can use http://libguestfs.org/guestfish.1.html#adding-remote-storage for remote disks, and or also a different libvirt url if needed. Of course, people need to make sure that remote VM are shutdown for obvious concurency issue. I guess we could pass ansible_libvirt_uri variable or something, provided the connexion plugin can access that information (but I didn't dig that direction yet). |
Ok so we could pass more variable using the set_host_overrides system that @jimi-c spoke earlier, but I rather add the features later. |
@mscherer No need to include the image in git, just instructions on what is necessary to run the test, as it appears you've already done. If you can run |
So I did started to look at it (after Robyn talk on ansible container), and I did it a issue, and I think that's because we have test using UTF 8 string, but the Fedora cloud image I have do not seems to support utf-8. I will try to dig a bit, but if test requires a less featurefull image than the usual image for testing utf8, that might be a issue. |
9e6bb4c
to
514d51d
Compare
Ok so I fixed the first issue regarding utf8, now I hit another issue (so still trying to get it merged) |
c5ef5ec
to
76efc29
Compare
So I fixed the issue I had, and the tests are working (and are documented) on my laptop on RHEL 7. |
This permit to edit a base image in a similar fashion to packer, thus permitting for example to customize base image before uploading them directly to Glance or similar services for openstack, enabling immutable server deployment. It can be used like this: $ cat hosts fedora_22 ansible_ssh_host=Fedora-Cloud-Base-22-20150521.x86_64.qcow2 ansible_connection=guestfs $ cat deploy.yml --- - hosts: fedora_22 roles: - somerole $ ansible-playbook -i hosts deploy.yml [....] For now, it only support local VM with single OS, but it should be trivial to add support for remote disks and multiple OS.
76efc29
to
d8a6291
Compare
''' return the filename for the command output ''' | ||
return '%s/cmd.out' % self._tmp | ||
|
||
def exec_command(self, cmd, in_data=None, sudoable=False): |
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.
if not handling become methods update the class variable to indicate this
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 am not sure to understand. I suspect I need to change sudoable to True rather than False, becuase that seems to be the default everywhere else, but what class variable do I need to update ?
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 that, i'm talking about become_methods
list class variable.
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.
example that supports all defaults except 'su'
become_methods = frozenset(C.BECOME_METHODS).difference(('su',))
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 would need to test if sudo and su work I guess, as I didn't need them (but I can see why it would be required, at least 'su').
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.
su requires a tty, that is why most 'subprocess' plugins don't support it.
@mscherer Greetings! Thanks for taking the time to open this pullrequest. In order for the community to handle your pullrequest effectively, we need a bit more information. Here are the items we could not find in your description:
Please set the description of this pullrequest with this template: |
@mscherer Given that:
Therefore I'm going to close this. If you or anyone else wants to continue with this work then please do feel free to create a fresh PR and |
This permit to edit a base image in a similar fashion to packer,
thus permitting for example to customize base image before uploading
them directly to Glance or similar services for openstack, enabling
immutable server deployment.
It can be used like this:
For now, it only support local VM with single OS, but it should be trivial
to add support for remote disks and multiple OS.