Change in vdsm[master]: v2v: support for block devices

Shahar Havivi has uploaded a new change for review.

Change subject: v2v: support for block devices
......................................................................

v2v: support for block devices

extended support for block devices for KVM and VMWare.
currently we do support block devices that exported as file in libvirt:
<disk type='file' device='disk'>
  <source file='...'/>
  ...

Added support for type block:
<disk type='block' device='disk'>
  <source dev='/dev/mapper/...'/>
  ...

Bug-Url: https://bugzilla.redhat.com/1378340
Change-Id: I5f7a85715764efded7b296e858b130a05fe10f2a
Signed-off-by: Shahar Havivi 
---
M lib/vdsm/v2v.py
1 file changed, 6 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/72/64272/1

diff --git a/lib/vdsm/v2v.py b/lib/vdsm/v2v.py
index 4752477..ceb2b2b 100644
--- a/lib/vdsm/v2v.py
+++ b/lib/vdsm/v2v.py
@@ -1066,6 +1066,7 @@
 def _add_disks(root, params):
     params['disks'] = []
     disks = root.findall('.//disk[@type="file"]')
+    disks = disks + root.findall('.//disk[@type="block"]')
     for disk in disks:
         d = {}
         device = disk.get('device')
@@ -1077,6 +1078,11 @@
         source = disk.find('./source/[@file]')
         if source is not None:
             d['alias'] = source.get('file')
+        else:
+            source = disk.find('./source/[@dev]')
+            if source is not None:
+                d['alias'] = source.get('dev')
+
         driver = disk.find('./driver/[@type]')
         if driver is not None:
             try:

gerrit-hooks has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 1:

* #1378340::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1378340::OK, public bug
* Check Product::#1378340::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

Shahar Havivi has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 1: Verified+1

[Tomas Golembiovsky]

Tomas Golembiovsky has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 1: Code-Review+1

Adding +1, but it leaves me thinking whether we should do something for the other types as well. Based on documentation there are three other disk types: dir, netowork and volume.

Francesco Romani has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 1: Code-Review+1

let's just add a test (this patch or a new one) and it's fine. Partial ACK until the test is uploaded.

gerrit-hooks has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 2:

* update_tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1378340::OK, public bug
* Check Product::#1378340::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

Francesco Romani has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 2: Code-Review+1

(2 comments)

nice, thanks, few inline comments for clarity. Partial ACK.

https://gerrit.ovirt.org/#/c/64272/2/tests/v2vTests.py
File tests/v2vTests.py:

PS2, Line 130: file
is this correct for the test you added?
It seems to me that this works by accident

Line 534:                                        ProtectedPassword('password'),
Line 535:                                        None)['vmList']
Line 536: 
Line 537:         self.assertEqual(len(vms), len(VM_SPECS))
Line 538:         self.assertTrue('mapper' in vms[0]['disks'][0]['alias'])
This is a bit clearer, IMO:

self.assertEqual('/dev/mapper/vdev', vms[0]['disks'][0]['alias'])

but it's untested, could be broken; also, please consider making '/dev/mapper/vdev' a constant.
Line 539: 
Line 540:     def testXenBlockDevice(self):
Line 541:         def _connect(uri, username, passwd):
Line 542:             self._vms[0].setDiskType('block')

gerrit-hooks has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 3:

* update_tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1378340::OK, public bug
* Check Product::#1378340::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

Shahar Havivi has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 2:

(2 comments)

https://gerrit.ovirt.org/#/c/64272/2/tests/v2vTests.py
File tests/v2vTests.py:

PS2, Line 130: file
> is this correct for the test you added?

yes,
the source can be file or dev for block.

Line 534:                                        ProtectedPassword('password'),
Line 535:                                        None)['vmList']
Line 536: 
Line 537:         self.assertEqual(len(vms), len(VM_SPECS))
Line 538:         self.assertTrue('mapper' in vms[0]['disks'][0]['alias'])
> This is a bit clearer, IMO:

Done
Line 539: 
Line 540:     def testXenBlockDevice(self):
Line 541:         def _connect(uri, username, passwd):
Line 542:             self._vms[0].setDiskType('block')

Francesco Romani has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 3: Code-Review+2

seems good enough. Please make jenkins happy, or add a comment why we shouldn't care (and notify infra or the test owners)

gerrit-hooks has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 4:

* update_tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1378340::OK, public bug
* Check Product::#1378340::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

Shahar Havivi has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 4: Verified+1

Francesco Romani has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 4: Code-Review+2

[Tomas Golembiovsky]

Tomas Golembiovsky has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 4: Code-Review-1

(1 comment)

As pointed out by a user on ML the change breaks engine. VDSM fails to fetch disk size and logs an exception. Although the the disk size field is marked as optional in the scheme engine does not consider the size opotional aparently.

Maybe we should provide an alternative logic of fetching disk size for block devices. Adding -1 for the moment.

https://gerrit.ovirt.org/#/c/64272/2/tests/v2vTests.py
File tests/v2vTests.py:

PS2, Line 130: {dis
> yes,

Are you sure? Based on what I see in docs it's more like file attribute is for file and dev attribute is for block.

Francesco Romani has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 4: Code-Review-1

Ooops, good catch Tomas.

[Tomas Golembiovsky]

Tomas Golembiovsky has posted comments on this change.

Change subject: v2v: support for block devices
......................................................................

Patch Set 4:

(1 comment)

I investigated this a little and we can get the device size with libvirt call virDomainGetBlockInfo().

Another thing is that the kvm2ovirt tool needs to be updated too. It also assumes that the disk is volume in storage pool, which a block device is not. Another downloading logic has to be implemented around virDomainBlockPeek().

https://gerrit.ovirt.org/#/c/64272/4//COMMIT_MSG
Commit Message:

PS4, Line 9: VMWare
Is that really possible in VMware?

[Code Review]

From Dan Kenigsberg :

Dan Kenigsberg has submitted this change and it was merged.

Change subject: v2v: support for block devices
......................................................................

v2v: support for block devices

extended support for block devices for KVM and VMWare.
currently we do support block devices that exported as file in libvirt:
<disk type='file' device='disk'>
  <source file='...'/>
  ...

Added support for type block:
<disk type='block' device='disk'>
  <source dev='/dev/mapper/...'/>
  ...

Bug-Url: https://bugzilla.redhat.com/1378340
Change-Id: I5f7a85715764efded7b296e858b130a05fe10f2a
Signed-off-by: Shahar Havivi 
---
M lib/vdsm/v2v.py
M tests/v2vTests.py
2 files changed, 50 insertions(+), 8 deletions(-)

Approvals:
  Shahar Havivi: Verified
  Jenkins CI: Passed CI tests
  Francesco Romani: Looks good to me, approved
  Tomas Golembiovsky: Looks good to me, but someone else must approve