- bhyveload -f guest.conf
- bhyve -f guest.conf
Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
Updating D2448: Bring the capability to bhyveload and bhyve to parse and
use a guest configuration file to load the vm.
usr.sbin/bhyve/bhyverun.c | ||
---|---|---|
719 | 'f' ought to be inserted in alpha order in the switch statement | |
879 | Seems a shame to lose this | |
882–884 | unrelated whitespace changes | |
usr.sbin/bhyveload/Makefile | ||
13 | don't commit with this | |
usr.sbin/bhyveload/bhyveload.c | ||
656 | 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 ? |
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.uclIt 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.
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.
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). |
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); errx(1, "number of cpus is %s: %s", errstr, value); |
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. |
usr.sbin/bhyve/bhyverun.c | ||
---|---|---|
858 | this crashes if no vmname is included on the command line (such as running just 'bhyve') |
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.
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,
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.
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,
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,
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.
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!
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 | ||
---|---|---|
90 | 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. | |
317 | I guess that ident is a typo, right? | |
usr.sbin/bhyveload/bhyveload.8 | ||
90 | 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 | ||
751 | Is there a reason why this error message starts with a lowercase letter? |
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 :)