Change in vdsm[master]: v2v: add fake virt-v2v process for testing without virt-v2v

Shahar Havivi has uploaded a new change for review.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................

v2v: add fake virt-v2v process for testing without virt-v2v

Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Signed-off-by: Shahar Havivi <shaharh(a)redhat.com&gt;
---
M tests/Makefile.am
A tests/fakeVirtV2V.py
M tests/v2vTests.py
3 files changed, 49 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/38/47738/1

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 1168862..185c9d7 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -196,6 +196,7 @@
 	$(test_modules) \
 	apiData.py \
 	hookValidation.py \
+	fakeVirtV2V.py \
 	monkeypatch.py \
 	nettestlib.py \
 	storagefakelib.py \
diff --git a/tests/fakeVirtV2V.py b/tests/fakeVirtV2V.py
new file mode 100755
index 0000000..72c78c8
--- /dev/null
+++ b/tests/fakeVirtV2V.py
@@ -0,0 +1,27 @@
+#!/usr/bin/env python
+
+import sys
+
+if len(sys.argv) != 2:
+    print ('usage: %s <target dir>\n' % sys.argv[0])
+    sys.exit(1)
+
+
+target_dir = sys.argv[1]
+
+
+def write_progress():
+    for i in range(101):
+        print("    (%s/100%%)\r" % str(i))
+
+
+print('[   0.0] Opening the source -i libvirt\n')
+print('[   1.0] Creating an overlay to protect\n')
+print('[  88.0] Copying disk 1/2 to %s/4d8daea0-bf69-4984-9775-c0e826325669'
+      '/images/1c1e7572-a23a-4b19-89b9-09564b5c575e\n' % target_dir)
+write_progress()
+print('[  88.0] Copying disk 2/2 to %s/4d8daea0-bf69-4984-9775-c0e826325669'
+      '/images/1c1e7572-a23a-4b19-89b9-09564b5c575e\n' % target_dir)
+write_progress()
+print('[ 256.0] Creating output metadata')
+print('[ 256.0] Finishing off')
diff --git a/tests/v2vTests.py b/tests/v2vTests.py
index 3264d26..b3dd869 100644
--- a/tests/v2vTests.py
+++ b/tests/v2vTests.py
@@ -20,6 +20,7 @@
 from StringIO import StringIO
 
 import libvirt
+import os
 
 import v2v
 from vdsm import libvirtconnection
@@ -237,3 +238,23 @@
         self.assertEquals(network['type'], 'bridge')
         self.assertEquals(network['macAddr'], '00:0c:29:c6:a6:11')
         self.assertEquals(network['bridge'], 'VM Network')
+
+    def prepare_volumes(self):
+        return 0
+
+    def create_command(self):
+        return [os.path.join(os.path.abspath('.'), 'fakeVirtV2V.py'),
+                '/tmp/images']
+
+    def execution_environments(self):
+        return {}
+
+    @MonkeyPatch(v2v.ImportVm, '_prepare_volumes', prepare_volumes)
+    @MonkeyPatch(v2v.ImportVm, '_from_libvirt_command', create_command)
+    @MonkeyPatch(v2v.ImportVm, '_execution_environments',
+                 execution_environments)
+    def testVirtV2V(self):
+        ivm = v2v.ImportVm.from_libvirt(None, None, None, None,
+                                        'e3e52628-5c12-414d-9283-28914a3a7e7e',
+                                        None)
+        ivm._import()


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;

automation(a)ovirt.org has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 1: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Francesco Romani has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 1: Code-Review-1

(3 comments)

very nice first step, but we should have much less invasive changes in our v2v code. -1
for visibility.

https://gerrit.ovirt.org/#/c/47738/1/tests/Makefile.am
File tests/Makefile.am:

Line 195: dist_vdsmtests_PYTHON = \
Line 196: 	$(test_modules) \
Line 197: 	apiData.py \
Line 198: 	hookValidation.py \
Line 199: 	fakeVirtV2V.py \
I'd use something like fake-virt-v2v, to closely mimic the original virt-v2v. But it
is mostly appearance.
Line 200: 	monkeypatch.py \
Line 201: 	nettestlib.py \
Line 202: 	storagefakelib.py \
Line 203: 	storagetestlib.py \


https://gerrit.ovirt.org/#/c/47738/1/tests/fakeVirtV2V.py
File tests/fakeVirtV2V.py:

Line 3: import sys
Line 4: 
Line 5: if len(sys.argv) != 2:
Line 6:     print ('usage: %s <target dir>\n' % sys.argv[0])
Line 7:     sys.exit(1)
is this the same option/format the real virt-v2v expects?
Line 8: 
Line 9: 
Line 10: target_dir = sys.argv[1]
Line 11: 


https://gerrit.ovirt.org/#/c/47738/1/tests/v2vTests.py
File tests/v2vTests.py:

Line 243:         return 0
Line 244: 
Line 245:     def create_command(self):
Line 246:         return [os.path.join(os.path.abspath('.'),
'fakeVirtV2V.py'),
Line 247:                 '/tmp/images']
we should just override the path of the command to spawn.
I'd like to see the very same command line of the real virt-v2v being used.
Line 248: 
Line 249:     def execution_environments(self):
Line 250:         return {}
Line 251: 


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 1:

(8 comments)

Thanks Shahar!

To make it easier to make progress, I think we should start with really stupid
fake-virt-v2v script that can be used in real setup, not in the tests.

Having this script, one can run real import without vmware license, checking that process
management works using engine ui.

Using this script in the tests can be a second step, since it requires much more work.

https://gerrit.ovirt.org/#/c/47738/1/tests/Makefile.am
File tests/Makefile.am:

Line 195: dist_vdsmtests_PYTHON = \
Line 196: 	$(test_modules) \
Line 197: 	apiData.py \
Line 198: 	hookValidation.py \
Line 199: 	fakeVirtV2V.py \
>  I'd use something like fake-virt-v2v, to closely mimic the
> original virt-v2 
+1
Line 200: 	monkeypatch.py \
Line 201: 	nettestlib.py \
Line 202: 	storagefakelib.py \
Line 203: 	storagetestlib.py \


https://gerrit.ovirt.org/#/c/47738/1/tests/fakeVirtV2V.py
File tests/fakeVirtV2V.py:

Line 3: import sys
Line 4: 
Line 5: if len(sys.argv) != 2:
Line 6:     print ('usage: %s <target dir>\n' % sys.argv[0])
Line 7:     sys.exit(1)
>  is this the same option/format the real virt-v2v expects? 
This
should use the same option v2v uses. I don't think there is a need to implement all of
them, but we can use these that make sense.
Line 8: 
Line 9: 
Line 10: target_dir = sys.argv[1]
Line 11: 


Line 11: 
Line 12: 
Line 13: def write_progress():
Line 14:     for i in range(101):
Line 15:         print("    (%s/100%%)\r" % str(i))
This will not work without flushing.

Instaed of print, we should use:

    def write_output(msg):
        sys.stdout.write(msg)
        sys.stdout.flush()

Otherwise, vdsm may get all the output at once when this tool exit, unlike the original
tool.
Line 16: 
Line 17: 
Line 18: print('[   0.0] Opening the source -i libvirt\n')
Line 19: print('[   1.0] Creating an overlay to protect\n')


Line 16: 
Line 17: 
Line 18: print('[   0.0] Opening the source -i libvirt\n')
Line 19: print('[   1.0] Creating an overlay to protect\n')
Line 20: print('[  88.0] Copying disk 1/2 to
%s/4d8daea0-bf69-4984-9775-c0e826325669'
88.0 may be good for the first disk...
Line 21:       '/images/1c1e7572-a23a-4b19-89b9-09564b5c575e\n' % target_dir)
Line 22: write_progress()
Line 23: print('[  88.0] Copying disk 2/2 to
%s/4d8daea0-bf69-4984-9775-c0e826325669'
Line 24:       '/images/1c1e7572-a23a-4b19-89b9-09564b5c575e\n' % target_dir)


Line 19: print('[   1.0] Creating an overlay to protect\n')
Line 20: print('[  88.0] Copying disk 1/2 to
%s/4d8daea0-bf69-4984-9775-c0e826325669'
Line 21:       '/images/1c1e7572-a23a-4b19-89b9-09564b5c575e\n' % target_dir)
Line 22: write_progress()
Line 23: print('[  88.0] Copying disk 2/2 to
%s/4d8daea0-bf69-4984-9775-c0e826325669'
But probbaly not for the seconds disk, right?
Line 24:       '/images/1c1e7572-a23a-4b19-89b9-09564b5c575e\n' % target_dir)
Line 25: write_progress()
Line 26: print('[ 256.0] Creating output metadata')


https://gerrit.ovirt.org/#/c/47738/1/tests/v2vTests.py
File tests/v2vTests.py:

Line 243:         return 0
Line 244: 
Line 245:     def create_command(self):
Line 246:         return [os.path.join(os.path.abspath('.'),
'fakeVirtV2V.py'),
Line 247:                 '/tmp/images']
>  we should just override the path of the command to spawn. 
+1
Line 248: 
Line 249:     def execution_environments(self):
Line 250:         return {}
Line 251: 


Line 246:         return [os.path.join(os.path.abspath('.'),
'fakeVirtV2V.py'),
Line 247:                 '/tmp/images']
Line 248: 
Line 249:     def execution_environments(self):
Line 250:         return {}
Any reason to avoid the environment changes?
Line 251: 
Line 252:     @MonkeyPatch(v2v.ImportVm, '_prepare_volumes', prepare_volumes)
Line 253:     @MonkeyPatch(v2v.ImportVm, '_from_libvirt_command', create_command)
Line 254:     @MonkeyPatch(v2v.ImportVm, '_execution_environments',


Line 248: 
Line 249:     def execution_environments(self):
Line 250:         return {}
Line 251: 
Line 252:     @MonkeyPatch(v2v.ImportVm, '_prepare_volumes', prepare_volumes)
- You can use lambda: 0 instead of using a real function.
- You should probably monkeypatch also tearing down the volumes.

Or, nice change, use mock irs that record calls and does nothing, so we can verify that we
prepared and teardown the volumes.
Line 253:     @MonkeyPatch(v2v.ImportVm, '_from_libvirt_command', create_command)
Line 254:     @MonkeyPatch(v2v.ImportVm, '_execution_environments',
Line 255:                  execution_environments)
Line 256:     def testVirtV2V(self):


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 1:

(6 comments)

https://gerrit.ovirt.org/#/c/47738/1/tests/Makefile.am
File tests/Makefile.am:

Line 195: dist_vdsmtests_PYTHON = \
Line 196: 	$(test_modules) \
Line 197: 	apiData.py \
Line 198: 	hookValidation.py \
Line 199: 	fakeVirtV2V.py \
>  +1 
Done
Line 200: 	monkeypatch.py \
Line 201: 	nettestlib.py \
Line 202: 	storagefakelib.py \
Line 203: 	storagetestlib.py \


https://gerrit.ovirt.org/#/c/47738/1/tests/fakeVirtV2V.py
File tests/fakeVirtV2V.py:

Line 3: import sys
Line 4: 
Line 5: if len(sys.argv) != 2:
Line 6:     print ('usage: %s <target dir>\n' % sys.argv[0])
Line 7:     sys.exit(1)
>  This should use the same option v2v uses. I don't think there is
> a need to  
The options are vary
The reason that I added only the target dir is for the log. (which is not must...)
Line 8: 
Line 9: 
Line 10: target_dir = sys.argv[1]
Line 11: 


Line 11: 
Line 12: 
Line 13: def write_progress():
Line 14:     for i in range(101):
Line 15:         print("    (%s/100%%)\r" % str(i))
>  This will not work without flushing. 
Done
Line 16: 
Line 17: 
Line 18: print('[   0.0] Opening the source -i libvirt\n')
Line 19: print('[   1.0] Creating an overlay to protect\n')


Line 19: print('[   1.0] Creating an overlay to protect\n')
Line 20: print('[  88.0] Copying disk 1/2 to
%s/4d8daea0-bf69-4984-9775-c0e826325669'
Line 21:       '/images/1c1e7572-a23a-4b19-89b9-09564b5c575e\n' % target_dir)
Line 22: write_progress()
Line 23: print('[  88.0] Copying disk 2/2 to
%s/4d8daea0-bf69-4984-9775-c0e826325669'
>  But probbaly not for the seconds disk, right? 
yes... we do
ignore that but I will fix it.
This input is minimal, I will change it once I have a vmware environment and I will be
able to capture the full log.
Line 24:       '/images/1c1e7572-a23a-4b19-89b9-09564b5c575e\n' % target_dir)
Line 25: write_progress()
Line 26: print('[ 256.0] Creating output metadata')


https://gerrit.ovirt.org/#/c/47738/1/tests/v2vTests.py
File tests/v2vTests.py:

Line 246:         return [os.path.join(os.path.abspath('.'),
'fakeVirtV2V.py'),
Line 247:                 '/tmp/images']
Line 248: 
Line 249:     def execution_environments(self):
Line 250:         return {}
>  Any reason to avoid the environment changes? 
no...
I didn't have the _vminfo but I will fix it.
Line 251: 
Line 252:     @MonkeyPatch(v2v.ImportVm, '_prepare_volumes', prepare_volumes)
Line 253:     @MonkeyPatch(v2v.ImportVm, '_from_libvirt_command', create_command)
Line 254:     @MonkeyPatch(v2v.ImportVm, '_execution_environments',


Line 248: 
Line 249:     def execution_environments(self):
Line 250:         return {}
Line 251: 
Line 252:     @MonkeyPatch(v2v.ImportVm, '_prepare_volumes', prepare_volumes)
>  - You can use lambda: 0 instead of using a real function. 
Done
Line 253:     @MonkeyPatch(v2v.ImportVm, '_from_libvirt_command', create_command)
Line 254:     @MonkeyPatch(v2v.ImportVm, '_execution_environments',
Line 255:                  execution_environments)
Line 256:     def testVirtV2V(self):


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/47738/2/tests/fake-virt-v2v.py
File tests/fake-virt-v2v.py:

Line 26: parser.add_option('--os', dest='outputStorage',
Line 27:                   help='Output directory for the images')
Line 28: parser.add_option('--machine-readable', dest='machineReadable',
Line 29:                   help='Set the terminal output to be readable')
Line 30: 
the --ic should be -ic but optparse insist that its suppose to be double hyphen if its
more then one character...
any ideas to workaround that?
Line 31: (opt, args) = parser.parse_args()
Line 32: 
Line 33: 
Line 34: def write_output(msg):


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 2:

(6 comments)

https://gerrit.ovirt.org/#/c/47738/2/tests/fake-virt-v2v.py
File tests/fake-virt-v2v.py:

Line 26: parser.add_option('--os', dest='outputStorage',
Line 27:                   help='Output directory for the images')
Line 28: parser.add_option('--machine-readable', dest='machineReadable',
Line 29:                   help='Set the terminal output to be readable')
Line 30: 
>  the --ic should be -ic but optparse insist that its suppose to be
> double hy 
I don't think optparse support this non-standard argument style. Maybe
argparse or getopt.

If nothing support this, you can parse the options you need manually.

Another (crazy) way, write this tool in ocaml using the same option parsing code used by
virt-v2v.
Line 31: (opt, args) = parser.parse_args()
Line 32: 
Line 33: 
Line 34: def write_output(msg):


Line 27:                   help='Output directory for the images')
Line 28: parser.add_option('--machine-readable', dest='machineReadable',
Line 29:                   help='Set the terminal output to be readable')
Line 30: 
Line 31: (opt, args) = parser.parse_args()
() not needed:

> >> x = 1, 2
> >> type(x) 
 <type 'tuple'>
Line 32: 
Line 33: 
Line 34: def write_output(msg):
Line 35:     sys.stdout.write(msg)


Line 39: def write_progress():
Line 40:     for i in range(101):
Line 41:         write_output('    (%s/100%%)\r' % str(i))
Line 42: 
Line 43: progress = 0
This is not progress but elapsed time.
Line 44: write_output('[   %d.0] Opening the source -i libvirt\n' % progress)
Line 45: progress = progress + 1
Line 46: write_output('[   %d.0] Creating an overlay to protect\n' % progress)
Line 47: progress = progress + 1


Line 49:     write_output('[  %d.0] Copying disk %d/2 to %s/%s/images/%s\n' %
Line 50:                  (progress, i+1, opt.outputStorage, opt.vdsmVmId, o))
Line 51:     write_progress()
Line 52:     progress = progress + 100
Line 53: write_output('[ %d.0] Creating output metadata' % progress)
missing \n


Line 50:                  (progress, i+1, opt.outputStorage, opt.vdsmVmId, o))
Line 51:     write_progress()
Line 52:     progress = progress + 100
Line 53: write_output('[ %d.0] Creating output metadata' % progress)
Line 54: write_output('[ %d.0] Finishing off' % progress)
Same


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

Line 269:                '/tmp/v2v_output',
Line 270:                '--os',
'/tmp/v2v_output/00000000-0000-0000-0000-000000000005'
Line 271:                '--machine-readable',
Line 272:                'test_vm']
Line 273:         return cmd
There is no point in replacing create_command with different style of options - this does
not test v2v code.

For now, lets monkeypatch the path to virt-v2v, and let the original code call the fake
one with the original options.
Line 274: 
Line 275:     @MonkeyPatch(v2v.ImportVm, '_prepare_volumes', prepare_volumes)
Line 276:     @MonkeyPatch(v2v.ImportVm, '_teardown_volumes', teardown_volumes)
Line 277:     @MonkeyPatch(v2v.ImportVm, '_from_libvirt_command', create_command)


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/47738/2/tests/fake-virt-v2v.py
File tests/fake-virt-v2v.py:

Line 26: parser.add_option('--os', dest='outputStorage',
Line 27:                   help='Output directory for the images')
Line 28: parser.add_option('--machine-readable', dest='machineReadable',
Line 29:                   help='Set the terminal output to be readable')
Line 30: 
>  I don't think optparse support this non-standard argument style.
> Maybe argp 
Turns out that argparse supports this style:
> >> import argparse
> >> p = argparse.ArgumentParser()
> >> p.add_argument("-ic") 
_StoreAction(option_strings=['-ic'], dest='ic', nargs=None, const=None, 
default=None, type=None, choices=None, help=None, metavar=None)
> >> p.parse_args(["-ic", "value"]) 
Namespace(ic='value')
Line 31: (opt, args) = parser.parse_args()
Line 32: 
Line 33: 
Line 34: def write_output(msg):


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 2:

(5 comments)

https://gerrit.ovirt.org/#/c/47738/2/tests/fake-virt-v2v.py
File tests/fake-virt-v2v.py:

Line 26: parser.add_option('--os', dest='outputStorage',
Line 27:                   help='Output directory for the images')
Line 28: parser.add_option('--machine-readable', dest='machineReadable',
Line 29:                   help='Set the terminal output to be readable')
Line 30: 
>  I don't think optparse support this non-standard argument style.
> Maybe argp 
oCaml will be hard to maintain and will add another layer (oCaml compile)
to the build.
getopt is not taking short option more then one character.
we can either stay with the long options or we can parse the argv...
Line 31: (opt, args) = parser.parse_args()
Line 32: 
Line 33: 
Line 34: def write_output(msg):


Line 27:                   help='Output directory for the images')
Line 28: parser.add_option('--machine-readable', dest='machineReadable',
Line 29:                   help='Set the terminal output to be readable')
Line 30: 
Line 31: (opt, args) = parser.parse_args()
>  () not needed: 
Done
Line 32: 
Line 33: 
Line 34: def write_output(msg):
Line 35:     sys.stdout.write(msg)


Line 39: def write_progress():
Line 40:     for i in range(101):
Line 41:         write_output('    (%s/100%%)\r' % str(i))
Line 42: 
Line 43: progress = 0
>  This is not progress but elapsed time. 
Done
Line 44: write_output('[   %d.0] Opening the source -i libvirt\n' % progress)
Line 45: progress = progress + 1
Line 46: write_output('[   %d.0] Creating an overlay to protect\n' % progress)
Line 47: progress = progress + 1


Line 49:     write_output('[  %d.0] Copying disk %d/2 to %s/%s/images/%s\n' %
Line 50:                  (progress, i+1, opt.outputStorage, opt.vdsmVmId, o))
Line 51:     write_progress()
Line 52:     progress = progress + 100
Line 53: write_output('[ %d.0] Creating output metadata' % progress)
>  missing \n 
Done


Line 50:                  (progress, i+1, opt.outputStorage, opt.vdsmVmId, o))
Line 51:     write_progress()
Line 52:     progress = progress + 100
Line 53: write_output('[ %d.0] Creating output metadata' % progress)
Line 54: write_output('[ %d.0] Finishing off' % progress)
>  Same 
Done


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/47738/2/tests/fake-virt-v2v.py
File tests/fake-virt-v2v.py:

Line 26: parser.add_option('--os', dest='outputStorage',
Line 27:                   help='Output directory for the images')
Line 28: parser.add_option('--machine-readable', dest='machineReadable',
Line 29:                   help='Set the terminal output to be readable')
Line 30: 
>  oCaml will be hard to maintain and will add another layer (oCaml
> compile) t 
Check my comment about argparse
Line 31: (opt, args) = parser.parse_args()
Line 32: 
Line 33: 
Line 34: def write_output(msg):


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/47738/2/tests/fake-virt-v2v.py
File tests/fake-virt-v2v.py:

Line 26: parser.add_option('--os', dest='outputStorage',
Line 27:                   help='Output directory for the images')
Line 28: parser.add_option('--machine-readable', dest='machineReadable',
Line 29:                   help='Set the terminal output to be readable')
Line 30: 
>  Check my comment about argparse 
I missed that one...
Line 31: (opt, args) = parser.parse_args()
Line 32: 
Line 33: 
Line 34: def write_output(msg):


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 3: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Francesco Romani has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 3: Code-Review+1

(2 comments)

thanks for the updates. At first glance looks ok, deeper review later.

https://gerrit.ovirt.org/#/c/47738/3/tests/v2vTests.py
File tests/v2vTests.py:

Line 246: 
Line 247:     @recorded
Line 248:     def prepare_volumes(self):
Line 249:         for disk in self._vminfo['disks']:
Line 250:             drive = {'volumeID': disk['volumeID'],
minor nit: in new code, please let's use this formatting:

  a_dict = {
     key1: value1,
     ...
     keyN: valueN
  }

this is 100% equivalent to the one you are using, but it is easier to change in the future
(smaller patches).
Here and in the other places.
Line 251:                      'imageID': disk['imageID'],
Line 252:                      'path':
'/rhev/data-center/mydomain/images'}
Line 253:             self._prepared_volumes.append(drive)
Line 254: 


Line 259:     @MonkeyPatch(v2v.ImportVm, '_prepare_volumes', prepare_volumes)
Line 260:     @MonkeyPatch(v2v.ImportVm, '_teardown_volumes', teardown_volumes)
Line 261:     @MonkeyPatch(v2v, '_VIRT_V2V', FakeV2VCommand())
Line 262:     def testVirtV2V(self):
Line 263:         self._id = '00000000-0000-0000-0000-000000000005'
why is this needed? could be a local variable of this method, perhaps?
Line 264:         vminfo = {'vmName': 'TEST',
Line 265:                   'disks': [{'imageID':
'00000000-0000-0000-0000-000000000001',
Line 266:                              'volumeID':
Line 267:                              '00000000-0000-0000-0000-000000000002'},


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 3:

(8 comments)

Nice, few minor things can be improved.

https://gerrit.ovirt.org/#/c/47738/3/tests/fake-virt-v2v.py
File tests/fake-virt-v2v.py:

Line 1: #!/usr/bin/env python
We need the standard vdsm license here.
Line 2: 
Line 3: import sys
Line 4: import argparse
Line 5: 


Line 30:                     action='store_true',
Line 31:                     help='Set the terminal output to be readable')
Line 32: parser.add_argument('vmname')
Line 33: 
Line 34: opt = parser.parse_args(sys.argv)
This should be options or arguments (plural, nicer to read).
Line 35: print(opt)
Line 36: 
Line 37: 
Line 38: def write_output(msg):


Line 31:                     help='Set the terminal output to be readable')
Line 32: parser.add_argument('vmname')
Line 33: 
Line 34: opt = parser.parse_args(sys.argv)
Line 35: print(opt)
Is this original code behavior? I would try to mimic the original code here.
Line 36: 
Line 37: 
Line 38: def write_output(msg):
Line 39:     sys.stdout.write(msg)


Line 40:     sys.stdout.flush()
Line 41: 
Line 42: 
Line 43: def write_progress():
Line 44:     for i in range(101):
This create numbers from 0 to 100 - is this what you want, or maybe you like 1 to 100?
(range(1, 101))
Line 45:         write_output('    (%s/100%%)\r' % str(i))
Line 46: 
Line 47: elp_time = 0
Line 48: write_output('[   %d.0] Opening the source -i libvirt\n' % elp_time)


Line 41: 
Line 42: 
Line 43: def write_progress():
Line 44:     for i in range(101):
Line 45:         write_output('    (%s/100%%)\r' % str(i))
Update the time here:

    global elapsed_time
    for ...:
        write_ouput(...)
        elapsed_time += 1
Line 46: 
Line 47: elp_time = 0
Line 48: write_output('[   %d.0] Opening the source -i libvirt\n' % elp_time)
Line 49: elp_time = elp_time + 1


Line 43: def write_progress():
Line 44:     for i in range(101):
Line 45:         write_output('    (%s/100%%)\r' % str(i))
Line 46: 
Line 47: elp_time = 0
Whats wrong with elapsed_time?
Line 48: write_output('[   %d.0] Opening the source -i libvirt\n' % elp_time)
Line 49: elp_time = elp_time + 1
Line 50: write_output('[   %d.0] Creating an overlay to protect\n' % elp_time)
Line 51: elp_time = elp_time + 1


Line 52: for i, o in enumerate(opt.vdsmImageId):
Line 53:     write_output('[  %d.0] Copying disk %d/2 to %s/%s/images/%s\n' %
Line 54:                  (elp_time, i+1, opt.outputStorage, opt.vdsmVmId, o))
Line 55:     write_progress()
Line 56:     elp_time = elp_time + 100
Instead of updating manually here, update in write_progress, so you don't have
dependency on how much time write_progress should take.
Line 57:     write_output('[ %d.0] Creating output metadata\n' % elp_time)


https://gerrit.ovirt.org/#/c/47738/3/tests/v2vTests.py
File tests/v2vTests.py:

Line 246: 
Line 247:     @recorded
Line 248:     def prepare_volumes(self):
Line 249:         for disk in self._vminfo['disks']:
Line 250:             drive = {'volumeID': disk['volumeID'],
>  minor nit: in new code, please let's use this formatting:
Unless you the dict is only 2-3 items and fit in one line.

When you use the format suggested by Francesco, do sort the keys, to make it easier to
find.
Line 251:                      'imageID': disk['imageID'],
Line 252:                      'path':
'/rhev/data-center/mydomain/images'}
Line 253:             self._prepared_volumes.append(drive)
Line 254: 


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 3:

(10 comments)

https://gerrit.ovirt.org/#/c/47738/3/tests/fake-virt-v2v.py
File tests/fake-virt-v2v.py:

Line 1: #!/usr/bin/env python
>  We need the standard vdsm license here. 
Done
Line 2: 
Line 3: import sys
Line 4: import argparse
Line 5: 


Line 30:                     action='store_true',
Line 31:                     help='Set the terminal output to be readable')
Line 32: parser.add_argument('vmname')
Line 33: 
Line 34: opt = parser.parse_args(sys.argv)
>  This should be options or arguments (plural, nicer to read).
Done
Line 35: print(opt)
Line 36: 
Line 37: 
Line 38: def write_output(msg):


Line 31:                     help='Set the terminal output to be readable')
Line 32: parser.add_argument('vmname')
Line 33: 
Line 34: opt = parser.parse_args(sys.argv)
Line 35: print(opt)
>  Is this original code behavior? I would try to mimic the original
> code here 
It was a debugging issue...
Line 36: 
Line 37: 
Line 38: def write_output(msg):
Line 39:     sys.stdout.write(msg)


Line 40:     sys.stdout.flush()
Line 41: 
Line 42: 
Line 43: def write_progress():
Line 44:     for i in range(101):
>  This create numbers from 0 to 100 - is this what you want, or maybe
> you lik 
yes 0-100
Line 45:         write_output('    (%s/100%%)\r' % str(i))
Line 46: 
Line 47: elp_time = 0
Line 48: write_output('[   %d.0] Opening the source -i libvirt\n' % elp_time)


Line 41: 
Line 42: 
Line 43: def write_progress():
Line 44:     for i in range(101):
Line 45:         write_output('    (%s/100%%)\r' % str(i))
>  Update the time here: 
Done
Line 46: 
Line 47: elp_time = 0
Line 48: write_output('[   %d.0] Opening the source -i libvirt\n' % elp_time)
Line 49: elp_time = elp_time + 1


Line 43: def write_progress():
Line 44:     for i in range(101):
Line 45:         write_output('    (%s/100%%)\r' % str(i))
Line 46: 
Line 47: elp_time = 0
>  Whats wrong with elapsed_time? 
Done
Line 48: write_output('[   %d.0] Opening the source -i libvirt\n' % elp_time)
Line 49: elp_time = elp_time + 1
Line 50: write_output('[   %d.0] Creating an overlay to protect\n' % elp_time)
Line 51: elp_time = elp_time + 1


Line 52: for i, o in enumerate(opt.vdsmImageId):
Line 53:     write_output('[  %d.0] Copying disk %d/2 to %s/%s/images/%s\n' %
Line 54:                  (elp_time, i+1, opt.outputStorage, opt.vdsmVmId, o))
Line 55:     write_progress()
Line 56:     elp_time = elp_time + 100
>  Instead of updating manually here, update in write_progress, so you
> don't h 
Done
Line 57:     write_output('[ %d.0] Creating output metadata\n' % elp_time)


https://gerrit.ovirt.org/#/c/47738/3/tests/v2vTests.py
File tests/v2vTests.py:

Line 246: 
Line 247:     @recorded
Line 248:     def prepare_volumes(self):
Line 249:         for disk in self._vminfo['disks']:
Line 250:             drive = {'volumeID': disk['volumeID'],
>  minor nit: in new code, please let's use this formatting:
Done
Line 251:                      'imageID': disk['imageID'],
Line 252:                      'path':
'/rhev/data-center/mydomain/images'}
Line 253:             self._prepared_volumes.append(drive)
Line 254: 


Line 246: 
Line 247:     @recorded
Line 248:     def prepare_volumes(self):
Line 249:         for disk in self._vminfo['disks']:
Line 250:             drive = {'volumeID': disk['volumeID'],
>  Unless you the dict is only 2-3 items and fit in one line.
Done
Line 251:                      'imageID': disk['imageID'],
Line 252:                      'path':
'/rhev/data-center/mydomain/images'}
Line 253:             self._prepared_volumes.append(drive)
Line 254: 


Line 259:     @MonkeyPatch(v2v.ImportVm, '_prepare_volumes', prepare_volumes)
Line 260:     @MonkeyPatch(v2v.ImportVm, '_teardown_volumes', teardown_volumes)
Line 261:     @MonkeyPatch(v2v, '_VIRT_V2V', FakeV2VCommand())
Line 262:     def testVirtV2V(self):
Line 263:         self._id = '00000000-0000-0000-0000-000000000005'
>  why is this needed? could be a local variable of this method,
> perhaps? 
This is the job id that v2v use.
I will set it as local variable
Line 264:         vminfo = {'vmName': 'TEST',
Line 265:                   'disks': [{'imageID':
'00000000-0000-0000-0000-000000000001',
Line 266:                              'volumeID':
Line 267:                              '00000000-0000-0000-0000-000000000002'},


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 4:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 4:

(4 comments)

The fake-virt-v2v looks fine now, I don't understand the test.

https://gerrit.ovirt.org/#/c/47738/4/tests/fake-virt-v2v.py
File tests/fake-virt-v2v.py:

Line 48:                     action='store_true',
Line 49:                     help='Set the terminal output to be readable')
Line 50: parser.add_argument('vmname')
Line 51: 
Line 52: elapsed_time = 0
I would define it after parsing arguments.
Line 53: options = parser.parse_args(sys.argv)
Line 54: 
Line 55: 
Line 56: def write_output(msg):


Line 66: 
Line 67: write_output('[   %d.0] Opening the source -i libvirt\n' % elapsed_time)
Line 68: elapsed_time = elapsed_time + 1
Line 69: write_output('[   %d.0] Creating an overlay to protect\n' %
elapsed_time)
Line 70: elapsed_time = elapsed_time + 1
An empty line separating the first part from the loop would be nice.
Line 71: for i, o in enumerate(options.vdsmImageId):
Line 72:     write_output('[  %d.0] Copying disk %d/2 to %s/%s/images/%s\n' %
Line 73:                  (elapsed_time, i+1, options.outputStorage,
Line 74:                   options.vdsmVmId, o))


https://gerrit.ovirt.org/#/c/47738/4/tests/v2vTests.py
File tests/v2vTests.py:

Line 36: import vmfakelib as fake
Line 37: 
Line 38: 
Line 39: VmSpec = namedtuple('VmSpec', ['name', 'vmid'])
Line 40: _job_id = '00000000-0000-0000-0000-000000000005'
Is this constant? should be in uppercase.
Line 41: 
Line 42: 
Line 43: def _mac_from_uuid(vmid):
Line 44:     return "52:54:%s:%s:%s:%s" % (


Line 315:                              '00000000-0000-0000-0000-000000000004'}]}
Line 316:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 317:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 318:                                         self._id, None)
Line 319:         ivm._import()
What are you trying to test here? What kind of failures are possible?


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/47738/4/tests/v2vTests.py
File tests/v2vTests.py:

Line 315:                              '00000000-0000-0000-0000-000000000004'}]}
Line 316:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 317:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 318:                                         self._id, None)
Line 319:         ivm._import()
>  What are you trying to test here? What kind of failures are possible?
I would add a trivial test that run fake-virt-v2v tool, collect its output, and
check that it writes the expected output.


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 4:

(5 comments)

https://gerrit.ovirt.org/#/c/47738/4/tests/fake-virt-v2v.py
File tests/fake-virt-v2v.py:

Line 48:                     action='store_true',
Line 49:                     help='Set the terminal output to be readable')
Line 50: parser.add_argument('vmname')
Line 51: 
Line 52: elapsed_time = 0
>  I would define it after parsing arguments. 
Done
Line 53: options = parser.parse_args(sys.argv)
Line 54: 
Line 55: 
Line 56: def write_output(msg):


Line 66: 
Line 67: write_output('[   %d.0] Opening the source -i libvirt\n' % elapsed_time)
Line 68: elapsed_time = elapsed_time + 1
Line 69: write_output('[   %d.0] Creating an overlay to protect\n' %
elapsed_time)
Line 70: elapsed_time = elapsed_time + 1
>  An empty line separating the first part from the loop would be nice.
Done
Line 71: for i, o in enumerate(options.vdsmImageId):
Line 72:     write_output('[  %d.0] Copying disk %d/2 to %s/%s/images/%s\n' %
Line 73:                  (elapsed_time, i+1, options.outputStorage,
Line 74:                   options.vdsmVmId, o))


https://gerrit.ovirt.org/#/c/47738/4/tests/v2vTests.py
File tests/v2vTests.py:

Line 36: import vmfakelib as fake
Line 37: 
Line 38: 
Line 39: VmSpec = namedtuple('VmSpec', ['name', 'vmid'])
Line 40: _job_id = '00000000-0000-0000-0000-000000000005'
>  Is this constant? should be in uppercase. 
Done
Line 41: 
Line 42: 
Line 43: def _mac_from_uuid(vmid):
Line 44:     return "52:54:%s:%s:%s:%s" % (


Line 315:                              '00000000-0000-0000-0000-000000000004'}]}
Line 316:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 317:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 318:                                         self._id, None)
Line 319:         ivm._import()
>  What are you trying to test here? What kind of failures are possible?
As long as the v2v code is working this code will pass without exception.
I would add an assert for the status.


Line 315:                              '00000000-0000-0000-0000-000000000004'}]}
Line 316:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 317:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 318:                                         self._id, None)
Line 319:         ivm._import()
>  I would add a trivial test that run fake-virt-v2v tool, collect its
> output, 
is that a test only for fake-virt-v2v?


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 4: Code-Review-1

(5 comments)

https://gerrit.ovirt.org/#/c/47738/4/tests/v2vTests.py
File tests/v2vTests.py:

Line 118: 
Line 119: 
Line 120: class FakeV2VCommand(object):
Line 121:     def __init__(self):
Line 122:         self.cmd = os.path.join(os.path.abspath('.'),
'fake-virt-v2v.py')
Use CommandPath class for this, specifying the path to fake-virt-v2v.py
Line 123: 
Line 124: 
Line 125: def hypervisorConnect(uri, username, passwd):
Line 126:     return LibvirtMock()


Line 293:                 'imageID': disk['imageID'],
Line 294:                 'path': '/rhev/data-center/mydomain/images',
Line 295:                 'volumeID': disk['volumeID']
Line 296:             }
Line 297:             self._prepared_volumes.append(drive)
You are duplicating ImportVm code here, touching internal implementation details. This
make this test fragile and will make it hard to modify the code under test.

Better use fake irs object that does nothing in prepareImage and teardownImage:

    class FakeIRS(object):

        @recorded
        def prepareImage(self, domainID, poolID, imageID, volumeID):
            pass

        @recorded
        def teardownImage(self, domainID, poolID, imageID):
            pass

Now we can add 2 tests that for preparing and tearing down images during the import.
Line 298: 
Line 299:     @recorded
Line 300:     def teardown_volumes(self):
Line 301:         return 0


Line 314:                              'volumeID':
Line 315:                              '00000000-0000-0000-0000-000000000004'}]}
Line 316:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 317:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 318:                                         self._id, None)
Pass FakeIRS instance here instead of None.


Line 315:                              '00000000-0000-0000-0000-000000000004'}]}
Line 316:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 317:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 318:                                         self._id, None)
Line 319:         ivm._import()
>  is that a test only for fake-virt-v2v? 
Right, to be sure it is
producing the expected output.

When we know it is good, we can use it to test v2v code.


Line 315:                              '00000000-0000-0000-0000-000000000004'}]}
Line 316:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 317:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 318:                                         self._id, None)
Line 319:         ivm._import()
>  As long as the v2v code is working this code will pass without
> exception. 
OK, run import, check that the import was successful.  Would you name
this testSuccessfulImport?


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 4:

(5 comments)

https://gerrit.ovirt.org/#/c/47738/4/tests/v2vTests.py
File tests/v2vTests.py:

Line 118: 
Line 119: 
Line 120: class FakeV2VCommand(object):
Line 121:     def __init__(self):
Line 122:         self.cmd = os.path.join(os.path.abspath('.'),
'fake-virt-v2v.py')
>  Use CommandPath class for this, specifying the path to
> fake-virt-v2v.py 
Done
Line 123: 
Line 124: 
Line 125: def hypervisorConnect(uri, username, passwd):
Line 126:     return LibvirtMock()


Line 293:                 'imageID': disk['imageID'],
Line 294:                 'path': '/rhev/data-center/mydomain/images',
Line 295:                 'volumeID': disk['volumeID']
Line 296:             }
Line 297:             self._prepared_volumes.append(drive)
>  You are duplicating ImportVm code here, touching internal
> implementation de 
The reason that I override prepare_volumes is for the
drive['path'] which prepare_volumes is setting.
I will change it by adding a FakeIrs class that fakes prepare_volumes and return the
path.
Line 298: 
Line 299:     @recorded
Line 300:     def teardown_volumes(self):
Line 301:         return 0


Line 314:                              'volumeID':
Line 315:                              '00000000-0000-0000-0000-000000000004'}]}
Line 316:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 317:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 318:                                         self._id, None)
>  Pass FakeIRS instance here instead of None. 
Done


Line 315:                              '00000000-0000-0000-0000-000000000004'}]}
Line 316:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 317:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 318:                                         self._id, None)
Line 319:         ivm._import()
>  OK, run import, check that the import was successful.  Would you name
> this  
Done


Line 315:                              '00000000-0000-0000-0000-000000000004'}]}
Line 316:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 317:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 318:                                         self._id, None)
Line 319:         ivm._import()
>  Right, to be sure it is producing the expected output. 
given
the same input that we send in this test we are getting the following output, do you mean
to compare to that long output?


['[   0.0] Opening the source -i libvirt', '[   1.0] Creating an overlay to
protect', '[  2.0] Copying disk 1/2 to
/rhev/data-center/00000000-0000-0000-0000-000000000005/images/00000000-0000-0000-0000-    
       
000000000001', '    (0/100%)', '    (1/100%)', '    (2/100%)',
'    (3/100%)', '    (4/100%)', '    (5/100%)', '   
(6/100%)', '    (7/100%)', '    (8/100%)', '    (9/100%)',
'    (10/100%)', '    (11/100%)',     
'    (12/100%)', '    (13/100%)', '    (14/100%)', '   
(15/100%)', '    (16/100%)', '    (17/100%)', '    (18/100%)',
'    (19/100%)', '    (20/100%)', '    (21/100%)', '   
(22/100%)', '    (23/100%)', '    (24/
100%)', '    (25/100%)', '    (26/100%)', '    (27/100%)',
'    (28/100%)', '    (29/100%)', '    (30/100%)', '   
(31/100%)', '    (32/100%)', '    (33/100%)', '    (34/100%)',
'    (35/100%)', '    (36/100%)',  
'    (37/100%)', '    (38/100%)', '    (39/100%)', '   
(40/100%)', '    (41/100%)', '    (42/100%)', '    (43/100%)',
'    (44/100%)', '    (45/100%)', '    (46/100%)', '   
(47/100%)', '    (48/100%)', '    (49/
100%)', '    (50/100%)', '    (51/100%)', '    (52/100%)',
'    (53/100%)', '    (54/100%)', '    (55/100%)', '   
(56/100%)', '    (57/100%)', '    (58/100%)', '    (59/100%)',
'    (60/100%)', '    (61/100%)',  
'    (62/100%)', '    (63/100%)', '    (64/100%)', '   
(65/100%)', '    (66/100%)', '    (67/100%)', '    (68/100%)',
'    (69/100%)', '    (70/100%)', '    (71/100%)', '   
(72/100%)', '    (73/100%)', '    (74/
100%)', '    (75/100%)', '    (76/100%)', '    (77/100%)',
'    (78/100%)', '    (79/100%)', '    (80/100%)', '   
(81/100%)', '    (82/100%)', '    (83/100%)', '    (84/100%)',
'    (85/100%)', '    (86/100%)',  
'    (87/100%)', '    (88/100%)', '    (89/100%)', '   
(90/100%)', '    (91/100%)', '    (92/100%)', '    (93/100%)',
'    (94/100%)', '    (95/100%)', '    (96/100%)', '   
(97/100%)', '    (98/100%)', '    (99/
100%)', '    (100/100%)', '[ 103.0] Creating output metadata', '[
103.0] Finishing off', '[  103.0] Copying disk 2/2 to
/rhev/data-center/00000000-0000-0000-0000-000000000005/images/00000000-0000-0000-0000-    
  
000000000003', '    (0/100%)', '    (1/100%)', '    (2/100%)',
'    (3/100%)', '    (4/100%)', '    (5/100%)', '   
(6/100%)', '    (7/100%)', '    (8/100%)', '    (9/100%)',
'    (10/100%)', '    (11/100%)',     
'    (12/100%)', '    (13/100%)', '    (14/100%)', '   
(15/100%)', '    (16/100%)', '    (17/100%)', '    (18/100%)',
'    (19/100%)', '    (20/100%)', '    (21/100%)', '   
(22/100%)', '    (23/100%)', '    (24/
100%)', '    (25/100%)', '    (26/100%)', '    (27/100%)',
'    (28/100%)', '    (29/100%)', '    (30/100%)', '   
(31/100%)', '    (32/100%)', '    (33/100%)', '    (34/100%)',
'    (35/100%)', '    (36/100%)',  
'    (37/100%)', '    (38/100%)', '    (39/100%)', '   
(40/100%)', '    (41/100%)', '    (42/100%)', '    (43/100%)',
'    (44/100%)', '    (45/100%)', '    (46/100%)', '   
(47/100%)', '    (48/100%)', '    (49/
100%)', '    (50/100%)', '    (51/100%)', '    (52/100%)',
'    (53/100%)', '    (54/100%)', '    (55/100%)', '   
(56/100%)', '    (57/100%)', '    (58/100%)', '    (59/100%)',
'    (60/100%)', '    (61/100%)',  
'    (62/100%)', '    (63/100%)', '    (64/100%)', '   
(65/100%)', '    (66/100%)', '    (67/100%)', '    (68/100%)',
'    (69/100%)', '    (70/100%)', '    (71/100%)', '   
(72/100%)', '    (73/100%)', '    (74/
100%)', '    (75/100%)', '    (76/100%)', '    (77/100%)',
'    (78/100%)', '    (79/100%)', '    (80/100%)', '   
(81/100%)', '    (82/100%)', '    (83/100%)', '    (84/100%)',
'    (85/100%)', '    (86/100%)',  
'    (87/100%)', '    (88/100%)', '    (89/100%)', '   
(90/100%)', '    (91/100%)', '    (92/100%)', '    (93/100%)',
'    (94/100%)', '    (95/100%)', '    (96/100%)', '   
(97/100%)', '    (98/100%)', '    (99/
100%)', '    (100/100%)', '[ 204.0] Creating output metadata', '[
204.0] Finishing off']


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/47738/4/tests/v2vTests.py
File tests/v2vTests.py:

Line 293:                 'imageID': disk['imageID'],
Line 294:                 'path': '/rhev/data-center/mydomain/images',
Line 295:                 'volumeID': disk['volumeID']
Line 296:             }
Line 297:             self._prepared_volumes.append(drive)
>  The reason that I override prepare_volumes is for the
> drive['path'] which p 
Sounds like a good plan - return fake path:

   
"rhev/data-center"/<poolid>/<dominid>/images/<imageid>/<volumeid>"
Line 298: 
Line 299:     @recorded
Line 300:     def teardown_volumes(self):
Line 301:         return 0


Line 315:                              '00000000-0000-0000-0000-000000000004'}]}
Line 316:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 317:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 318:                                         self._id, None)
Line 319:         ivm._import()
>  given the same input that we send in this test we are getting the
> following 
Yes, unless you have a better idea how to test that this tools works.


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 5:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 5: Verified-1

ignore the patch set.

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

automation(a)ovirt.org has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 6:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 6:

(1 comment)

Patch set 5 suppose to be with all the fixes but it mixed in my env...
Nir I hope I got all your comments.

https://gerrit.ovirt.org/#/c/47738/6/tests/v2vTests.py
File tests/v2vTests.py:

Line 320:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 321:                                         _JOB_ID, FakeIRS())
Line 322:         ivm._import()
Line 323: 
Line 324:         # test fake-virt-v2v.py output with pre saved output
The reason that it is not a new test is because we are using the cmd that was build after
ivm.import() called.
We can add a new test for the output but it will need to duplicate this test...
Line 325:         rc, output, error = execCmd(ivm._from_libvirt_command())
Line 326:         output_path = os.path.join(os.path.abspath('.'),
Line 327:                                    'fake-virt-v2v.output')
Line 328:         with open(output_path, 'r') as f:


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 6:

(7 comments)

https://gerrit.ovirt.org/#/c/47738/6/tests/v2vTests.py
File tests/v2vTests.py:

Line 117:         def info(self):
Line 118:             return [0, 0, 0]
Line 119: 
Line 120: 
Line 121: class FakeIRS(object):
Why not @record this method, so you can test that ImaportVm did prepare the images?
Line 122:     def prepareImage(self, domainId, poolId, imageId, volumeId):
Line 123:         return {'status': {'code': 0},
Line 124:                 'path': os.path.join('/rhev/data-center',
poolId, domainId,
Line 125:                                      'images', imageId, volumeId)}


Line 129:     def __init__(self):
Line 130:         fake_v2v = CommandPath('fake-virt-v2v.py',
Line 131:                                os.path.join(os.path.abspath('.'),
Line 132:                                             'fake-virt-v2v.py'))
Line 133:         self.cmd = fake_v2v.cmd
Why not use CommandPath directly?

    FAKE_VIRT_V2V = CommandPath('fake-virt-v2v.py',
                                os.path.abspath('fake-virt-v2v.py'))

And later:

    @MonkeyPatch(v2v, '_VIRT_V2V', FAKE_VIRT_V2V)
Line 134: 
Line 135: 
Line 136: def hypervisorConnect(uri, username, passwd):
Line 137:     return LibvirtMock()


Line 298:         self.assertEquals(network['bridge'], 'VM Network')
Line 299: 
Line 300:     @recorded
Line 301:     def teardown_volumes(self):
Line 302:         return 0
Why implement this here and not in FakeIRS, where you can record it and check that 
ImportVm did teardown the volumes? This is pretty critical behavior which is not tested
today.
Line 303: 
Line 304:     @MonkeyPatch(v2v.ImportVm, '_teardown_volumes', teardown_volumes)
Line 305:     @MonkeyPatch(v2v, '_VIRT_V2V', FakeV2VCommand())
Line 306:     def testSuccessfulImport(self):


Line 320:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 321:                                         _JOB_ID, FakeIRS())
Line 322:         ivm._import()
Line 323: 
Line 324:         # test fake-virt-v2v.py output with pre saved output
>  The reason that it is not a new test is because we are using the cmd
> that w 
We don't test 2 things in one test.

Use hardcoded command for the fake-virt-v2v test, we don't want this test to fail if
you modify ImportVm.
Line 325:         rc, output, error = execCmd(ivm._from_libvirt_command())
Line 326:         output_path = os.path.join(os.path.abspath('.'),
Line 327:                                    'fake-virt-v2v.output')
Line 328:         with open(output_path, 'r') as f:


Line 321:                                         _JOB_ID, FakeIRS())
Line 322:         ivm._import()
Line 323: 
Line 324:         # test fake-virt-v2v.py output with pre saved output
Line 325:         rc, output, error = execCmd(ivm._from_libvirt_command())
Use raw=True so ouptut will be a string instead of list of lines.
Line 326:         output_path = os.path.join(os.path.abspath('.'),
Line 327:                                    'fake-virt-v2v.output')
Line 328:         with open(output_path, 'r') as f:


Line 323: 
Line 324:         # test fake-virt-v2v.py output with pre saved output
Line 325:         rc, output, error = execCmd(ivm._from_libvirt_command())
Line 326:         output_path = os.path.join(os.path.abspath('.'),
Line 327:                                    'fake-virt-v2v.output')
To get absolute path, use:

    os.path.abspath("relativepath")

Current directory is implied, the is not need to create "./relativepath"

But you don't need absolute path for reading the data file, just do:

    with ('fake-virt-v2v.output') as f:
        expected_output = f.read()
Line 328:         with open(output_path, 'r') as f:


Line 325:         rc, output, error = execCmd(ivm._from_libvirt_command())
Line 326:         output_path = os.path.join(os.path.abspath('.'),
Line 327:                                    'fake-virt-v2v.output')
Line 328:         with open(output_path, 'r') as f:
Line 329:             self.assertEqual(''.join(output).strip(), f.read().strip())
No need to strip anyting, just do:

    self.assertEqual(output, expected_output)

And add asserts for rc and err as well.


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 6:

(6 comments)

https://gerrit.ovirt.org/#/c/47738/6/tests/v2vTests.py
File tests/v2vTests.py:

Line 117:         def info(self):
Line 118:             return [0, 0, 0]
Line 119: 
Line 120: 
Line 121: class FakeIRS(object):
>  Why not @record this method, so you can test that ImaportVm did
> prepare the 
Done
Line 122:     def prepareImage(self, domainId, poolId, imageId, volumeId):
Line 123:         return {'status': {'code': 0},
Line 124:                 'path': os.path.join('/rhev/data-center',
poolId, domainId,
Line 125:                                      'images', imageId, volumeId)}


Line 129:     def __init__(self):
Line 130:         fake_v2v = CommandPath('fake-virt-v2v.py',
Line 131:                                os.path.join(os.path.abspath('.'),
Line 132:                                             'fake-virt-v2v.py'))
Line 133:         self.cmd = fake_v2v.cmd
>  Why not use CommandPath directly? 
Done
Line 134: 
Line 135: 
Line 136: def hypervisorConnect(uri, username, passwd):
Line 137:     return LibvirtMock()


Line 298:         self.assertEquals(network['bridge'], 'VM Network')
Line 299: 
Line 300:     @recorded
Line 301:     def teardown_volumes(self):
Line 302:         return 0
>  Why implement this here and not in FakeIRS, where you can record it
> and che 
Done
Line 303: 
Line 304:     @MonkeyPatch(v2v.ImportVm, '_teardown_volumes', teardown_volumes)
Line 305:     @MonkeyPatch(v2v, '_VIRT_V2V', FakeV2VCommand())
Line 306:     def testSuccessfulImport(self):


Line 321:                                         _JOB_ID, FakeIRS())
Line 322:         ivm._import()
Line 323: 
Line 324:         # test fake-virt-v2v.py output with pre saved output
Line 325:         rc, output, error = execCmd(ivm._from_libvirt_command())
>  Use raw=True so ouptut will be a string instead of list of lines.
Done
Line 326:         output_path = os.path.join(os.path.abspath('.'),
Line 327:                                    'fake-virt-v2v.output')
Line 328:         with open(output_path, 'r') as f:


Line 323: 
Line 324:         # test fake-virt-v2v.py output with pre saved output
Line 325:         rc, output, error = execCmd(ivm._from_libvirt_command())
Line 326:         output_path = os.path.join(os.path.abspath('.'),
Line 327:                                    'fake-virt-v2v.output')
>  To get absolute path, use: 
Done
Line 328:         with open(output_path, 'r') as f:


Line 325:         rc, output, error = execCmd(ivm._from_libvirt_command())
Line 326:         output_path = os.path.join(os.path.abspath('.'),
Line 327:                                    'fake-virt-v2v.output')
Line 328:         with open(output_path, 'r') as f:
Line 329:             self.assertEqual(''.join(output).strip(), f.read().strip())
>  No need to strip anyting, just do: 
yes, only when I hex edit
the file I remove the last character and now no need to strip...


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 7:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

automation(a)ovirt.org has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 8:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 7:

patch fix CI error regarding fake-virt-v2v.output,
rename fake-virt-v2v.output to fake-virt-v2v.out to match other resources pattern.

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 8: Code-Review-1

(6 comments)

https://gerrit.ovirt.org/#/c/47738/8/tests/Makefile.am
File tests/Makefile.am:

Line 197: dist_vdsmtests_PYTHON = \
Line 198: 	$(test_modules) \
Line 199: 	apiData.py \
Line 200: 	hookValidation.py \
Line 201: 	fake-virt-v2v.py \
This is really a script, not a python module, so we can drop the .py suffix
(fake-virt-v2v).
Line 202: 	monkeypatch.py \
Line 203: 	nettestlib.py \
Line 204: 	storagefakelib.py \
Line 205: 	storagetestlib.py \


https://gerrit.ovirt.org/#/c/47738/8/tests/v2vTests.py
File tests/v2vTests.py:

Line 308:                              '00000000-0000-0000-0000-000000000002'},
Line 309:                             {'imageID':
Line 310:                              '00000000-0000-0000-0000-000000000003',
Line 311:                              'volumeID':
Line 312:                              '00000000-0000-0000-0000-000000000004'}]}
Should be class or module constant, used in two tests.
Line 313:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 314:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 315:                                         _JOB_ID, FakeIRS())
Line 316:         ivm._import()


Line 309:                             {'imageID':
Line 310:                              '00000000-0000-0000-0000-000000000003',
Line 311:                              'volumeID':
Line 312:                              '00000000-0000-0000-0000-000000000004'}]}
Line 313:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Should be class or module constant, used in two tests.
Line 314:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 315:                                         _JOB_ID, FakeIRS())
Line 316:         ivm._import()
Line 317: 


Line 311:                              'volumeID':
Line 312:                              '00000000-0000-0000-0000-000000000004'}]}
Line 313:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 314:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 315:                                         _JOB_ID, FakeIRS())
This can be in a test setUp() method, same instance is used in two tests.
Line 316:         ivm._import()
Line 317: 
Line 318:     @MonkeyPatch(v2v, '_VIRT_V2V', FAKE_VIRT_V2V)
Line 319:     def testV2VOutput(self):


Line 312:                              '00000000-0000-0000-0000-000000000004'}]}
Line 313:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 314:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 315:                                         _JOB_ID, FakeIRS())
Line 316:         ivm._import()
Please verify the job status after import finished.
Line 317: 
Line 318:     @MonkeyPatch(v2v, '_VIRT_V2V', FAKE_VIRT_V2V)
Line 319:     def testV2VOutput(self):
Line 320:         '''


Line 337:                              '00000000-0000-0000-0000-000000000004'}]}
Line 338:         url =
'vpx://adminr%40vsphere@0.0.0.0/ovirt/0.0.0.0?no_verify=1'
Line 339:         ivm = v2v.ImportVm.from_libvirt(url, 'root',
'mypassword', vminfo,
Line 340:                                         _JOB_ID, FakeIRS())
Line 341:         ivm._import()
Why do we need to run _import() here?

I think we should avoid all this setup and use a hardcoded command line for
fake-virt-v2v.

This test is for fake-virt-v2v, not for ImportVm class, but it may fail because of changes
in ImportVm.
Line 342: 
Line 343:         rc, output, error = execCmd(ivm._from_libvirt_command(), raw=True)
Line 344:         self.assertEqual(rc, 0)
Line 345: 


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 9:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 9: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

gerrit-hooks has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 10:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 10:

(1 comment)

Shahahr, I rebased this on the thread-names branch, trying to verify  the thread-names
change using the new test. Unfortunately, you don't run the job normally (using
start()), so it does not help to test the thread change.

Feel free to rebase back on master.

https://gerrit.ovirt.org/#/c/47738/10/tests/v2vTests.py
File tests/v2vTests.py:

Line 328:             Do not change this parameters without modifying
Line 329:             fake-virt-v2v.output.
Line 330:         '''
Line 331:         self.ivm._import()
Line 332:         self.assertEqual(self.ivm.status, v2v.STATUS.DONE)
This part may break and cause this test to break while there is nothing wrong
with the tool. We want a simple test that test *only* the fake-virt-v2v tool.

I commented about it more then once but it seems that you did not understand what I mean.
I uploaded a draft showing how this test should be implemented:
https://gerrit.ovirt.org/47738

If you agree that this is the way to go, please squash that draft with your patch.
Line 333: 
Line 334:         rc, output, error = execCmd(self.ivm._from_libvirt_command(), raw=True)
Line 335:         self.assertEqual(rc, 0)
Line 336: 


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 10: Code-Review-1

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 10:

(1 comment)

https://gerrit.ovirt.org/#/c/47738/10/tests/v2vTests.py
File tests/v2vTests.py:

Line 325:         '''
Line 326:             We are testing the output of fake-virt-v2v with vminfo input
Line 327:             against pre-saved output;
Line 328:             Do not change this parameters without modifying
Line 329:             fake-virt-v2v.output.
The extra indentation is not needed, use:

    """
    text ...
    """
Line 330:         '''
Line 331:         self.ivm._import()
Line 332:         self.assertEqual(self.ivm.status, v2v.STATUS.DONE)
Line 333: 


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 10:

(1 comment)

https://gerrit.ovirt.org/#/c/47738/10/tests/v2vTests.py
File tests/v2vTests.py:

Line 316:         self.assertEquals(network['bridge'], 'VM Network')
Line 317: 
Line 318:     @MonkeyPatch(v2v, '_VIRT_V2V', FAKE_VIRT_V2V)
Line 319:     def testSuccessfulImport(self):
Line 320:         self.ivm._import()
You should run this job using the job interface:

    self.ivm.start()

Then wait until the job is complete and test that it succeeded

This will allow testing the entire class, which is needed for example for testing this
change: https://gerrit.ovirt.org/45616

The ImportVm class does not make it easy to wait for the thread. To make it easier to
test, we can keep the thread instance and do:

    self.ivm._thread.join()

Or poll on self.ivm.status until the job is done.

Or, do something like this:

    done = threading.Event()

    def run():
        self.ivm._run()
        done.set()

    self.ivm._run = run
    self.ivm.start()
    done.wait()

It would be nicer if the standard job interface would allow to wait until a job finish,
even if this is needed only for testing now.
Line 321:         self.assertEqual(self.ivm.status, v2v.STATUS.DONE)
Line 322: 
Line 323:     @MonkeyPatch(v2v, '_VIRT_V2V', FAKE_VIRT_V2V)
Line 324:     def testV2VOutput(self):


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 10:

(3 comments)

https://gerrit.ovirt.org/#/c/47738/10/tests/v2vTests.py
File tests/v2vTests.py:

Line 316:         self.assertEquals(network['bridge'], 'VM Network')
Line 317: 
Line 318:     @MonkeyPatch(v2v, '_VIRT_V2V', FAKE_VIRT_V2V)
Line 319:     def testSuccessfulImport(self):
Line 320:         self.ivm._import()
>  You should run this job using the job interface: 
I will go with
join()
I will need to patch v2v.py to make the thread a member...
Line 321:         self.assertEqual(self.ivm.status, v2v.STATUS.DONE)
Line 322: 
Line 323:     @MonkeyPatch(v2v, '_VIRT_V2V', FAKE_VIRT_V2V)
Line 324:     def testV2VOutput(self):


Line 325:         '''
Line 326:             We are testing the output of fake-virt-v2v with vminfo input
Line 327:             against pre-saved output;
Line 328:             Do not change this parameters without modifying
Line 329:             fake-virt-v2v.output.
>  The extra indentation is not needed, use: 
Done
Line 330:         '''
Line 331:         self.ivm._import()
Line 332:         self.assertEqual(self.ivm.status, v2v.STATUS.DONE)
Line 333: 


Line 328:             Do not change this parameters without modifying
Line 329:             fake-virt-v2v.output.
Line 330:         '''
Line 331:         self.ivm._import()
Line 332:         self.assertEqual(self.ivm.status, v2v.STATUS.DONE)
>  This part may break and cause this test to break while there is
> nothing wro 
I guess you meant https://gerrit.ovirt.org/#/c/48581/
Ok
Line 333: 
Line 334:         rc, output, error = execCmd(self.ivm._from_libvirt_command(), raw=True)
Line 335:         self.assertEqual(rc, 0)
Line 336: 


-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

gerrit-hooks has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 11:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Shahar Havivi has posted comments on this change.

Change subject: v2v: add fake virt-v2v process for testing without virt-v2v
......................................................................


Patch Set 11: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/47738
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If062a1136921af19a2ffcb4d147611bbc9cf5464
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No