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