Bug 1332960

Summary: Detach a vm's disk using the API will remove the disk permanently
Product: [oVirt] ovirt-engine Reporter: Raz Tamir <ratamir>
Component: RestAPIAssignee: Juan Hernández <juan.hernandez>
Status: CLOSED CURRENTRELEASE QA Contact: Raz Tamir <ratamir>
Severity: high Docs Contact:
Priority: unspecified    
Version: 4.0.0CC: amureini, bugs, juan.hernandez, ratamir, sbonazzo, tnisan, ylavi
Target Milestone: ovirt-4.0.0-betaKeywords: Automation
Target Release: 4.0.0Flags: rule-engine: ovirt-4.0.0+
ylavi: planning_ack+
amureini: devel_ack+
acanan: testing_ack+
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-07-05 07:49:22 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: Storage RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
engine and vdsm logs none

Description Raz Tamir 2016-05-04 12:29:09 UTC
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:

Comment 1 Tal Nisan 2016-05-05 09:25:16 UTC
Is it a regression?

Comment 2 Raz Tamir 2016-05-05 09:35:20 UTC
Yes it is

Comment 3 Allon Mureinik 2016-05-05 09:39:40 UTC
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.

Comment 4 Allon Mureinik 2016-05-05 09:42:43 UTC
(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?

Comment 5 Yaniv Lavi 2016-05-05 10:18:18 UTC
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.

Comment 6 Juan Hernández 2016-05-05 12:54:54 UTC
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.

Comment 7 Allon Mureinik 2016-05-05 15:10:16 UTC
(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?

Comment 8 Yaniv Lavi 2016-05-05 15:21:19 UTC
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?

Comment 9 Juan Hernández 2016-05-05 16:07:18 UTC
(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.

Comment 10 Raz Tamir 2016-05-05 16:10:37 UTC
(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

Comment 11 Juan Hernández 2016-05-05 16:16:29 UTC
(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.

Comment 12 Juan Hernández 2016-05-06 10:29:53 UTC
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

Comment 13 Raz Tamir 2016-06-15 07:00:15 UTC
Verified on ovirt-engine-4.0.0.4-0.1.el7ev.noarch using the steps to reproduced

Comment 14 Sandro Bonazzola 2016-07-05 07:49:22 UTC
oVirt 4.0.0 has been released, closing current release.