Change in vdsm[master]: [WIP] core: Expose API for qemu-img commit

Ala Hino has uploaded a new change for review.

Change subject: [WIP] core: Expose API for qemu-img commit
......................................................................

[WIP] core: Expose API for qemu-img commit

Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Signed-off-by: Ala Hino <ahino(a)redhat.com&gt;
---
M lib/vdsm/qemuimg.py
M tests/qemuimg_test.py
2 files changed, 35 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/22/64222/1

diff --git a/lib/vdsm/qemuimg.py b/lib/vdsm/qemuimg.py
index 61ebec3..4ad7073 100644
--- a/lib/vdsm/qemuimg.py
+++ b/lib/vdsm/qemuimg.py
@@ -185,6 +185,16 @@
     return QemuImgOperation(cmd, cwd=cwdPath)
 
 
+def commit(top, base=None):
+    cmd = [_qemuimg.cmd, "commit", "-p"]
+    if base:
+        cmd.extend(("-b", base))
+    else:
+        cmd.append("-d")
+
+    return QemuImgOperation(cmd)
+
+
 class QemuImgOperation(object):
     REGEXPR = re.compile(r'\s*\(([\d.]+)/100%\)\s*')
 
diff --git a/tests/qemuimg_test.py b/tests/qemuimg_test.py
index d7d9fab..3fd97ea 100644
--- a/tests/qemuimg_test.py
+++ b/tests/qemuimg_test.py
@@ -23,6 +23,7 @@
 from functools import partial
 
 from monkeypatch import MonkeyPatch, MonkeyPatchScope
+from storagetestlib import qemu_pattern_write, qemu_pattern_verify
 from testlib import VdsmTestCase as TestCaseBase
 from testlib import permutations, expandPermutations
 from testlib import make_config
@@ -329,3 +330,27 @@
 
         p.poll()
         self.assertEquals(p.finished, True)
+
+class TestCommit(TestCaseBase):
+
+    @MonkeyPatch(qemuimg, 'config', CONFIG)
+    def test_commit(self):
+        with namedTemporaryDir() as tmpdir:
+            base = os.path.join(tmpdir, 'base.img')
+            top = os.path.join(tmpdir, 'top.img')
+            size = 1048576
+            qemuimg.create(base, size=size, format=qemuimg.FORMAT.RAW)
+            qemu_pattern_write(base, qemuimg.FORMAT.RAW, '0', 0xf0)
+
+            qemuimg.create(top, format=qemuimg.FORMAT.QCOW2, backing=base)
+            qemu_pattern_write(top, qemuimg.FORMAT.QCOW2, '1024', 0xf1)
+
+            op = qemuimg.commit(top=top)
+            op.wait_for_completion()
+
+            # top should not change after commit
+            qemu_pattern_verify(top, qemuimg.FORMAT.RAW, '0', 0xf1)
+
+            # base now should include original data and top's data
+            qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '0', 0xf0)
+            qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '1024', 0xf1)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;

gerrit-hooks has posted comments on this change.

Change subject: [WIP] core: Expose API for qemu-img commit
......................................................................


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.6',
'ovirt-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

gerrit-hooks has posted comments on this change.

Change subject: [WIP] core: Expose API for qemu-img commit
......................................................................


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.6',
'ovirt-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: [WIP] core: Expose API for qemu-img commit
......................................................................


Patch Set 2:

(6 comments)

Great work!

https://gerrit.ovirt.org/#/c/64222/2//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2016-09-20 20:30:58 +0300
Line 4: Commit:     Ala Hino <ahino(a)redhat.com&gt;
Line 5: CommitDate: 2016-09-20 20:36:48 +0300
Line 6: 
Line 7: [WIP] core: Expose API for qemu-img commit
Use qemuimg: ...
Line 8: 
Line 9: Change-Id: If7a13be40541fb268541bd8614a642263b96b487


https://gerrit.ovirt.org/#/c/64222/2/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 185:     return QemuImgOperation(cmd, cwd=cwdPath)
Line 186: 
Line 187: 
Line 188: def commit(top, base=None):
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p"]
We should also use -t none like we use in convert, and if possible, also -T none. This use
directio, avoiding spamming the page cache with data from the image, and also safer since
the page cache may not be correct if another host was writing to the same image before.
Line 190:     if base:
Line 191:         cmd.extend(("-b", base))
Line 192:     else:
Line 193:         cmd.append("-d")


Line 188: def commit(top, base=None):
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p"]
Line 190:     if base:
Line 191:         cmd.extend(("-b", base))
Line 192:     else:
Please explain why -d is needed, the code is very clear about adding it when base is not
specified, but we need to know why. I see the patch - "removed unneeded -d option
(works for me)".
Line 193:         cmd.append("-d")
Line 194: 
Line 195:     return QemuImgOperation(cmd)
Line 196: 


Line 189:     cmd = [_qemuimg.cmd, "commit", "-p"]
Line 190:     if base:
Line 191:         cmd.extend(("-b", base))
Line 192:     else:
Line 193:         cmd.append("-d")
We must use -f to enforce the format in the metadata. Letting qemu detect the format is a
security issue.
Line 194: 
Line 195:     return QemuImgOperation(cmd)
Line 196: 
Line 197: 


Line 190:     if base:
Line 191:         cmd.extend(("-b", base))
Line 192:     else:
Line 193:         cmd.append("-d")
Line 194: 
Appending top the command will increase the chance for successful commit.
Line 195:     return QemuImgOperation(cmd)
Line 196: 
Line 197: 
Line 198: class QemuImgOperation(object):


Line 191:         cmd.extend(("-b", base))
Line 192:     else:
Line 193:         cmd.append("-d")
Line 194: 
Line 195:     return QemuImgOperation(cmd)
I think we must change the working directory to the image directory as we do in convert()
- we need the same logic, maybe we should move the code to a little helper (in another
patch before this one).
Line 196: 
Line 197: 
Line 198: class QemuImgOperation(object):
Line 199:     REGEXPR = re.compile(r'\s*\(([\d.]+)/100%\)\s*')


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(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: [WIP] core: Expose API for qemuimg commit
......................................................................


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.6',
'ovirt-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

gerrit-hooks has posted comments on this change.

Change subject: [WIP] core: Expose API for qemuimg commit
......................................................................


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.6',
'ovirt-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(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: [WIP] core: Expose API for qemuimg commit
......................................................................


Patch Set 4:

(5 comments)

https://gerrit.ovirt.org/#/c/64222/4/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 184: 
Line 185:     return QemuImgOperation(cmd, cwd=cwdPath)
Line 186: 
Line 187: 
Line 188: def commit(top, topFormat=None, base=None):
We should require topFormat, more secure to never let qemu detect the format.
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p"]
Line 190:     cwdir = None
Line 191: 
Line 192:     if base:


Line 185:     return QemuImgOperation(cmd, cwd=cwdPath)
Line 186: 
Line 187: 
Line 188: def commit(top, topFormat=None, base=None):
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p"]
Add -t none.
Line 190:     cwdir = None
Line 191: 
Line 192:     if base:
Line 193:         cmd.extend(("-b", base))


Line 186: 
Line 187: 
Line 188: def commit(top, topFormat=None, base=None):
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p"]
Line 190:     cwdir = None
workdir?
Line 191: 
Line 192:     if base:
Line 193:         cmd.extend(("-b", base))
Line 194:         if not os.path.isabs(base):


Line 191: 
Line 192:     if base:
Line 193:         cmd.extend(("-b", base))
Line 194:         if not os.path.isabs(base):
Line 195:             cwdir = cwdPath = os.path.dirname(base)
Lets always use workdir, to simplify the code.
Line 196:     else:
Line 197:         # If base volume is not provided, qemuimg commit will empty the top
Line 198:         # volume after the operation has succeeded. Providing '-d'
option
Line 199:         # will cause qemuimg commit not to empty the top volume. Note that


Line 201:         # '-d' is always implied.
Line 202:         cmd.append("-d")
Line 203: 
Line 204:     if topFormat:
Line 205:         cmd.extend(("-f", str(topFormat)))
str not needed
Line 206: 
Line 207:     cmd.append(top)
Line 208: 
Line 209:     return QemuImgOperation(cmd, cwd=cwdir)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(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: [WIP] core: Expose API for qemuimg commit
......................................................................


Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/64222/4/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 342:             size = 1048576
Line 343: 
Line 344:             qemuimg.create(base, size=size, format=qemuimg.FORMAT.RAW)
Line 345:             qemu_pattern_write(base, qemuimg.FORMAT.RAW, '0')
Line 346:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '0', 0xf1)
This is broken because 0xf1 is not the pattern but the length.

Please use call kwargs using key=value - for example:

    qemu_pattern_write(base, qemuimg.FORMAT.RAW, offset='0', len='1k',
pattern=0xf0)
Line 347: 
Line 348: 
Line 349:     @MonkeyPatch(qemuimg, 'config', CONFIG)
Line 350:     @permutations(((qemuimg.FORMAT.RAW,), (qemuimg.FORMAT.QCOW2,)))


Line 349:     @MonkeyPatch(qemuimg, 'config', CONFIG)
Line 350:     @permutations(((qemuimg.FORMAT.RAW,), (qemuimg.FORMAT.QCOW2,)))
Line 351:     def test_commit(self, baseFormat):
Line 352:         with namedTemporaryDir() as tmpdir:
Line 353:             base = os.path.join(tmpdir, 'base.img')
Lets move all the operations together for each volume:

    base = ...
    qemuimg.create(base, ...)
    qemu_pattern_write(base, ...)

Then we probably can move this into a little helper:

    def make_image(path, size, format, index, backing=None):
        qemuimg.create(path, size=size, format=format, backing=backing)
        qemu_pattern_write(path, format, offset="%dk" % index, len='1k',
0xf0 + index)

This will make the test much easier to get wrong, image always have data at the correct
offset with the correct pattern.
Line 354:             top = os.path.join(tmpdir, 'top.img')
Line 355:             size = 1048576
Line 356: 
Line 357:             qemuimg.create(base, size=size, format=baseFormat)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(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: [WIP] core: Expose API for qemuimg commit
......................................................................


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/64222/4/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 184: 
Line 185:     return QemuImgOperation(cmd, cwd=cwdPath)
Line 186: 
Line 187: 
Line 188: def commit(top, topFormat=None, base=None):
>  We should require topFormat, more secure to never let qemu detect the
> forma 
I think topFormat should be baseFormat. qemu-img does not specify what is the
-f fmt argument, but it cannot be the top format since the top must be qcow image to be a
"top" image.

Since qemu allow fmt as optional parameter, lest keep it as optional. This is also more
consistent with other functions in this module.
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p"]
Line 190:     cwdir = None
Line 191: 
Line 192:     if base:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(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: [WIP] core: Expose API for qemuimg commit
......................................................................


Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/64222/4/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 363:             op = qemuimg.commit(top)
Line 364:             op.wait_for_completion()
Line 365: 
Line 366:             # top should not change after commit
Line 367:             qemu_pattern_verify(top, qemuimg.FORMAT.RAW, '0', 0xf1)
Issues:
- Top always use qemuimg.FORMAT.QCOW2
- Use kwargs
- 0xf1 is the length in this call, so the verification is incorrect
Line 368: 
Line 369:             # base now should include original data and top's data
Line 370:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '0', 0xf2)
Line 371:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '512', 0xf1)


Line 367:             qemu_pattern_verify(top, qemuimg.FORMAT.RAW, '0', 0xf1)
Line 368: 
Line 369:             # base now should include original data and top's data
Line 370:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '0', 0xf2)
Line 371:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '512', 0xf1)
Issues:
- Both call must use baseFormat
- Use offset and len in 1k units
- The pattern should be 0xf0 for the first 1k and 0xf1 for the second 1k.
Line 372: 
Line 373:     @MonkeyPatch(qemuimg, 'config', CONFIG)
Line 374:     def test_commit_backing_chain(self):
Line 375:         with namedTemporaryDir() as tmpdir:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(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: [WIP] core: Expose API for qemuimg commit
......................................................................


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/64222/4/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 369:             # base now should include original data and top's data
Line 370:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '0', 0xf2)
Line 371:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '512', 0xf1)
Line 372: 
Line 373:     @MonkeyPatch(qemuimg, 'config', CONFIG)
Running this with both raw and qcow base can be useful to detect qemu-img regression, and
cost almost nothing.
Line 374:     def test_commit_backing_chain(self):
Line 375:         with namedTemporaryDir() as tmpdir:
Line 376:             base = os.path.join(tmpdir, 'base.img')
Line 377:             internal = os.path.join(tmpdir, 'internal.img')


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(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: [WIP] core: Expose API for qemuimg commit
......................................................................


Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/64222/4/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 338:     @MonkeyPatch(qemuimg, 'config', CONFIG)
Line 339:     def test_write_and_verify(self):
Line 340:         with namedTemporaryDir() as tmpdir:
Line 341:             base = os.path.join(tmpdir, 'base.img')
Line 342:             size = 1048576
We don't need this variable, just inline the value bellow.
Line 343: 
Line 344:             qemuimg.create(base, size=size, format=qemuimg.FORMAT.RAW)
Line 345:             qemu_pattern_write(base, qemuimg.FORMAT.RAW, '0')
Line 346:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '0', 0xf1)


Line 340:         with namedTemporaryDir() as tmpdir:
Line 341:             base = os.path.join(tmpdir, 'base.img')
Line 342:             size = 1048576
Line 343: 
Line 344:             qemuimg.create(base, size=size, format=qemuimg.FORMAT.RAW)
size=10485576 is fine.
Line 345:             qemu_pattern_write(base, qemuimg.FORMAT.RAW, '0')
Line 346:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '0', 0xf1)
Line 347: 
Line 348: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(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: [WIP] core: Expose API for qemuimg commit
......................................................................


Patch Set 4:

(3 comments)

https://gerrit.ovirt.org/#/c/64222/4/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 380: 
Line 381:             qemuimg.create(base, size=size, format=qemuimg.FORMAT.RAW)
Line 382:             qemu_pattern_write(base, qemuimg.FORMAT.RAW, '0', 0xf0)
Line 383: 
Line 384:             qemuimg.create(internal, size=size, format=qemuimg.FORMAT.QCOW2,
size is not needed when creating image with backing chain.
Line 385:                            backing=base)
Line 386:             qemu_pattern_write(internal, qemuimg.FORMAT.QCOW2, '512',
0xf1)
Line 387: 
Line 388:             qemuimg.create(top, format=qemuimg.FORMAT.QCOW2, backing=internal)


Line 385:                            backing=base)
Line 386:             qemu_pattern_write(internal, qemuimg.FORMAT.QCOW2, '512',
0xf1)
Line 387: 
Line 388:             qemuimg.create(top, format=qemuimg.FORMAT.QCOW2, backing=internal)
Line 389:             qemu_pattern_write(top, qemuimg.FORMAT.QCOW2, '800', 0xf2)
Use same offsets and patterns as in the previous test :

- base: 0-1k, 0xf0
- internal: 1k-2k, 0xf1
- top: 2k-3k, 0xf2
Line 390: 
Line 391:             op = qemuimg.commit(top, base=base)
Line 392:             op.wait_for_completion()
Line 393: 


Line 396: 
Line 397:             # base now should include original data and top's data
Line 398:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '0', 0xf2)
Line 399:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '512', 0xf1)
Line 400:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '800', 0xf2)
Use kwargs and same offsets.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(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: [WIP] core: Expose API for qemuimg commit
......................................................................


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/64222/4/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 334: 
Line 335: @expandPermutations
Line 336: class TestCommit(TestCaseBase):
Line 337: 
Line 338:     @MonkeyPatch(qemuimg, 'config', CONFIG)
This class does not care about the qcow_compat, so we don't need to monkeypatch the
config.
Line 339:     def test_write_and_verify(self):
Line 340:         with namedTemporaryDir() as tmpdir:
Line 341:             base = os.path.join(tmpdir, 'base.img')
Line 342:             size = 1048576


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(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: [WIP] core: Expose API for qemuimg commit
......................................................................


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/64222/4/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 347: 
Line 348: 
Line 349:     @MonkeyPatch(qemuimg, 'config', CONFIG)
Line 350:     @permutations(((qemuimg.FORMAT.RAW,), (qemuimg.FORMAT.QCOW2,)))
Line 351:     def test_commit(self, baseFormat):
Use only lower_case names - in the module api we want to be consistent with the old apis,
but in the tests we are free to use new vdsm style.
Line 352:         with namedTemporaryDir() as tmpdir:
Line 353:             base = os.path.join(tmpdir, 'base.img')
Line 354:             top = os.path.join(tmpdir, 'top.img')
Line 355:             size = 1048576


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(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: [WIP] core: Expose API for qemuimg commit
......................................................................


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/64222/4/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 363:             op = qemuimg.commit(top)
Line 364:             op.wait_for_completion()
Line 365: 
Line 366:             # top should not change after commit
Line 367:             qemu_pattern_verify(top, qemuimg.FORMAT.RAW, '0', 0xf1)
>  Issues: 
This actually tests nothing, since top will show the
data from base even if it was empty.

We can verify that top size did not change after the operation.
Line 368: 
Line 369:             # base now should include original data and top's data
Line 370:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '0', 0xf2)
Line 371:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '512', 0xf1)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(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: [WIP] core: Expose API for qemuimg commit
......................................................................


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/64222/4/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 197:         # If base volume is not provided, qemuimg commit will empty the top
Line 198:         # volume after the operation has succeeded. Providing '-d'
option
Line 199:         # will cause qemuimg commit not to empty the top volume. Note that
Line 200:         # if a backing chain is provided, i.e. a base volume is provided,
Line 201:         # '-d' is always implied.
This comment is repeating what we can read in the manual twice. What is missing here and
is more important, is why we don't want to empty the image - we may need to wipe the
top volume before deleting it, and emptying it may leave the data on the the underlying
storage (qemu probably mark sectors as zero instead of wiping the data).
Line 202:         cmd.append("-d")
Line 203: 
Line 204:     if topFormat:
Line 205:         cmd.extend(("-f", str(topFormat)))


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(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: [WIP] core: Expose API for qemuimg commit
......................................................................


Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/64222/4/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 332:         self.assertEquals(p.finished, True)
Line 333: 
Line 334: 
Line 335: @expandPermutations
Line 336: class TestCommit(TestCaseBase):
Add test for progress - testing chain with two empty images without writing any pattern,
and check that op.progress is 100.0 when operation is completed.
Line 337: 
Line 338:     @MonkeyPatch(qemuimg, 'config', CONFIG)
Line 339:     def test_write_and_verify(self):
Line 340:         with namedTemporaryDir() as tmpdir:


Line 370:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '0', 0xf2)
Line 371:             qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '512', 0xf1)
Line 372: 
Line 373:     @MonkeyPatch(qemuimg, 'config', CONFIG)
Line 374:     def test_commit_backing_chain(self):
Maybe test_commit_internal?
Line 375:         with namedTemporaryDir() as tmpdir:
Line 376:             base = os.path.join(tmpdir, 'base.img')
Line 377:             internal = os.path.join(tmpdir, 'internal.img')
Line 378:             top = os.path.join(tmpdir, 'top.img')


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

gerrit-hooks has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


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.6',
'ovirt-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: core: Expose API for qemuimg commit

Ala Hino has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 5:

(7 comments)

Nir, note that I created a single test that covers all use cases. I am hoping that I
addressed all your comments

https://gerrit.ovirt.org/#/c/64222/2/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 188: def commit(top, topFormat, base=None):
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p", "-t",
"none"]
Line 190:     # For simplicity, we always run commit in the working directory.
Line 191:     workdir = os.path.dirname(top)
Line 192: 
>  Please explain why -d is needed, the code is very clear about adding
> it whe 
Done
Line 193:     if base:
Line 194:         cmd.extend(("-b", base))
Line 195:     else:
Line 196:         # If base volume is not provided, qemuimg commit will empty the top


https://gerrit.ovirt.org/#/c/64222/4/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 184: 
Line 185:     return QemuImgOperation(cmd, cwd=cwdPath)
Line 186: 
Line 187: 
Line 188: def commit(top, topFormat, base=None):
>  We should require topFormat, more secure to never let qemu detect the
> forma 
Done
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p", "-t",
"none"]
Line 190:     # For simplicity, we always run commit in the working directory.
Line 191:     workdir = os.path.dirname(top)
Line 192: 


Line 185:     return QemuImgOperation(cmd, cwd=cwdPath)
Line 186: 
Line 187: 
Line 188: def commit(top, topFormat, base=None):
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p", "-t",
"none"]
>  Add -t none. 
Done
Line 190:     # For simplicity, we always run commit in the working directory.
Line 191:     workdir = os.path.dirname(top)
Line 192: 
Line 193:     if base:


Line 186: 
Line 187: 
Line 188: def commit(top, topFormat, base=None):
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p", "-t",
"none"]
Line 190:     # For simplicity, we always run commit in the working directory.
>  workdir? 
Done
Line 191:     workdir = os.path.dirname(top)
Line 192: 
Line 193:     if base:
Line 194:         cmd.extend(("-b", base))


Line 191:     workdir = os.path.dirname(top)
Line 192: 
Line 193:     if base:
Line 194:         cmd.extend(("-b", base))
Line 195:     else:
>  Lets always use workdir, to simplify the code. 
Done
Line 196:         # If base volume is not provided, qemuimg commit will empty the top
Line 197:         # volume after the operation has succeeded. Providing '-d'
option
Line 198:         # will cause qemuimg commit not to empty the top volume. Note that
Line 199:         # if a backing chain is provided, i.e. a base volume is provided,


Line 201:         cmd.append("-d")
Line 202: 
Line 203:     cmd.extend(("-f", topFormat))
Line 204: 
Line 205:     cmd.append(top)
>  str not needed 
Done
Line 206: 
Line 207:     return QemuImgOperation(cmd, cwd=workdir)
Line 208: 
Line 209: 


https://gerrit.ovirt.org/#/c/64222/4/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 342:         (1, qemuimg.FORMAT.QCOW2, False),
Line 343:         (1, qemuimg.FORMAT.QCOW2),
Line 344:         (3, qemuimg.FORMAT.RAW),
Line 345:         (3, qemuimg.FORMAT.QCOW2)
Line 346:     ])
>  This is broken because 0xf1 is not the pattern but the length.
Done
Line 347:     def test_commit(self, chainLen, baseFormat, useBacking=True):
Line 348:         size = 1048576
Line 349:         with namedTemporaryDir() as tmpdir:
Line 350:             # create base


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

gerrit-hooks has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


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.6',
'ovirt-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: core: Expose API for qemuimg commit

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 6:

(5 comments)

Partial review.

https://gerrit.ovirt.org/#/c/64222/6/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 335: @expandPermutations
Line 336: class TestCommit(TestCaseBase):
Line 337: 
Line 338:     @MonkeyPatch(qemuimg, 'config', CONFIG)
Line 339:     @permutations([
We need a comment here for the argument

    # name1, name2, ...
Line 340:         (1, qemuimg.FORMAT.RAW, False),
Line 341:         (1, qemuimg.FORMAT.RAW),
Line 342:         (1, qemuimg.FORMAT.QCOW2, False),
Line 343:         (1, qemuimg.FORMAT.QCOW2),


Line 337: 
Line 338:     @MonkeyPatch(qemuimg, 'config', CONFIG)
Line 339:     @permutations([
Line 340:         (1, qemuimg.FORMAT.RAW, False),
Line 341:         (1, qemuimg.FORMAT.RAW),
Please keep the same number of arguments to make the format consistent and simple.
Line 342:         (1, qemuimg.FORMAT.QCOW2, False),
Line 343:         (1, qemuimg.FORMAT.QCOW2),
Line 344:         (3, qemuimg.FORMAT.RAW),
Line 345:         (3, qemuimg.FORMAT.QCOW2)


Line 343:         (1, qemuimg.FORMAT.QCOW2),
Line 344:         (3, qemuimg.FORMAT.RAW),
Line 345:         (3, qemuimg.FORMAT.QCOW2)
Line 346:     ])
Line 347:     def test_commit(self, chainLen, baseFormat, useBacking=True):
This looks like use_base, or with_base. backing is another term not related to commit.
Line 348:         size = 1048576
Line 349:         with namedTemporaryDir() as tmpdir:
Line 350:             # create base
Line 351:             base = os.path.join(tmpdir, "base.img")


Line 361:                 chain.append(vol)
Line 362:                 parent = vol
Line 363: 
Line 364:             if useBacking:
Line 365:                 op = qemuimg.commit(chain[chainLen-1],
You can use:

    chain[-1]

The last item.
Line 366:                                     topFormat=topFormat, base=base)
Line 367:             else:
Line 368:                 op = qemuimg.commit(chain[chainLen-1], topFormat)
Line 369:             op.wait_for_completion()


Line 364:             if useBacking:
Line 365:                 op = qemuimg.commit(chain[chainLen-1],
Line 366:                                     topFormat=topFormat, base=base)
Line 367:             else:
Line 368:                 op = qemuimg.commit(chain[chainLen-1], topFormat)
This is little ugly, can we have the same call, taking None as default value when we
don't have a backing?

For example, always use:

    op = qemuimg.commit(chain[-1], format, base=base)

This call is equivalent to:

    op = qemuimg.commit(chain[-1], format)

When base is None.
Line 369:             op.wait_for_completion()
Line 370: 
Line 371:             # verify base data at offset 0 after commit
Line 372:             qemu_pattern_verify(base, baseFormat, offset='0',
len='1k',


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 6:

(3 comments)

https://gerrit.ovirt.org/#/c/64222/6/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 186: 
Line 187: 
Line 188: def commit(top, topFormat, base=None):
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p", "-t",
"none"]
Line 190:     # For simplicity, we always run commit in the working directory.
in the image directory
Line 191:     workdir = os.path.dirname(top)
Line 192: 
Line 193:     if base:
Line 194:         cmd.extend(("-b", base))


Line 196:         # If base volume is not provided, qemuimg commit will empty the top
Line 197:         # volume after the operation has succeeded. Providing '-d'
option
Line 198:         # will cause qemuimg commit not to empty the top volume. Note that
Line 199:         # if a backing chain is provided, i.e. a base volume is provided,
Line 200:         # '-d' is always implied.
See my comment about this comment in previous version.
Line 201:         cmd.append("-d")
Line 202: 
Line 203:     cmd.extend(("-f", topFormat))
Line 204: 


Line 202: 
Line 203:     cmd.extend(("-f", topFormat))
Line 204: 
Line 205:     cmd.append(top)
Line 206: 
We use workdir only here, so better create it just before we use it. Smaller scope leave
no place for bugs.
Line 207:     return QemuImgOperation(cmd, cwd=workdir)
Line 208: 
Line 209: 
Line 210: class QemuImgOperation(object):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

gerrit-hooks has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 7:

* update_tracker: OK
* 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-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: core: Expose API for qemuimg commit

Ala Hino has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 6:

(8 comments)

https://gerrit.ovirt.org/#/c/64222/6/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 186: 
Line 187: 
Line 188: def commit(top, topFormat, base=None):
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p", "-t",
"none"]
Line 190:     # For simplicity, we always run commit in the working directory.
>  in the image directory 
Done
Line 191:     workdir = os.path.dirname(top)
Line 192: 
Line 193:     if base:
Line 194:         cmd.extend(("-b", base))


Line 196:         # If base volume is not provided, qemuimg commit will empty the top
Line 197:         # volume after the operation has succeeded. Providing '-d'
option
Line 198:         # will cause qemuimg commit not to empty the top volume. Note that
Line 199:         # if a backing chain is provided, i.e. a base volume is provided,
Line 200:         # '-d' is always implied.
>  See my comment about this comment in previous version. 
Done
Line 201:         cmd.append("-d")
Line 202: 
Line 203:     cmd.extend(("-f", topFormat))
Line 204: 


Line 202: 
Line 203:     cmd.extend(("-f", topFormat))
Line 204: 
Line 205:     cmd.append(top)
Line 206: 
>  We use workdir only here, so better create it just before we use it.
> Smalle 
Done
Line 207:     return QemuImgOperation(cmd, cwd=workdir)
Line 208: 
Line 209: 
Line 210: class QemuImgOperation(object):


https://gerrit.ovirt.org/#/c/64222/6/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 335: @expandPermutations
Line 336: class TestCommit(TestCaseBase):
Line 337: 
Line 338:     @MonkeyPatch(qemuimg, 'config', CONFIG)
Line 339:     @permutations([
>  We need a comment here for the argument 
Done
Line 340:         (1, qemuimg.FORMAT.RAW, False),
Line 341:         (1, qemuimg.FORMAT.RAW),
Line 342:         (1, qemuimg.FORMAT.QCOW2, False),
Line 343:         (1, qemuimg.FORMAT.QCOW2),


Line 337: 
Line 338:     @MonkeyPatch(qemuimg, 'config', CONFIG)
Line 339:     @permutations([
Line 340:         (1, qemuimg.FORMAT.RAW, False),
Line 341:         (1, qemuimg.FORMAT.RAW),
>  Please keep the same number of arguments to make the format
> consistent and  
Done
Line 342:         (1, qemuimg.FORMAT.QCOW2, False),
Line 343:         (1, qemuimg.FORMAT.QCOW2),
Line 344:         (3, qemuimg.FORMAT.RAW),
Line 345:         (3, qemuimg.FORMAT.QCOW2)


Line 343:         (1, qemuimg.FORMAT.QCOW2),
Line 344:         (3, qemuimg.FORMAT.RAW),
Line 345:         (3, qemuimg.FORMAT.QCOW2)
Line 346:     ])
Line 347:     def test_commit(self, chainLen, baseFormat, useBacking=True):
>  This looks like use_base, or with_base. backing is another term not
> related 
Done
Line 348:         size = 1048576
Line 349:         with namedTemporaryDir() as tmpdir:
Line 350:             # create base
Line 351:             base = os.path.join(tmpdir, "base.img")


Line 361:                 chain.append(vol)
Line 362:                 parent = vol
Line 363: 
Line 364:             if useBacking:
Line 365:                 op = qemuimg.commit(chain[chainLen-1],
>  You can use: 
Done
Line 366:                                     topFormat=topFormat, base=base)
Line 367:             else:
Line 368:                 op = qemuimg.commit(chain[chainLen-1], topFormat)
Line 369:             op.wait_for_completion()


Line 364:             if useBacking:
Line 365:                 op = qemuimg.commit(chain[chainLen-1],
Line 366:                                     topFormat=topFormat, base=base)
Line 367:             else:
Line 368:                 op = qemuimg.commit(chain[chainLen-1], topFormat)
>  This is little ugly, can we have the same call, taking None as
> default valu 
Done
Line 369:             op.wait_for_completion()
Line 370: 
Line 371:             # verify base data at offset 0 after commit
Line 372:             qemu_pattern_verify(base, baseFormat, offset='0',
len='1k',


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

gerrit-hooks has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 8:

* update_tracker: OK
* 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-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: core: Expose API for qemuimg commit

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 8:

(11 comments)

Tests are nice, but we are reinventing storagetestlib.write_qemu_chain and
storagetestlib.verify_qemu_chain.

I think we should instead fix these methods so they work with generic tuples instead of
using volume objects, so we can reuse them for other tests.

https://gerrit.ovirt.org/#/c/64222/8/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 334: 
Line 335: @expandPermutations
Line 336: class TestCommit(TestCaseBase):
Line 337: 
Line 338:     @MonkeyPatch(qemuimg, 'config', CONFIG)
We can remove the config mocking.

Instead, I would add qcow2 compat to permutations - note that qeumimg.create supports now
qcow2Compat argument - we should test with both compat="0.10" and
"1.1" to make sure commit works for both old volumes and new volumes.
Line 339:     @permutations([
Line 340:         # chain_len, base_format, use_base
Line 341:         (1, qemuimg.FORMAT.RAW, False),
Line 342:         (1, qemuimg.FORMAT.RAW, True),


Line 343:         (1, qemuimg.FORMAT.QCOW2, False),
Line 344:         (1, qemuimg.FORMAT.QCOW2, True),
Line 345:         (3, qemuimg.FORMAT.RAW, True),
Line 346:         (3, qemuimg.FORMAT.QCOW2, True)
Line 347:     ])
Nice!
Line 348:     def test_commit(self, chain_len, base_format, use_base=True):
Line 349:         size = 1048576
Line 350:         with namedTemporaryDir() as tmpdir:
Line 351:             # create base


Line 347:     ])
Line 348:     def test_commit(self, chain_len, base_format, use_base=True):
Line 349:         size = 1048576
Line 350:         with namedTemporaryDir() as tmpdir:
Line 351:             # create base
It would be nice to create base in the same loop as the rest of the volumes. We can call
it "vol0.img".

To set the base format, we can do:

    for i, name in enumerate(chain):
       ...
       make_image(..., format=base_format if i == 0 else format, ...)
Line 352:             base_vol = os.path.join(tmpdir, "base.img")
Line 353:             self.make_image(base_vol, size, base_format, 0)
Line 354: 
Line 355:             # create chain


Line 349:         size = 1048576
Line 350:         with namedTemporaryDir() as tmpdir:
Line 351:             # create base
Line 352:             base_vol = os.path.join(tmpdir, "base.img")
Line 353:             self.make_image(base_vol, size, base_format, 0)
We can add qcow2_compat argument here...
Line 354: 
Line 355:             # create chain
Line 356:             chain = []
Line 357:             parent = base_vol


Line 355:             # create chain
Line 356:             chain = []
Line 357:             parent = base_vol
Line 358:             top_format = qemuimg.FORMAT.QCOW2
Line 359:             for i in range(0, chain_len):
This can and should be written as:

    for i in range(chain_len):
Line 360:                 vol = os.path.join(tmpdir, "vol%d.img" % i)
Line 361:                 self.make_image(vol, size, top_format, i+1, parent)
Line 362:                 chain.append(vol)
Line 363:                 parent = vol


Line 363:                 parent = vol
Line 364: 
Line 365:             base = None
Line 366:             if use_base:
Line 367:                 base = base_vol
This works, but we can use more modern conditional expression:

    base = base_vol if use_base else None
Line 368: 
Line 369:             op = qemuimg.commit(chain[-1], topFormat=top_format,
Line 370:                                 base=base)
Line 371:             op.wait_for_completion()


Line 366:             if use_base:
Line 367:                 base = base_vol
Line 368: 
Line 369:             op = qemuimg.commit(chain[-1], topFormat=top_format,
Line 370:                                 base=base)
Or we can just do:

    op = qemuimg.commit(chain[-1], topFormat=top_format,
                        base=base if use_base else None)
Line 371:             op.wait_for_completion()
Line 372: 
Line 373:             # verify base data at offset 0 after commit
Line 374:             qemu_pattern_verify(base_vol, base_format, offset='0',


Line 371:             op.wait_for_completion()
Line 372: 
Line 373:             # verify base data at offset 0 after commit
Line 374:             qemu_pattern_verify(base_vol, base_format, offset='0',
Line 375:                                 len='1k', pattern=0xf0)
If you include base in chain, can verify all volume in the same loop.
Line 376:             for i, vol in enumerate(chain):
Line 377:                 offset = "{}k".format(i+1)
Line 378:                 pattern = 0xf0 + (i+1)
Line 379:                 qemu_pattern_verify(base_vol, base_format, offset=offset,


Line 385:     @permutations([
Line 386:         # base_format
Line 387:         (qemuimg.FORMAT.RAW,),
Line 388:         (qemuimg.FORMAT.QCOW2,)
Line 389:     ])
We don't need permutations for this test, and we don't care about the config. The
only think we care about is if qemu-img progress works for commit.
Line 390:     def test_commit_progress(self, base_format):
Line 391:         with namedTemporaryDir() as tmpdir:
Line 392:             size = 1048576
Line 393:             base = os.path.join(tmpdir, "base.img")


Line 399:             op = qemuimg.commit(top, topFormat=qemuimg.FORMAT.QCOW2)
Line 400:             op.wait_for_completion()
Line 401:             self.assertEquals(100, op.progress)
Line 402: 
Line 403:     def make_image(self, path, size, format, index, backing=None):
We don't need self for this, this will be more useable as a function in the module,
and later maybe in storagetestlib.

Add qcow2_compat argument here.
Line 404:         qemuimg.create(path, size=size, format=format, backing=backing)
Line 405:         qemu_pattern_write(path, format, offset="%dk" % index,
len='1k',


Line 400:             op.wait_for_completion()
Line 401:             self.assertEquals(100, op.progress)
Line 402: 
Line 403:     def make_image(self, path, size, format, index, backing=None):
Line 404:         qemuimg.create(path, size=size, format=format, backing=backing)
Specify the qcow2Compat:

    qemuimg.create(path, size=size, format=format, qcow2Compat=qcow2_compat,
backing=backing)
Line 405:         qemu_pattern_write(path, format, offset="%dk" % index,
len='1k',


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 8:

(1 comment)

https://gerrit.ovirt.org/#/c/64222/8/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 193:     else:
Line 194:         # We don't want to empty the volume as a result of the commit
Line 195:         # operation. To do so, we provide the '-d' option when the base
Line 196:         # isn't provided. When the base volume is provided, the top volume
Line 197:         # will not be emptied after the commit operation.
Can we shorten this to one line?
Line 198:         # This is important mainly when we want to wipe the volume before
Line 199:         # deleting it. Emptying it may leave the data on the underlying
Line 200:         # storage.
Line 201:         cmd.append("-d")


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

gerrit-hooks has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


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.6',
'ovirt-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: core: Expose API for qemuimg commit

Ala Hino has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 8:

(12 comments)

https://gerrit.ovirt.org/#/c/64222/8/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 193:     else:
Line 194:         # We don't want to empty the volume as a result of the commit
Line 195:         # operation. To do so, we provide the '-d' option when the base
Line 196:         # isn't provided. When the base volume is provided, the top volume
Line 197:         # will not be emptied after the commit operation.
>  Can we shorten this to one line? 
I tried to ...
Line 198:         # This is important mainly when we want to wipe the volume before
Line 199:         # deleting it. Emptying it may leave the data on the underlying
Line 200:         # storage.
Line 201:         cmd.append("-d")


https://gerrit.ovirt.org/#/c/64222/8/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 334: 
Line 335: @expandPermutations
Line 336: class TestCommit(TestCaseBase):
Line 337: 
Line 338:     @MonkeyPatch(qemuimg, 'config', CONFIG)
>  We can remove the config mocking. 
Done
Line 339:     @permutations([
Line 340:         # chain_len, base_format, use_base
Line 341:         (1, qemuimg.FORMAT.RAW, False),
Line 342:         (1, qemuimg.FORMAT.RAW, True),


Line 343:         (1, qemuimg.FORMAT.QCOW2, False),
Line 344:         (1, qemuimg.FORMAT.QCOW2, True),
Line 345:         (3, qemuimg.FORMAT.RAW, True),
Line 346:         (3, qemuimg.FORMAT.QCOW2, True)
Line 347:     ])
>  Nice! 
:-)
Line 348:     def test_commit(self, chain_len, base_format, use_base=True):
Line 349:         size = 1048576
Line 350:         with namedTemporaryDir() as tmpdir:
Line 351:             # create base


Line 347:     ])
Line 348:     def test_commit(self, chain_len, base_format, use_base=True):
Line 349:         size = 1048576
Line 350:         with namedTemporaryDir() as tmpdir:
Line 351:             # create base
>  It would be nice to create base in the same loop as the rest of the
> volumes 
Done
Line 352:             base_vol = os.path.join(tmpdir, "base.img")
Line 353:             self.make_image(base_vol, size, base_format, 0)
Line 354: 
Line 355:             # create chain


Line 349:         size = 1048576
Line 350:         with namedTemporaryDir() as tmpdir:
Line 351:             # create base
Line 352:             base_vol = os.path.join(tmpdir, "base.img")
Line 353:             self.make_image(base_vol, size, base_format, 0)
>  We can add qcow2_compat argument here... 
Done
Line 354: 
Line 355:             # create chain
Line 356:             chain = []
Line 357:             parent = base_vol


Line 355:             # create chain
Line 356:             chain = []
Line 357:             parent = base_vol
Line 358:             top_format = qemuimg.FORMAT.QCOW2
Line 359:             for i in range(0, chain_len):
>  This can and should be written as: 
Done
Line 360:                 vol = os.path.join(tmpdir, "vol%d.img" % i)
Line 361:                 self.make_image(vol, size, top_format, i+1, parent)
Line 362:                 chain.append(vol)
Line 363:                 parent = vol


Line 363:                 parent = vol
Line 364: 
Line 365:             base = None
Line 366:             if use_base:
Line 367:                 base = base_vol
>  This works, but we can use more modern conditional expression:
Done
Line 368: 
Line 369:             op = qemuimg.commit(chain[-1], topFormat=top_format,
Line 370:                                 base=base)
Line 371:             op.wait_for_completion()


Line 366:             if use_base:
Line 367:                 base = base_vol
Line 368: 
Line 369:             op = qemuimg.commit(chain[-1], topFormat=top_format,
Line 370:                                 base=base)
>  Or we can just do: 
Done
Line 371:             op.wait_for_completion()
Line 372: 
Line 373:             # verify base data at offset 0 after commit
Line 374:             qemu_pattern_verify(base_vol, base_format, offset='0',


Line 371:             op.wait_for_completion()
Line 372: 
Line 373:             # verify base data at offset 0 after commit
Line 374:             qemu_pattern_verify(base_vol, base_format, offset='0',
Line 375:                                 len='1k', pattern=0xf0)
>  If you include base in chain, can verify all volume in the same loop.
Done
Line 376:             for i, vol in enumerate(chain):
Line 377:                 offset = "{}k".format(i+1)
Line 378:                 pattern = 0xf0 + (i+1)
Line 379:                 qemu_pattern_verify(base_vol, base_format, offset=offset,


Line 385:     @permutations([
Line 386:         # base_format
Line 387:         (qemuimg.FORMAT.RAW,),
Line 388:         (qemuimg.FORMAT.QCOW2,)
Line 389:     ])
>  We don't need permutations for this test, and we don't care
> about the confi 
Done
Line 390:     def test_commit_progress(self, base_format):
Line 391:         with namedTemporaryDir() as tmpdir:
Line 392:             size = 1048576
Line 393:             base = os.path.join(tmpdir, "base.img")


Line 399:             op = qemuimg.commit(top, topFormat=qemuimg.FORMAT.QCOW2)
Line 400:             op.wait_for_completion()
Line 401:             self.assertEquals(100, op.progress)
Line 402: 
Line 403:     def make_image(self, path, size, format, index, backing=None):
>  We don't need self for this, this will be more useable as a
> function in the 
Done
Line 404:         qemuimg.create(path, size=size, format=format, backing=backing)
Line 405:         qemu_pattern_write(path, format, offset="%dk" % index,
len='1k',


Line 400:             op.wait_for_completion()
Line 401:             self.assertEquals(100, op.progress)
Line 402: 
Line 403:     def make_image(self, path, size, format, index, backing=None):
Line 404:         qemuimg.create(path, size=size, format=format, backing=backing)
>  Specify the qcow2Compat: 
Done
Line 405:         qemu_pattern_write(path, format, offset="%dk" % index,
len='1k',


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 9:

(5 comments)

https://gerrit.ovirt.org/#/c/64222/9/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 391:             parent = None
Line 392:             for i in range(chain_len):
Line 393:                 vol = os.path.join(tmpdir, "vol%d.img" % i)
Line 394:                 format = base_format if i == 0 else qemuimg.FORMAT.QCOW2
Line 395:                 make_image(vol, size, format, i, qcow2_compat, parent)
We can take the size of each volume here:

    blocks = os.stat(vol).st_blocks

And append tuples (vol, blocks):

    chain.append((vol, blocks))
Line 396:                 chain.append(vol)
Line 397:                 parent = vol
Line 398: 
Line 399:             op = qemuimg.commit(chain[-1], topFormat=qemuimg.FORMAT.QCOW2,


Line 399:             op = qemuimg.commit(chain[-1], topFormat=qemuimg.FORMAT.QCOW2,
Line 400:                                 base=chain[0] if use_base else None)
Line 401:             op.wait_for_completion()
Line 402: 
Line 403:             for i, vol in enumerate(chain):
If we stored tuples, we get back:

    for i, (vol, blocks) in enumerate(chain):
        ...
Line 404:                 offset = "{}k".format(i)
Line 405:                 pattern = 0xf0 + (i)
Line 406:                 qemu_pattern_verify(chain[0], base_format, offset=offset,
Line 407:                                     len='1k', pattern=pattern)


Line 401:             op.wait_for_completion()
Line 402: 
Line 403:             for i, vol in enumerate(chain):
Line 404:                 offset = "{}k".format(i)
Line 405:                 pattern = 0xf0 + (i)
Why (i)? This is same as i.
Line 406:                 qemu_pattern_verify(chain[0], base_format, offset=offset,
Line 407:                                     len='1k', pattern=pattern)
Line 408:                 qemu_pattern_verify(vol, qemuimg.FORMAT.QCOW2, offset=offset,
Line 409:                                     len='1k', pattern=pattern)


Line 403:             for i, vol in enumerate(chain):
Line 404:                 offset = "{}k".format(i)
Line 405:                 pattern = 0xf0 + (i)
Line 406:                 qemu_pattern_verify(chain[0], base_format, offset=offset,
Line 407:                                     len='1k', pattern=pattern)
We need to explain this:

    # The base volume must have the data from all the volumes merged into it.
    check base...
Line 408:                 qemu_pattern_verify(vol, qemuimg.FORMAT.QCOW2, offset=offset,
Line 409:                                     len='1k', pattern=pattern)
Line 410: 
Line 411:     def test_commit_progress(self):


Line 405:                 pattern = 0xf0 + (i)
Line 406:                 qemu_pattern_verify(chain[0], base_format, offset=offset,
Line 407:                                     len='1k', pattern=pattern)
Line 408:                 qemu_pattern_verify(vol, qemuimg.FORMAT.QCOW2, offset=offset,
Line 409:                                     len='1k', pattern=pattern)
This check will always succeed, even if you forget to add the -d flag (try it).
When you verify a volume, you see the data in the base.

We can compare instead the number of used blocks on each file:

    if i > 0:
        # internal and top volumes should keep the data, we may want to
        # wipe this data when deleting the volumes later.
        self.assertEqual(os.stat(vol).st_blocks, blocks)
Line 410: 
Line 411:     def test_commit_progress(self):
Line 412:         with namedTemporaryDir() as tmpdir:
Line 413:             size = 1048576


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 9:

(1 comment)

https://gerrit.ovirt.org/#/c/64222/9/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 380:         (2, qemuimg.FORMAT.QCOW2, "0.10", True),
Line 381:         (4, qemuimg.FORMAT.RAW, "1.1", True),
Line 382:         (4, qemuimg.FORMAT.RAW, "0.10", True),
Line 383:         (4, qemuimg.FORMAT.QCOW2, "1.1", True),
Line 384:         (4, qemuimg.FORMAT.QCOW2, "0.10", True)
In all these options, we merge entire chain. It would be nice to test merge of a sub
chain.

For example, create chain of 4 volumes:

    vol0, vol1, vol2, vol3

And merge vol3 into vol1.

I would create the same chain in all the tests (4 volumes), and check different merges, so
the permutations can be:

    # base_format, qcow2_compat, top, base
    ("raw", "0.10", 3, 2),

This will merge chain[3] into chain[2].

For verification, you can check that chain[2] contains the entire data, and that chain[3]
blocks count did not change.
Line 385:     ])
Line 386:     def test_commit(self, chain_len, base_format, qcow2_compat,
Line 387:                     use_base=True):
Line 388:         size = 1048576


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

gerrit-hooks has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


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-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: core: Expose API for qemuimg commit

Ala Hino has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 9:

(6 comments)

https://gerrit.ovirt.org/#/c/64222/9/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 380:         (2, qemuimg.FORMAT.QCOW2, "0.10", True),
Line 381:         (4, qemuimg.FORMAT.RAW, "1.1", True),
Line 382:         (4, qemuimg.FORMAT.RAW, "0.10", True),
Line 383:         (4, qemuimg.FORMAT.QCOW2, "1.1", True),
Line 384:         (4, qemuimg.FORMAT.QCOW2, "0.10", True)
>  In all these options, we merge entire chain. It would be nice to test
> merge 
Done
Line 385:     ])
Line 386:     def test_commit(self, chain_len, base_format, qcow2_compat,
Line 387:                     use_base=True):
Line 388:         size = 1048576


Line 391:             parent = None
Line 392:             for i in range(chain_len):
Line 393:                 vol = os.path.join(tmpdir, "vol%d.img" % i)
Line 394:                 format = base_format if i == 0 else qemuimg.FORMAT.QCOW2
Line 395:                 make_image(vol, size, format, i, qcow2_compat, parent)
>  We can take the size of each volume here: 
Done
Line 396:                 chain.append(vol)
Line 397:                 parent = vol
Line 398: 
Line 399:             op = qemuimg.commit(chain[-1], topFormat=qemuimg.FORMAT.QCOW2,


Line 399:             op = qemuimg.commit(chain[-1], topFormat=qemuimg.FORMAT.QCOW2,
Line 400:                                 base=chain[0] if use_base else None)
Line 401:             op.wait_for_completion()
Line 402: 
Line 403:             for i, vol in enumerate(chain):
>  If we stored tuples, we get back: 
Done
Line 404:                 offset = "{}k".format(i)
Line 405:                 pattern = 0xf0 + (i)
Line 406:                 qemu_pattern_verify(chain[0], base_format, offset=offset,
Line 407:                                     len='1k', pattern=pattern)


Line 401:             op.wait_for_completion()
Line 402: 
Line 403:             for i, vol in enumerate(chain):
Line 404:                 offset = "{}k".format(i)
Line 405:                 pattern = 0xf0 + (i)
>  Why (i)? This is same as i. 
Done
Line 406:                 qemu_pattern_verify(chain[0], base_format, offset=offset,
Line 407:                                     len='1k', pattern=pattern)
Line 408:                 qemu_pattern_verify(vol, qemuimg.FORMAT.QCOW2, offset=offset,
Line 409:                                     len='1k', pattern=pattern)


Line 403:             for i, vol in enumerate(chain):
Line 404:                 offset = "{}k".format(i)
Line 405:                 pattern = 0xf0 + (i)
Line 406:                 qemu_pattern_verify(chain[0], base_format, offset=offset,
Line 407:                                     len='1k', pattern=pattern)
>  We need to explain this: 
Done
Line 408:                 qemu_pattern_verify(vol, qemuimg.FORMAT.QCOW2, offset=offset,
Line 409:                                     len='1k', pattern=pattern)
Line 410: 
Line 411:     def test_commit_progress(self):


Line 405:                 pattern = 0xf0 + (i)
Line 406:                 qemu_pattern_verify(chain[0], base_format, offset=offset,
Line 407:                                     len='1k', pattern=pattern)
Line 408:                 qemu_pattern_verify(vol, qemuimg.FORMAT.QCOW2, offset=offset,
Line 409:                                     len='1k', pattern=pattern)
>  This check will always succeed, even if you forget to add the -d flag
> (try  
Done
Line 410: 
Line 411:     def test_commit_progress(self):
Line 412:         with namedTemporaryDir() as tmpdir:
Line 413:             size = 1048576


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

gerrit-hooks has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


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-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: core: Expose API for qemuimg commit

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 11:

(15 comments)

https://gerrit.ovirt.org/#/c/64222/11/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 368: @expandPermutations
Line 369: class TestCommit(TestCaseBase):
Line 370: 
Line 371:     @permutations([
Line 372:         # base_format, qcow2_compat, base, top, use_base
Lets add comments explaining the use case:

    # Merging internal volume
Line 373:         (qemuimg.FORMAT.RAW, "1.1", 0, 1, False),
Line 374:         (qemuimg.FORMAT.RAW, "1.1", 0, 1, True),
Line 375:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, False),
Line 376:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, True),


Line 372:         # base_format, qcow2_compat, base, top, use_base
Line 373:         (qemuimg.FORMAT.RAW, "1.1", 0, 1, False),
Line 374:         (qemuimg.FORMAT.RAW, "1.1", 0, 1, True),
Line 375:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, False),
Line 376:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, True),
Add comment - seems like:

    # Merging entire chain into the base
Line 377:         (qemuimg.FORMAT.RAW, "1.1", 0, 4, True),
Line 378:         (qemuimg.FORMAT.RAW, "0.10", 0, 4, True),
Line 379:         (qemuimg.FORMAT.RAW, "1.1", 1, 2, True),
Line 380:         (qemuimg.FORMAT.RAW, "0.10", 1, 2, True),


Line 374:         (qemuimg.FORMAT.RAW, "1.1", 0, 1, True),
Line 375:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, False),
Line 376:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, True),
Line 377:         (qemuimg.FORMAT.RAW, "1.1", 0, 4, True),
Line 378:         (qemuimg.FORMAT.RAW, "0.10", 0, 4, True),
Comment:

    # Merging internal volume into its parent volume in cow format
Line 379:         (qemuimg.FORMAT.RAW, "1.1", 1, 2, True),
Line 380:         (qemuimg.FORMAT.RAW, "0.10", 1, 2, True),
Line 381:         (qemuimg.FORMAT.RAW, "1.1", 3, 4, True),
Line 382:         (qemuimg.FORMAT.RAW, "0.10", 3, 4, True),


Line 376:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, True),
Line 377:         (qemuimg.FORMAT.RAW, "1.1", 0, 4, True),
Line 378:         (qemuimg.FORMAT.RAW, "0.10", 0, 4, True),
Line 379:         (qemuimg.FORMAT.RAW, "1.1", 1, 2, True),
Line 380:         (qemuimg.FORMAT.RAW, "0.10", 1, 2, True),
This looks like the previous case (base=1, top=2) - do we need these permutations?
Line 381:         (qemuimg.FORMAT.RAW, "1.1", 3, 4, True),
Line 382:         (qemuimg.FORMAT.RAW, "0.10", 3, 4, True),
Line 383:         (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True),
Line 384:         (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True),


Line 378:         (qemuimg.FORMAT.RAW, "0.10", 0, 4, True),
Line 379:         (qemuimg.FORMAT.RAW, "1.1", 1, 2, True),
Line 380:         (qemuimg.FORMAT.RAW, "0.10", 1, 2, True),
Line 381:         (qemuimg.FORMAT.RAW, "1.1", 3, 4, True),
Line 382:         (qemuimg.FORMAT.RAW, "0.10", 3, 4, True),
This is different since we merge a subchain.
Line 383:         (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True),
Line 384:         (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True),
Line 385:     ])
Line 386:     def test_commit(self, base_format, qcow2_compat, base, top,


Line 380:         (qemuimg.FORMAT.RAW, "0.10", 1, 2, True),
Line 381:         (qemuimg.FORMAT.RAW, "1.1", 3, 4, True),
Line 382:         (qemuimg.FORMAT.RAW, "0.10", 3, 4, True),
Line 383:         (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True),
Line 384:         (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True),
Do we really care about the case when the first volume is not raw? we can simplify the
tests by using always raw format for volume 0.
Line 385:     ])
Line 386:     def test_commit(self, base_format, qcow2_compat, base, top,
Line 387:                     use_base=True):
Line 388:         size = 1048576


Line 382:         (qemuimg.FORMAT.RAW, "0.10", 3, 4, True),
Line 383:         (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True),
Line 384:         (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True),
Line 385:     ])
Line 386:     def test_commit(self, base_format, qcow2_compat, base, top,
We cannot call this base_format when base is some volume in the middle. We need another
name.
Line 387:                     use_base=True):
Line 388:         size = 1048576
Line 389:         with namedTemporaryDir() as tmpdir:
Line 390:             chain = []


Line 383:         (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True),
Line 384:         (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True),
Line 385:     ])
Line 386:     def test_commit(self, base_format, qcow2_compat, base, top,
Line 387:                     use_base=True):
We don't need defaults, the permutations set the value.
Line 388:         size = 1048576
Line 389:         with namedTemporaryDir() as tmpdir:
Line 390:             chain = []
Line 391:             parent = None


Line 388:         size = 1048576
Line 389:         with namedTemporaryDir() as tmpdir:
Line 390:             chain = []
Line 391:             parent = None
Line 392:             # Create a chain of 5 volumes.
Better use only 4 volumes, will make the tests little faster.
Line 393:             for i in range(0, 5):
Line 394:                 vol = os.path.join(tmpdir, "vol%d.img" % i)
Line 395:                 format = base_format if i == 0 else qemuimg.FORMAT.QCOW2
Line 396:                 make_image(vol, size, format, i, qcow2_compat, parent)


Line 389:         with namedTemporaryDir() as tmpdir:
Line 390:             chain = []
Line 391:             parent = None
Line 392:             # Create a chain of 5 volumes.
Line 393:             for i in range(0, 5):
range(0, 5) is equal to range(5), and more idomatic.
Line 394:                 vol = os.path.join(tmpdir, "vol%d.img" % i)
Line 395:                 format = base_format if i == 0 else qemuimg.FORMAT.QCOW2
Line 396:                 make_image(vol, size, format, i, qcow2_compat, parent)
Line 397:                 blocks = os.stat(vol).st_blocks


Line 402:                                 topFormat=qemuimg.FORMAT.QCOW2,
Line 403:                                 base=chain[base][0] if use_base else None)
Line 404:             op.wait_for_completion()
Line 405: 
Line 406:             for i in range(base, top):
Nice!

But it will never return top - we need range(base, top + 1)
Line 407:                 offset = "{}k".format(i)
Line 408:                 pattern = 0xf0 + i
Line 409:                 # The base volume must have the data from all the volumes
Line 410:                 # merged into it.


Line 407:                 offset = "{}k".format(i)
Line 408:                 pattern = 0xf0 + i
Line 409:                 # The base volume must have the data from all the volumes
Line 410:                 # merged into it.
Line 411:                 format = base_format if i == 0 else qemuimg.FORMAT.QCOW2
Removing base_format and using always raw would make this simpler.
Line 412:                 qemu_pattern_verify(chain[base][0], format, offset=offset,
Line 413:                                     len='1k', pattern=pattern)
Line 414:                 if i > 0:
Line 415:                     # internal and top volumes should keep the data, we


Line 408:                 pattern = 0xf0 + i
Line 409:                 # The base volume must have the data from all the volumes
Line 410:                 # merged into it.
Line 411:                 format = base_format if i == 0 else qemuimg.FORMAT.QCOW2
Line 412:                 qemu_pattern_verify(chain[base][0], format, offset=offset,
chain[base][0] makes the code harder to follow. Since base is constant, we can do this
before the loop:

    base_vol = chain[base][0]

And use base_vol here.
Line 413:                                     len='1k', pattern=pattern)
Line 414:                 if i > 0:
Line 415:                     # internal and top volumes should keep the data, we
Line 416:                     # may want to wipe this data when deleting the volumes


Line 410:                 # merged into it.
Line 411:                 format = base_format if i == 0 else qemuimg.FORMAT.QCOW2
Line 412:                 qemu_pattern_verify(chain[base][0], format, offset=offset,
Line 413:                                     len='1k', pattern=pattern)
Line 414:                 if i > 0:
Should be i > base.
Line 415:                     # internal and top volumes should keep the data, we
Line 416:                     # may want to wipe this data when deleting the volumes
Line 417:                     # later.
Line 418:                     self.assertEqual(os.stat(chain[i][0]).st_blocks,


Line 415:                     # internal and top volumes should keep the data, we
Line 416:                     # may want to wipe this data when deleting the volumes
Line 417:                     # later.
Line 418:                     self.assertEqual(os.stat(chain[i][0]).st_blocks,
Line 419:                                      chain[i][1])
This should work, but we can be more clear.

    vol, blocks = chain[i]
    self.assertEqual(os.stat(vol).st_blocks, blocks)
Line 420: 
Line 421:     def test_commit_progress(self):
Line 422:         with namedTemporaryDir() as tmpdir:
Line 423:             size = 1048576


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

gerrit-hooks has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 12:

* 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-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: core: Expose API for qemuimg commit

Ala Hino has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 11:

(15 comments)

https://gerrit.ovirt.org/#/c/64222/11/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 368: @expandPermutations
Line 369: class TestCommit(TestCaseBase):
Line 370: 
Line 371:     @permutations([
Line 372:         # base_format, qcow2_compat, base, top, use_base
>  Lets add comments explaining the use case: 
Done
Line 373:         (qemuimg.FORMAT.RAW, "1.1", 0, 1, False),
Line 374:         (qemuimg.FORMAT.RAW, "1.1", 0, 1, True),
Line 375:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, False),
Line 376:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, True),


Line 372:         # base_format, qcow2_compat, base, top, use_base
Line 373:         (qemuimg.FORMAT.RAW, "1.1", 0, 1, False),
Line 374:         (qemuimg.FORMAT.RAW, "1.1", 0, 1, True),
Line 375:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, False),
Line 376:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, True),
>  Add comment - seems like: 
Done
Line 377:         (qemuimg.FORMAT.RAW, "1.1", 0, 4, True),
Line 378:         (qemuimg.FORMAT.RAW, "0.10", 0, 4, True),
Line 379:         (qemuimg.FORMAT.RAW, "1.1", 1, 2, True),
Line 380:         (qemuimg.FORMAT.RAW, "0.10", 1, 2, True),


Line 374:         (qemuimg.FORMAT.RAW, "1.1", 0, 1, True),
Line 375:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, False),
Line 376:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, True),
Line 377:         (qemuimg.FORMAT.RAW, "1.1", 0, 4, True),
Line 378:         (qemuimg.FORMAT.RAW, "0.10", 0, 4, True),
>  Comment: 
Done
Line 379:         (qemuimg.FORMAT.RAW, "1.1", 1, 2, True),
Line 380:         (qemuimg.FORMAT.RAW, "0.10", 1, 2, True),
Line 381:         (qemuimg.FORMAT.RAW, "1.1", 3, 4, True),
Line 382:         (qemuimg.FORMAT.RAW, "0.10", 3, 4, True),


Line 376:         (qemuimg.FORMAT.RAW, "0.10", 0, 1, True),
Line 377:         (qemuimg.FORMAT.RAW, "1.1", 0, 4, True),
Line 378:         (qemuimg.FORMAT.RAW, "0.10", 0, 4, True),
Line 379:         (qemuimg.FORMAT.RAW, "1.1", 1, 2, True),
Line 380:         (qemuimg.FORMAT.RAW, "0.10", 1, 2, True),
>  This looks like the previous case (base=1, top=2) - do we need these
> permut 
Done
Line 381:         (qemuimg.FORMAT.RAW, "1.1", 3, 4, True),
Line 382:         (qemuimg.FORMAT.RAW, "0.10", 3, 4, True),
Line 383:         (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True),
Line 384:         (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True),


Line 378:         (qemuimg.FORMAT.RAW, "0.10", 0, 4, True),
Line 379:         (qemuimg.FORMAT.RAW, "1.1", 1, 2, True),
Line 380:         (qemuimg.FORMAT.RAW, "0.10", 1, 2, True),
Line 381:         (qemuimg.FORMAT.RAW, "1.1", 3, 4, True),
Line 382:         (qemuimg.FORMAT.RAW, "0.10", 3, 4, True),
>  This is different since we merge a subchain. 
Done
Line 383:         (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True),
Line 384:         (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True),
Line 385:     ])
Line 386:     def test_commit(self, base_format, qcow2_compat, base, top,


Line 380:         (qemuimg.FORMAT.RAW, "0.10", 1, 2, True),
Line 381:         (qemuimg.FORMAT.RAW, "1.1", 3, 4, True),
Line 382:         (qemuimg.FORMAT.RAW, "0.10", 3, 4, True),
Line 383:         (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True),
Line 384:         (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True),
>  Do we really care about the case when the first volume is not raw? we
> can s 
Done
Line 385:     ])
Line 386:     def test_commit(self, base_format, qcow2_compat, base, top,
Line 387:                     use_base=True):
Line 388:         size = 1048576


Line 382:         (qemuimg.FORMAT.RAW, "0.10", 3, 4, True),
Line 383:         (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True),
Line 384:         (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True),
Line 385:     ])
Line 386:     def test_commit(self, base_format, qcow2_compat, base, top,
>  We cannot call this base_format when base is some volume in the
> middle. We  
Now that I use RAW format for base volumes, I don't this arg
anymore
Line 387:                     use_base=True):
Line 388:         size = 1048576
Line 389:         with namedTemporaryDir() as tmpdir:
Line 390:             chain = []


Line 383:         (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True),
Line 384:         (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True),
Line 385:     ])
Line 386:     def test_commit(self, base_format, qcow2_compat, base, top,
Line 387:                     use_base=True):
>  We don't need defaults, the permutations set the value.
Done
Line 388:         size = 1048576
Line 389:         with namedTemporaryDir() as tmpdir:
Line 390:             chain = []
Line 391:             parent = None


Line 388:         size = 1048576
Line 389:         with namedTemporaryDir() as tmpdir:
Line 390:             chain = []
Line 391:             parent = None
Line 392:             # Create a chain of 5 volumes.
>  Better use only 4 volumes, will make the tests little faster.
Done
Line 393:             for i in range(0, 5):
Line 394:                 vol = os.path.join(tmpdir, "vol%d.img" % i)
Line 395:                 format = base_format if i == 0 else qemuimg.FORMAT.QCOW2
Line 396:                 make_image(vol, size, format, i, qcow2_compat, parent)


Line 389:         with namedTemporaryDir() as tmpdir:
Line 390:             chain = []
Line 391:             parent = None
Line 392:             # Create a chain of 5 volumes.
Line 393:             for i in range(0, 5):
>  range(0, 5) is equal to range(5), and more idomatic. 
Done
Line 394:                 vol = os.path.join(tmpdir, "vol%d.img" % i)
Line 395:                 format = base_format if i == 0 else qemuimg.FORMAT.QCOW2
Line 396:                 make_image(vol, size, format, i, qcow2_compat, parent)
Line 397:                 blocks = os.stat(vol).st_blocks


Line 402:                                 topFormat=qemuimg.FORMAT.QCOW2,
Line 403:                                 base=chain[base][0] if use_base else None)
Line 404:             op.wait_for_completion()
Line 405: 
Line 406:             for i in range(base, top):
>  Nice! 
Done
Line 407:                 offset = "{}k".format(i)
Line 408:                 pattern = 0xf0 + i
Line 409:                 # The base volume must have the data from all the volumes
Line 410:                 # merged into it.


Line 407:                 offset = "{}k".format(i)
Line 408:                 pattern = 0xf0 + i
Line 409:                 # The base volume must have the data from all the volumes
Line 410:                 # merged into it.
Line 411:                 format = base_format if i == 0 else qemuimg.FORMAT.QCOW2
>  Removing base_format and using always raw would make this simpler.
Done
Line 412:                 qemu_pattern_verify(chain[base][0], format, offset=offset,
Line 413:                                     len='1k', pattern=pattern)
Line 414:                 if i > 0:
Line 415:                     # internal and top volumes should keep the data, we


Line 408:                 pattern = 0xf0 + i
Line 409:                 # The base volume must have the data from all the volumes
Line 410:                 # merged into it.
Line 411:                 format = base_format if i == 0 else qemuimg.FORMAT.QCOW2
Line 412:                 qemu_pattern_verify(chain[base][0], format, offset=offset,
>  chain[base][0] makes the code harder to follow. Since base is
> constant, we  
Done
Line 413:                                     len='1k', pattern=pattern)
Line 414:                 if i > 0:
Line 415:                     # internal and top volumes should keep the data, we
Line 416:                     # may want to wipe this data when deleting the volumes


Line 410:                 # merged into it.
Line 411:                 format = base_format if i == 0 else qemuimg.FORMAT.QCOW2
Line 412:                 qemu_pattern_verify(chain[base][0], format, offset=offset,
Line 413:                                     len='1k', pattern=pattern)
Line 414:                 if i > 0:
>  Should be i > base. 
Done
Line 415:                     # internal and top volumes should keep the data, we
Line 416:                     # may want to wipe this data when deleting the volumes
Line 417:                     # later.
Line 418:                     self.assertEqual(os.stat(chain[i][0]).st_blocks,


Line 415:                     # internal and top volumes should keep the data, we
Line 416:                     # may want to wipe this data when deleting the volumes
Line 417:                     # later.
Line 418:                     self.assertEqual(os.stat(chain[i][0]).st_blocks,
Line 419:                                      chain[i][1])
>  This should work, but we can be more clear. 
Done
Line 420: 
Line 421:     def test_commit_progress(self):
Line 422:         with namedTemporaryDir() as tmpdir:
Line 423:             size = 1048576


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 12: Code-Review+1 Continuous-Integration+1

Thanks Ala, excellent work!

Network tests failing again.

Waiting for Adam ack.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: core: Expose API for qemuimg commit

Ala Hino has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 12: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: core: Expose API for qemuimg commit

Adam Litke has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 12: Code-Review+2

(2 comments)

https://gerrit.ovirt.org/#/c/64222/12/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 205:     cmd.append(top)
Line 206: 
Line 207:     # For simplicity, we always run commit in the image directory.
Line 208:     workdir = os.path.dirname(top)
Line 209:     return QemuImgOperation(cmd, cwd=workdir)
Have you tested that progress output is the same also for this command?  If so, cool!
Line 210: 
Line 211: 
Line 212: class QemuImgOperation(object):
Line 213:     REGEXPR = re.compile(r'\s*\(([\d.]+)/100%\)\s*')


https://gerrit.ovirt.org/#/c/64222/12/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 433:             make_image(top, size, qemuimg.FORMAT.QCOW2, 1, "1.1",
base)
Line 434: 
Line 435:             op = qemuimg.commit(top, topFormat=qemuimg.FORMAT.QCOW2)
Line 436:             op.wait_for_completion()
Line 437:             self.assertEquals(100, op.progress)
Cool!
Line 438: 
Line 439: 
Line 440: def make_image(path, size, format, index, qcow2_compat, backing=None):
Line 441:     qemuimg.create(path, size=size, format=format, qcow2Compat=qcow2_compat,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Expose API for qemuimg commit

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 12: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: core: Expose API for qemuimg commit

[Nir Soffer]

Nir Soffer has submitted this change and it was merged.

Change subject: core: Expose API for qemuimg commit
......................................................................


core: Expose API for qemuimg commit

Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Signed-off-by: Ala Hino <ahino(a)redhat.com&gt;
Reviewed-on: https://gerrit.ovirt.org/64222
Continuous-Integration: Nir Soffer <nsoffer(a)redhat.com&gt;
Reviewed-by: Adam Litke <alitke(a)redhat.com&gt;
Reviewed-by: Nir Soffer <nsoffer(a)redhat.com&gt;
---
M lib/vdsm/qemuimg.py
M tests/qemuimg_test.py
2 files changed, 101 insertions(+), 0 deletions(-)

Approvals:
  Adam Litke: Looks good to me, approved
  Nir Soffer: Looks good to me, approved; Passed CI tests
  Ala Hino: Verified

Objections:
  Jenkins CI: Failed CI tests



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

Gerrit-MessageType: merged
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;

Change in vdsm[master]: core: Expose API for qemuimg commit

gerrit-hooks has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
......................................................................


Patch Set 13:

* Update tracker: IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No