[libvirt] [PATCHv5 1/6] bhyve: Support /domain/bootloader configuration for non-FreeBSD guests.
Daniel P. Berrange
berrange at redhat.com
Tue Oct 28 14:39:00 UTC 2014
On Mon, Oct 27, 2014 at 10:37:36AM -0400, Conrad Meyer wrote:
> We still default to bhyveloader(1) if no explicit bootloader
> configuration is supplied in the domain.
>
> If the /domain/bootloader looks like grub-bhyve and the user doesn't
> supply /domain/bootloader_args, we make an intelligent guess and try
> chainloading the first partition on the disk (or a CD if one exists,
> under the assumption that for a VM a CD is likely an install source).
>
> Caveat: Assumes the HDD boots from the msdos1 partition. I think this is
> a pretty reasonable assumption for a VM. (DrvBhyve with Bhyveload
> already assumes that the first disk should be booted.)
>
> I've tested both HDD and CD boot and they seem to work.
>
> Sponsored by: EMC / Isilon storage division
>
> Signed-off-by: Conrad Meyer <conrad.meyer at isilon.com>
> ---
> docs/drvbhyve.html.in | 94 ++++++++++++++++++++-
> docs/formatdomain.html.in | 4 +-
> src/bhyve/bhyve_command.c | 202 +++++++++++++++++++++++++++++++++++++++++-----
> src/bhyve/bhyve_command.h | 5 +-
> src/bhyve/bhyve_domain.c | 5 ++
> src/bhyve/bhyve_domain.h | 1 +
> src/bhyve/bhyve_driver.c | 2 +-
> src/bhyve/bhyve_process.c | 13 ++-
> 8 files changed, 298 insertions(+), 28 deletions(-)
>
> diff --git a/docs/drvbhyve.html.in b/docs/drvbhyve.html.in
> index 39afdf5..ee1317e 100644
> --- a/docs/drvbhyve.html.in
> +++ b/docs/drvbhyve.html.in
> @@ -37,8 +37,7 @@ bhyve+ssh://root@example.com/system (remote access, SSH tunnelled)
> <h3>Example config</h3>
> <p>
> The bhyve driver in libvirt is in its early stage and under active development. So it supports
> -only limited number of features bhyve provides. All the supported features could be found
> -in this sample domain XML.
> +only limited number of features bhyve provides.
> </p>
>
> <p>
> @@ -48,10 +47,21 @@ disk device were supported per-domain. However,
> up to 31 PCI devices.
> </p>
>
> +<p>
> +Note: the Bhyve driver in libvirt will boot whichever device is first. If you
> +want to install from CD, put the CD device first. If not, put the root HDD
> +first.
> +</p>
FYI, the libvirt XML allows for a <boot order='NNN'/> element to be
included by the user in any <disk>, <interface> or <hostdev> element.
This is considered to obsolete the <boot dev="cdrom|disk|network"/>
config we originally used, so that we can get fine grained control
over device boot order, independant of XML format.
Your patch is fine as it is, i just mention this as something you
might consider adding support for in later work.
> <h2><a name="usage">Guest usage / management</a></h2>
>
> @@ -119,6 +177,14 @@ to let a guest boot or start a guest using:</p>
>
> <pre>start --console domname</pre>
>
> +<p><b>NB:</b> An interactive bootloader will keep the domain from starting (and
> +thus <code>virsh console</code> or <code>start --console</code>) until the
> +console is opened by a client. To select a boot option and allow the domain to
> +finish starting, one must use an alternative terminal client to connect to the
> +null modem device. One example is:</p>
Can you clarify what you mean by this. It seems like this is saying that
the 'virDomainCreate' API (virsh start GUEST command) will not return
controll to the caller until you've interacted with the bootloader.
If correct, I don't think this is a very satisfactory solution. There
should not be any requirement to interact with the guest domain until
after the virDomainCreate API call completes successfully. If succesfully
booting requires that you go via an out-of-band channel to interact with
the bootloader that's even more of a problem.
Because libvirt has an RPC based mechanism for API access, we need to
always bear in mind that the application/user talking to libvirt drivers
is either local but unprivileged, or even entirely remote from the hypervisor
node. The libvirt built-in console tunnels over our RPC channel to provide
apps access.
So if my understanding is correct, this limitation you describe is going
to prevent use of this kind of setup from most apps using libvirt.
We need to at least come up with a plan of how we'd address this problem
in the medium term.
> +static virCommandPtr
> +virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def,
> + bhyveDomainObjPrivatePtr priv,
> + virConnectPtr conn)
> +{
> + virDomainDiskDefPtr disk, cd;
> + virCommandPtr cmd;
> + size_t i;
> + FILE *f;
> + int rc;
> +
> + if (def->os.bootloaderArgs != NULL)
> + return virBhyveProcessBuildCustomLoaderCmd(def);
> +
> + /* Search disk list for CD or HDD device. */
> + cd = disk = NULL;
> + for (i = 0; i < def->ndisks; i++) {
> + if (!virBhyveUsableDisk(conn, def->disks[i]))
> + continue;
> +
> + if (cd == NULL &&
> + def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
> + cd = def->disks[i];
> + VIR_INFO("Picking %s as boot CD", virDomainDiskGetSource(cd));
> + }
> +
> + if (disk == NULL &&
> + def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> + disk = def->disks[i];
> + VIR_INFO("Picking %s as HDD", virDomainDiskGetSource(disk));
> + }
> + }
> +
> + cmd = virCommandNew(def->os.bootloader);
> +
> + VIR_DEBUG("grub-bhyve with default arguments");
> +
> + if (priv != NULL) {
> + rc = virAsprintf(&priv->grub_devicesmap_file,
> + "%s/grub_bhyve-%s-device.map", BHYVE_STATE_DIR,
> + def->name);
> + if (rc < 0)
> + goto error;
> +
> + f = fopen(priv->grub_devicesmap_file, "wb");
> + if (f == NULL) {
> + virReportSystemError(errno, _("Failed to open '%s'"),
> + priv->grub_devicesmap_file);
> + goto error;
> + }
> +
> + /* Grub device.map (just for boot) */
> + if (disk != NULL)
> + fprintf(f, "(hd0) %s\n", virDomainDiskGetSource(disk));
> +
> + if (cd != NULL)
> + fprintf(f, "(cd) %s\n", virDomainDiskGetSource(cd));
> +
> + if (VIR_FCLOSE(f) < 0) {
> + virReportSystemError(errno, "%s", _("failed to close file"));
> + goto error;
> + }
As a general rule we prefer that the APIs for constructing command line
args don't have side effects on system state, such as creating external
files, because we want to be able to test all these code paths in the
unit tests.
The 'if (priv)' approach is somewhat fragile and means we don't get test
coverage of the grub file format writing code.
I think I'd suggest we need a way to just write the grub device.map
to a 'char **' parameter we pass into this method, and bubble that
all the wayy back to the top
Then make the caller of virBhyveProcessBuildLoadCmd be responsible
for writing it to disk, using virFileWriteStr. That way we can fully
test the code in virBhyveProcessBuildLoadCmd without hitting the file
writing path.
> + }
> +
> + if (cd != NULL) {
> + VIR_WARN("Trying to boot cd with grub-bhyve. If this is "
> + "not what you wanted, specify <bootloader_args>");
We have a general policy that VIR_WARN should not be used for messages that
are related to user API actions / choices. This is because the person who
is causing this warning, is typically not going to be the person who has
visibility into the libvirtd daemon log file.
> +
> + virCommandAddArg(cmd, "--root");
> + virCommandAddArg(cmd, "cd");
> + } else {
> + VIR_WARN("Trying to boot hd0,msdos1 with grub-bhyve. If this is "
> + "not what you wanted, specify <bootloader_args>");
> +
> + virCommandAddArg(cmd, "--root");
> + virCommandAddArg(cmd, "hd0,msdos1");
> + }
As mentioned above we have spport for per-device boot indexes, but in
the absence of that, I think you should at least be honouring the
traditianal <boot dev="cdrom|disk|network"> element in the XML config
rather than hardcoding priority for cdrom over disks.
> +
> + virCommandAddArg(cmd, "--device-map");
> + if (priv != NULL)
> + virCommandAddArg(cmd, priv->grub_devicesmap_file);
> + else
> + virCommandAddArg(cmd, "<device.map>");
> +
> + /* Memory in MB */
> + virCommandAddArg(cmd, "--memory");
> virCommandAddArgFormat(cmd, "%llu",
> VIR_DIV_UP(def->mem.max_balloon, 1024));
>
> - /* Image path */
> - virCommandAddArg(cmd, "-d");
> - virCommandAddArg(cmd, virDomainDiskGetSource(disk));
> -
> /* VM name */
> virCommandAddArg(cmd, def->name);
>
> return cmd;
> +
> + error:
> + virCommandFree(cmd);
> + return NULL;
> +}
> +
> +virCommandPtr
> +virBhyveProcessBuildLoadCmd(virConnectPtr conn, virDomainDefPtr def,
> + bhyveDomainObjPrivatePtr priv)
> +{
> + virDomainDiskDefPtr disk;
> +
> + if (def->ndisks < 1) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("domain should have at least one disk defined"));
> + return NULL;
> + }
> +
> + if (def->os.bootloader == NULL) {
> + disk = def->disks[0];
> +
> + if (!virBhyveUsableDisk(conn, disk))
> + return NULL;
> +
> + if (def->ndisks > 1) {
> + VIR_WARN("drvbhyve attempting to boot from device %s of multiple-"
> + "device configuration", virDomainDiskGetSource(disk));
> + }
> +
> + return virBhyveProcessBuildBhyveloadCmd(def, disk);
> + } else if (strstr(def->os.bootloader, "grub-bhyve") != NULL) {
> + return virBhyveProcessBuildGrubbhyveCmd(def, priv, conn);
> + } else {
> + return virBhyveProcessBuildCustomLoaderCmd(def);
> + }
> }
> diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
> index ecb1758..77d34a7 100644
> --- a/src/bhyve/bhyve_domain.c
> +++ b/src/bhyve/bhyve_domain.c
> @@ -49,6 +49,11 @@ bhyveDomainObjPrivateFree(void *data)
>
> virDomainPCIAddressSetFree(priv->pciaddrs);
>
> + if (priv->grub_devicesmap_file != NULL) {
> + ignore_value(unlink(priv->grub_devicesmap_file));
> + VIR_FREE(priv->grub_devicesmap_file);
> + }
> +
> VIR_FREE(priv);
> }
> diff --git a/src/bhyve/bhyve_domain.h b/src/bhyve/bhyve_domain.h
> index b8ef22a..6ecd395 100644
> --- a/src/bhyve/bhyve_domain.h
> +++ b/src/bhyve/bhyve_domain.h
> @@ -31,6 +31,7 @@ typedef bhyveDomainObjPrivate *bhyveDomainObjPrivatePtr;
> struct _bhyveDomainObjPrivate {
> virDomainPCIAddressSetPtr pciaddrs;
> bool persistentAddrs;
> + char *grub_devicesmap_file;
> };
I'm wondering if we need to store this filename here. If we restart
libvirtd while a bhyve guest is running, then I think we loose this
filename data, so we'd then miss the cleanup.
Perhaps it is better if we just make the bhve guest shutdown method
re-create the filename string and unconditionally unlink it, ignoring
any ENOENT error.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
More information about the libvir-list
mailing list