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

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 - 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. VM without CD, changeCD inserts new ISO ----------------------------------------------------------------- - add new drivespec to VM metadata - prepare new drivespec - attach new device to VM - if attaching to VM fails, tear down drivespec and remove drivespec from VM metadata 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 - if drivespec is used by another VM, don’t deactivate volume -remove drivespec from VM metadata 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 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. 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 > 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/LAQR3RW4RM > > T > > 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/VKPDAB7XXT > > Q > > 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/W2U55OKENZP2 > Z > MNG7AJDYWE3NKBOPGOA/