[libvirt] [PATCHv5 1/6] bhyve: Support /domain/bootloader configuration for non-FreeBSD guests.

[Conrad Meyer]

On Tue, Oct 28, 2014 at 10:39 AM, Daniel P. Berrange
<berrange at 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