Differential D35454
bhyve: snapshot capsicum support Authored by gusev.vitaliy_gmail.com on Jun 10 2022, 11:50 PM. Tags Referenced Files
Details Snapshots are placed to a directory obtained from --checkpoint/--snapshot argument. Example: bhyvectl --suspend=/disk/dump/x1--vm=ubuntu Snapshot 'x1' will be stored to the '/disk/dump' directory. Sponsored by vStack. vm destroy context needs more efforts, so ignore that error Start VM, suspend and then resume. Start VM, do checkpoint several times. Verify no errors occurred.
Diff Detail
Event TimelineComment Actions I'd rather see the snapshot functionality write it's data to a single file. A file descriptor for said file would be passed in from the nvlist via an IPC command. In other words, bhyvectl would open the snapshot file and pass that file descriptor over the socket to bhyve. Comment Actions Good point. I have one more proposal. Instead of allowing bhyve to do all things at once (dump memory, kernel pages, etc), I would suggest to introduce several small commands that bhyve can perform:
In other words, bhyvectl will send several commands to get VM memory file, kernel&device&conf file and 'bhyve' will handle it. That approach will help in implementing Live Migration, that will imply addition commands:
Comment Actions @rew What do you think about idea implementing "bricks/handlers" in bhyve and full logic in bhyvectl ? Comment Actions not sold on it My perspective about save/restore is that the limitations/file format issues described in the original commit message, 483d953a86a2507355f8287c5107dc827a0ff516, need to be resolved/discussed. Until those issues are addressed, the save/restore feature (and any feature that builds off of it) will not make it into main. While capsicum support is useful towards obtaining that goal, I'm not convinced that save/restore needs multiple fd's to do what it needs - and I'd prefer to not let that design bleed into other userland tools (bhyvectl). Comment Actions Do these get bhyve closer to what you believe to be required? https://reviews.freebsd.org/D29262 Comment Actions So I see the two major issues there :
While it is under discussing how to save binary data, I would create review that saves entire VM config to a .meta file with config's format, and all fields from .meta are saved in config's format. Example: kern_structs.device.vioapic.size=388 kern_structs.device.vioapic.file_offset=7844 etc. This will allow to resume VM just provide this command: bhyve -r $snapshot_name
@rew bhyvectl doesn't need pass multiple fd's. It can pass one fd at time or pass bitmask of commands. If use one fd for all steps (save all things - ram, kernel, config to one file) - it will require completly rework saving format. Comment Actions Yes, I'm suggesting that the saving/file format will need an overhaul before the save/restore feature can be turned on by default. From my point of view, what we have in the tree right now is a working prototype which needs to be iterated on. And given that the file format is likely to change, the usage/requirement of these fd's might also change - which is why I'm not convinced that these fd's are worth messing with right now. Let me make it clear though...this is my current line of thinking and I'm not requesting or blocking any changes here. Comment Actions I haven't fully reviewed in detail, but it seems that this is an incremental step forward, and it makes sense to go ahead with this change [ Comment Actions You want to set the directory a snapshot file is written to via an environment variable? And if you want to change that directory, have to restart the bhyve process? Comment Actions Updated to the latest main branch. Open directory at bhyvectl side and pass fd to bhyve so that it can use openat() under capsicum. Comment Actions I still hold the opinion that the snapshot data should be stored in a single file and the file descriptor for that file should be passed to bhyve. The snapshot file format should be (more or less) in its final state before we start polluting this nvlist with potentially obsolete file descriptors. In other words, these changes are putting the cart before the horse.
Comment Actions @rew Could you take a look? @corvink Created separate review https://reviews.freebsd.org/D38836
Comment Actions You could move more parts into seperate commits. I know phab is not a good tool for multiple small commits and it's some additional work. However, it speeds up merging because uncritical parts can be merged earlier.
Comment Actions Correct 'fd' check: - return (fd > 0 ? fd : -errno); + return (fd >= 0 ? fd : -errno); Comment Actions @corvink As you suggested I split this patch to the several reviews: libvmm: Add missed ioctl to vm_ioctl_cmds bhyve: [snapshot] Do not flush readonly device at blockif_pause() bhyve: [snapshot] Add cap limits for ipc socket bhyve: Exit with EX_OSERR if init checkpoint or restore time failed bhyve: Init checkpoint before caph_enter() bhyve: Use directory fd with checkpoint bhyve: Enable Capsicum for snapshots Comment Actions FYI if you use just the D# notation Phabricator will add markup to show which reviews are open/closed e.g. D38866 libvmm: Add missed ioctl to vm_ioctl_cmds | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||