[RFC] Add basic driver for the Cloud-Hypervisor

[Douglas, William]

On Tue, Sep 15, 2020 at 5:53 AM Daniel P. Berrangé <berrange at redhat.com> wrote:
>
> On Thu, Aug 27, 2020 at 11:24:32AM -0700, William Douglas wrote:

<snip>

> > +virCHMonitorPtr
> > +virCHMonitorNew(virDomainObjPtr vm, const char *socketdir)
> > +{
> > +    virCHMonitorPtr ret = NULL;
> > +    virCHMonitorPtr mon = NULL;
> > +    virCommandPtr cmd = NULL;
> > +    int pings = 0;
> > +
> > +    if (virCHMonitorInitialize() < 0)
> > +        return NULL;
> > +
> > +    if (!(mon = virObjectLockableNew(virCHMonitorClass)))
> > +        return NULL;
> > +
> > +    mon->socketpath = g_strdup_printf("%s/%s-socket", socketdir, vm->def->name);
> > +
> > +    /* prepare to launch Cloud-Hypervisor socket */
> > +    if (!(cmd = chMonitorBuildSocketCmd(vm, mon->socketpath)))
> > +        goto cleanup;
> > +
> > +    if (virFileMakePath(socketdir) < 0) {
> > +        virReportSystemError(errno,
> > +                             _("Cannot create socket directory '%s'"),
> > +                             socketdir);
> > +        goto cleanup;
> > +    }
> > +
> > +    /* launch Cloud-Hypervisor socket */
> > +    if (virCommandRunAsync(cmd, &mon->pid) < 0)
> > +        goto cleanup;
> > +
> > +    /* get a curl handle */
> > +    mon->handle = curl_easy_init();
> > +
> > +    /* try to ping VMM socket 5 times to make sure it is ready */
> > +    while (pings < 5) {
> > +        if (virCHMonitorPingVMM(mon) == 0)
> > +            break;
> > +        if (pings == 5)
> > +            goto cleanup;
> > +
> > +        g_usleep(100 * 1000);
> > +    }
>
> This is highly undesirable. Is there no way to launch the CH process
> such that the socket is guaranteed to be accepting requests by the
> time it has forked into the background ? Or is there a way to pass
> in a pre-opened FD for the monitor socket ?
>
> This kind of wait with timeout will cause startup failures due to
> timeout under load.

Currently the cloud-hypervisor process doesn't fork into the
background so that was initially what I was thinking to add to
cloud-hypervsior. Would tracking the pid of the cloud-hypervsior
running in the background be required at that point (assuming the
socket path to control the daemon would be known and working given
virCommandRun returns successfully)?

<snip>

> What is the security model for the processes ? The libvirt driver
> here is running as root and spawning CH as root. We don't really
> want the VM process running as root though. It really needs to
> be unprivileged from a DAC POV. Obviously that's quite a bit more
> work for you todo, but it should be opssible to share alot of the
> security mgmt infrastructure that we already have for QEMU and
> LXC drivers. It can manage DAC permissions and apply SELinux or
> AppArmor MAC labelling/profiles.

Currently cloud-hypervisor runs with cap_net_admin permitted and
effective on it. The implementation will be updated to run under the
user's session using a non-root driver (with its own daemon as you had
mentioned earlier).

>
> The other big question is around device addressing. If using PCI,
> then PCI address assignment logic is critical to ensure consistent
> guest ABI.
>

I'll be adding the device passthrough for the actual driver submission
but if you would like to see an updated RFC with that added I could do
so.

> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>

Thank you very much for the very thorough comments, I'll be updating
my patch addressing all your feedback.

William