Add bhyve support to virt by jeroen92 · Pull Request #47620 · saltstack/salt

jeroen92 · 2018-05-11T21:15:38Z

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

cbosdo

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.

cbosdo · 2018-05-25T10:01:38Z

The whole connection mechanism has been rewritten recently. You don't need to add that anymore. See commit 2dfdc4e for more details.

cbosdo · 2018-05-25T10:06:32Z

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)

This changes the default value of enable_vnc. I'm not sure if you're ok with that.

@astronouth7303 and yeah that's right

cbosdo · 2018-05-25T10:08:32Z

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.

That comment still applies: nmdm_id should be moved into a normal parameter with default value rather than a **kwargs.

Now bhyve has good support for VNC I'd suggest to omit the nmdm serial type.

cbosdo · 2018-05-25T11:39:33Z

For more flexibility I would deprecate the is_.*hyper functions and replace them by a hypervisor_type one.

rallytime · 2018-06-08T17:24:55Z

Hi @jeroen92 - Did you get a chance to look over the feedback from @cbosdo above?

This change will also need a rebase as there is a merge conflict.

cbosdo · 2018-06-11T11:36:45Z

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

nicoonoclaste · 2018-06-18T15:59:08Z

@jeroen92 Ping?

I might have a stab at rebasing this and pushing it past the finish line, if you are too busy/unavailable.

jeroen92 · 2018-06-18T18:14:37Z

Pong :)

I just got back from a holiday. I'll take a look at the comments and update the MR accordingly.

cbosdo · 2018-06-19T06:54:25Z

@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.

nicoonoclaste · 2018-06-19T22:55:41Z

@jeroen92 OK, thanks a lot :)

rallytime · 2018-07-06T18:54:56Z

@jeroen92 and @cbosdo where did we land with this change? I think a lot of the work @cbosdo has been working was merged in recently. (And this PR will need to be rebased once it's time to work on this again as there is a merge conflict.

Let us know when this is ready. :)

jeroen92 · 2018-07-08T16:52:35Z

@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.

jeroen92 · 2018-07-18T21:39:02Z

@rallytime @cbosdo This MR has been updated :)

cbosdo · 2018-07-18T22:27:01Z

@jeroen92 Cool! I'm traveling this week: don't expect fast validation from my side now, it'll be rather earlier next week.

rallytime · 2018-07-20T14:52:05Z

@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: unit.modules.test_virt.VirtTestCase.test_capabilities

https://jenkinsci.saltstack.com/job/pr-kitchen-ubuntu1604-py2/job/PR-47620/2/

Mind fixing those up in the mean time?

cbosdo

Could you please address the comments and resubmit?

cbosdo · 2018-07-23T13:12:04Z

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.

cbosdo · 2018-07-23T13:13:24Z

There are enough **kwargs to be removed in that module, don't add yet another one. Add a normal parameter with default value instead.

Check, fixed this.

cbosdo · 2018-07-23T13:14:56Z

That comment still applies: nmdm_id should be moved into a normal parameter with default value rather than a **kwargs.

cbosdo · 2018-07-23T13:16:40Z

This documentation will not be visible to the end user: move this into the virt.init disks documentation

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..

@jeroen92 for sure some of the other docs need to be moved somewhere else... but let's do it right for new docs

cbosdo · 2018-07-23T13:18:26Z

you'll need to change the function name in a separate commit since it doesn't create only qemu images ;)

Change which function name?

_qemu_image_create()

I didn't touch the contents of _qemu_image_create(). The code block referenced by your comment resides in _disk_profile()

oh my bad... sorry for the inconvenience, it looks like I misread the patch ;)

cbosdo · 2018-07-23T13:22:16Z

That hunk should go in a separate commit since that has nothing to do with bhyve

It does. On FreeBSD calling undefineFlags raises libvirt: Domain Config error : this function is not supported by the connection driver: virDomainUndefineFlags

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

cbosdo · 2018-07-23T13:23:11Z

This will likely break the capabilities unit tests, and I'm not even sure it is needed with the next hunk

cbosdo · 2018-07-23T13:25:29Z

Keep the line around. Cosmetic changes should go in their own commit.

cbosdo · 2018-07-23T13:30:25Z

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.

Ack, changed the loader argument accordingly.

cbosdo · 2018-07-23T13:31:30Z

The indentation is off here. Please adjust to match the surrounding code.

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.

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.

cbosdo · 2018-07-24T14:46:14Z

Note, there may be a race with PR #48736, the last one to be in needs to rebase on the other ;)

jeroen92 · 2018-07-29T22:07:31Z

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..

............................................................................. -> unit.modules.test_virt.VirtTestCase.test_gen_xml_loader_default ......... Traceback (most recent call last): File "/root/salt/tests/unit/modules/test_virt.py", line 361, in test_gen_xml_loader_default self.assertEqual(root.find('os/loader').attrib['type'], 'pflash') AssertionError: 'fplash' != u'pflash' .............................................................................

cbosdo · 2018-07-30T07:35:00Z

@jeroen92 How are you running the tests? the assert looks good to me. Which version of python is that?

jeroen92 · 2018-07-30T20:02:43Z

See below on how I'm running the tests. I've followed Salt's Test Suite: An Introduction when setting up the develop environment. Besides the unicode thingie, I'm also getting a bunch of other errors though.

$ python runtests.py --name unit.modules.test_virt
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 * Python Version: 2.7.15 (default, Jun 6 2018, 21:13:14) [GCC 4.2.1 Compatible FreeBSD Clang 4.0.0 (tags/RELEASE_400/final 297347)]
 * Transplanting configuration files to '/tmp/salt-tests-tmpdir/config'
 * Current Directory: /root/salt
 * Test suite is running under PID 89553
 * Logging tests on /tmp/salt-runtests.log
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Starting unit.modules.test_virt Tests
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
EEEFEE..........EEEEEEEEEFEEEEEEE...................EE
======================================================================
ERROR: test_boot_custom_dev (unit.modules.test_virt.VirtTestCase)
[CPU:44.5%|MEM:97.3%]
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/salt/tests/unit/modules/test_virt.py", line 136, in test_boot_custom_dev
    boot_dev='cdrom'
  File "/root/salt/salt/modules/virt.py", line 627, in _gen_xml
    for key, val in loader.items():
AttributeError: 'NoneType' object has no attribute 'items'

======================================================================
ERROR: test_boot_default_dev (unit.modules.test_virt.VirtTestCase)
[CPU:50.0%|MEM:97.3%]
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/salt/tests/unit/modules/test_virt.py", line 118, in test_boot_default_dev
    'kvm'
  File "/root/salt/salt/modules/virt.py", line 627, in _gen_xml
    for key, val in loader.items():
AttributeError: 'NoneType' object has no attribute 'items'
....

cbosdo · 2018-07-31T07:56:18Z

@jeroen92 What I do is that I created a virtualenv in the base folder of the tree, I source it's activate file, then installed all the py3 deps. to run the test I just

cd tests
./runtests.py -n unit.modules.test_virt

jeroen92 · 2018-08-12T18:13:06Z

@cbosdo Updated the MR. All tests now pass, rebased onto develop and discussions are resolved.

cachedout · 2018-08-15T08:54:01Z

Bump @cbosdo for re-review please. :)

cbosdo

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 ;)

cbosdo · 2018-08-20T08:33:49Z

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.

cbosdo · 2018-08-20T08:42:50Z

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 · 2018-08-20T14:01:49Z

@cbosdo Thank you! (Hope your vacation was wonderful.)

@jeroen92 Could you review the latest feedback?

jeroen92 · 2018-08-20T20:34:03Z

@cachedout @cbosdo Updated :)

cbosdo

LGTM, thanks for the effort!

rallytime

LGTM

rallytime · 2018-08-23T16:09:23Z

@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/

jeroen92 · 2018-08-27T11:30:23Z

@rallytime Thanks for checking, I missed that one. Should be fixed now.

cbosdo suggested changes May 25, 2018

View reviewed changes

cbosdo reviewed May 25, 2018

View reviewed changes

jeroen92 force-pushed the virt_bhyve branch from a7ab3fe to 0482c03 Compare June 18, 2018 20:15

jeroen92 force-pushed the virt_bhyve branch from 0482c03 to e2399ba Compare July 18, 2018 21:29

cbosdo suggested changes Jul 23, 2018

View reviewed changes

jeroen92 force-pushed the virt_bhyve branch from e2399ba to 942b0c6 Compare July 29, 2018 22:02

jeroen92 force-pushed the virt_bhyve branch 6 times, most recently from b28bd1a to eecd88a Compare August 12, 2018 18:12

jeroen92 force-pushed the virt_bhyve branch from eecd88a to 45f634f Compare August 13, 2018 06:31

cbosdo suggested changes Aug 20, 2018

View reviewed changes

jeroen92 force-pushed the virt_bhyve branch from 75f7df5 to 3aab313 Compare August 20, 2018 20:31

cbosdo approved these changes Aug 21, 2018

View reviewed changes

rallytime approved these changes Aug 23, 2018

View reviewed changes

jeroen92 added 3 commits August 26, 2018 21:34

Add support to virt for libvirt loader

9cace9a

Add bhyve compatibility to virt

18d605e

Try/except undefineFlags() as this operation is not supported on bhyve

29a44ac

jeroen92 force-pushed the virt_bhyve branch 2 times, most recently from 51b4b04 to 29a44ac Compare August 26, 2018 19:38

rallytime merged commit fd60279 into saltstack:develop Aug 28, 2018

Akm0d added the master-port label Oct 18, 2019

jeroen92 mentioned this pull request Dec 7, 2019

Bhyve support in Fluorine is broken #55556

Closed

waynew removed the master-port label Dec 9, 2019

sagetherage added code-jam Has Testcase labels Apr 15, 2020

Ch3LL mentioned this pull request Apr 21, 2020

Port #47620 to master #56817

Closed

Ch3LL added the has master-port port to master has been created label Apr 21, 2020

cbosdo mentioned this pull request Apr 24, 2020

Bhyve support backport #56882

Merged

Uh oh!

Conversation

jeroen92 commented May 11, 2018

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

New Behavior

Tests written?

Commits signed with GPG?

Uh oh!

cbosdo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rallytime commented Jun 8, 2018

Uh oh!

cbosdo commented Jun 11, 2018

Uh oh!

nicoonoclaste commented Jun 18, 2018

Uh oh!

jeroen92 commented Jun 18, 2018

Uh oh!

cbosdo commented Jun 19, 2018

Uh oh!

nicoonoclaste commented Jun 19, 2018

Uh oh!

rallytime commented Jul 6, 2018

Uh oh!

jeroen92 commented Jul 8, 2018

Uh oh!

jeroen92 commented Jul 18, 2018

Uh oh!

cbosdo commented Jul 18, 2018

Uh oh!

rallytime commented Jul 20, 2018

Uh oh!

cbosdo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeroen92 Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeroen92 Jul 25, 2018 •

edited

Loading

jeroen92 commented Jul 29, 2018 •

edited

Loading