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