Bug 1366960 - Disk element under storagedomains/{domain-id}/disks;unregistered should have 'register' Action
Summary: Disk element under storagedomains/{domain-id}/disks;unregistered should have ...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: ovirt-engine
Classification: oVirt
Component: RestAPI
Version: 4.0.2.6
Hardware: Unspecified
OS: Unspecified
unspecified
low
Target Milestone: ovirt-4.2.0
: 4.2.0
Assignee: Maor
QA Contact: Elad
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-08-15 07:13 UTC by Eyal Shenitzky
Modified: 2017-12-20 10:47 UTC (History)
6 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2017-12-20 10:47:02 UTC
oVirt Team: Storage
Embargoed:
rule-engine: ovirt-4.2+
rule-engine: planning_ack+
amureini: devel_ack+
ratamir: testing_ack+


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
oVirt gerrit 69634 0 master MERGED Add 'StorageDomainDisks' and 'AttachedStorageDomainDisks' 2017-01-24 10:59:25 UTC
oVirt gerrit 69635 0 master MERGED Add operation to register storage domain disk 2017-01-27 16:28:58 UTC
oVirt gerrit 70587 0 master MERGED restapi: Implement 'StorageDomainDisks' and 'AttachedStorageDomainDisks' 2017-02-09 10:14:47 UTC
oVirt gerrit 81424 0 master POST restapi: Support register disk from BASDDR 2017-09-04 20:54:55 UTC

Description Eyal Shenitzky 2016-08-15 07:13:35 UTC
Description of problem:
On REST-API, in storage domain with unregister disk
under storagedomains/{doamin-id}/disks;unregistered,
Action 'register' to disk is missing.

Currently the command to register unregistered disk using REST is:

/api/storagedomains/{domain-id}/disks;unregistered

<disk id='{the id of the disk to be registered}'></disk>

To register unregistered VM/Template from storage domain the command is:

/api/storagedomains/{domain-id}/vms|templates/{vm/template-id}/register

<action>
    <cluster id='xxxxxxx-xxxx-xxxx-xxxxxx'></cluster>
</action>

Register Disk should be in the same way as register VM or Template.


Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.Create a floating disk on storage domain
2.Detach the storage domain from the data-center 
3.Attach the storage domain from step 2 back to the data center
4.Using REST - /api/storagedomains/{domain-id}/disks;unregistered

<disk id='{the id of the disk to be registered}'></disk>

Actual results:


Expected results:


Additional info:

Comment 1 Eyal Shenitzky 2016-08-16 08:51:23 UTC
Furthermore,

The job notification for disk registration is - Registering Disk ...
And for registering Vm or template is: Importing Template/VM ...

The notification should be consistent.

Comment 2 Maor 2016-08-17 09:45:17 UTC
I've uploaded a patch which fixes the job issue (see https://gerrit.ovirt.org/#/c/62407).
Eyal, if you think it should be verified or backported please open a separate bug on it.

Regarding the register disk operation in REST, this operation was introduced before the import VM/Template (IINM at version 3.3) and it is also being used for other operations except DR, like moving netapp disks to oVirt storage domains.

Replacing the existing API might reflect on existing REST scripts, so we can do one of the following:
1) Add a new API so register disk will be executed in both ways.
2) Change the existing API to be compatible with import VM/Template and add a doc text.
3) Leave the API as it is

Comment 3 Juan Hernández 2016-08-17 10:21:17 UTC
If I understand correctly retrieving an unregistered disk doesn't register it. I mean, if you send this request:

  GET /storagedomains/{storagedomain:id}/disks/{disk:id};unregistered

What you get is the description of that disk, but it will stay unregistered. There is actually now way to register the disk, so adding a new "register" operation to the disk doesn't break backwards compatibility, in my opinion is perfectly OK to add it. It should work as follows:

  POST /storagedomains/{storagedomain:id}/disks/{disk:id}/register
  <action/>

If parameters are needed they should go inside the "action" tag.

Comment 4 Juan Hernández 2016-08-17 10:22:06 UTC
I meant "there is *no* way to register the disk", sorry.

Comment 5 Maor 2016-08-17 11:44:12 UTC
(In reply to Juan Hernández from comment #3)
> If I understand correctly retrieving an unregistered disk doesn't register
> it. I mean, if you send this request:
> 
>   GET /storagedomains/{storagedomain:id}/disks/{disk:id};unregistered

If you will use POST and use the same API and add the disk id, as so:

  POST /api/storagedomains/60cec75d-f01d-44a0-9c75-8b415547bc3d/disks;unregistered HTTP/1.1
  Accept: application/xml
  Content-type: application/xml

  <disk id='8ddb988f-6ab8-4c19-9ea0-b03ab3035347'></disk>

It should register it to the DB.

 
> 
> What you get is the description of that disk, but it will stay unregistered.
> There is actually now way to register the disk, so adding a new "register"
> operation to the disk doesn't break backwards compatibility, in my opinion
> is perfectly OK to add it. It should work as follows:
> 
>   POST /storagedomains/{storagedomain:id}/disks/{disk:id}/register
>   <action/>
> 
> If parameters are needed they should go inside the "action" tag.

Comment 6 Juan Hernández 2016-08-17 11:59:41 UTC
Thanks Maor, you are right. As there is already a way to register the disk I think that we can consider this a "nice to have", thus lowering the severity and retargetting to 4.1.

Comment 7 Allon Mureinik 2016-08-21 07:45:51 UTC
(In reply to Juan Hernández from comment #6)
> Thanks Maor, you are right. As there is already a way to register the disk I
> think that we can consider this a "nice to have", thus lowering the severity
> and retargetting to 4.1.

Agreed.

Comment 8 Juan Hernández 2017-01-24 11:01:20 UTC
The changes to the specification required for this fix are too risky for 4.1. I am re-targeting to 4.2.

Comment 9 Allon Mureinik 2017-04-24 14:22:55 UTC
Maor, all the attached patches are merged. Are we missing anything else?

Comment 10 Maor 2017-04-24 14:56:53 UTC
(In reply to Allon Mureinik from comment #9)
> Maor, all the attached patches are merged. Are we missing anything else?

Yes, we need to implement this in the backend as well, I will update the bug to new until I will get to it.

Comment 11 Yaniv Kaul 2017-09-04 14:46:06 UTC
(In reply to Maor from comment #10)
> (In reply to Allon Mureinik from comment #9)
> > Maor, all the attached patches are merged. Are we missing anything else?
> 
> Yes, we need to implement this in the backend as well, I will update the bug
> to new until I will get to it.

Any updates?

Comment 12 Maor 2017-09-06 15:14:38 UTC
(In reply to Yaniv Kaul from comment #11)
> (In reply to Maor from comment #10)
> > (In reply to Allon Mureinik from comment #9)
> > > Maor, all the attached patches are merged. Are we missing anything else?
> > 
> > Yes, we need to implement this in the backend as well, I will update the bug
> > to new until I will get to it.
> 
> Any updates?

Posted a patch, should be merged soon

Comment 13 Elad 2017-09-18 07:41:59 UTC
Maor, I still can't find the unregistered disks under /api/storagedomains/%domainid%/disks/

The unregistered disk exist under /api/storagedomains/%domainid%/disks;

Comment 14 Elad 2017-09-18 07:45:21 UTC
*The unregistered disk exist under /api/storagedomains/%domainid%/disks;unregistered

Comment 15 Maor 2017-09-18 08:23:31 UTC
It is now done through an attached storage domain.
The API is:
  /api/datacenters/111/storagedomains/222/disks/333/register

Comment 16 Elad 2017-09-28 14:44:24 UTC
Maor, still can't find the unregistered disk under /api/datacenters/%id%/storagedomains/%id%/disks

While in Webadmin, the disk is listed under Disk import.

Comment 17 Maor 2017-09-28 15:16:23 UTC
Try to add ";unregistered" at the end:
   api/datacenters/%id&/storagedomains/%id&/disks;unregistered

Comment 18 Elad 2017-10-01 13:35:01 UTC
POST to:

/api/datacenters/111/storagedomains/222/disks/333/register with <action></action> 

For an unregistered disk works properly.

Verified using:
ovirt-engine-4.2.0-0.0.master.20170929123516.git007c392.el7.centos.noarch
ovirt-engine-restapi-4.2.0-0.0.master.20170929123516.git007c392.el7.centos.noarch

Comment 19 Sandro Bonazzola 2017-12-20 10:47:02 UTC
This bugzilla is included in oVirt 4.2.0 release, published on Dec 20th 2017.

Since the problem described in this bug report should be
resolved in oVirt 4.2.0 release, published on Dec 20th 2017, it has been closed with a resolution of CURRENT RELEASE.

If the solution does not work for you, please open a new bug report.


Note You need to log in before you can comment on or make changes to this bug.