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