On Tue, Oct 28, 2014 at 10:39 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Mon, Oct 27, 2014 at 10:37:36AM -0400, Conrad Meyer wrote:
+<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.
Right, that would be nice to fix eventually. libvirt's bhyve driver doesn't support boot order at all yet (it just picks 'domdef->drives[0]'). Fixing that is orthogonal to adding grub-bhyve as a loader option, IMO.
<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.
For GRUB configurations that do not automatically boot something without user interaction, you are correct.
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.
I agree, however — this is the status quo of libvirt-bhyve. This patchset doesn't make it any worse than it already is.
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.
I think most apps probably have automatic boot in their GRUB configuration. But for anything that requires user input before GRUB will load the OS, you are correct.
We need to at least come up with a plan of how we'd address this problem in the medium term.
I think the medium- or long-term plan is to not require an external bootloader at all, this is sort of an interim solution.
+ 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.
Definitely.
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.
I have some reservations about this (additional complexity, not a lot of value IMO), but it can be done if you want it.
+ } + + 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.
Ack. Will fix.
+ + 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.
I think that's an orthogonal improvement (bhyveload currently doesn't support ordering at all). This patch set is already getting large, can this improvement wait?
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.
I think we can probably just remove it from the object if we're returning the string contents out to the caller — the file is very short-lived; we only need it in the routine that launches the loader, synchronously waits for it to complete, and then asynchronously launches bhyve itself. Thanks for reviewing. Conrad