Re: [Engine-devel] Floating Disks implementation in REST-API

On 10/04/12 14:35, Geert Jansen wrote: > > > On 04/09/2012 05:14 PM, Ori Liel wrote: >> The "Floating Disks" feature makes disks into stand-alone >> entities: a >> given disk may be attached to a VM (as all disks are today), or it >> may >> be not attached to any VM, which makes it a floating disk >> (http://www.ovirt.org/wiki/Features/FloatingDisk) >> >> To implement attach/detach of disk to/from VM in REST-API, we >> intend >> to introduce two new actions: >> >> POST .../api/vms/{vm:id}/disks/{disk:id}/attach >> POST .../api/vms/{vm:id}/disks/{disk:id}/detach >> >> Since we try not to add new actions unless we have to, I want to >> explain why I believe that these actions are necessary. >> The other implementation would use existing add/remove flows: >> >> POST .../api/vms/{vm:id}/disks - if the disk >> was >> passed with an ID, attach it to this VM. If no id - create a new >> disk. >> DELETE .../api/vms/{vm:id}/disks/{disk:id} - *ambiguity >> problem, need to add a flag* >> >> We can't break existing API, so regular DELETE must remove the >> disk, >> as it does today. To detach a disk using DELETE we'd have to add a >> flag to the DETELE command. This is quite risky, because if the >> user >> forgets to pass this flag, the disk which he wanted to detach will >> actually be deleted. >> >> Theoretically, if we could break the API, the following modelling >> would resolve the ambiguity and perhaps be ideal: >> - POST/DELETE disk in root context means create or delete it. >> - POST/DELETE disk in VM context means attach or detach it. >> But we don't have the privilege of breaking the API. >> >> Considering all of the above - and the fact that attach/detach >> nics >> to/from host is also implemented using actions - I believe that >> the >> new actions are justifiable. >> >> Any comments? > > I assume that a floating disk can be attached to 0 VMs as well, > right? floating == attached to 0 VMs > So i would assume we get a top-level /disks collection correct?? Yes > And I assume that collection would only list floating disks? IMO the /disks collection includes all disks in the setup regardless if they are floating or attached to VM/s > > In my view, backwards compatible DELETE semantics for a floating > disk > aren't that bad: > > DELETE /vms/{vm:id}/disks/{disk:id} > > => Accept a <detach>true|false</detach> argument. > => Defaults to "false" for compatibility. > I think that ideally : Detach a disk from VM (becomes floating): DELETE api/vms/{vm:id}/disks/{disk:id} Delete a disk ('real' delete)" DELETE api/disks/{disk:id}

I understand that it is changing the current API behavior, it means that a user who deleted disks in 3.0 will end up only detaching them in 3.1, is that something we can't 'live' with? For attach, I would go with Eoghan approach: POST /api/vms/{vm:id}/disks <disk id="{disk:id}"/> => attach disk > This means that DELETE will by default really DELETE any > non-floating > disk. That is compatibility with today. For safety, implement this: > > => When deleting a disk that is floating, fail unless <force> is > also > set to "true". And always fail if one of the other VMs is running > (obviously). > > To attach a disk and make it float, use POST and you and Eoghan > described. > > To detach a disk, use DELETE with <detach>true</detach>. > > To delete a non-attached, floating disks, use: > > DELETE /disks/{disk:id} > > To delete an attached, floating disk, use: > > DELETE /vm/{vm:id}/disks/{disk:id} with <force>true</force>; OR ^^ isn't that considered breaking the API? we require force flag for deleting disk which we did not require before. > DELETE /disks/{disk:id} > > To create a non-attached, floating, disk, use: > > POST /disks > > Regards, > Geert > _______________________________________________ > Engine-devel mailing list > Engine-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel