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

On 25 Aug 2020, at 13:02, Vojtech Juranek <vjuranek(a)redhat.com&gt; wrote: On čtvrtek 20. srpna 2020 14:42:15 CEST Michal Skrivanek wrote: >> On 20 Aug 2020, at 14:28, Nir Soffer <nsoffer(a)redhat.com&gt; wrote: >> >> On Thu, Aug 20, 2020 at 12:19 PM Vojtech Juranek <vjuranek(a)redhat.com&gt; >> wrote: > >>> >>> Hi, >>> as Fedor is on PTO, I was asked to take over. >>> >>> I don't think new functionality is need, more detail list of proposed >>> flows and changes is bellow, TL;DR: I'd suggest to >>> >>> - change in engine: send PDIV instead of CD path from engine to vdsm > > > the biggest issue here is always with older versions not having this > functionality yet…. yes, I was wrong, this requires new functionality. Therefore Nir proposed to create a feature page for it. We put together flows which should cover all the possible issues (including VM recovery). Please review and comment directly under https://github.com/oVirt/ovirt-site/pull/2320 (unless you think creating feature page for it is wrong way to go:-)

> >>> - change in vdsm: implement counter of ISOs being used by VMs to know >>> when we can deactivate volume - change in vdsm: remove old drivespec >>> from VM XML when changing/removing CD (and eventually deactivate >>> volume)>> >>> >>> You comments are welcome. >>> Thanks >>> Vojta >>> >>> Flows >>> === >>> >>> VM without a CD >>> ------------------------- >>> >>> - Should not be possible to insert any CD, this option should not be >>> available/active in the UI. > >> >> 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 > > > we don’t. All VMs have always at least that one implicit CD (empty or > loaded) We have a case where there is additional CD for > cloud-init/payload, that additional CD is not “changeable” nor exposed in > UI anywhere > >> possible, the menu/button >> should be disabled in the UI. >> >> >>> VM without CD, changeCD inserts new ISO > > > please update to make it really clear, there’s no “VM without CD”. VM always > has CD, just not loaded (empty tray) so this case doesn’t exist… > > >>> ----------------------------------------------------------------- >>> >>> - add new drivespec to VM metadata >> >> >> How failure is handled? >> >> >>> - prepare new drivespec >> >> >> What if vdsm is restarted at this point? >> >> How VM recovery should handle this drivespec referring to inactive >> volume? >> >> >>> - attach new device to VM >>> - if attaching to VM fails, tear down drivespec and remove drivespec >>> from VM metadata > >> >> 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. >> >> >>> VM with CD, changeCD removes current ISO >>> ------------------------------------------------------------------- >>> >>> - 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 > >> >> Sounds good, and may be already handled in lvm module. You can try to >> deactivate LV twice to confirm this. >> >> >>> - if drivespec is used by another VM, don’t deactivate volume >> >> >> 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. > > > We do have the list of active VMs and their xml/devices, so we do know if > there are other users, it’s nt an expensive check. Locking it properly so > there’s no in flight request for change cd could be a bit tricky but it > doesn’t sound that complex >> >> >>> -remove drivespec from VM metadata >> >> >> 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. > > > remove what exactly? CD device stays there, the "tray is ejected”, device’s > path is updated to “". > >> >> >>> 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 - if drivespec is used by another >>> VM, don’t deactivate volume >>> >>> - remove previous drivespec from VM metadata >>> - add new drivespec to VM metadata >>> - prepare new drivespec >>> - 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. > > > there’s a big difference between hotplugs and change CD in terms of the > devices. It’s supposedly an atomic change. Though having a separate actions > for load/eject would be probably ok. But not sure if worth it > >> >> 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 > > > please update the flows. no device detach/attach… > > >> 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/f6de2ca915faa719381e22086feb8b65aa4de94 >> 2/lib/vdsm/storage/lvm.py#L1023 >> We probably need to add periodic job in storage to deactivate unused > > > sounds good enough indeed > > >> 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. >> >> >>> Proposed changes >>> =========== >>> >>> Engine >>> ---------- >>> >>> - change ChangeDiskCommand to use PDIV (pool, domain, image, volume ID) >>> instead of ISO path - modify UI not to provide an option to change CD >>> for VMs without CD>> >>> >>> vdsm >>> -------- >>> >>> - add volume/ISO counter which would count the number of VM using the >>> given ISO. When CDROM is inserted, the counter is increased, when >>> ejected, the counter is decreased. ISO volume is deactivated only when >>> on VM uses it. > >> >> 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. >> >> >>> We already have sqlite DB for managed storage, so we can add DB for >>> ISO quite easily. >> >>> - modify Vm._changeBlockDev to reflect flows suggested above >> >> >> 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 >> >> >> >>>> Hi Fedor, >>>> >>>> >>>> >>>>> On 27 Jun 2020, at 14:46, Fedor Gavrilov <fgavrilo(a)redhat.com&gt; wrote: >>>>> >>>>> >>>>> >>>>> So from what I was able to see in hotplugDisk example, for CDs I would >>>>> have to do similar: >>>> >>>> >>>> >>>>> >>>>> >>>>> 1. parse disk params from stored xml metadata >>>>> 2. call prepareVolumePath with these params >>>>> (skip normalizeVdsmImg) >>>>> 3. call updateDriveIndex >>>> >>>> >>>> >>>> Why? >>>> >>>> >>>> >>>>> (skip validating whether volume has leases) >>>>> 4. drive.getXML() and then hooks.before_disk_hotplug with it >>>> >>>> >>>> >>>> If only the storage stuff would ever finish moving to xml.... >>>> But it’s not right to call this hook anyway, iiuc you’re not doing a >>>> hotplug >>> >>> >>> >>>> >>>> >>>>> 5. call attachDevice with drive XML and handle possible errors >>>>> >>>>> >>>>> >>>>> I am not sure, however, whether we need what "else" does in >>>>> hotplugDisk >>>>> (vm.py line 3468)... >>>> >>>> >>>> >>>>> >>>>> >>>>> Looking forward to hearing your opinions, >>>>> Fedor >>>>> >>>>> >>>>> >>>>> ----- Original Message ----- >>>>> From: "Fedor Gavrilov" <fgavrilo(a)redhat.com&gt; >>>>> To: "devel" <devel(a)ovirt.org&gt; >>>>> Sent: Monday, June 22, 2020 4:37:59 PM >>>>> Subject: [ovirt-devel] implementing hotplugCd/hotunplugCd in vdsm >>>>> >>>>> >>>>> >>>>> Hey, >>>>> >>>>> >>>>> >>>>> So in an attempt to fix change CD functionality we discovered a few >>>>> other >>>>> potential >>>> >>>> >>>> CD has a long history of issues caused by historical simplifications. >>>> What is the intended fix, stop using string paths? >>>> >>>> >>>> >>>>> issues and what Nir suggested was to implement two [somewhat] new >>>>> functions in VDSM: hotplug and hotunplug for CDs similar to how it >>>>> works >>>>> for normal disks now. >>>> >>>> >>>> This is conceptually different, CD is a removable media, >>>> hotplug/unplug is for the device itself. We never supported hotplug of >>>> the device though. >>>> >>>> >>>> >>>>> Existing changeCD function will be left as is for backwards >>>>> compatibility. >>>> >>>> >>>> >>>> Who would use the new function? >>>> >>>> >>>> >>>>> As I found out, engine already calculates iface and index before >>>>> invoking >>>>> VDSM functions, so we will just pass these along with PDIV to the >>>>> VDSM. >>>> >>>> >>>> >>>>> >>>>> >>>>> Suggested flow is, let me quote: >>>>> >>>>> >>>>> >>>>> >>>>>> So the complete change CD flow should be: >>>>>> >>>>>> >>>>>> >>>>>> 1. get the previous drivespec from vm metadata >>>>>> 2. prepare new drivespec >>>>>> 3. add new drivespec to vm metadata >>>>>> 4. attach a new device to vm >>>> >>>> >>>> >>>> Don’t call it a new device when you just change a media >>>> >>>> >>>> >>>>>> 5. teardown the previous drivespec >>>>>> 6. remove previous drivespec from vm metadata >>>>>> >>>>>> >>>>>> >>>>>> When the vm is stopped, it must do: >>>>>> >>>>>> >>>>>> >>>>>> 1. get drive spec from vm metadata >>>>>> 2. teardown drivespec >>>>>> >>>>>> >>>>>> >>>>>> During attach, there are interesting races: >>>>>> - what happens if vdsm crashes after step 2? who will teardown the >>>>>> volume? >>>>>> maybe you need to add the new drivespec to the metadata first, >>>>>> before preparing it. >>>>>> - what happens if attach failed? who will remove the new drive from >>>>>> the metadata? >>>>> >>>>> >>>>> >>>>> >>>>> Now, what makes hotplugDisk/hotunplugDisk different? From what I >>>>> understand, the flow is same there, so what difference is there as far >>>>> as >>>>> VDSM is concerned? If none, this means if I more or less copy that >>>>> code, >>>>> changing minor details and data accordingly for CDs, this should work, >>>>> shouldn't it? >>>> >>>> >>>> Hotplug/unplug is similar, but I would like to see the proposed change >>>> in context of engine as well, and I expect it to be a bit more complex >>>> there. Without that this is not worth it. >>>> >>>> Thanks, >>>> michal >>>> >>>> >>>>> >>>>> >>>>> Thanks! >>>>> Fedor >>>>> _______________________________________________ >>>>> Devel mailing list -- devel(a)ovirt.org >>>>> To unsubscribe send an email to devel-leave(a)ovirt.org >>>>> Privacy Statement: https://www.ovirt.org/privacy-policy.html >>>>> oVirt Code of Conduct: >>>>> https://www.ovirt.org/community/about/community-guidelines/ >>> >>> List >>> >>>>> Archives: >>>>> https://lists.ovirt.org/archives/list/devel@ovirt.org/message/LAQR3RW4R >>>>> MT >>>>> UNFUXL5T4HWLPKXJKEC3Y/ _______________________________________________ >>>>> Devel mailing list -- devel(a)ovirt.org >>>>> To unsubscribe send an email to devel-leave(a)ovirt.org >>>>> Privacy Statement: https://www.ovirt.org/privacy-policy.html >>>>> oVirt Code of Conduct: >>>>> https://www.ovirt.org/community/about/community-guidelines/ >>> >>> List >>> >>>>> Archives: >>>>> https://lists.ovirt.org/archives/list/devel@ovirt.org/message/VKPDAB7XX >>>>> TQ >>>>> KPYWAZEHQYTV7TRCQQI2E/ >>>> >>>> _______________________________________________ >>>> Devel mailing list -- devel(a)ovirt.org >>>> To unsubscribe send an email to devel-leave(a)ovirt.org >>>> Privacy Statement: https://www.ovirt.org/privacy-policy.html >>>> oVirt Code of Conduct: >>>> https://www.ovirt.org/community/about/community-guidelines/ List >>>> Archives: >>>> https://lists.ovirt.org/archives/list/devel@ovirt.org/message/W2U55OKENZ >>>> P2Z MNG7AJDYWE3NKBOPGOA/ >>> >>> >> >> > > _______________________________________________ > Devel mailing list -- devel(a)ovirt.org > To unsubscribe send an email to devel-leave(a)ovirt.org > Privacy Statement: https://www.ovirt.org/privacy-policy.html > oVirt Code of Conduct: > https://www.ovirt.org/community/about/community-guidelines/ List Archives: > https://lists.ovirt.org/archives/list/devel@ovirt.org/message/BJ2J73ODFQWNM > HRIOXTKI2YGQD4YV76S/