Hide Forgot
Created attachment 1153830 [details] engine and vdsm logs Description of problem: When trying to detach a vm's disk using REST API: DELETE on /api/vms/{vm: id}/disks/{disk: id} <action> <detach>true</detach> </action> will remove the disk permanently from the system Version-Release number of selected component (if applicable): ovirt-engine-4.0.0-0.0.master.20160502171442.git6123627.el7.centos.noarch vdsm-4.17.999-1036.git19a27af.el7.centos.noarch How reproducible: 100% Steps to Reproduce: 1. Create vm and attach disk to it 2. Senf HTTP DELETE request on /api/vms/{vm: id}/disks/{disk: id} with body: <action> <detach>true</detach> </action> 3. Actual results: The disk will remove from the system Expected results: The disk should be detached from the vm and remain as part of the disks collection Additional info:
Is it a regression?
Yes it is
Detaching a disk (as opposed to removing it) should be done with the **detach_only** action parameter, not **detach**. Regardless, looking forwards, having the ability to remove a disk from the system via a subcollection (as opposed to via its main collection) sounds risky and error prone at best (consider, e.g., the usecase in this bug - it's a simple mistake, but with dire consequences) and plain wrong at worst. Since we have the opportunity to break backwards compatibility in v4, this is a good time to remove it. Yaniv - please give pm ack/nack to removing this option altogether, and leaving the DELETE action under /api/vms/{vm: id}/disks/{disk: id} for detaching only.
(In reply to Allon Mureinik from comment #3) > Detaching a disk (as opposed to removing it) should be done with the > **detach_only** action parameter, not **detach**. Correction: in V4, this is a matrix parameter (/vms/{vm:id}/disks/{disk:id};detach_only=true), which was intentionally introduced in commit https://gerrit.ovirt.org/#/c/47892/. Raz - does this problem reproduce in V3 API?
We should only allow deletion of disk from the disks service not via VMs service. Via VM service the only action should be attach and detach.
The option to remove the disk completely can be removed from version 4 of the API, but it must be preserved in version 3. It can be achieved with the following patch: restapi: Translate "action.detach" into "detach_only" https://gerrit.ovirt.org/57091 But that patch is incompatible with removing the "deatch_only" parameter from version 4 of the API. If you decide to remove it then the V3 to V4 translation will be more complicated, as V3 would need to translate the call to remove the disk into two calls to V4: one to detach the disk (to the VM disks sub-collection) and another one to remove it (to the top level disks collection). You can first merge this patch I'm proposing, and rework the one that you are proposing, or just rework the one you are proposing, but in V3 the operation needs to work as it used to work.
(In reply to Juan Hernández from comment #6) > The option to remove the disk completely can be removed from version 4 of > the API, but it must be preserved in version 3. It can be achieved with the > following patch: > > restapi: Translate "action.detach" into "detach_only" > https://gerrit.ovirt.org/57091 > > But that patch is incompatible with removing the "deatch_only" parameter > from version 4 of the API. If you decide to remove it then the V3 to V4 > translation will be more complicated, as V3 would need to translate the call > to remove the disk into two calls to V4: one to detach the disk (to the VM > disks sub-collection) and another one to remove it (to the top level disks > collection). > > You can first merge this patch I'm proposing, and rework the one that you > are proposing, or just rework the one you are proposing, but in V3 the > operation needs to work as it used to work. To reiterate the comment on gerrit - I don't have anything against your suggestion, but I must admit I don't quite understand the motivation. IIUC, patch https://gerrit.ovirt.org/#/c/47892/ ALREADY broke backwards compatibility by moving the parameter from the <action> in the request body to the matrix. If we're already breaking backwards compatibility, why not go all the way and just remove this confusing option altogether?
Can we disable it in 4.0 version of the API and use it in 3.x API and remove it completely in 4.1?
(In reply to Allon Mureinik from comment #7) > To reiterate the comment on gerrit - I don't have anything against your > suggestion, but I must admit I don't quite understand the motivation. IIUC, > patch https://gerrit.ovirt.org/#/c/47892/ ALREADY broke backwards > compatibility by moving the parameter from the <action> in the request body > to the matrix. If we're already breaking backwards compatibility, why not go > all the way and just remove this confusing option altogether? Patch https://gerrit.ovirt.org/#/c/47892/ did broke backwards compatibility, because it was done before we decided that version 4 of the engine will support versions 3 and 4 of the API and I forgot to re-implement the V3 behaviour. That breakage is a regression, and needs to be fixed. It is OK to remove the confusing parameter from V4, but second priority in my opinion.
(In reply to Allon Mureinik from comment #4) > (In reply to Allon Mureinik from comment #3) > > Detaching a disk (as opposed to removing it) should be done with the > > **detach_only** action parameter, not **detach**. > Correction: in V4, this is a matrix parameter > (/vms/{vm:id}/disks/{disk:id};detach_only=true), which was intentionally > introduced in commit https://gerrit.ovirt.org/#/c/47892/. > > Raz - does this problem reproduce in V3 API? Yes, this happens on V3 API
(In reply to Yaniv Dary from comment #8) > Can we disable it in 4.0 version of the API and use it in 3.x API and remove > it completely in 4.1? We can do that, but it would pollute the V4 code, which I prefer to avoid. If you don have an objection I'll prepare a patch to remove completely the parameter from V4 and still support the operation in V3. It will be a mix of the two patches that we suggested, and some other ingredients. Please assign me the bug if you agree.
These patches, for the specification and implementation of the API, show a possible way to drop the "detach" parameter in V4, and preserve backwards compatibility in V3: https://gerrit.ovirt.org/#/q/topic:detach_doesnt_remove_disk
Verified on ovirt-engine-4.0.0.4-0.1.el7ev.noarch using the steps to reproduced
oVirt 4.0.0 has been released, closing current release.