Re: [RFC] Add basic driver for the Cloud-Hypervisor
On Tue, Sep 15, 2020 at 5:53 AM Daniel P. Berrangé <berrange(a)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