Page MenuHomeFreeBSD

give bhyve the ability to parse a libucl guest configuration file
Needs RevisionPublic

Authored by araujo on May 5 2015, 3:56 PM.

Details

Reviewers
rgrimes
bcr
allanjude
jhb
tychon
Group Reviewers
manpages
bhyve
Summary
  1. bhyveload -f guest.conf
  2. bhyve -f guest.conf

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7844
Build 7987: arc lint + arc unit

Event Timeline

araujo updated this revision to Diff 5203.May 5 2015, 3:56 PM
araujo retitled this revision from to Bring the capability to bhyveload and bhyve to parse and use a guest configuration file to load the vm..
araujo updated this object.
araujo edited the test plan for this revision. (Show Details)
araujo updated this object.May 5 2015, 4:24 PM
araujo edited the test plan for this revision. (Show Details)
araujo updated this revision to Diff 5204.May 5 2015, 4:28 PM
araujo updated this object.

Updating D2448: Bring the capability to bhyveload and bhyve to parse and

use a guest configuration file to load the vm.

emaste added a subscriber: emaste.May 5 2015, 4:40 PM
emaste added inline comments.May 5 2015, 7:43 PM
usr.sbin/bhyve/bhyverun.c
834

'f' ought to be inserted in alpha order in the switch statement

998

Seems a shame to lose this

999–1001

unrelated whitespace changes

usr.sbin/bhyveload/Makefile
14

don't commit with this
You might like WITH_DEBUG_FILES=yes in src.conf?

usr.sbin/bhyveload/bhyveload.c
717

options in alpha order

Is the config file meant to be Xen like or something?

What do you think of this format instead:
https://github.com/allanjude/bhyveucl/blob/config_to_ucl/vmconfig.ucl

It is a bit more expressive,

(Currently, the 'host' section is implemented by the script, but probably could be done by bhyve as well)

usr.sbin/bhyve/config.c
86

might there be cases where a user doesn't want any network interfaces?

129

should probably use the new 'safe' iterator instead.

137

did you consider ucl_object_toint ?

araujo added a comment.May 6 2015, 2:32 AM

Is the config file meant to be Xen like or something?

Yes, the idea is to be Xen like. Xen config file is pretty much clean and easy to understand and to setup it.

What do you think of this format instead:
https://github.com/allanjude/bhyveucl/blob/config_to_ucl/vmconfig.ucl
It is a bit more expressive,

Some parts of your configuration file I quite like, however in that format seemed to let things more complex, I agree it is more expressive, but also more expensive and can lead the user to make mistake. It is personal opinion, but if other people wants a different config files it is possible to support it.

I just kept in mind, make it as simple as possible.

(Currently, the 'host' section is implemented by the script, but probably could be done by bhyve as well)

Here I think, it is not a good idea, because bhyve should not care about the HOST configuration, I mean, network devices, block devices and so on. The bhyve shall only care about the guest side.

For all the rest, I agree and I will make the changes soon.

araujo updated this revision to Diff 5223.May 6 2015, 4:10 AM

emaste@

  • Fix the f option inserted now in alpha order.
  • Remove unrelated whitespace.
  • Remove the -g option from Makefile.
  • Bring back setproctitle.

allanjude@

  • Fix the case where users doesn't want any network interfaces.
  • Use ucl_object_iterate_safe.

Updating D2448: Bring the capability to bhyveload and bhyve to parse and

use a guest configuration file to load the vm.

araujo added inline comments.May 6 2015, 4:27 AM
usr.sbin/bhyve/config.c
137

Hi, I forgot to reply this one: Yes, I did consider to use it, but in the end to simplify code, I choose to use atoi(3).

In D2448#45144, @araujo wrote:

What do you think of this format instead:
https://github.com/allanjude/bhyveucl/blob/config_to_ucl/vmconfig.ucl
It is a bit more expressive,

Some parts of your configuration file I quite like, however in that format seemed to let things more complex, I agree it is more expressive, but also more expensive and can lead the user to make mistake. It is personal opinion, but if other people wants a different config files it is possible to support it.
I just kept in mind, make it as simple as possible.

With that in mind, what do you think of this:

https://github.com/allanjude/bhyveucl/blob/config_to_ucl/simple.ucl

much closer to what you have, but allows the full set of options for disks.

most of the parameters can have a default, like 'type' for a disk can default to virtio-blk
and for the network, we should support setting the mac address, but not require it, etc.

uuid is also optional.

I am actually working on a way to 'inherit' a default config file, and only need to override the settings on wishes to change, but, I need to get some upstream fixed into libucl first.

usr.sbin/bhyve/config.c
135

not sure if it matters to anyone, but using ucl_object_toint() here, would allow the user to use UCL's unit expansion, (k, kb, m, mb) instead of the internal call to expand_number that is used later in bhyve. UCL supports both power of 10 and power of 2 units, although for memory, bhyve will always 'do the right thing' which might be better, except in the case where the user specifies memory = 1024mb.

However, with the goal of converting all config files to UCL, standardizing on using 'mb' etc may have value.

137
The atoi() function has been deprecated by strtol() and should not be
used in new code.

You might want to use strtonum() here, as it also includes an upper and lower bounds check, so you can prevent the user from trying to pass negative or insanely high numbers, so something like:

c->vcpus = strtonum(strdup(value), 1, MAXCPU, &errstr);
if (errstr != NULL)

errx(1, "number of cpus is %s: %s", errstr, value);
araujo added a comment.May 6 2015, 4:47 PM

With that in mind, what do you think of this:
https://github.com/allanjude/bhyveucl/blob/config_to_ucl/simple.ucl
much closer to what you have, but allows the full set of options for disks.

Yes, this version looks much better, more simple and still keep the additional options missing in my version.
Please, keep this version on github for future checking. This version is much more clean.

most of the parameters can have a default, like 'type' for a disk can default to virtio-blk
and for the network, we should support setting the mac address, but not require it, etc.

For the MAC ADDRESS, it is good to keep the option.

uuid is also optional.

It is still far to be good to cover all options supported by bhyve, we need to have more discussion to find the best approach, but we are in the way.

usr.sbin/bhyve/config.c
135

bhyve has an internal function vm_parse_memsize() that parses the memory option, I didn't bother too much to use ucl_object_toint(), but I have no problem to use it.

137

Make sense, I will make the changes.

I have expanded on my ideas in a separate review, to avoid commandeering this one

https://reviews.freebsd.org/D2505

allanjude added inline comments.May 10 2015, 5:09 AM
usr.sbin/bhyve/bhyverun.c
981

this crashes if no vmname is included on the command line (such as running just 'bhyve')

araujo abandoned this revision.May 10 2015, 6:53 AM

Allan now you are allowed to hijack the patch as you did. I just want point out, this is not the way how do things in an open source project. I exchanged few emails with other guys and I was waiting their reply to change this patch.

you have

Feel free to move forward with it.

In D2448#46069, @araujo wrote:

Allan now you are allowed to hijack the patch as you did. I just want point out, this is not the way how do things in an open source project. I exchanged few emails with other guys and I was waiting their reply to change this patch.
you have
Feel free to move forward with it.

I explicitly was trying not to step on your work, or cut you out of the process.

I have discussed this topic with Peter and Neel at MeetBSD 2014, and Peter against at AsiaBSDCon 2015. I only became aware of your work when Ed Maste pointed it out to me.

I started the separate review specifically to avoid seeming to just take over after all the work you put into this. I would very much welcome your help to make this the best patch that we can.

Allan now you are allowed to hijack the patch as you did. I just want point out, this is not the way how do things in an open source project. I exchanged few emails with other guys and I was waiting their reply to change this patch.

you have my email, you have the first review and I'm very open for discussion.

Feel free to move forward with it.

In D2448#46069, @araujo wrote:

Allan now you are allowed to hijack the patch as you did. I just want point out, this is not the way how do things in an open source project. I exchanged few emails with other guys and I was waiting their reply to change this patch.
you have
Feel free to move forward with it.

I explicitly was trying not to step on your work, or cut you out of the process.
I have discussed this topic with Peter and Neel at MeetBSD 2014, and Peter against at AsiaBSDCon 2015. I only became aware of your work when Ed Maste pointed it out to me.
I started the separate review specifically to avoid seeming to just take over after all the work you put into this. I would very much welcome your help to make this the best patch that we can.

If you discussed it before with other people, you could just send an email, we have emails if you are not aware of it. Also I have sent some emails to Peter and Neel. No one replied to me that you were working in something, or you want do something.

It is just not right to do what you did.

Now that you already did, feel free to finish it.
I will just ask you in a very clear way, never again do it with my patches or any of my reviews request.

Unfortunately it sounds to me something like a small and closed group. Fortunately FreeBSD is made by contributions, this kind of attitude is against it.

I have one or two more patches for bhyve, now I'm really in doubt if I should send it or not, because maybe you or something else were thinking about it before and will hijack it, and in this case... sounds pretty much right.

Best,

araujo reclaimed this revision.May 11 2015, 2:11 AM

Review D2505 just hijacked my patch without any talk. So, I'm re-open this review.

joel added a subscriber: joel.May 23 2015, 6:34 AM
joel removed a subscriber: joel.
joel added a subscriber: joel.
joel added a comment.May 23 2015, 9:33 AM

Any progress with this? I kind of prefer the ucl format of the configuration file.

In D2448#48900, @joel wrote:

Any progress with this? I kind of prefer the ucl format of the configuration file.

Yes, we have some very good progress, however I'm still waiting Neel and Peter as they have some suggestions that is necessary to add on the config file.

araujo added a comment.Jun 7 2015, 1:38 PM

After change several emails with @allanjude he comes up with an improve of the original patch, however it depends of a newer libucl version that is not in our base system yet.

Here is the patch: http://www.allanjude.com/bsd/bhyve_ucl_20150601.patch

I pretty much like the idea, although it needs some rework with style(9) and also make the code less repetitive and more simple.

I'm updating this revision to push the guys to talk with @allanjude at BSDCAN, unfortunately I won't be there this year again and I'm in touch with @allanjude to finish this patch together.

I'm not update this review with this patch, because it doesn't build without some additional changes related with libucl, but as soon as we have the new libucl version in our base system, I will update it.

Thanks for the work @allanjude.

@greehan and @neel do you have any comments or any feedback?

Best Regards,

araujo abandoned this revision.Jul 8 2015, 1:37 AM

I'm not interested on it any more as there is a gap of communication here.

allanjude commandeered this revision.Mar 2 2017, 3:40 AM
allanjude added a reviewer: araujo.

I am reviving this project

allanjude reclaimed this revision.Mar 2 2017, 3:41 AM
allanjude retitled this revision from Bring the capability to bhyveload and bhyve to parse and use a guest configuration file to load the vm. to give bhyve the ability to parse a libucl guest configuration file.
allanjude updated this object.
allanjude edited the test plan for this revision. (Show Details)
allanjude added a reviewer: rgrimes.
allanjude updated this revision to Diff 25862.Mar 2 2017, 3:43 AM
allanjude updated this object.

Bring my patch forward by 30,000 revisions

allanjude updated this revision to Diff 25864.Mar 2 2017, 3:54 AM
allanjude edited edge metadata.

Catch up to more changes so the code actually compiles

araujo accepted this revision.Mar 2 2017, 2:46 PM
araujo edited edge metadata.

Thanks to revive this patch, we really need this capability. Please go ahead to commit it and I will support you with the coming bugs in case any arises in the future.

bhyve really needs it.

Best,

This revision is now accepted and ready to land.Mar 2 2017, 2:46 PM
bcr accepted this revision.Mar 2 2017, 3:41 PM
bcr added a reviewer: bcr.
bcr added a subscriber: bcr.

OK from manpages. Make sure to bump the document date.

grehan requested changes to this revision.Mar 3 2017, 4:03 AM
grehan edited edge metadata.

Thanks for keeping this going Allan.

I'd like this not to be committed as-is. While this isn't really implemented how I'd like, that can always be fixed, However, the format of the config file is going to live for a long time and I think some things in the proposal need to be fixed up somewhat.

Ideally I'd like all features that are and will be available in bhyve to be expressed in this file, even those not available as options. While this seems opposite to the goal of having a file that is simple for users to create, I'm hoping that features of UCL such as includes and macro expansion can be used to create templates to hide a lot of the complexity.

I'll commit to coming up with the additions I'd like to see.

This revision now requires changes to proceed.Mar 3 2017, 4:03 AM

Thanks for keeping this going Allan.
I'd like this not to be committed as-is. While this isn't really implemented how I'd like, that can always be fixed, However, the format of the config file is going to live for a long time and I think some things in the proposal need to be fixed up somewhat.
Ideally I'd like all features that are and will be available in bhyve to be expressed in this file, even those not available as options. While this seems opposite to the goal of having a file that is simple for users to create, I'm hoping that features of UCL such as includes and macro expansion can be used to create templates to hide a lot of the complexity.
I'll commit to coming up with the additions I'd like to see.

I had no intention of commit this as-is. It doesn't support any of the newer flags that were added in the last 2 years.

I just revived the patch after being prodded by rgrimes@

Yes, the current config file is too simple, but I agree, using includes, we can abstract a lot of the complexity into some stock files. So the config files created by a user can be very simple, but can also override the more complex settings if they actually have a need to do so.

They will also be able to basically 'clone' VMs by just including the original config file, and overriding a few of the variables like name, and backing disk.

araujo commandeered this revision.Oct 26 2018, 3:04 AM
araujo edited reviewers, added: allanjude; removed: araujo.

I have spoke with @allanjude about it, we at bhyve group are discussing this topic again and we will become with a new format for the configuration file.
So, I'm commandeer this revision again!

0mp requested changes to this revision.Oct 26 2018, 9:39 AM
0mp added a subscriber: 0mp.

Just to be sure: the usage function mentions that the configuration file is in the UCL format but it is not mentioned anywhere in the manual page.

Am I missing something or the format of the configuration file is not yet documented?

usr.sbin/bhyve/bhyve.8
91

I am not sure but I feel that it should be from the specified file.

Correct me if it does not make sense to you.

400

I guess that ident is a typo, right?

usr.sbin/bhyveload/bhyveload.8
97

I am not sure but I feel that it should be from the specified file.

Correct me if it does not make sense to you.

usr.sbin/bhyveload/bhyveload.c
810

Is there a reason why this error message starts with a lowercase letter?

In D2448#378445, @0mp wrote:

Just to be sure: the usage function mentions that the configuration file is in the UCL format but it is not mentioned anywhere in the manual page.
Am I missing something or the format of the configuration file is not yet documented?

The configuration file is not yet defined and not yet documented!
We have a bhyve biweekly meeting call and we are under discussion about the format of this file.

Be patient :)

araujo removed a reviewer: 0mp.Oct 26 2018, 9:52 AM
araujo edited reviewers, added: jhb, tychon; removed: grehan, neel.