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',