Re: [RFC] Add basic driver for the Cloud-Hypervisor

On Thu, Aug 27, 2020 at 11:24:32AM -0700, William Douglas wrote: It would be useful to outline the general architecture / approach to mgmt. IIUC, cloud-hypervisor is similar to QEMU in that launching a VM just involves spawning a process for that VM. There is no central daemon or authority over all cloud-hypervisor VMs, thus libvirtd fills that role by acting as the mgmt daemon. In terms of communicating with the process, I'm seeing that basically everyting is REST API calls with JSON encoded data. It would be good to have a drvch.html.in file, well a drvch.rst file actually to give users an intro to the new driver. It should probably mention it is an early PoC not really targetting production usage at this time since there's many key features missing. The spec file targets Fedora and RHEL, and IIUC, cloud-hypervisor is not present in either of them. So for now the spec file should always disable building of the new driver. The driver relies on Curl and YAJL, so you'll need to do more there to automatically disable the driver if either of those is not present. Take a look a the handling of 'drive_qemu' for a reasonable illustration for YAJL, which you can adapt and include curl too. I'm surprised 'ninja test' didn't complain that the filename mentioned here doesn't match the actual filename. Likewise for other files. IIUC, cloud hypervisor is still using KVM as the hypervisor, merely providing a different userspace emulation layer. So in terms of the libvirt virt type, we should still be using VIR_DOMAIN_VIRT_KVM. No need to invent a VIR_DOMAIN_VIRT_CH. This should also be testing of the required binary exists on the host, and if it does not, then don't claim to support guest domains. On 64-bit hosts does CH provide a way to strictly emulate just a 32-bit CPU, or will it always be 64-bit only ? If the former, then you need to add the 32-bit equivalent guest too. Use g_free for these. This is a bit more convulted than needed - just encode the min version you want as an integer and do a single compare eg #define MIN_VERSION ((0 * 1000000) + (9 * 1000) + (0)) if (version < MIN_VERSION) { ..report error... } This method is pretty pointless and its also not used anywhere FWIW, I'd encourage you to use GObject rather than virObject for anything new, as our long term goal is to convert all code to use GObject alone and eliminate virObject What is this initpid needed for ? The virDomainObjPtr struct already lets us record a PID for the guest in the <domstatus> element. Are you not able to support the ch:///session mode for running under the user's unprivileged session ? This isn't right - the virDomainCreateXML method is for creating a transient guest only. Also from this point onwards, for any failure you need to remove the dangling guest from the driver->domains list. These two shutdown methods would really only be for container based virt. When you have a KVM guest, only an APIC based trigger, or guest agent trigger make conceptual sense. Same note as above. Just to explicitly confirm, "suspend" in libvirt terminology means pause execution of the guest CPUs, nothing else. Does that match semantics from CH ? Same question as earlier about why its forbidding non-root usage ? Your driver URI is "ch:///system", so the only thing that should be in this list is "ch". I'd suggest "cloud-hypervisor" to be a bit clearer I'm kinda inclined to say it is overkill to have separate methods for kernel, initrd and cmdline. I'd handle all three at same time since they go hand in hand with each other. You must validate diskdef->type before accesing any fields for the source otherwise it is highly liable to crash on unexpected user input. Generally we'd expect a switch(diskdef->type) and cases to each applicable type you want to support, with errors raised for others. else { ....report VIR_ERR_CONFIG_UNSUPPORTED... } As a general rule, when building the config from the user's XML, report as many errors as possible. It is preferrable for apps to see an error report than to have their config silently ignored. For _LAST/defalt, use virReportEnumRangeError Obviously you're doing the bare minimum here to get something running which is fine for an initial merge. Ultimately though, it should be reporting erors for any types of devices you don't wish to support. If you populate the guest capabilities with the default emulator path, it should get populated into the virDomainDef, and thus be able to assume it is non-NULL. 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. I presume this is left over from when you were debugging this. Printing to stderr is not really something we would want in the code. Change to... g_autofree char *url = NULL; and... ...you can drop this. Likewise for any where else you malloc a variable and free it in the same scope. Is that empty "Expect:" header intentional ? Does this curl handle keep a persistent connection open to the CH process ? And is that able to be used to detect immedaitely when the CH process shuts down or crashes unexpectedly ? We're moving to a world where each stateful virt driver has its own daemon. The rules here are enough for libvirtd, but you'll need to add rules to create a "virtchd" daemon. The qemu/meson.build will show how - look for "virt_daemons". As mentioned earlier, not required since you're still using KVM IIUC. Overall, this looks like a reasonable start of the driver. 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. The other big question is around device addressing. If using PCI, then PCI address assignment logic is critical to ensure consistent guest ABI. 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 :|