[ovirt-devel] Re: implementing hotplugCd/hotunplugCd in vdsm

On Thu, Aug 20, 2020 at 12:19 PM Vojtech Juranek <vjuranek(a)redhat.com&gt; wrote: I don't think we have such configuration, all VMs have empty cdrom by default: <disk type='file' device='cdrom'> <driver name='qemu' error_policy='report'/> <source startupPolicy='optional'/> <target dev='sdc' bus='sata'/> <readonly/> <alias name='ua-d95b29f2-e8a2-4119-9ac2-5b4267e6637d'/> <address type='drive' controller='0' bus='0' target='0' unit='2'/> </disk> But of course if we have such configuration when adding CD is not possible, the menu/button should be disabled in the UI. How failure is handled? What if vdsm is restarted at this point? How VM recovery should handle this drivespec referring to inactive volume? Tearing down and removing drivespec cannot be done atomically. any operation may fail and vdsm may be killed at any point. The design should suggest how vdsm recover from all errors. Sounds good, and may be already handled in lvm module. You can try to deactivate LV twice to confirm this. This is the tricky part, vdsm does not have a mechanism for tracking usage of block devices, and adding such mechanism is much bigger work than supporting CD on block devices, so it cannot be part of this work. What if this fails, or vdsm is restarted before we try to do this? When vdsm starts it recovers running vms, we need to handle this case - drivespec referencing non-existing volume in this flow. > VM with CD, changeCD inserts new ISO and removes old > -------------------------------------------------------------------------------------- > - tear down previous drivespec > - if volume with ISO is inactive (as a result e.g. of failure of vdsm after inserting drivespec into VM metadata, but before activating volume), continue without error > - remove previous drivespec from VM metadata > - add new drivespec to VM metadata > - attach new device to VM > - if attaching new drivespac fails, tear down new drivespec and attach back previous drivespec This is too complicated. When I discussed this Fedor, our conclusion was that we don't want to support this complexity, and it will be much easier to implement 2 APIs, one for removing a CD, and one for inserting a CD. With separate APIs, error handling is much easier, and is similar to existing error handling in hotplugDisk and hotunplugDisk. You have good example how to handle all errors in these APIs. For example, if we have API for removing a CD: 1. engine send VM.removeCD 2. vdsm mark drivespec for removal Possible implementation: <ovirt-vm:device devtype="disk" name="sdc" removing="yes"> ... </ovirt-vm:device> 3. vdsm detach device from VM 4. vsdm deactivate volume 5. vdsm remove drivespec from metadata Vdsm may be killed after any step. When recovering the VM, we can have these cases: 1. vdsm was killed after step 2, the CD is marked for removal, but device is attached to VM, and the volume is active: - change the metadata un-marking the drivespec for removal. 2. vdsm was killed after step 3, the drivespec is marked for removal, and the device is not attached to the VM, but the volume is active: - deactivate the volume - remove the drivespec from the metadata Note that this should not happen before vdsm deactivate unused logical volumes when starting so we should really have only case 3. 3. vdsm was killed after step 4, the drivespec is marked for removal, device not attached to the VM, and the volume is not active: - remove the drivespec from the metadata 4. vdsm was killed after step 5, everything is good in vdsm side. Teardown should not fail in storage layer if volume is still used. In current code we cannot avoid the lvm failures, but we don't have to pass the failure up to the caller, or we can pass specific error like "volume is used", which can be ignored silently by the caller, since this error means someone else is using this volume and is responsible for deactivating it. In case of simple errors that vdsm can report, engine may fail the change CD, or maybe continue to insert the new one, if the old Cd was detached. For example if old CD failed to deactivate, this is storage issue that should not fail attaching of a new CD. Storage layer should make sure the volume is deactivate if nobody uses it, but the user should not care about that. If vdsm was killed, engine will fail the operation, but it must wait until the VM is recovered. At this point engine get the VM XML and can update the real state of the CD, either still attached, or detached. Virt code should not care about deactivation failure. Storage layer should be concerned with deactivation of active volumes, but I don't think we handle such issues in engine yet. On vdsm side, unused logical volumes are deactivated during startup, see: https://github.com/oVirt/vdsm/blob/f6de2ca915faa719381e22086feb8b65aa4de9... We probably need to add periodic job in storage to deactivate unused volumes so we don't wait until the next vdsm restart. This can also be handled in engine but I don't think it is the right place to handle host state. This is a hack for supporting ISO, while the issue of shared block volume is not limited to ISO. We have this issue in many flows with many partial and broken solution like avoiding activation in engine (image transfer), or avoiding activation in vdsm (snapshot). This should be solved separately in a proper way in vdsm storage. This is a large and complex change that we may have in future version, not something for the next zstream. Unfortunately this area in vdsm was neglected for years, and we have huge technical debt. So for now we will have to live with warning when active ISO image is deactivated, or improve error handling so the caller can detect and handle properly the case of volume in use. I think better error handling will be easy to do and good enough for now. The solution require persistence, since vdsm can be killed, and most we would most likely use a database for this, but this should not be part of the mangedvolume database, since sqlite is not good with concurrent usage. Nir