-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 bhyve support to virt #47620
Add bhyve support to virt #47620
Conversation
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.
Your last two commits should be merged into one since they fix the first one. See my inline comments for the possible improvements, the most important one being rebasing on develop branch to benefit from the recent connection changes.
salt/modules/virt.py
Outdated
@@ -145,6 +146,7 @@ def __esxi_auth(): | |||
__esxi_auth(), | |||
0]], | |||
'qemu': [libvirt.open, [conn_str]], | |||
'bhyve': [libvirt.open, ['bhyve:///system']], |
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 whole connection mechanism has been rewritten recently. You don't need to add that anymore. See commit 2dfdc4e for more details.
salt/modules/virt.py
Outdated
context['enable_vnc'] = True | ||
else: | ||
context['enable_vnc'] = False | ||
context['enable_vnc'] = kwargs.get('enable_vnc', 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.
I know it's not yours, but I don't really like the enable_vnc
option in the first place since that rules out the other graphic types supported by libvirt like spice. Refactoring it for a broader support would be good (I may get to it sooner or later)
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 changes the default value of enable_vnc
. I'm not sure if you're ok with 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.
@astronouth7303 and yeah that's right
salt/modules/virt.py
Outdated
@@ -273,21 +274,28 @@ def _gen_xml(name, | |||
else: | |||
context['telnet_port'] = 23023 # FIXME: use random unused port | |||
if 'serial_type' in context: | |||
if context['serial_type'] == 'nmdm' and 'nmdm_id' in kwargs: | |||
context['nmdm_id'] = kwargs['nmdm_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.
It seems to me that this function is getting a lot of kwargs now, most of them undocumented. I wonder if we could come up with something cleaner than just adding kwargs every now and then.
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 comment still applies: nmdm_id
should be moved into a normal parameter with default value rather than a **kwargs
.
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.
Now bhyve has good support for VNC I'd suggest to omit the nmdm serial type.
salt/modules/virt.py
Outdated
@@ -1741,6 +1875,19 @@ def is_xen_hyper(): | |||
return 'libvirtd' in __salt__['cmd.run'](__grains__['ps']) | |||
|
|||
|
|||
def is_bhyve_hyper(): |
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.
For more flexibility I would deprecate the is_.*hyper
functions and replace them by a hypervisor_type
one.
I'm just starting to dive into the profiles system to improve it... and the more I think about it, the more I feel the hypervisor should be deduced from the libvirt connection that is used. This would make things simpler and more reliable in the virt module |
@jeroen92 Ping? I might have a stab at rebasing this and pushing it past the finish line, if you are too busy/unavailable. |
Pong :) I just got back from a holiday. I'll take a look at the comments and update the MR accordingly. |
@jeroen92 as I said to @nbraud yesterday on IRC, there is a massive refactoring of the virt module on going: https://github.com/cbosdo/salt/tree/virt-init It would be great to rebase your work on this once it stabilizes a bit (in a week or so) as I'm still working on it. |
@jeroen92 OK, thanks a lot :) |
@rallytime Ack, didn't know @cbosdo his changes were merged in already. Although I'm a bit short on time this week, I think I should be able to fix up this MR. |
@rallytime @cbosdo This MR has been updated :) |
@jeroen92 Cool! I'm traveling this week: don't expect fast validation from my side now, it'll be rather earlier next week. |
@jeroen92 Looks like there are some minor lint failures on this change: https://jenkinsci.saltstack.com/job/pr-lint/job/PR-47620/9/warnings52Result/new/ And one related test is failing: https://jenkinsci.saltstack.com/job/pr-kitchen-ubuntu1604-py2/job/PR-47620/2/ Mind fixing those up in the mean time? |
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.
Could you please address the comments and resubmit?
salt/modules/virt.py
Outdated
@@ -463,6 +464,7 @@ def _get_disks(dom): | |||
Get domain disks from a libvirt domain object. | |||
''' | |||
disks = {} | |||
hypervisor = __salt__['config.get']('libvirt:hypervisor', 'kvm') |
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.
don't use the libvirt:hypervisor
configuration: you should rather use the domain type. I have a pending commit here that removes the use of minidom though the whole module, but you can temporarily do it with minidom and I'll rebase my change later on.
Note that libvirt:hypervisor
is now deprecated.
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.
Ack.
salt/modules/virt.py
Outdated
@@ -600,6 +607,8 @@ def _gen_xml(name, | |||
else: | |||
context['boot_dev'] = ['hd'] | |||
|
|||
if 'loader' in kwargs: | |||
context['loader'] = kwargs['loader'] |
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.
There are enough **kwargs
to be removed in that module, don't add yet another one. Add a normal parameter with default value instead.
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.
Check, fixed this.
salt/modules/virt.py
Outdated
@@ -273,21 +274,28 @@ def _gen_xml(name, | |||
else: | |||
context['telnet_port'] = 23023 # FIXME: use random unused port | |||
if 'serial_type' in context: | |||
if context['serial_type'] == 'nmdm' and 'nmdm_id' in kwargs: | |||
context['nmdm_id'] = kwargs['nmdm_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.
That comment still applies: nmdm_id
should be moved into a normal parameter with default value rather than a **kwargs
.
salt/modules/virt.py
Outdated
disk_filesystem: tank/disks | ||
size: 20G | ||
hostname_property: virt:hostname | ||
sparse_volume: True |
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 documentation will not be visible to the end user: move this into the virt.init
disks documentation
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 goes for a lot of documented functions (i.e. disk_profile). The dict keys should be documented at init(), but I feel the rest of the docs are a bit out of place there..
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.
@jeroen92 for sure some of the other docs need to be moved somewhere else... but let's do it right for new docs
elif hypervisor in ['bhyve']: | ||
overlay = {'format': 'raw', | ||
'model': 'virtio', | ||
'sparse_volume': 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.
you'll need to change the function name in a separate commit since it doesn't create only qemu 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.
Change which function name?
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.
_qemu_image_create()
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 touch the contents of _qemu_image_create()
. The code block referenced by your comment resides in _disk_profile()
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.
oh my bad... sorry for the inconvenience, it looks like I misread the patch ;)
salt/modules/virt.py
Outdated
try: | ||
dom.undefineFlags(libvirt.VIR_DOMAIN_UNDEFINE_NVRAM) | ||
except libvirt.libvirtError as ex: | ||
dom.undefine() |
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 hunk should go in a separate commit since that has nothing to do with bhyve
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.
It does. On FreeBSD calling undefineFlags raises libvirt: Domain Config error : this function is not supported by the connection driver: virDomainUndefineFlags
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.
ok, but I suspect this is not the only driver in that case. I still think it would be better to have it in a separate commit with what you just wrote as commit description to explain the reason of the change
salt/modules/virt.py
Outdated
@@ -3466,6 +3610,7 @@ def _parse_caps_guest(guest): | |||
'machines': {}, | |||
'domains': {} | |||
}, | |||
'features': {} |
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 will likely break the capabilities unit tests, and I'm not even sure it is needed with the next hunk
salt/modules/virt.py
Outdated
@@ -3664,7 +3809,6 @@ def capabilities(**kwargs): | |||
conn = __get_conn(**kwargs) | |||
caps = _capabilities(conn) | |||
conn.close() | |||
|
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.
Keep the line around. Cosmetic changes should go in their own commit.
@@ -7,6 +7,9 @@ | |||
{% for dev in boot_dev %} | |||
<boot dev='{{ dev }}' /> | |||
{% endfor %} | |||
{% if loader %} | |||
<loader readonly='yes' type='pflash'>{{ loader }}</loader> |
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.
readonly
should be configurable and allow a no
value.
type
should be configurable too to allow the rom
value
While at it, could you add a secure
flag (value yes or no) as documented in https://libvirt.org/formatdomain.html#elementsOS
Add the whole loader section in the template should go in it own commit since that can also be used for Xen or QEMU guests.
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.
Ack, changed the loader argument accordingly.
<serial type='nmdm'> | ||
<source master='/dev/nmdm{{ nmdm_id }}A' slave='/dev/nmdm{{ nmdm_id }}B'/> | ||
</serial> | ||
{% elif serial_type == 'pty' %} |
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 indentation is off here. Please adjust to match the surrounding code.
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'ld love to see some unit tests for the new serial type and the loader element to avoid breaking the features you need with future potential refactoring.
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.
Since bhyve/libvirt has excellent support for VNC, this part isn't useful for me anymore. Hence, I'm going to revert the nmdm changes from my commits as I don't have the means to test this bit.
Note, there may be a race with PR #48736, the last one to be in needs to rebase on the other ;) |
I haven't tested the unittests yet, since I was getting all kinds of unicode related errors. So those are still pending, as well as splitting the commits. Oh well, and the rebase ofc..
|
@jeroen92 How are you running the tests? the assert looks good to me. Which version of python is that? |
See below on how I'm running the tests. I've followed
|
@jeroen92 What I do is that I created a virtualenv in the base folder of the tree, I source it's
|
b28bd1a
to
eecd88a
Compare
@cbosdo Updated the MR. All tests now pass, rebased onto develop and discussions are resolved. |
Bump @cbosdo for re-review please. :) |
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.
Sorry for the late review: I'm just back from vacation.
Looks good to me, with two small things to fix. Next time we'll be able to push ;)
salt/modules/virt.py
Outdated
@@ -454,6 +468,11 @@ def _get_disks(dom): | |||
qemu_target = source.get('file', '') | |||
if not qemu_target: | |||
qemu_target = source.get('dev', '') | |||
if source.get('file').startswith('/dev/zvol/'): |
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 problem with source.get('file')
is that you may get None
if no there is no file
. Either you move that block above the if not qemu_target:
and use qemu_target
value or you need to default to the empty string.
salt/modules/virt.py
Outdated
:param vm_: name of the domain | ||
:param connection: libvirt connection URI, overriding defaults | ||
|
||
.. versionadded:: Fluorine |
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 don't need to have a .. versionadded:: Fluorine
on all these properties since the whole function is brand new. Just add one .. versionadded
on the function level.
@cachedout @cbosdo Updated :) |
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.
LGTM, thanks for the effort!
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.
LGTM
@jeroen92 There is a related test failing on this change. Can you take a look? https://jenkinsci.saltstack.com/job/pr-kitchen-centos7-py3/job/PR-47620/11/ |
51b4b04
to
29a44ac
Compare
@rallytime Thanks for checking, I missed that one. Should be fixed now. |
What does this PR do?
Add bhyve support to the virt module, and add ZFS compatibility for cloning/creating zvols when using bhyve as a hypervisor.
What issues does this PR fix or reference?
#47619
Previous Behavior
Not implemented. Crashes when performing virt.init for a bhyve hypervisor.
New Behavior
Using virt.init for a bhyve hypervisor will create a VM with (multiple) ZFS provisioned disks. Using virt.purge will destroy the VM and the corresponding ZFS datasets.
Tests written?
No
Commits signed with GPG?
No