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

[automation]

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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[nsoffer]

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 
Reviewed-on: https://gerrit.ovirt.org/64222
Continuous-Integration: Nir Soffer 
Reviewed-by: Adam Litke 
Reviewed-by: Nir Soffer 
---
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[nsoffer]

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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[alitke]

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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[ahino]

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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[nsoffer]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[automation]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[ahino]

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 

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

[nsoffer]

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:  

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

[automation]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[ahino]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[automation]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[nsoffer]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[nsoffer]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[automation]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[ahino]

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 

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

[nsoffer]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[nsoffer]

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 

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

[automation]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[automation]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[ahino]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[nsoffer]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[nsoffer]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[automation]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[ahino]

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 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[automation]

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 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org