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