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>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes