Change in vdsm[master]: qemuimg: Expose API for qemuimg map

[ahino]

Ala Hino has posted comments on this change.

Change subject: qemuimg: Expose API for qemuimg map
..


Patch Set 5:

(7 comments)

https://gerrit.ovirt.org/#/c/65112/5/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 459: # Empty run
Line 460: run = img_map[0]
Line 461: self.assertEqual(run["length"], size)
Line 462: self.assertEqual(run["data"], False)
Line 463: self.assertEqual(run["zero"], True)
> Lets use the same format as in the other tests, and call the new shiny help
Done
Line 464: 
Line 465: @permutations([
Line 466: # offset, qcow2_compat
Line 467: (4 * 1024, "0.10"),


Line 462: self.assertEqual(run["data"], False)
Line 463: self.assertEqual(run["zero"], True)
Line 464: 
Line 465: @permutations([
Line 466: # offset, qcow2_compat
> This is length, not offset.
Done
Line 467: (4 * 1024, "0.10"),
Line 468: (4 * 1024, "1.1"),
Line 469: (64 * 1024, "0.10"),
Line 470: (64 * 1024, "1.1"),


Line 479: qemu_pattern_write(image, fmt, offset=offset, len=length,
Line 480:pattern=0xf0)
Line 481: 
Line 482: img_map = qemuimg.map(image)
Line 483: self.assertEqual(3, len(img_map))
> We can move this check into the helper:
Done
Line 484: 
Line 485: expected = [
Line 486: # run 1 - empty
Line 487: {


Line 508: ]
Line 509: 
Line 510: for actual, expected in zip(img_map, expected):
Line 511: for key in expected:
Line 512: self.assertEqual(actual[key], expected[key])
> We have same verification code twice, and we actually want to use it for th
Done
Line 513: 
Line 514: @permutations([
Line 515: # offset, length, expected_length, expected_start, 
qcow2_compat
Line 516: (64 * 1024, 4 * 1024, 65536, "0.10"),


Line 528: qemu_pattern_write(image, fmt, offset=offset, len=length,
Line 529:pattern=0xf0)
Line 530: 
Line 531: img_map = qemuimg.map(image)
Line 532: self.assertEqual(3, len(img_map))
> Move to check_map helper
Done
Line 533: 
Line 534: expected = [
Line 535: # run 1 - empty
Line 536: {


Line 541: },
Line 542: # run 2 - data
Line 543: {
Line 544: "start": offset,
Line 545: "offset": 327680,
> Lets add a comment abut this (no clue what is this value), or just remove i
Removed ...
Line 546: "length": expected_length,
Line 547: "data": True,
Line 548: "zero": False,
Line 549: },


Line 557: ]
Line 558: 
Line 559: for actual, expected in zip(img_map, expected):
Line 560: for key in expected:
Line 561: self.assertEqual(actual[key], expected[key])
> Repalce wiith call to check_map
Done
Line 562: 
Line 563: 
Line 564: def make_image(path, size, format, index, qcow2_compat, backing=None):
Line 565: qemuimg.create(path, size=size, format=format, 
qcow2Compat=qcow2_compat,


-- 
To view, visit https://gerrit.ovirt.org/65112
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d6c936239d3dcc7a3d236dbaab0a93501ada8c
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Expose API for qemuimg map

[ahino]

Ala Hino has posted comments on this change.

Change subject: qemuimg: Expose API for qemuimg map
..


Patch Set 4:

(4 comments)

https://gerrit.ovirt.org/#/c/65112/4/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 497: # Empty run
Line 498: run = img_map[2]
Line 499: self.assertEqual(run["length"], size - offset - length)
Line 500: self.assertEqual(run["data"], False)
Line 501: self.assertEqual(run["zero"], True)
> Please use the same style as in the qcow2 test for verifying.
Done
Line 502: 
Line 503: @permutations([
Line 504: # offset, length, expected_length, expected_start, 
qcow2_compat
Line 505: (64 * 1024, 4 * 1024, 65536, 131072, "0.10"),


Line 530: },
Line 531: # run 2 - data
Line 532: {
Line 533: "start": offset,
Line 534: "offset": 327680,
> Do you have a clue about this value?
No idea :(
Line 535: "length": expected_length,
Line 536: "data": True,
Line 537: "zero": False,
Line 538: },


Line 537: "zero": False,
Line 538: },
Line 539: # run 3 - empty
Line 540: {
Line 541: "start": expected_start,
> Isn't this offset + expected_length?
Fixed
Line 542: "length": size - offset - expected_length,
Line 543: "data": False,
Line 544: "zero": True,
Line 545: },


Line 540: {
Line 541: "start": expected_start,
Line 542: "length": size - offset - expected_length,
Line 543: "data": False,
Line 544: "zero": True,
> Do we have an offsect in this run? what is the value?
No offset here
Line 545: },
Line 546: ]
Line 547: 
Line 548: for actual, expected in zip(img_map, expected):


-- 
To view, visit https://gerrit.ovirt.org/65112
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d6c936239d3dcc7a3d236dbaab0a93501ada8c
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Expose API for qemuimg map

[ahino]

Ala Hino has posted comments on this change.

Change subject: qemuimg: Expose API for qemuimg map
..


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/65112/3//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2016-10-05 12:57:15 +0300
Line 4: Commit: Ala Hino 
Line 5: CommitDate: 2016-10-10 12:08:27 +0300
Line 6: 
Line 7: core: Expose API for qemuimg map
> Use qemuimg: prefix for such changes. There is no "core".
Done
Line 8: 
Line 9: Change-Id: I54d6c936239d3dcc7a3d236dbaab0a93501ada8c
Line 10: Signed-off-by: Nir Soffer 


-- 
To view, visit https://gerrit.ovirt.org/65112
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d6c936239d3dcc7a3d236dbaab0a93501ada8c
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: core: Expose API for qemuimg map

[ahino]

Ala Hino has posted comments on this change.

Change subject: core: Expose API for qemuimg map
..


Patch Set 2:

(8 comments)

https://gerrit.ovirt.org/#/c/65112/2/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 219: 
Line 220: try:
Line 221: return json.loads(out)
Line 222: except ValueError:
Line 223: raise QImgError(rc, out, err, "Failed to process qemuimg map 
output")
> Please base this on https://gerrit.ovirt.org/65178 so we can use the new sh
Done
Line 224: 
Line 225: 
Line 226: class QemuImgOperation(object):
Line 227: REGEXPR = re.compile(r'\s*\(([\d.]+)/100%\)\s*')


https://gerrit.ovirt.org/#/c/65112/2/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 459: # Empty run
Line 460: run = img_map[0]
Line 461: self.assertEqual(run["length"], size)
Line 462: self.assertFalse(run["data"])
Line 463: self.assertTrue(run["zero"])
> Lets use assertEqual to match the next tests. Using assertTrue and assertFa
Done
Line 464: 
Line 465: @permutations([
Line 466: # offset, length, expected_offset, expected_length, 
qcow2_compat
Line 467: (64 * 1024, 4 * 1024, 65536, 65536, "0.10"),


Line 466: # offset, length, expected_offset, expected_length, 
qcow2_compat
Line 467: (64 * 1024, 4 * 1024, 65536, 65536, "0.10"),
Line 468: (64 * 1024, 4 * 1024, 65536, 65536, "1.1"),
Line 469: (64 * 1024, 72 * 1024, 65536, 131072, "0.10"),
Line 470: (64 * 1024, 72 * 1024, 65536, 131072, "1.1"),
> offset and expected_offset are always the same - are you sure we need both?
Removed expected_offset
Line 471: ])
Line 472: def test_one_block_qcow2(self, offset, length, expected_offset,
Line 473:  expected_length, qcow2_compat):
Line 474: with namedTemporaryDir() as tmpdir:


Line 498: run = img_map[2]
Line 499: self.assertEqual(run["length"],
Line 500:  size-expected_offset-expected_length)
Line 501: self.assertEqual(run["data"], False)
Line 502: self.assertEqual(run["zero"], True)
> Lets apply the same comments from the raw version to this test.
Done
Line 503: 
Line 504: @permutations([
Line 505: # offset, qcow2_compat
Line 506: (4 * 1024, "0.10"),


Line 507: (4 * 1024, "1.1"),
Line 508: (64 * 1024, "0.10"),
Line 509: (64 * 1024, "1.1"),
Line 510: ])
Line 511: def test_one_block_raw(self, length, qcow2_compat):
> Put the raw test before the qcow2 tests since it is the simpler case.
Done
Line 512: with namedTemporaryDir() as tmpdir:
Line 513: image = os.path.join(tmpdir, "base.img")
Line 514: fmt = qemuimg.FORMAT.RAW
Line 515: size = 1048576


Line 513: image = os.path.join(tmpdir, "base.img")
Line 514: fmt = qemuimg.FORMAT.RAW
Line 515: size = 1048576
Line 516: qemuimg.create(image, size=size, format=fmt)
Line 517: offset = 64 * 1024
> Lets put the size and the offset at the top of the tests, since they are us
Done
Line 518: qemu_pattern_write(image, fmt, offset=offset, len=length,
Line 519:pattern=0xf0)
Line 520: 
Line 521: img_map = qemuimg.map(image)


Line 534: self.assertEqual(run["zero"], False)
Line 535: 
Line 536: # Empty run
Line 537: run = img_map[2]
Line 538: self.assertEqual(run["length"], size-offset-length)
> Use space between binary operators:
Done
Line 539: self.assertEqual(run["data"], False)
Line 540: self.assertEqual(run["zero"], True)
Line 541: 
Line 542: 


Line 536: # Empty run
Line 537: run = img_map[2]
Line 538: self.assertEqual(run["length"], size-offset-length)
Line 539: self.assertEqual(run["data"], False)
Line 540: self.assertEqual(run["zero"], True)
> I wonder if we can simplify the tests like this:
Done
Line 541: 
Line 542: 
Line 543: def make_image(path, size, format, index, qcow2_compat, backing=None):
Line 544: qemuimg.create(path, size=size, format=format, 
qcow2Compat=qcow2_compat,


-- 
To view, visit https://gerrit.ovirt.org/65112
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d6c936239d3dcc7a3d236dbaab0a93501ada8c
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Introduce InvalidOutput exception

[ahino]

Ala Hino has posted comments on this change.

Change subject: qemuimg: Introduce InvalidOutput exception
..


Patch Set 6: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/65208
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If3f801f05f130c2417e4946877e31030260269a1
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Add the command to QImgError

[ahino]

Ala Hino has posted comments on this change.

Change subject: qemuimg: Add the command to QImgError
..


Patch Set 6: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/65179
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I226a54b29ae4afc3854056efea78767383b89619
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Add wrapper to execute command

[ahino]

Ala Hino has posted comments on this change.

Change subject: qemuimg: Add wrapper to execute command
..


Patch Set 7: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/65178
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a202f9543098ad75e8be8bf4b398d26c8e4a
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Add the command to QImgError

[ahino]

Ala Hino has posted comments on this change.

Change subject: qemuimg: Add the command to QImgError
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/65179/5/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 223: self._stderr = bytearray()
Line 224: 
Line 225: self.cmd = cmdutils.wrap_command(cmd,
Line 226:  
with_nice=utils.NICENESS.HIGH,
Line 227:  
with_ioclass=utils.IOCLASS.IDLE)
> And indent all the params one per line, starting at column 5.
Done
Line 228: _log.debug(cmdutils.command_log_line(self.cmd, cwd=cwd))
Line 229: self._command = CPopen(self.cmd, cwd=cwd,
Line 230:deathSignal=signal.SIGKILL)
Line 231: self._stream = utils.CommandStream(


-- 
To view, visit https://gerrit.ovirt.org/65179
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I226a54b29ae4afc3854056efea78767383b89619
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Introduce InvalidOutput exception

[ahino]

Ala Hino has posted comments on this change.

Change subject: qemuimg: Introduce InvalidOutput exception
..


Patch Set 3:

(3 comments)

https://gerrit.ovirt.org/#/c/65208/3/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 67: 
Line 68: class InvalidOutput(QImgError):
Line 69: """
Line 70: Raised when the command output is not valid.
Line 71: """
> You wan to use class attributes (like java static member):
Done
Line 72: 
Line 73: def __init__(self, cmd, stdout, message):
Line 74: self.cmd = cmd
Line 75: self.ecode = 0


Line 71: """
Line 72: 
Line 73: def __init__(self, cmd, stdout, message):
Line 74: self.cmd = cmd
Line 75: self.ecode = 0
> This creates an instance variable with constant value for each instance.
Done
Line 76: self.stdout = stdout
Line 77: self.stderr = ""
Line 78: self.message = message
Line 79: 


Line 73: def __init__(self, cmd, stdout, message):
Line 74: self.cmd = cmd
Line 75: self.ecode = 0
Line 76: self.stdout = stdout
Line 77: self.stderr = ""
> Same
Done
Line 78: self.message = message
Line 79: 
Line 80: def __str__(self):
Line 81: return "cmd=%s, stdout=%s, message=%s" % (


-- 
To view, visit https://gerrit.ovirt.org/65208
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If3f801f05f130c2417e4946877e31030260269a1
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Introduce InvalidOutput exception

[ahino]

Ala Hino has posted comments on this change.

Change subject: qemuimg: Introduce InvalidOutput exception
..


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/65208/1/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 67: 
Line 68: class InvalidOutput(QImgError):
Line 69: """
Line 70: Raised when the command output is not valid.
Line 71: """
> We can do this ugly hack to make sure that no client fail to get ecode and 
Done
Line 72: 
Line 73: def __init__(self, cmd, stdout, message):
Line 74: self.cmd = cmd
Line 75: self.stdout = stdout


-- 
To view, visit https://gerrit.ovirt.org/65208
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If3f801f05f130c2417e4946877e31030260269a1
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Add the command to QImgError

[ahino]

Ala Hino has posted comments on this change.

Change subject: qemuimg: Add the command to QImgError
..


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/65179/3/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 218
Line 219
Line 220
Line 221
Line 222
> You want to save the wrapped command, not the original.
Done


-- 
To view, visit https://gerrit.ovirt.org/65179
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I226a54b29ae4afc3854056efea78767383b89619
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Introduce InvalidOutput exception

[ahino]

Ala Hino has posted comments on this change.

Change subject: qemuimg: Introduce InvalidOutput exception
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/65208/1//COMMIT_MSG
Commit Message:

Line 5: CommitDate: 2016-10-06 21:31:05 +0300
Line 6: 
Line 7: qemuimg: Introduce InvalidOutput exception
Line 8: 
Line 9: Raised when the command output is not valid.
> Lets give some more context:
Done
Line 10: 
Line 11: Change-Id: If3f801f05f130c2417e4946877e31030260269a1


-- 
To view, visit https://gerrit.ovirt.org/65208
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If3f801f05f130c2417e4946877e31030260269a1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Add wrapper to execute command

[ahino]

Ala Hino has posted comments on this change.

Change subject: qemuimg: Add wrapper to execute command
..


Patch Set 2:

(3 comments)

https://gerrit.ovirt.org/#/c/65178/2/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 86: if format:
Line 87: cmd.extend(("-f", format))
Line 88: 
Line 89: cmd.append(image)
Line 90: out = _run_cmd(cmd, raw=True)
> raw=True is not needed, it enforced by _run_cmd.
Done
Line 91: 
Line 92: try:
Line 93: qemu_info = _parse_qemuimg_json(out)
Line 94: except ValueError:


Line 148: if format:
Line 149: cmd.extend(("-f", format))
Line 150: 
Line 151: cmd.append(image)
Line 152: out = _run_cmd(cmd, raw=True)
> raw=True is not needed, it enforced by _run_cmd.
Done
Line 153: 
Line 154: try:
Line 155: qemu_check = _parse_qemuimg_json(out)
Line 156: except ValueError:


Line 360: 
Line 361: def _run_cmd(cmd, cwd=None):
Line 362: rc, out, err = commands.execCmd(cmd, raw=True, cwd=cwd)
Line 363: if rc != 0:
Line 364: raise QImgError(rc, out, err)
> cmd missing
Done


-- 
To view, visit https://gerrit.ovirt.org/65178
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a202f9543098ad75e8be8bf4b398d26c8e4a
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Add the command to QImgError

[ahino]

Ala Hino has posted comments on this change.

Change subject: qemuimg: Add the command to QImgError
..


Patch Set 2:

> (1 comment)

how do I get the cmd from the CPopen instance?

-- 
To view, visit https://gerrit.ovirt.org/65179
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I226a54b29ae4afc3854056efea78767383b89619
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Introduce InvalidOutput exception

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: qemuimg: Introduce InvalidOutput exception
..

qemuimg: Introduce InvalidOutput exception

Raised when the command output is not valid.

Change-Id: If3f801f05f130c2417e4946877e31030260269a1
Signed-off-by: Ala Hino 
---
M lib/vdsm/qemuimg.py
1 file changed, 20 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/08/65208/1

diff --git a/lib/vdsm/qemuimg.py b/lib/vdsm/qemuimg.py
index 28cddee..8f6dc35 100644
--- a/lib/vdsm/qemuimg.py
+++ b/lib/vdsm/qemuimg.py
@@ -65,6 +65,21 @@
 self.cmd, self.ecode, self.stdout, self.stderr, self.message)
 
 
+class InvalidOutput(QImgError):
+"""
+Raised when the command output is not valid.
+"""
+
+def __init__(self, cmd, stdout, message):
+self.cmd = cmd
+self.stdout = stdout
+self.message = message
+
+def __str__(self):
+return "cmd=%s, stdout=%s, message=%s" % (
+self.cmd, self.stdout, self.message)
+
+
 def info(image, format=None):
 cmd = [_qemuimg.cmd, "info", "--output", "json"]
 
@@ -79,7 +94,7 @@
 try:
 qemu_info = _parse_qemuimg_json(out)
 except ValueError:
-raise QImgError(cmd, rc, out, err, "Failed to process qemu-img output")
+raise InvalidOutput(cmd, out, "Failed to process qemu-img output")
 
 try:
 info = {
@@ -87,7 +102,7 @@
 'virtualsize': qemu_info['virtual-size'],
 }
 except KeyError as key:
-raise QImgError(cmd, rc, out, err, "Missing field: %r" % key)
+raise InvalidOutput(cmd, out, "Missing field: %r" % key)
 
 if 'cluster-size' in qemu_info:
 info['clustersize'] = qemu_info['cluster-size']
@@ -97,7 +112,7 @@
 try:
 info['compat'] = qemu_info['format-specific']['data']['compat']
 except KeyError:
-raise QImgError(cmd, rc, out, err, "'compat' expected but not 
found")
+raise InvalidOutput(cmd, out, "'compat' expected but not found")
 
 return info
 
@@ -148,11 +163,11 @@
 try:
 qemu_check = _parse_qemuimg_json(out)
 except ValueError:
-raise QImgError(cmd, rc, out, err, "Failed to process qemu-img output")
+raise InvalidOutput(cmd, out, "Failed to process qemu-img output")
 try:
 return {"offset": qemu_check["image-end-offset"]}
 except KeyError:
-raise QImgError(cmd, rc, out, err, "unable to parse qemu-img check 
output")
+raise InvalidOutput(cmd, out, "unable to parse qemu-img check output")
 
 
 def convert(srcImage, dstImage, srcFormat=None, dstFormat=None,


-- 
To view, visit https://gerrit.ovirt.org/65208
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: If3f801f05f130c2417e4946877e31030260269a1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Add the command to QImgError

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: qemuimg: Add the command to QImgError
..

qemuimg: Add the command to QImgError

Change-Id: I226a54b29ae4afc3854056efea78767383b89619
Signed-off-by: Ala Hino 
---
M lib/vdsm/qemuimg.py
1 file changed, 5 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/79/65179/1

diff --git a/lib/vdsm/qemuimg.py b/lib/vdsm/qemuimg.py
index c9e72ec..26d0ec1 100644
--- a/lib/vdsm/qemuimg.py
+++ b/lib/vdsm/qemuimg.py
@@ -53,15 +53,16 @@
 
 
 class QImgError(Exception):
-def __init__(self, ecode, stdout, stderr, message=None):
+def __init__(self, cmd, ecode, stdout, stderr, message=None):
+self.cmd = cmd
 self.ecode = ecode
 self.stdout = stdout
 self.stderr = stderr
 self.message = message
 
 def __str__(self):
-return "ecode=%s, stdout=%s, stderr=%s, message=%s" % (
-self.ecode, self.stdout, self.stderr, self.message)
+return "cmd=%s, ecode=%s, stdout=%s, stderr=%s, message=%s" % (
+self.cmd, self.ecode, self.stdout, self.stderr, self.message)
 
 
 def info(image, format=None):
@@ -344,5 +345,5 @@
 def _run_cmd(cmd, cwd=None):
 rc, out, err = commands.execCmd(cmd, raw=True, cwd=cwd)
 if rc != 0:
-raise QImgError(rc, out, err)
+raise QImgError(cmd, rc, out, err)
 return out


-- 
To view, visit https://gerrit.ovirt.org/65179
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I226a54b29ae4afc3854056efea78767383b89619
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: qemuimg: Add wrapper to execute command

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: qemuimg: Add wrapper to execute command
..

qemuimg: Add wrapper to execute command

Add wrapper to execute qemuimg command and raise QImgError if error
encountered.

Change-Id: I3483a202f9543098ad75e8be8bf4b398d26c8e4a
Signed-off-by: Ala Hino 
---
M lib/vdsm/qemuimg.py
1 file changed, 11 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/78/65178/1

diff --git a/lib/vdsm/qemuimg.py b/lib/vdsm/qemuimg.py
index ef98149..c9e72ec 100644
--- a/lib/vdsm/qemuimg.py
+++ b/lib/vdsm/qemuimg.py
@@ -71,9 +71,7 @@
 cmd.extend(("-f", format))
 
 cmd.append(image)
-rc, out, err = commands.execCmd(cmd, raw=True)
-if rc != 0:
-raise QImgError(rc, out, err)
+out = _run_cmd(cmd)
 
 try:
 qemu_info = _parse_qemuimg_json(out)
@@ -125,10 +123,7 @@
 if size is not None:
 cmd.append(str(size))
 
-rc, out, err = commands.execCmd(cmd, cwd=cwdPath)
-
-if rc != 0:
-raise QImgError(rc, out, err)
+_run_cmd(cmd, cwd=cwdPath)
 
 
 def check(image, format=None):
@@ -138,11 +133,7 @@
 cmd.extend(("-f", format))
 
 cmd.append(image)
-rc, out, err = commands.execCmd(cmd, raw=True)
-
-# FIXME: handle different error codes and raise errors accordingly
-if rc != 0:
-raise QImgError(rc, out, err)
+out = _run_cmd(cmd)
 
 try:
 qemu_check = _parse_qemuimg_json(out)
@@ -300,10 +291,7 @@
 cmd.extend(("-f", format))
 
 cmd.extend((image, str(newSize)))
-rc, out, err = commands.execCmd(cmd)
-
-if rc != 0:
-raise QImgError(rc, out, err)
+_run_cmd(cmd)
 
 
 def rebase(image, backing, format=None, backingFormat=None, unsafe=False,
@@ -351,3 +339,10 @@
 if value not in _QCOW2_COMPAT_SUPPORTED:
 raise ValueError("Invalid compat version %r" % value)
 return value
+
+
+def _run_cmd(cmd, cwd=None):
+rc, out, err = commands.execCmd(cmd, raw=True, cwd=cwd)
+if rc != 0:
+raise QImgError(rc, out, err)
+return out


-- 
To view, visit https://gerrit.ovirt.org/65178
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3483a202f9543098ad75e8be8bf4b398d26c8e4a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: tests: Rename storagetestlibTests.py to new Vdsm convention

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: tests: Rename storagetestlibTests.py to new Vdsm convention
..

tests: Rename storagetestlibTests.py to new Vdsm convention

Change-Id: Ida07628aba9ab9813489f75c2ddd77ef6ed437e7
Signed-off-by: Ala Hino 
---
M tests/Makefile.am
R tests/storagetestlib_test.py
2 files changed, 2 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/73/65173/1

diff --git a/tests/Makefile.am b/tests/Makefile.am
index dcb36f8..1ee0b45 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -119,7 +119,7 @@
storagefakelibTests.py \
storageMailboxTests.py \
storageServerTests.py \
-   storagetestlibTests.py \
+   storagetestlib_test.py \
storage_asyncevent_test.py \
storage_check_test.py \
storage_directio_test.py \
@@ -233,7 +233,7 @@
storage_volume_metadata_test.py \
storage_workarounds_test.py \
storagefakelibTests.py \
-   storagetestlibTests.py \
+   storagetestlib_test.py \
toolBondingTests.py \
unicode_test.py \
utilsTests.py \
diff --git a/tests/storagetestlibTests.py b/tests/storagetestlib_test.py
similarity index 100%
rename from tests/storagetestlibTests.py
rename to tests/storagetestlib_test.py


-- 
To view, visit https://gerrit.ovirt.org/65173
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ida07628aba9ab9813489f75c2ddd77ef6ed437e7
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: tests: Change offset and len arguments type

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: tests: Change offset and len arguments type
..

tests: Change offset and len arguments type

Change offset and len arguments type from string to integer.

Change-Id: I0118332c75db873c3fb8c54fc0bced208d9c50e9
Signed-off-by: Ala Hino 
---
M tests/qemuimg_test.py
M tests/storagetestlib.py
M tests/storagetestlib_test.py
3 files changed, 17 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/74/65174/1

diff --git a/tests/qemuimg_test.py b/tests/qemuimg_test.py
index f6e8b35..238985f 100644
--- a/tests/qemuimg_test.py
+++ b/tests/qemuimg_test.py
@@ -408,14 +408,14 @@
 op.wait_for_completion()
 
 for i in range(base, top + 1):
-offset = "{}k".format(i)
+offset = i * 1024
 pattern = 0xf0 + i
 # The base volume must have the data from all the volumes
 # merged into it.
 format = (qemuimg.FORMAT.RAW if i == 0 else
   qemuimg.FORMAT.QCOW2)
 qemu_pattern_verify(base_vol, format, offset=offset,
-len='1k', pattern=pattern)
+len=1024, pattern=pattern)
 if i > base:
 # internal and top volumes should keep the data, we
 # may want to wipe this data when deleting the volumes
@@ -440,5 +440,6 @@
 def make_image(path, size, format, index, qcow2_compat, backing=None):
 qemuimg.create(path, size=size, format=format, qcow2Compat=qcow2_compat,
backing=backing)
-qemu_pattern_write(path, format, offset="%dk" % index, len='1k',
+offset = index * 1024
+qemu_pattern_write(path, format, offset=offset, len=1024,
pattern=0xf0 + index)
diff --git a/tests/storagetestlib.py b/tests/storagetestlib.py
index 24cd27e..959a5fa 100644
--- a/tests/storagetestlib.py
+++ b/tests/storagetestlib.py
@@ -272,16 +272,16 @@
 pass
 
 
-def qemu_pattern_write(path, format, offset='512', len='1k', pattern=5):
-write_cmd = 'write -P %d %s %s' % (pattern, offset, len)
+def qemu_pattern_write(path, format, offset=512, len=1024, pattern=5):
+write_cmd = 'write -P %d %d %d' % (pattern, offset, len)
 cmd = ['qemu-io', '-f', format, '-c', write_cmd, path]
 rc, out, err = commands.execCmd(cmd, raw=True)
 if rc != 0:
 raise cmdutils.Error(cmd, rc, out, err)
 
 
-def qemu_pattern_verify(path, format, offset='512', len='1k', pattern=5):
-read_cmd = 'read -P %d -s 0 -l %s %s %s' % (pattern, len, offset, len)
+def qemu_pattern_verify(path, format, offset=512, len=1024, pattern=5):
+read_cmd = 'read -P %d -s 0 -l %d %d %d' % (pattern, len, offset, len)
 cmd = ['qemu-io', '-f', format, '-c', read_cmd, path]
 rc, out, err = commands.execCmd(cmd, raw=True)
 if rc != 0:
@@ -304,10 +304,10 @@
 # This allows us to verify the integrity of the whole chain.
 for i, vol in enumerate(vol_list):
 vol_fmt = sc.fmt2str(vol.getFormat())
-offset = "{}k".format(i)
+offset = i * 1024
 pattern = 0xf0 + i
 qemu_pattern_write(vol.volumePath, vol_fmt, offset=offset,
-   len='1k', pattern=pattern)
+   len=1024, pattern=pattern)
 
 
 def verify_qemu_chain(vol_list):
@@ -317,24 +317,24 @@
 top_vol = vol_list[-1]
 top_vol_fmt = sc.fmt2str(top_vol.getFormat())
 for i, vol in enumerate(vol_list):
-offset = "{}k".format(i)
+offset = i * 1024
 pattern = 0xf0 + i
 
 # Check that the correct pattern can be read through the top volume
 qemu_pattern_verify(top_vol.volumePath, top_vol_fmt, offset=offset,
-len='1k', pattern=pattern)
+len=1024, pattern=pattern)
 
 # Check the volume where the pattern was originally written
 vol_fmt = sc.fmt2str(vol.getFormat())
-qemu_pattern_verify(vol.volumePath, vol_fmt, offset=offset, len='1k',
+qemu_pattern_verify(vol.volumePath, vol_fmt, offset=offset, len=1024,
 pattern=pattern)
 
 # Check that the next offset contains zeroes.  If we know this layer
 # has zeroes at next_offset we can be sure that data read at the same
 # offset in the next layer belongs to that layer.
-next_offset = "{}K".format(i + 1)
+next_offset = (i + 1) * 1024
 qemu_pattern_verify(vol.volumePath, vol_fmt, offset=next_offset,
-len='1k', pattern=0)
+len=1024, pattern=0)
 
 
 def make_qemu_chain(env, size, base_vol_fmt, chain_len):
diff --git a/tests/storagetestlib_test.py b/tests/storagetestlib_test.py
index a7d6a2c..170082d 100644
--- a/tests/storagetestlib_test.py
+++ b/test

Change in vdsm[master]: tests: Rename storagetestlibTests.py to new Vdsm convention

[ahino]

Ala Hino has posted comments on this change.

Change subject: tests: Rename storagetestlibTests.py to new Vdsm convention
..


Patch Set 1: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/65173
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ida07628aba9ab9813489f75c2ddd77ef6ed437e7
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: tests: Change offset and len arguments type

[ahino]

Ala Hino has posted comments on this change.

Change subject: tests: Change offset and len arguments type
..


Patch Set 1: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/65174
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0118332c75db873c3fb8c54fc0bced208d9c50e9
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: core: Expose API for qemuimg commit

[ahino]

Ala Hino has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
..


Patch Set 12: Verified+1

-- 
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: [WIP] core: Expose API for qemuimg map

[ahino]

Ala Hino has posted comments on this change.

Change subject: [WIP] core: Expose API for qemuimg map
..


Patch Set 1:

(3 comments)

https://gerrit.ovirt.org/#/c/65112/1/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 213: cmd = [_qemuimg.cmd, "map", "--output", "json", image]
Line 214: # For simplicity, we always run commit in the image directory.
Line 215: rc, out, err = commands.execCmd(cmd, raw=True)
Line 216: if rc != 0:
Line 217: raise QImgError(rc, out, err)
> This boilerplate is repeated too many times in this module. Can you send an
Will do
Line 218: 
Line 219: try:
Line 220: print "out: %s" % out
Line 221: except ValueError:


Line 216: if rc != 0:
Line 217: raise QImgError(rc, out, err)
Line 218: 
Line 219: try:
Line 220: print "out: %s" % out
> We want to parse the json text and return the parsed result.
I know; this code isn't complete. I just wanted to share with you to discuss 
the output
Line 221: except ValueError:
Line 222: raise QImgError(rc, out, err, "Failed to process qemuimg map 
output")
Line 223: 
Line 224: 


Line 220: print "out: %s" % out
Line 221: except ValueError:
Line 222: raise QImgError(rc, out, err, "Failed to process qemuimg map 
output")
Line 223: 
Line 224: 
> You have 3 empty lines, we need 2 to satisfy pep8 tool.
True, yet again - this is just Work In Progress
Line 225: 
Line 226: class QemuImgOperation(object):
Line 227: REGEXPR = re.compile(r'\s*\(([\d.]+)/100%\)\s*')
Line 228: 


-- 
To view, visit https://gerrit.ovirt.org/65112
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d6c936239d3dcc7a3d236dbaab0a93501ada8c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: [WIP] core: Expose API for qemuimg map

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: [WIP] core: Expose API for qemuimg map
..

[WIP] core: Expose API for qemuimg map

Change-Id: I54d6c936239d3dcc7a3d236dbaab0a93501ada8c
Signed-off-by: Ala Hino 
---
M lib/vdsm/qemuimg.py
M tests/qemuimg_test.py
2 files changed, 45 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/12/65112/1

diff --git a/lib/vdsm/qemuimg.py b/lib/vdsm/qemuimg.py
index ef98149..a6e89d8 100644
--- a/lib/vdsm/qemuimg.py
+++ b/lib/vdsm/qemuimg.py
@@ -209,6 +209,20 @@
 return QemuImgOperation(cmd, cwd=workdir)
 
 
+def map(image):
+cmd = [_qemuimg.cmd, "map", "--output", "json", image]
+# For simplicity, we always run commit in the image directory.
+rc, out, err = commands.execCmd(cmd, raw=True)
+if rc != 0:
+raise QImgError(rc, out, err)
+
+try:
+print "out: %s" % out
+except ValueError:
+raise QImgError(rc, out, err, "Failed to process qemuimg map output")
+
+
+
 class QemuImgOperation(object):
 REGEXPR = re.compile(r'\s*\(([\d.]+)/100%\)\s*')
 
diff --git a/tests/qemuimg_test.py b/tests/qemuimg_test.py
index f6e8b35..f00b830 100644
--- a/tests/qemuimg_test.py
+++ b/tests/qemuimg_test.py
@@ -437,6 +437,37 @@
 self.assertEquals(100, op.progress)
 
 
+@expandPermutations
+class TestMap(TestCaseBase):
+
+@permutations([
+# format, size
+(qemuimg.FORMAT.RAW, 0),
+(qemuimg.FORMAT.QCOW2, 0),
+(qemuimg.FORMAT.RAW, 1048576),
+(qemuimg.FORMAT.QCOW2, 1048576)
+])
+def test_map_single_volume(self, format, size):
+with namedTemporaryDir() as tmpdir:
+image = os.path.join(tmpdir, "base.img")
+make_image(image, size, format, 0, "1.1")
+map = qemuimg.map(image)
+
+def test_map_chain(self):
+with namedTemporaryDir() as tmpdir:
+parent = None
+chain = []
+for i in range(4):
+vol = os.path.join(tmpdir, "vol%d.img" % i)
+format = (qemuimg.FORMAT.RAW if i == 0 else
+  qemuimg.FORMAT.QCOW2)
+make_image(vol, 1048576, format, i, "1.1", parent)
+chain.append(vol)
+parent = vol
+
+map = qemuimg.map(chain[0])
+
+
 def make_image(path, size, format, index, qcow2_compat, backing=None):
 qemuimg.create(path, size=size, format=format, qcow2Compat=qcow2_compat,
backing=backing)


-- 
To view, visit https://gerrit.ovirt.org/65112
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I54d6c936239d3dcc7a3d236dbaab0a93501ada8c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: core: Expose API for qemuimg commit

[ahino]

Ala Hino 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:
Done
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:
Done
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:
Done
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 permut
Done
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.
Done
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 s
Done
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 
Now that I use RAW format for base volumes, I don't this arg anymore
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.
Done
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.
Done
Line 3

Change in vdsm[master]: core: Expose API for qemuimg commit

[ahino]

Ala Hino has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
..


Patch Set 9:

(6 comments)

https://gerrit.ovirt.org/#/c/64222/9/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 380: (2, qemuimg.FORMAT.QCOW2, "0.10", True),
Line 381: (4, qemuimg.FORMAT.RAW, "1.1", True),
Line 382: (4, qemuimg.FORMAT.RAW, "0.10", True),
Line 383: (4, qemuimg.FORMAT.QCOW2, "1.1", True),
Line 384: (4, qemuimg.FORMAT.QCOW2, "0.10", True)
> In all these options, we merge entire chain. It would be nice to test merge
Done
Line 385: ])
Line 386: def test_commit(self, chain_len, base_format, qcow2_compat,
Line 387: use_base=True):
Line 388: size = 1048576


Line 391: parent = None
Line 392: for i in range(chain_len):
Line 393: vol = os.path.join(tmpdir, "vol%d.img" % i)
Line 394: format = base_format if i == 0 else 
qemuimg.FORMAT.QCOW2
Line 395: make_image(vol, size, format, i, qcow2_compat, parent)
> We can take the size of each volume here:
Done
Line 396: chain.append(vol)
Line 397: parent = vol
Line 398: 
Line 399: op = qemuimg.commit(chain[-1], 
topFormat=qemuimg.FORMAT.QCOW2,


Line 399: op = qemuimg.commit(chain[-1], 
topFormat=qemuimg.FORMAT.QCOW2,
Line 400: base=chain[0] if use_base else None)
Line 401: op.wait_for_completion()
Line 402: 
Line 403: for i, vol in enumerate(chain):
> If we stored tuples, we get back:
Done
Line 404: offset = "{}k".format(i)
Line 405: pattern = 0xf0 + (i)
Line 406: qemu_pattern_verify(chain[0], base_format, 
offset=offset,
Line 407: len='1k', pattern=pattern)


Line 401: op.wait_for_completion()
Line 402: 
Line 403: for i, vol in enumerate(chain):
Line 404: offset = "{}k".format(i)
Line 405: pattern = 0xf0 + (i)
> Why (i)? This is same as i.
Done
Line 406: qemu_pattern_verify(chain[0], base_format, 
offset=offset,
Line 407: len='1k', pattern=pattern)
Line 408: qemu_pattern_verify(vol, qemuimg.FORMAT.QCOW2, 
offset=offset,
Line 409: len='1k', pattern=pattern)


Line 403: for i, vol in enumerate(chain):
Line 404: offset = "{}k".format(i)
Line 405: pattern = 0xf0 + (i)
Line 406: qemu_pattern_verify(chain[0], base_format, 
offset=offset,
Line 407: len='1k', pattern=pattern)
> We need to explain this:
Done
Line 408: qemu_pattern_verify(vol, qemuimg.FORMAT.QCOW2, 
offset=offset,
Line 409: len='1k', pattern=pattern)
Line 410: 
Line 411: def test_commit_progress(self):


Line 405: pattern = 0xf0 + (i)
Line 406: qemu_pattern_verify(chain[0], base_format, 
offset=offset,
Line 407: len='1k', pattern=pattern)
Line 408: qemu_pattern_verify(vol, qemuimg.FORMAT.QCOW2, 
offset=offset,
Line 409: len='1k', pattern=pattern)
> This check will always succeed, even if you forget to add the -d flag (try 
Done
Line 410: 
Line 411: def test_commit_progress(self):
Line 412: with namedTemporaryDir() as tmpdir:
Line 413: 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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[ovirt-4.0]: Live Merge: Teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Teardown volume on HSM after live merge
..


Patch Set 3:

> (1 comment)

BZ 1377849: LV is not deactivated after live merge when the VM is running on HSM

-- 
To view, visit https://gerrit.ovirt.org/64985
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[ovirt-4.0]: Live Merge: Teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Teardown volume on HSM after live merge
..


Patch Set 3:

> (1 comment)

This is the correct bug. BZ 1321018 is about the symlinks.

-- 
To view, visit https://gerrit.ovirt.org/64985
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[ovirt-4.0]: Live Merge: Remove volume run link after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
..


Patch Set 3: Verified+1

Verified on master

-- 
To view, visit https://gerrit.ovirt.org/64986
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[ovirt-4.0]: Live Merge: Teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Teardown volume on HSM after live merge
..


Patch Set 3: Verified+1

Verified on master

-- 
To view, visit https://gerrit.ovirt.org/64985
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[ovirt-4.0]: Live Merge: Teardown volume on HSM after live merge

[ahino]

Hello Adam Litke, Nir Soffer,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/64985

to review the following change.

Change subject: Live Merge: Teardown volume on HSM after live merge
..

Live Merge: Teardown volume on HSM after live merge

If a VM is running on HSM and live merge is performed, the LV isn't
deactivated because the deactivation is done when deleting the volume.
However, deleting the volume is done on SPM and this means that the LV
is not deactivated on the HSM. In this patch, a logic to teardown the
volume is added after live merge has completed.

Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Bug-Url: https://bugzilla.redhat.com/1377849
Signed-off-by: Ala Hino 
Reviewed-on: https://gerrit.ovirt.org/64301
Reviewed-by: Nir Soffer 
Continuous-Integration: Nir Soffer 
Continuous-Integration: Jenkins CI
Reviewed-by: Adam Litke 
---
M vdsm/storage/blockSD.py
M vdsm/storage/sd.py
M vdsm/virt/vm.py
3 files changed, 32 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/85/64985/1

diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py
index 8de13b1..58d2507 100644
--- a/vdsm/storage/blockSD.py
+++ b/vdsm/storage/blockSD.py
@@ -799,6 +799,21 @@
 if preallocate == sc.SPARSE_VOL and volFormat == sc.RAW_FORMAT:
 raise se.IncorrectFormat(sc.type2name(volFormat))
 
+def getVolumeLease(self, imgUUID, volUUID):
+"""
+Return the volume lease (leasePath, leaseOffset)
+"""
+if not self.hasVolumeLeases():
+return clusterlock.Lease(None, None, None)
+# TODO: use the sanlock specific offset when present
+slot = self.produceVolume(imgUUID, volUUID).getMetaOffset()
+offset = ((slot + blockVolume.RESERVED_LEASES) * self.logBlkSize *
+  sd.LEASE_BLOCKS)
+return clusterlock.Lease(volUUID, self.getLeasesFilePath(), offset)
+
+def teardownVolume(self, imgUUID, volUUID):
+lvm.deactivateLVs(self.sdUUID, [volUUID])
+
 
 class BlockStorageDomain(sd.StorageDomain):
 manifestClass = BlockStorageDomainManifest
diff --git a/vdsm/storage/sd.py b/vdsm/storage/sd.py
index 2b151a6..d004794 100644
--- a/vdsm/storage/sd.py
+++ b/vdsm/storage/sd.py
@@ -487,6 +487,14 @@
 if preallocate is not None and preallocate not in sc.VOL_TYPE:
 raise se.IncorrectType(preallocate)
 
+def teardownVolume(self, imgUUID, volUUID):
+"""
+Called when a volume is detached from a prepared image during live
+merge flow. In this case, the volume will not be torn down when
+the image is torn down.
+This does nothing, subclass should override this if needed.
+"""
+
 
 class StorageDomain(object):
 log = logging.getLogger("Storage.StorageDomain")
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index a896f58..22e656c 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -66,6 +66,7 @@
 from vdsm.virt.utils import isVdsmImage, cleanup_guest_socket
 from storage import outOfProcess as oop
 from storage import sd
+from storage import sdc
 
 # local imports
 # In future those should be imported via ..
@@ -4936,6 +4937,12 @@
self.drive.imageID, baseVolUUID,
topVolInfo['capacity'])
 
+def teardown_top_volume(self):
+# TODO move this method to storage public API
+sd_manifest = sdc.sdCache.produce_manifest(self.drive.domainID)
+sd_manifest.teardownVolume(self.drive.imageID,
+   self.job['topVolume'])
+
 @utils.traceback()
 def run(self):
 self.update_base_size()
@@ -4946,6 +4953,8 @@
 self.vm._syncVolumeChain(self.drive)
 if self.doPivot:
 self.vm.startDisksStatsCollection()
+self.vm.enableDriveMonitor()
+self.teardown_top_volume()
 self.success = True
 self.vm.log.info("Synchronization completed (job %s)",
  self.job['jobID'])


-- 
To view, visit https://gerrit.ovirt.org/64985
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Nir Soffer 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[ovirt-4.0]: Live Merge: Remove volume run link after live merge

[ahino]

Hello Adam Litke, Nir Soffer,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/64986

to review the following change.

Change subject: Live Merge: Remove volume run link after live merge
..

Live Merge: Remove volume run link after live merge

When deleting a volume while the VM is running, volume teardown doesn't
remove the volume run symbolic link: /run/vdsm/storage/sdUUID/volUUID.
In patch Iec3b6a (Live Merge: teardown volume on HSM after live merge)
we added volume teardown logic that, for block storage it deactivated
the volume. In this patch we extend volume teardown logic to unlink
volume run link.
Note that this change isn't required for file storage as no symbolic
links are created.

Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Bug-Url: https://bugzilla.redhat.com/1321018
Signed-off-by: Ala Hino 
Reviewed-on: https://gerrit.ovirt.org/59725
Reviewed-by: Nir Soffer 
Reviewed-by: Adam Litke 
Continuous-Integration: Nir Soffer 
---
M vdsm/storage/blockSD.py
1 file changed, 15 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/86/64986/1

diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py
index 58d2507..f977150 100644
--- a/vdsm/storage/blockSD.py
+++ b/vdsm/storage/blockSD.py
@@ -813,6 +813,21 @@
 
 def teardownVolume(self, imgUUID, volUUID):
 lvm.deactivateLVs(self.sdUUID, [volUUID])
+self.removeVolumeRunLink(imgUUID, volUUID)
+
+def removeVolumeRunLink(self, imgUUID, volUUID):
+"""
+Remove /run/vdsm/storage/sdUUID/imgUUID/volUUID
+"""
+vol_run_link = os.path.join(constants.P_VDSM_STORAGE,
+self.sdUUID, imgUUID, volUUID)
+self.log.info("Unlinking volme runtime link: %r", vol_run_link)
+try:
+os.unlink(vol_run_link)
+except OSError as e:
+if e.error != errno.ENOENT:
+raise
+self.log.debug("Volume run link %r does not exist", vol_run_link)
 
 
 class BlockStorageDomain(sd.StorageDomain):


-- 
To view, visit https://gerrit.ovirt.org/64986
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Nir Soffer 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: core: Expose API for qemuimg commit

[ahino]

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 namedTemporaryD

Change in vdsm[master]: Live Merge: Teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Teardown volume on HSM after live merge
..


Patch Set 7: Verified+1

Verified both on SPM and HSM that after live merge the volume is deactivated. 
Verified top volume merge and internal volume merge

-- 
To view, visit https://gerrit.ovirt.org/64301
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Teardown volume on HSM after live merge
..


Patch Set 6:

(2 comments)

https://gerrit.ovirt.org/#/c/64301/6/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4779:  "(job %s)", self.job['jobID'])
Line 4780: self.vm._syncVolumeChain(self.drive)
Line 4781: if self.doPivot:
Line 4782: self.vm.enableDriveMonitor()
Line 4783: self.teardown_top_volume()
> How is this needed only after pivot?
Done
Line 4784: self.success = True
Line 4785: self.vm.log.info("Synchronization completed (job %s)",
Line 4786:  self.job['jobID'])
Line 4787: # At this point, merge has succesfully completed and the top 
volume is


Line 4788: # not part of the chain.  Now, we want to teardown the top 
volume. Note
Line 4789: # that if volume deactivation fails, we don't want to fail 
the merge
Line 4790: # whole operation as the VM is running without issues.  It 
is worth to
Line 4791: # note that if volume deactivation fails, chances are high 
that the
Line 4792: # environment is severely damaged.
> This comment conflict with the code now.
Removed
Line 4793: 
Line 4794: def isSuccessful(self):
Line 4795: """
Line 4796: Returns True if this phase completed successfully.


-- 
To view, visit https://gerrit.ovirt.org/64301
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: core: Expose API for qemuimg commit

[ahino]

Ala Hino has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
..


Patch Set 6:

(8 comments)

https://gerrit.ovirt.org/#/c/64222/6/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 186: 
Line 187: 
Line 188: def commit(top, topFormat, base=None):
Line 189: cmd = [_qemuimg.cmd, "commit", "-p", "-t", "none"]
Line 190: # For simplicity, we always run commit in the working directory.
> in the image directory
Done
Line 191: workdir = os.path.dirname(top)
Line 192: 
Line 193: if base:
Line 194: cmd.extend(("-b", base))


Line 196: # If base volume is not provided, qemuimg commit will empty 
the top
Line 197: # volume after the operation has succeeded. Providing '-d' 
option
Line 198: # will cause qemuimg commit not to empty the top volume. Note 
that
Line 199: # if a backing chain is provided, i.e. a base volume is 
provided,
Line 200: # '-d' is always implied.
> See my comment about this comment in previous version.
Done
Line 201: cmd.append("-d")
Line 202: 
Line 203: cmd.extend(("-f", topFormat))
Line 204: 


Line 202: 
Line 203: cmd.extend(("-f", topFormat))
Line 204: 
Line 205: cmd.append(top)
Line 206: 
> We use workdir only here, so better create it just before we use it. Smalle
Done
Line 207: return QemuImgOperation(cmd, cwd=workdir)
Line 208: 
Line 209: 
Line 210: class QemuImgOperation(object):


https://gerrit.ovirt.org/#/c/64222/6/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 335: @expandPermutations
Line 336: class TestCommit(TestCaseBase):
Line 337: 
Line 338: @MonkeyPatch(qemuimg, 'config', CONFIG)
Line 339: @permutations([
> We need a comment here for the argument
Done
Line 340: (1, qemuimg.FORMAT.RAW, False),
Line 341: (1, qemuimg.FORMAT.RAW),
Line 342: (1, qemuimg.FORMAT.QCOW2, False),
Line 343: (1, qemuimg.FORMAT.QCOW2),


Line 337: 
Line 338: @MonkeyPatch(qemuimg, 'config', CONFIG)
Line 339: @permutations([
Line 340: (1, qemuimg.FORMAT.RAW, False),
Line 341: (1, qemuimg.FORMAT.RAW),
> Please keep the same number of arguments to make the format consistent and 
Done
Line 342: (1, qemuimg.FORMAT.QCOW2, False),
Line 343: (1, qemuimg.FORMAT.QCOW2),
Line 344: (3, qemuimg.FORMAT.RAW),
Line 345: (3, qemuimg.FORMAT.QCOW2)


Line 343: (1, qemuimg.FORMAT.QCOW2),
Line 344: (3, qemuimg.FORMAT.RAW),
Line 345: (3, qemuimg.FORMAT.QCOW2)
Line 346: ])
Line 347: def test_commit(self, chainLen, baseFormat, useBacking=True):
> This looks like use_base, or with_base. backing is another term not related
Done
Line 348: size = 1048576
Line 349: with namedTemporaryDir() as tmpdir:
Line 350: # create base
Line 351: base = os.path.join(tmpdir, "base.img")


Line 361: chain.append(vol)
Line 362: parent = vol
Line 363: 
Line 364: if useBacking:
Line 365: op = qemuimg.commit(chain[chainLen-1],
> You can use:
Done
Line 366: topFormat=topFormat, base=base)
Line 367: else:
Line 368: op = qemuimg.commit(chain[chainLen-1], topFormat)
Line 369: op.wait_for_completion()


Line 364: if useBacking:
Line 365: op = qemuimg.commit(chain[chainLen-1],
Line 366: topFormat=topFormat, base=base)
Line 367: else:
Line 368: op = qemuimg.commit(chain[chainLen-1], topFormat)
> This is little ugly, can we have the same call, taking None as default valu
Done
Line 369: op.wait_for_completion()
Line 370: 
Line 371: # verify base data at offset 0 after commit
Line 372: qemu_pattern_verify(base, baseFormat, offset='0', 
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: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: core: Introduce new Volume.reduceSize verb

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: core: Introduce new Volume.reduceSize verb
..

core: Introduce new Volume.reduceSize verb

This new verb will be used after merge to reduce volume size to optimal.

Change-Id: If4e1fadda1aa34274e568bcaae7ba1f8b350a48f
Signed-off-by: Ala Hino 
---
M lib/api/vdsm-api.yml
M vdsm/API.py
M vdsm/storage/hsm.py
3 files changed, 35 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/50/64450/1

diff --git a/lib/api/vdsm-api.yml b/lib/api/vdsm-api.yml
index 0745093..0ef9f22 100644
--- a/lib/api/vdsm-api.yml
+++ b/lib/api/vdsm-api.yml
@@ -9648,6 +9648,33 @@
 description: A task UUID
 type: *UUID
 
+Volume.reduceSize:
+added: '4.1'
+description: Reduces the virtual size of a volume.
+params:
+-   description: The Storage Pool associated with the Volume
+name: storagepoolID
+type: *UUID
+
+-   description: The Storage Domain associated with the Volume
+name: storagedomainID
+type: *UUID
+
+-   description: The Image associated with the Volume
+name: imageID
+type: *UUID
+
+-   description: The UUID of the Volume
+name: volumeID
+type: *UUID
+
+-   description: The new desired size (in bytes)
+name: newSize
+type: string
+return:
+description: A task UUID
+type: *UUID
+
 Volume.getInfo:
 added: '3.1'
 description: Get information about a Volume.
diff --git a/vdsm/API.py b/vdsm/API.py
index 267aad7..7922c76 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -867,6 +867,10 @@
 return self._irs.extendVolumeSize(
 self._spUUID, self._sdUUID, self._imgUUID, self._UUID, newSize)
 
+def reduceSize(self, newSize):
+return self._irs.reduceVolumeSize(
+self._spUUID, self._sdUUID, self._imgUUID, self._UUID, newSize)
+
 def updateSize(self, newSize):
 return self._irs.updateVolumeSize(
 self._spUUID, self._sdUUID, self._imgUUID, self._UUID, newSize)
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index faf3dfb..881165f 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -663,6 +663,10 @@
 imgUUID, volUUID, newSizeSectors)
 
 @public
+def reduceVolumeSize(self, spUUID, sdUUID, imgUUID, volUUID, newSize):
+raise NotImplementedError
+
+@public
 def updateVolumeSize(self, spUUID, sdUUID, imgUUID, volUUID, newSize):
 """
 Update the volume size with the given newSize (in bytes).


-- 
To view, visit https://gerrit.ovirt.org/64450
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: If4e1fadda1aa34274e568bcaae7ba1f8b350a48f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: core: Introduce new Volume.getWatermarks verb

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: core: Introduce new Volume.getWatermarks verb
..

core: Introduce new Volume.getWatermarks verb

This verb returns the volume watermarks and will be used in two cases:
1. Before merge in order to extend the base volume size to minimal
required size
2. After merge in order to reduce volume size to optimal

Change-Id: I9f1feeb1540b2cb887b431b8075c26da09b62ea8
Signed-off-by: Ala Hino 
---
M lib/api/vdsm-api.yml
M vdsm/API.py
M vdsm/storage/hsm.py
3 files changed, 32 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/51/64451/1

diff --git a/lib/api/vdsm-api.yml b/lib/api/vdsm-api.yml
index 0ef9f22..c27b6fa 100644
--- a/lib/api/vdsm-api.yml
+++ b/lib/api/vdsm-api.yml
@@ -9675,6 +9675,29 @@
 description: A task UUID
 type: *UUID
 
+Volume.getWatermarks:
+added: '4.1'
+description: Returns the volume watermarks.
+params:
+-   description: The Storage Pool associated with the Volume
+name: storagepoolID
+type: *UUID
+
+-   description: The Storage Domain associated with the Volume
+name: storagedomainID
+type: *UUID
+
+-   description: The Image associated with the Volume
+name: imageID
+type: *UUID
+
+-   description: The UUID of the Volume
+name: volumeID
+type: *UUID
+return:
+description: The volume size info
+type: *VolumeSizeInfo
+
 Volume.getInfo:
 added: '3.1'
 description: Get information about a Volume.
diff --git a/vdsm/API.py b/vdsm/API.py
index 7922c76..e46a3fc 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -871,6 +871,10 @@
 return self._irs.reduceVolumeSize(
 self._spUUID, self._sdUUID, self._imgUUID, self._UUID, newSize)
 
+def getWatermarks(self):
+return self._irs.getVolumeWatermarks(
+self._spUUID, self._sdUUID, self._imgUUID, self._UUID)
+
 def updateSize(self, newSize):
 return self._irs.updateVolumeSize(
 self._spUUID, self._sdUUID, self._imgUUID, self._UUID, newSize)
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 881165f..67bea22 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -667,6 +667,11 @@
 raise NotImplementedError
 
 @public
+def getVolumeWatermarks(self, spUUID, sdUUID, imgUUID, volUUID,
+options=None):
+raise NotImplementedError
+
+@public
 def updateVolumeSize(self, spUUID, sdUUID, imgUUID, volUUID, newSize):
 """
 Update the volume size with the given newSize (in bytes).


-- 
To view, visit https://gerrit.ovirt.org/64451
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9f1feeb1540b2cb887b431b8075c26da09b62ea8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: core: Expose API for qemuimg commit

[ahino]

Ala Hino has posted comments on this change.

Change subject: core: Expose API for qemuimg commit
..


Patch Set 5:

(7 comments)

Nir, note that I created a single test that covers all use cases. I am hoping 
that I addressed all your comments

https://gerrit.ovirt.org/#/c/64222/2/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 188: def commit(top, topFormat, base=None):
Line 189: cmd = [_qemuimg.cmd, "commit", "-p", "-t", "none"]
Line 190: # For simplicity, we always run commit in the working directory.
Line 191: workdir = os.path.dirname(top)
Line 192: 
> Please explain why -d is needed, the code is very clear about adding it whe
Done
Line 193: if base:
Line 194: cmd.extend(("-b", base))
Line 195: else:
Line 196: # If base volume is not provided, qemuimg commit will empty 
the top


https://gerrit.ovirt.org/#/c/64222/4/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 184: 
Line 185: return QemuImgOperation(cmd, cwd=cwdPath)
Line 186: 
Line 187: 
Line 188: def commit(top, topFormat, base=None):
> We should require topFormat, more secure to never let qemu detect the forma
Done
Line 189: cmd = [_qemuimg.cmd, "commit", "-p", "-t", "none"]
Line 190: # For simplicity, we always run commit in the working directory.
Line 191: workdir = os.path.dirname(top)
Line 192: 


Line 185: return QemuImgOperation(cmd, cwd=cwdPath)
Line 186: 
Line 187: 
Line 188: def commit(top, topFormat, base=None):
Line 189: cmd = [_qemuimg.cmd, "commit", "-p", "-t", "none"]
> Add -t none.
Done
Line 190: # For simplicity, we always run commit in the working directory.
Line 191: workdir = os.path.dirname(top)
Line 192: 
Line 193: if base:


Line 186: 
Line 187: 
Line 188: def commit(top, topFormat, base=None):
Line 189: cmd = [_qemuimg.cmd, "commit", "-p", "-t", "none"]
Line 190: # For simplicity, we always run commit in the working directory.
> workdir?
Done
Line 191: workdir = os.path.dirname(top)
Line 192: 
Line 193: if base:
Line 194: cmd.extend(("-b", base))


Line 191: workdir = os.path.dirname(top)
Line 192: 
Line 193: if base:
Line 194: cmd.extend(("-b", base))
Line 195: else:
> Lets always use workdir, to simplify the code.
Done
Line 196: # If base volume is not provided, qemuimg commit will empty 
the top
Line 197: # volume after the operation has succeeded. Providing '-d' 
option
Line 198: # will cause qemuimg commit not to empty the top volume. Note 
that
Line 199: # if a backing chain is provided, i.e. a base volume is 
provided,


Line 201: cmd.append("-d")
Line 202: 
Line 203: cmd.extend(("-f", topFormat))
Line 204: 
Line 205: cmd.append(top)
> str not needed
Done
Line 206: 
Line 207: return QemuImgOperation(cmd, cwd=workdir)
Line 208: 
Line 209: 


https://gerrit.ovirt.org/#/c/64222/4/tests/qemuimg_test.py
File tests/qemuimg_test.py:

Line 342: (1, qemuimg.FORMAT.QCOW2, False),
Line 343: (1, qemuimg.FORMAT.QCOW2),
Line 344: (3, qemuimg.FORMAT.RAW),
Line 345: (3, qemuimg.FORMAT.QCOW2)
Line 346: ])
> This is broken because 0xf1 is not the pattern but the length.
Done
Line 347: def test_commit(self, chainLen, baseFormat, useBacking=True):
Line 348: size = 1048576
Line 349: with namedTemporaryDir() as tmpdir:
Line 350: # create base


-- 
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Teardown volume on HSM after live merge
..


Patch Set 6: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/64301
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
..


Patch Set 12: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/59725
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Teardown volume on HSM after live merge
..


Patch Set 5:

(5 comments)

https://gerrit.ovirt.org/#/c/64301/5/vdsm/storage/sd.py
File vdsm/storage/sd.py:

PS5, Line 531: teared
> torn
Done


PS5, Line 532: teared
> torn
Done


https://gerrit.ovirt.org/#/c/64301/5/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS5, Line 4786: nvolume
> volume
Done


PS5, Line 4788: megre
> merge
Done


Line 4787: # not part of the chain.  Now, we want to teardown the top 
volume. Note
Line 4788: # that if volume deactivation fails, we don't want to fail 
the megre
Line 4789: # whole operation as the VM is running without issues.  It 
is worth to
Line 4790: # note that if volume deactivation fails, chances are high 
that the
Line 4791: # environment is severely damaged.
> I do want to the operation to fail, we cannot continue to use this environm
Moved
Line 4792: self.teardown_top_volume()
Line 4793: 
Line 4794: def isSuccessful(self):
Line 4795: """


-- 
To view, visit https://gerrit.ovirt.org/64301
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
..


Patch Set 10:

(2 comments)

https://gerrit.ovirt.org/#/c/59725/10/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 821: """
Line 822: volRunlink = os.path.join(constants.P_VDSM_STORAGE,
Line 823:   self.sdUUID, imgUUID, volUUID)
Line 824: try:
Line 825: self.log.info("Unlinking volme runtime link: %r", 
volRunLink)
> ./vdsm/storage/blockSD.py:825: undefined name 'volRunLink'
Which comment? I thought I addressed all comments
Line 826: os.unlink(volRunLink)
Line 827: except OSError as e:
Line 828: if e.error != errno.ENOENT:
Line 829: raise


Line 822: volRunlink = os.path.join(constants.P_VDSM_STORAGE,
Line 823:   self.sdUUID, imgUUID, volUUID)
Line 824: try:
Line 825: self.log.info("Unlinking volme runtime link: %r", 
volRunLink)
Line 826: os.unlink(volRunLink)
> Same error here.
Done
Line 827: except OSError as e:
Line 828: if e.error != errno.ENOENT:
Line 829: raise
Line 830: self.log.debug("Volume run link %r does not exist", 
volRunlink)


-- 
To view, visit https://gerrit.ovirt.org/59725
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: cleanup: Remove log messages when raising exceptions

[ahino]

Ala Hino has posted comments on this change.

Change subject: cleanup: Remove log messages when raising exceptions
..


Patch Set 1: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/64342
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cc68734d8c405343d118cfc2d9e8da09445dd88
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Teardown volume on HSM after live merge
..


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/64301/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4769
Line 4770
Line 4771
Line 4772
Line 4773
> Another place where we should not pass an argument.
Actually, _syncVolumeChain is defined on the VM and the drive isn't held as a 
variable of the VM


-- 
To view, visit https://gerrit.ovirt.org/64301
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: cleanup: Remove log messages when raising exceptions

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: cleanup: Remove log messages when raising exceptions
..

cleanup: Remove log messages when raising exceptions

Change-Id: I2cc68734d8c405343d118cfc2d9e8da09445dd88
Signed-off-by: Ala Hino 
---
M vdsm/storage/blockSD.py
1 file changed, 0 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/42/64342/1

diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py
index 8ac7433..6ce6af7 100644
--- a/vdsm/storage/blockSD.py
+++ b/vdsm/storage/blockSD.py
@@ -1149,8 +1149,6 @@
 self.log.debug("path to image directory already exists: %s",
dst)
 else:
-self.log.error("Failed to create path to image directory: %s",
-   dst)
 raise
 return dst
 
@@ -1184,7 +1182,6 @@
 if e.errno == errno.EEXIST:
 self.log.debug("img run vol already exists: %s", dstVol)
 else:
-self.log.error("Failed to create img run vol: %s", dstVol)
 raise
 
 return imgRunDir


-- 
To view, visit https://gerrit.ovirt.org/64342
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2cc68734d8c405343d118cfc2d9e8da09445dd88
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
..


Patch Set 9:

(2 comments)

https://gerrit.ovirt.org/#/c/59725/9/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

PS9, Line 829: self.log.error("Failed to unlink volume run link: %r",
 :volRunlink)
> I'd just raise without logging
Done


Line 826: os.unlink(volRunLink)
Line 827: except OSError as e:
Line 828: if e.error != errno.ENOENT:
Line 829: self.log.error("Failed to unlink volume run link: %r",
Line 830:volRunlink)
> I commented about this in
Done
Line 831: raise
Line 832: self.log.debug("Volume run link %r does not exist", 
volRunlink)
Line 833: 
Line 834: 


-- 
To view, visit https://gerrit.ovirt.org/59725
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Teardown volume on HSM after live merge
..


Patch Set 3:

(4 comments)

https://gerrit.ovirt.org/#/c/64301/3//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2016-09-22 15:47:20 +0300
Line 4: Commit: Ala Hino 
Line 5: CommitDate: 2016-09-23 00:01:49 +0300
Line 6: 
Line 7: Live Merge: teardown volume on HSM after live merge
> Teardown
Done
Line 8: 
Line 9: If a VM is running on HSM and live merge is performed, the LV isn't
Line 10: deactivated because, the deactivation is done when deleting the volume.
Line 11: However, deleting the volume is done on SPM and this means that the LV


PS3, Line 10: ,
> This comma is redundant.
Done


PS3, Line 13:  
> missing "is"
Done


https://gerrit.ovirt.org/#/c/64301/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4782: self.vm.enableDriveMonitor()
Line 4783: self.success = True
Line 4784: self.vm.log.info("Synchronization completed (job %s)",
Line 4785:  self.job['jobID'])
Line 4786: self.teardown_top_volume()
> This must be done *before* we set self.success to True, so it this fails, w
After another thought, I think it is better not to fail merge in this case, as 
the chances to fail here are really low and if failed, probably the env is 
severely damaged
Line 4787: 
Line 4788: def isSuccessful(self):
Line 4789: """
Line 4790: Returns True if this phase completed successfully.


-- 
To view, visit https://gerrit.ovirt.org/64301
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: teardown volume on HSM after live merge
..


Patch Set 2:

(3 comments)

https://gerrit.ovirt.org/#/c/64301/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4769
Line 4770
Line 4771
Line 4772
Line 4773
> Another place where we should not pass an argument.
separate patch ...


Line 4763: self.vm._setVolumeSize(self.drive.domainID, 
self.drive.poolID,
Line 4764:self.drive.imageID, baseVolUUID,
Line 4765:topVolInfo['capacity'])
Line 4766: 
Line 4767: def teardown_top_volume(self, imgUUID, volUUID):
> Don't pass arguments to this method, it make the code  harder to read, and 
Done
Line 4768: # TODO move this method to storage public API
Line 4769: sd_manifest = 
sdc.sdCache.produce_manifest(self.drive.domainID)
Line 4770: sd_manifest.teardownVolume(imgUUID, volUUID)
Line 4771: 


Line 4781: self.vm.enableDriveMonitor()
Line 4782: self.success = True
Line 4783: self.vm.log.info("Synchronization completed (job %s)",
Line 4784:  self.job['jobID'])
Line 4785: self.teardown_top_volume(self.drive.imageID, 
self.job['topVolume'])
> This would be much nicer as:
Done
Line 4786: 
Line 4787: def isSuccessful(self):
Line 4788: """
Line 4789: Returns True if this phase completed successfully.


-- 
To view, visit https://gerrit.ovirt.org/64301
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
..


Patch Set 6:

(9 comments)

https://gerrit.ovirt.org/#/c/59725/6//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2016-09-22 16:41:56 +0300
Line 4: Commit: Ala Hino 
Line 5: CommitDate: 2016-09-22 17:01:55 +0300
Line 6: 
Line 7: Live Merge: Unlink volume runtime dir after live merge
> Remove volume run link after live merge?
Done
Line 8: 
Line 9: Unlink volume runtime dir after live megre.
Line 10: 
Line 11: Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4


Line 5: CommitDate: 2016-09-22 17:01:55 +0300
Line 6: 
Line 7: Live Merge: Unlink volume runtime dir after live merge
Line 8: 
Line 9: Unlink volume runtime dir after live megre.
> Please explain the context like you did in the previous patch.
Done
Line 10: 
Line 11: Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Line 12: Bug-Url: https://bugzilla.redhat.com/1321018


Line 5: CommitDate: 2016-09-22 17:01:55 +0300
Line 6: 
Line 7: Live Merge: Unlink volume runtime dir after live merge
Line 8: 
Line 9: Unlink volume runtime dir after live megre.
> Please also explain here why this is not needed in file storage.
Done
Line 10: 
Line 11: Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Line 12: Bug-Url: https://bugzilla.redhat.com/1321018


https://gerrit.ovirt.org/#/c/59725/6/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 810: offset = ((slot + blockVolume.RESERVED_LEASES) * 
self.logBlkSize *
Line 811:   sd.LEASE_BLOCKS)
Line 812: return clusterlock.Lease(volUUID, self.getLeasesFilePath(), 
offset)
Line 813: 
Line 814: def teardownVolume(self, imgUUID, volUUID):
> This change belongs to the previous patch.
Done
Line 815: lvm.deactivateLVs(self.sdUUID, [volUUID])
Line 816: self.removeVolumeRunDir(imgUUID, volUUID)
Line 817: 
Line 818: def removeVolumeRunDir(self, imgUUID, volUUID):


Line 814: def teardownVolume(self, imgUUID, volUUID):
Line 815: lvm.deactivateLVs(self.sdUUID, [volUUID])
Line 816: self.removeVolumeRunDir(imgUUID, volUUID)
Line 817: 
Line 818: def removeVolumeRunDir(self, imgUUID, volUUID):
> removeVolumeRunLink
Done
Line 819: """
Line 820: Remove /run/vdsm/storage/sdUUID/imgUUID/volUUID
Line 821: """
Line 822: sdRunDir = os.path.join(constants.P_VDSM_STORAGE, self.sdUUID)


Line 820: Remove /run/vdsm/storage/sdUUID/imgUUID/volUUID
Line 821: """
Line 822: sdRunDir = os.path.join(constants.P_VDSM_STORAGE, self.sdUUID)
Line 823: imgRunDir = os.path.join(sdRunDir, imgUUID)
Line 824: volRunDir = os.path.join(imgRunDir, volUUID)
> volSymlink, this is not a directory
Done
Line 825: try:
Line 826: self.log.info("Unlinking volme runtime dir: %s", 
volRunDir)
Line 827: os.unlink(volRunDir)
Line 828: except OSError as e:


Line 822: sdRunDir = os.path.join(constants.P_VDSM_STORAGE, self.sdUUID)
Line 823: imgRunDir = os.path.join(sdRunDir, imgUUID)
Line 824: volRunDir = os.path.join(imgRunDir, volUUID)
Line 825: try:
Line 826: self.log.info("Unlinking volme runtime dir: %s", 
volRunDir)
> The log cannot raise OSError, it should always be out of the try block.
Done
Line 827: os.unlink(volRunDir)
Line 828: except OSError as e:
Line 829: if e.error == errno.ENOENT:
Line 830: self.log.debug("Link doesn't exist")


Line 830: self.log.debug("Link doesn't exist")
Line 831: else:
Line 832: self.log.error("Failed to unlink vol runtime dir: %s",
Line 833:volRunDir)
Line 834: raise
> Logging and raising is bad practice. If you cannot handle the error, raise,
Done
Line 835: 
Line 836: 
Line 837: class BlockStorageDomain(sd.StorageDomain):
Line 838: manifestClass = BlockStorageDomainManifest


https://gerrit.ovirt.org/#/c/59725/6/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4781: self.success = True
Line 4782: self.vm.log.info("Synchronization completed (job %s)",
Line 4783:  self.job['jobID'])
Line 4784: # teardown the merged volume
Line 4785: self.teardownVolume(self.drive.imageID, 
self.job['topVolume'])
> All these changes belong to the previous patch.
Done
Line 4786: 
Line 4787: def isSuccessful(self):
Line 4788: """
Line 4789: Returns True if this phase completed successfully.


-- 
To view, visit https://gerrit.ovirt.org/59725
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer

Change in vdsm[master]: Live Merge: teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: teardown volume on HSM after live merge
..


Patch Set 1:

(8 comments)

https://gerrit.ovirt.org/#/c/64301/1/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 810: offset = ((slot + blockVolume.RESERVED_LEASES) * 
self.logBlkSize *
Line 811:   sd.LEASE_BLOCKS)
Line 812: return clusterlock.Lease(volUUID, self.getLeasesFilePath(), 
offset)
Line 813: 
Line 814: def teardownVolume(self, volUUID):
> Add image uuid, standard api for volume methods.
Done
Line 815: lvm.deactivateLVs(self.sdUUID, [volUUID])
Line 816: 
Line 817: 
Line 818: class BlockStorageDomain(sd.StorageDomain):


https://gerrit.ovirt.org/#/c/64301/1/vdsm/storage/sd.py
File vdsm/storage/sd.py:

Line 524: 
Line 525: if preallocate is not None and preallocate not in sc.VOL_TYPE:
Line 526: raise se.IncorrectType(preallocate)
Line 527: 
Line 528: def teardownVolume(self, volUUID):
> Volume is defined by domain id, image id and volume id. Please add the imag
Done
Line 529: """
Line 530: By default, this method does nothing. It is overriden in 
blockSD
Line 531: to do the necessary cleaning.
Line 532: """


Line 527: 
Line 528: def teardownVolume(self, volUUID):
Line 529: """
Line 530: By default, this method does nothing. It is overriden in 
blockSD
Line 531: to do the necessary cleaning.
> Replace the text with something like:
Done
Line 532: """
Line 533: pass
Line 534: 
Line 535: 


Line 529: """
Line 530: By default, this method does nothing. It is overriden in 
blockSD
Line 531: to do the necessary cleaning.
Line 532: """
Line 533: pass
> pass is not needed, having a docstring is enough.
Done
Line 534: 
Line 535: 
Line 536: class StorageDomain(object):
Line 537: log = logging.getLogger("storage.StorageDomain")


https://gerrit.ovirt.org/#/c/64301/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 64: from vdsm.virt.vmpowerdown import VmShutdown, VmReboot
Line 65: from vdsm.virt.utils import isVdsmImage, cleanup_guest_socket
Line 66: from storage import outOfProcess as oop
Line 67: from storage import sd
Line 68: from storage.sdc import sdCache
> Please avoid this bad practice - this must be:
Done
Line 69: 
Line 70: # local imports
Line 71: # In future those should be imported via ..
Line 72: import caps


Line 4763: self.vm._setVolumeSize(self.drive.domainID, 
self.drive.poolID,
Line 4764:self.drive.imageID, baseVolUUID,
Line 4765:topVolInfo['capacity'])
Line 4766: 
Line 4767: def teardownVolume(self, volUUID):
> This should match other methods like update_base_size - no arguments, lower
Done
Line 4768: dom_manifest = sdCache.produce_manifest(self.drive.domainID)
Line 4769: dom_manifest.teardownVolume(volUUID)
Line 4770: 
Line 4771: @utils.traceback()


Line 4764:self.drive.imageID, baseVolUUID,
Line 4765:topVolInfo['capacity'])
Line 4766: 
Line 4767: def teardownVolume(self, volUUID):
Line 4768: dom_manifest = sdCache.produce_manifest(self.drive.domainID)
> We call this sd_manifest elsewhere.
Done
Line 4769: dom_manifest.teardownVolume(volUUID)
Line 4770: 
Line 4771: @utils.traceback()
Line 4772: def run(self):


Line 4780: self.vm.enableDriveMonitor()
Line 4781: self.success = True
Line 4782: self.vm.log.info("Synchronization completed (job %s)",
Line 4783:  self.job['jobID'])
Line 4784: # teardown the merged volume
> The comment is not needed, the function name should reveal the intent. If y
Done
Line 4785: self.teardownVolume(self.job['topVolume'])
Line 4786: 
Line 4787: def isSuccessful(self):
Line 4788: """


-- 
To view, visit https://gerrit.ovirt.org/64301
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Unlink volume runtime dir after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Unlink volume runtime dir after live merge
..


Patch Set 6: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/59725
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: teardown volume on HSM after live merge
..


Patch Set 1: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/64301
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: teardown volume on HSM after live merge

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: Live Merge: teardown volume on HSM after live merge
..

Live Merge: teardown volume on HSM after live merge

If a VM is running on HSM and live merge is performed, the LV isn't
deactivated because, the deactivation is done when deleting the volume.
However, deleting the volume is done on SPM and this means that the LV
is not deactivated on the HSM. In this patch, a logic to teardown the
volume added after live merge has completed.

Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Bug-Url: https://bugzilla.redhat.com/1377849
Signed-off-by: Ala Hino 
---
M vdsm/storage/blockSD.py
M vdsm/storage/sd.py
M vdsm/virt/vm.py
3 files changed, 17 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/01/64301/1

diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py
index 8ac7433..a538581 100644
--- a/vdsm/storage/blockSD.py
+++ b/vdsm/storage/blockSD.py
@@ -811,6 +811,9 @@
   sd.LEASE_BLOCKS)
 return clusterlock.Lease(volUUID, self.getLeasesFilePath(), offset)
 
+def teardownVolume(self, volUUID):
+lvm.deactivateLVs(self.sdUUID, [volUUID])
+
 
 class BlockStorageDomain(sd.StorageDomain):
 manifestClass = BlockStorageDomainManifest
diff --git a/vdsm/storage/sd.py b/vdsm/storage/sd.py
index b5b902e..29dcf53 100644
--- a/vdsm/storage/sd.py
+++ b/vdsm/storage/sd.py
@@ -525,6 +525,13 @@
 if preallocate is not None and preallocate not in sc.VOL_TYPE:
 raise se.IncorrectType(preallocate)
 
+def teardownVolume(self, volUUID):
+"""
+By default, this method does nothing. It is overriden in blockSD
+to do the necessary cleaning.
+"""
+pass
+
 
 class StorageDomain(object):
 log = logging.getLogger("storage.StorageDomain")
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 1d9a3d5..7004615 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -65,6 +65,7 @@
 from vdsm.virt.utils import isVdsmImage, cleanup_guest_socket
 from storage import outOfProcess as oop
 from storage import sd
+from storage.sdc import sdCache
 
 # local imports
 # In future those should be imported via ..
@@ -4763,6 +4764,10 @@
self.drive.imageID, baseVolUUID,
topVolInfo['capacity'])
 
+def teardownVolume(self, volUUID):
+dom_manifest = sdCache.produce_manifest(self.drive.domainID)
+dom_manifest.teardownVolume(volUUID)
+
 @utils.traceback()
 def run(self):
 self.update_base_size()
@@ -4776,6 +4781,8 @@
 self.success = True
 self.vm.log.info("Synchronization completed (job %s)",
  self.job['jobID'])
+# teardown the merged volume
+self.teardownVolume(self.job['topVolume'])
 
 def isSuccessful(self):
 """


-- 
To view, visit https://gerrit.ovirt.org/64301
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: teardown volume on HSM after live merge
..


Patch Set 4: Verified-1

-- 
To view, visit https://gerrit.ovirt.org/59725
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: teardown volume on HSM after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: teardown volume on HSM after live merge
..


Patch Set 4: Verified+1

Need to verify wipe after delete logic when deactivating the LV before deleting 
the volume

-- 
To view, visit https://gerrit.ovirt.org/59725
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: tests: Rename qemuimgTests.py to new Vdsm convention

[ahino]

Ala Hino has posted comments on this change.

Change subject: tests: Rename qemuimgTests.py to new Vdsm convention
..


Patch Set 3: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/64203
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcdf2cd43d681777c1fa5075a86ee044b378d840
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: [WIP] core: Expose API for qemu-img commit

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: [WIP] core: Expose API for qemu-img commit
..

[WIP] core: Expose API for qemu-img commit

Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Signed-off-by: Ala Hino 
---
M lib/vdsm/qemuimg.py
M tests/qemuimg_test.py
2 files changed, 35 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/22/64222/1

diff --git a/lib/vdsm/qemuimg.py b/lib/vdsm/qemuimg.py
index 61ebec3..4ad7073 100644
--- a/lib/vdsm/qemuimg.py
+++ b/lib/vdsm/qemuimg.py
@@ -185,6 +185,16 @@
 return QemuImgOperation(cmd, cwd=cwdPath)
 
 
+def commit(top, base=None):
+cmd = [_qemuimg.cmd, "commit", "-p"]
+if base:
+cmd.extend(("-b", base))
+else:
+cmd.append("-d")
+
+return QemuImgOperation(cmd)
+
+
 class QemuImgOperation(object):
 REGEXPR = re.compile(r'\s*\(([\d.]+)/100%\)\s*')
 
diff --git a/tests/qemuimg_test.py b/tests/qemuimg_test.py
index d7d9fab..3fd97ea 100644
--- a/tests/qemuimg_test.py
+++ b/tests/qemuimg_test.py
@@ -23,6 +23,7 @@
 from functools import partial
 
 from monkeypatch import MonkeyPatch, MonkeyPatchScope
+from storagetestlib import qemu_pattern_write, qemu_pattern_verify
 from testlib import VdsmTestCase as TestCaseBase
 from testlib import permutations, expandPermutations
 from testlib import make_config
@@ -329,3 +330,27 @@
 
 p.poll()
 self.assertEquals(p.finished, True)
+
+class TestCommit(TestCaseBase):
+
+@MonkeyPatch(qemuimg, 'config', CONFIG)
+def test_commit(self):
+with namedTemporaryDir() as tmpdir:
+base = os.path.join(tmpdir, 'base.img')
+top = os.path.join(tmpdir, 'top.img')
+size = 1048576
+qemuimg.create(base, size=size, format=qemuimg.FORMAT.RAW)
+qemu_pattern_write(base, qemuimg.FORMAT.RAW, '0', 0xf0)
+
+qemuimg.create(top, format=qemuimg.FORMAT.QCOW2, backing=base)
+qemu_pattern_write(top, qemuimg.FORMAT.QCOW2, '1024', 0xf1)
+
+op = qemuimg.commit(top=top)
+op.wait_for_completion()
+
+# top should not change after commit
+qemu_pattern_verify(top, qemuimg.FORMAT.RAW, '0', 0xf1)
+
+# base now should include original data and top's data
+qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '0', 0xf0)
+qemu_pattern_verify(base, qemuimg.FORMAT.RAW, '1024', 0xf1)


-- 
To view, visit https://gerrit.ovirt.org/64222
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: tests: Rename qemuimgTests.py to new Vdsm convention

[ahino]

Ala Hino has posted comments on this change.

Change subject: tests: Rename qemuimgTests.py to new Vdsm convention
..


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/64203/2/tests/Makefile.am
File tests/Makefile.am:

Line 97:persistentDictTests.py \
Line 98:properties_test.py \
Line 99:protocoldetectorTests.py \
Line 100:   pthreadTests.py \
Line 101:   qemuimg_test.py.py \
> py.py is little repetitive, and makes make check grumpy.
Done
Line 102:   resourceManagerTests.py \
Line 103:   responseTests.py \
Line 104:   rngsources_test.py \
Line 105:   samplingTests.py \


-- 
To view, visit https://gerrit.ovirt.org/64203
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcdf2cd43d681777c1fa5075a86ee044b378d840
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: tests: Rename qemuimgTests.py to new Vdsm convention

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: tests: Rename qemuimgTests.py to new Vdsm convention
..

tests: Rename qemuimgTests.py to new Vdsm convention

Change-Id: Ifcdf2cd43d681777c1fa5075a86ee044b378d840
Signed-off-by: Ala Hino 
---
M tests/Makefile.am
R tests/qemuimg_test.py
2 files changed, 2 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/03/64203/1

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 35248a0..71639b7 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -98,7 +98,7 @@
properties_test.py \
protocoldetectorTests.py \
pthreadTests.py \
-   qemuimgTests.py \
+   qemuimg_test.py.py \
resourceManagerTests.py \
responseTests.py \
rngsources_test.py \
@@ -209,7 +209,7 @@
persistentDictTests.py \
properties_test.py \
protocoldetectorTests.py \
-   qemuimgTests.py \
+   qemuimg_test.py.py \
resourceManagerTests.py \
samplingTests.py \
schemaTests.py \
diff --git a/tests/qemuimgTests.py b/tests/qemuimg_test.py
similarity index 100%
rename from tests/qemuimgTests.py
rename to tests/qemuimg_test.py


-- 
To view, visit https://gerrit.ovirt.org/64203
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifcdf2cd43d681777c1fa5075a86ee044b378d840
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: sdm: Introduce new SDM.merge verb

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: sdm: Introduce new SDM.merge verb
..

sdm: Introduce new SDM.merge verb

This API is another data operation separate from SPM. It merges data
from top volume to base volume and it replaces the existing
'mergeSnapshots' verb.

This verb will be used in the new cold merge flow:

1. Extend base volume (runs on the SPM)
2. Merge (runs on any host)
3. Shrink base volume to optimal size (runs on the SPM)

Change-Id: I96d57a5b9f21153ce1de2cd5619c7f9f78bbe75b
Signed-off-by: Ala Hino 
---
M lib/api/vdsm-api.yml
M vdsm/API.py
M vdsm/storage/hsm.py
3 files changed, 23 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/96/64196/1

diff --git a/lib/api/vdsm-api.yml b/lib/api/vdsm-api.yml
index c7ab7f5..0745093 100644
--- a/lib/api/vdsm-api.yml
+++ b/lib/api/vdsm-api.yml
@@ -9905,3 +9905,19 @@
 -   description: The destination endpoint
 name: destination
 type: *CopyDataEndpoint
+
+SDM.merge:
+added: '4.1'
+description: Merge data from top volume to base volume.
+params:
+-   description: A UUID to be used for tracking the job progress
+name: job_id
+type: *UUID
+
+-   description: The base volume
+name: base
+type: *CopyDataDivEndpoint
+
+-   description: The top volume
+name: top
+type: *CopyDataDivEndpoint
diff --git a/vdsm/API.py b/vdsm/API.py
index f000297..267aad7 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -1636,3 +1636,6 @@
 
 def copy_data(self, job_id, source, destination):
 return self._irs.sdm_copy_data(job_id, source, destination)
+
+def merge(self, job_id, base, top):
+return self._irs.sdm_merge(job_id, base, top)
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index d1b50d2..faf3dfb 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -3530,3 +3530,7 @@
 job = sdm.api.copy_data.Job(job_id, self._get_hostid(),
 source, destination)
 self.sdm_schedule(job)
+
+@public
+def sdm_merge(self, job_id, base, top):
+raise NotImplementedError


-- 
To view, visit https://gerrit.ovirt.org/64196
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I96d57a5b9f21153ce1de2cd5619c7f9f78bbe75b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[ovirt-3.6]: Live Merge: Refresh base volume before live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Refresh base volume before live merge
..


Patch Set 2: Verified+1

Verified following scenario on two different VMs - one running on SPM and other 
in HSM:
1. create a VM
2. add 1gb disk
3. create a snapshot
4. extend the disk by 1gb
5. run the VM
6. remove the snapshot

On both VMs, removing the snapshot successfully completed.

-- 
To view, visit https://gerrit.ovirt.org/63634
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Allon Mureinik 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: sd: Remove unused variable and evil thread

[ahino]

Ala Hino has posted comments on this change.

Change subject: sd: Remove unused variable and evil thread
..


Patch Set 2: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/63520
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2f17e591bcd03b83a0df5b8e2a31835bcd572064
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[ovirt-4.0]: Live Merge: Refresh base volume before live merge

[ahino]

Hello Nir Soffer,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/63532

to review the following change.

Change subject: Live Merge: Refresh base volume before live merge
..

Live Merge: Refresh base volume before live merge

When live merging raw base volume, engine extends the base volume
if it is smaller than the top volume. However, on the host running
the vm, the lv is already active and does not reflect the new size
until we refresh it.  During the merge we don't know whether base
volume extended, so we always refresh it.

Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Bug-Url: https://bugzilla.redhat.com/1367281
Signed-off-by: Ala Hino 
Reviewed-on: https://gerrit.ovirt.org/63454
Reviewed-by: Nir Soffer 
Continuous-Integration: Nir Soffer 
---
M vdsm/virt/vm.py
1 file changed, 19 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/32/63532/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 7fdcacf..a896f58 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -4590,6 +4590,25 @@
 if not self._can_merge_into(drive, baseInfo, topInfo):
 return errCode['destVolumeTooSmall']
 
+# If the base volume format is RAW and its size is smaller than its
+# capacity (this could happen because the engine extended the base
+# volume), we have to refresh the volume to cause lvm to get current lv
+# size from storage, and update the kernel so the lv reflects the real
+# size on storage. Not refreshing the volume may fail live merge.
+# This could happen if disk extended after taking a snapshot but before
+# performing the live merge.  See https://bugzilla.redhat.com/1367281
+if (drive.chunked and
+baseInfo['format'] == 'RAW' and
+int(baseInfo['apparentsize']) < int(baseInfo['capacity'])):
+self.log.info("Refreshing raw volume %r (apparentsize=%s, "
+  "capacity=%s)",
+  baseVolUUID, baseInfo['apparentsize'],
+  baseInfo['capacity'])
+self.__refreshDriveVolume({
+'domainID': drive.domainID, 'poolID': drive.poolID,
+'imageID': drive.imageID, 'volumeID': baseVolUUID,
+})
+
 # Take the jobs lock here to protect the new job we are tracking from
 # being cleaned up by queryBlockJobs() since it won't exist right away
 with self._jobsLock:


-- 
To view, visit https://gerrit.ovirt.org/63532
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Nir Soffer 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[ovirt-4.0]: Live Merge: Refresh base volume before live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Refresh base volume before live merge
..


Patch Set 1: Verified+1

Verified on master

-- 
To view, visit https://gerrit.ovirt.org/63532
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Refresh base volume before live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Refresh base volume before live merge
..


Patch Set 8: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/63454
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Refresh base volume before live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Refresh base volume before live merge
..


Patch Set 7:

> Please also rebase, the failing network test is hopefully fixed in
 > master.

done

-- 
To view, visit https://gerrit.ovirt.org/63454
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Refresh base volume before live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Refresh base volume before live merge
..


Patch Set 6:

(3 comments)

https://gerrit.ovirt.org/#/c/63454/6/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4442: # performing the live merge.  See 
https://bugzilla.redhat.com/1367281
Line 4443: if (drive.chunked and
Line : baseInfo['format'] == 'RAW' and
Line 4445: int(baseInfo['apparentsize']) < 
int(baseInfo['capacity'])):
Line 4446: self.log.info("Refreshing raw volume '%s' 
(apparentsize='%d', "
> To get quoted value for strings, use %r
Done
Line 4447:   "capacity='%d')",
Line 4448:   baseVolUUID, int(baseInfo['apparentsize']),
Line 4449:   int(baseInfo['capacity']))
Line 4450: self.__refreshDriveVolume({


Line 4443: if (drive.chunked and
Line : baseInfo['format'] == 'RAW' and
Line 4445: int(baseInfo['apparentsize']) < 
int(baseInfo['capacity'])):
Line 4446: self.log.info("Refreshing raw volume '%s' 
(apparentsize='%d', "
Line 4447:   "capacity='%d')",
> We don't need the quotes around the value.
Done
Line 4448:   baseVolUUID, int(baseInfo['apparentsize']),
Line 4449:   int(baseInfo['capacity']))
Line 4450: self.__refreshDriveVolume({
Line 4451: 'domainID': drive.domainID, 'poolID': drive.poolID,


Line 4445: int(baseInfo['apparentsize']) < 
int(baseInfo['capacity'])):
Line 4446: self.log.info("Refreshing raw volume '%s' 
(apparentsize='%d', "
Line 4447:   "capacity='%d')",
Line 4448:   baseVolUUID, int(baseInfo['apparentsize']),
Line 4449:   int(baseInfo['capacity']))
> We don't need to convert again to int, using the string value is good for t
Done
Line 4450: self.__refreshDriveVolume({
Line 4451: 'domainID': drive.domainID, 'poolID': drive.poolID,
Line 4452: 'imageID': drive.imageID, 'volumeID': baseVolUUID,
Line 4453: })


-- 
To view, visit https://gerrit.ovirt.org/63454
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Refresh base volume before live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Refresh base volume before live merge
..


Patch Set 6: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/63454
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Refresh base volume before live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Refresh base volume before live merge
..


Patch Set 5:

(5 comments)

https://gerrit.ovirt.org/#/c/63454/5/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4436: # If the base volume format is RAW and its size is smaller 
than its
Line 4437: # capacity (this could happen because the engine extended 
the base
Line 4438: # volume), we have to refresh the volume to cause lvm to get 
current lv
Line 4439: # size from storage, and update the kernel so the lv 
reflects the real
Line 4440: # size on storage. Not refreshing the volume may fail live 
merre
> merre -> merge
Done
Line 4441: # because the top volume size may be larger then the base 
volume size.
Line 4442: # This could happen if disk extended after taking a snapshot 
but before
Line 4443: # performing the live merge.  See 
https://bugzilla.redhat.com/1367281
Line : baseSize = int(baseInfo['apparentsize'])


Line 4439: # size from storage, and update the kernel so the lv 
reflects the real
Line 4440: # size on storage. Not refreshing the volume may fail live 
merre
Line 4441: # because the top volume size may be larger then the base 
volume size.
Line 4442: # This could happen if disk extended after taking a snapshot 
but before
Line 4443: # performing the live merge.  See 
https://bugzilla.redhat.com/1367281
> This comment became too long, the first part explain the issue very clearly
Done
Line : baseSize = int(baseInfo['apparentsize'])
Line 4445: baseCapacity = int(baseInfo['capacity'])
Line 4446: if (drive.chunked
Line 4447: and baseInfo['format'] == 'RAW'


Line 4441: # because the top volume size may be larger then the base 
volume size.
Line 4442: # This could happen if disk extended after taking a snapshot 
but before
Line 4443: # performing the live merge.  See 
https://bugzilla.redhat.com/1367281
Line : baseSize = int(baseInfo['apparentsize'])
Line 4445: baseCapacity = int(baseInfo['capacity'])
> Please avoid these variables, in particular using "size" for "apparentsize"
Done
Line 4446: if (drive.chunked
Line 4447: and baseInfo['format'] == 'RAW'
Line 4448: and baseSize < baseCapacity):
Line 4449: self.log.info("Base volume is RAW and its size is 
smaller "


Line 4443: # performing the live merge.  See 
https://bugzilla.redhat.com/1367281
Line : baseSize = int(baseInfo['apparentsize'])
Line 4445: baseCapacity = int(baseInfo['capacity'])
Line 4446: if (drive.chunked
Line 4447: and baseInfo['format'] == 'RAW'
> Always put "and" at the end of the previous line:
Done
Line 4448: and baseSize < baseCapacity):
Line 4449: self.log.info("Base volume is RAW and its size is 
smaller "
Line 4450:   "than its capacity hence, explicitly "
Line 4451:   "refreshing the base volume is required")


Line 4447: and baseInfo['format'] == 'RAW'
Line 4448: and baseSize < baseCapacity):
Line 4449: self.log.info("Base volume is RAW and its size is 
smaller "
Line 4450:   "than its capacity hence, explicitly "
Line 4451:   "refreshing the base volume is required")
> This is too verbose and not detailed enough - use something like:
Done
Line 4452: self.__refreshDriveVolume({
Line 4453: 'domainID': drive.domainID, 'poolID': drive.poolID,
Line 4454: 'imageID': drive.imageID, 'volumeID': baseVolUUID,
Line 4455: })


-- 
To view, visit https://gerrit.ovirt.org/63454
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Refresh base volume before live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Refresh base volume before live merge
..


Patch Set 5:

> (1 comment)

already done; see latest patch set

-- 
To view, visit https://gerrit.ovirt.org/63454
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Refresh base volume before live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Refresh base volume before live merge
..


Patch Set 4:

> (1 comment)

I found that in this case, volume capacity > volume apparent size

-- 
To view, visit https://gerrit.ovirt.org/63454
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Refresh base volume before live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Refresh base volume before live merge
..


Patch Set 4: Verified+1

In a deployment with two hosts, verified the following scenario that described 
in the BZ:
1. create a vm
2. add a 1gb disk
3. create a snapshot
4. extend the disk by 1gb
5. start the vm
6. delete the snapshot

Verified using 2 VMs, one running on the SPM and one on the HSM. In both cases, 
issue resolved.

-- 
To view, visit https://gerrit.ovirt.org/63454
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Refresh base volume before live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Refresh base volume before live merge
..


Patch Set 3:

(3 comments)

https://gerrit.ovirt.org/#/c/63454/3//COMMIT_MSG
Commit Message:

Line 9: When performing live merge to a RAW base volume, we have to
Line 10: refresh the volume before performing the live merge. This is
Line 11: needed because extending the base volume is done on the SPM and
Line 12: refresh volume will cause other hosts to refresh the volume to get
Line 13: the new size. Not doing so may end up with libvirt error
> I don't understand this part.
Done
Line 14: indicating that top volume size is larger than base volume, which
Line 15: exactly what happened in the reported BZ.
Line 16: 
Line 17: Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be


Line 11: needed because extending the base volume is done on the SPM and
Line 12: refresh volume will cause other hosts to refresh the volume to get
Line 13: the new size. Not doing so may end up with libvirt error
Line 14: indicating that top volume size is larger than base volume, which
Line 15: exactly what happened in the reported BZ.
> Also note that we don't know if the base was extended when starting a merge
Done
Line 16: 
Line 17: Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Line 18: Bug-Url: https://bugzilla.redhat.com/1367281


https://gerrit.ovirt.org/#/c/63454/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4433: if not self._can_merge_into(drive, baseInfo, topInfo):
Line 4434: return errCode['destVolumeTooSmall']
Line 4435: 
Line 4436: # If the base volume format is RAW, we have to refresh the 
volume
Line 4437: # in order to enforce other hosts to load new volume size. 
Not
> Refreshing the lv effect only this host, causing lvm to get the current lv 
Done
Line 4438: # doing so may end up with error that top volume size is 
larger
Line 4439: # base volume size. This could happen if disk extended after 
taking
Line 4440: # a snapshot but before performing the live merge.
Line 4441: # See https://bugzilla.redhat.com/1367281


-- 
To view, visit https://gerrit.ovirt.org/63454
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Refresh base volume before live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Refresh base volume before live merge
..


Patch Set 2:

(2 comments)

https://gerrit.ovirt.org/#/c/63454/2//COMMIT_MSG
Commit Message:

PS2, Line 10: there is a single snapshot
> This is not required - you can have an entire chain s4->s3->s2->s1->base an
Done


https://gerrit.ovirt.org/#/c/63454/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS2, Line 4442: if drive.chunked and baseInfo['format'] == 'RAW':
> This will always refresh a raw volume. Should we also check that the sizes 
The size will be equal as the engine already extended the base volume


-- 
To view, visit https://gerrit.ovirt.org/63454
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: core: Refresh base volume before live merge

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: core: Refresh base volume before live merge
..

core: Refresh base volume before live merge

When performing live merge where the base volume format is RAW,
i.e. there is a single snapshot and it is live merged, we have to
refresh the volume before performing the live merge. This is
needed because extending the base volume is done on the SPM and
refresh volume will cause other hosts to refresh the volume to get
the new size. Not doing so may end up with libvirt error
indicating that top volume size is larger than base volume, which
exactly what happened in the reported BZ.

Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Bug-Url: https://bugzilla.redhat.com/1367281
Signed-off-by: Ala Hino 
---
M vdsm/virt/vm.py
1 file changed, 12 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/54/63454/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 9cfa4a3..01b5d21 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -4433,6 +4433,18 @@
 if not self._can_merge_into(drive, baseInfo, topInfo):
 return errCode['destVolumeTooSmall']
 
+# If the base volume format is RAW, we have to refresh the volume
+# in order to enforce other hosts to load new volume size. Not
+# doing so may end up with error that top volume size is larger
+# base volume size. This could happen if disk extended after taking
+# a snapshot but before performing the live merge.
+# See https://bugzilla.redhat.com/1367281
+if drive.chunked and baseInfo['format'] == 'RAW':
+self.__refreshDriveVolume({
+'domainID': drive.domainID, 'poolID': drive.poolID,
+'imageID': drive.imageID, 'volumeID': baseVolUUID,
+})
+
 # Take the jobs lock here to protect the new job we are tracking from
 # being cleaned up by queryBlockJobs() since it won't exist right away
 with self._jobsLock:


-- 
To view, visit https://gerrit.ovirt.org/63454
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I787d6854e780035b09e4f09d71ca776342dff5be
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: core: Use constant instead of text

[ahino]

Ala Hino has abandoned this change.

Change subject: core: Use constant instead of text
..


Abandoned

squashed to https://gerrit.ovirt.org/60889

-- 
To view, visit https://gerrit.ovirt.org/62366
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I8e3b4d519140d570103eef09e76776a7330c568c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: core: Use constant instead of text

[ahino]

Ala Hino has posted comments on this change.

Change subject: core: Use constant instead of text
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/62366/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS1, Line 4752: vol_format = sc.name2type(baseInfo['format'])
> If baseInfo['format'] is a string (like COW or RAW) then just compare again
Done


-- 
To view, visit https://gerrit.ovirt.org/62366
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e3b4d519140d570103eef09e76776a7330c568c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Restore watermark tracking

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 14:

(1 comment)

https://gerrit.ovirt.org/#/c/60889/14/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS14, Line 1007: path
> Maybe change to '.path' to be safer?
Done


-- 
To view, visit https://gerrit.ovirt.org/60889
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I52199bb38a2d3f439d33b9e78743c3e611349672
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sampling: Rename stats_flags variable

[ahino]

Ala Hino has abandoned this change.

Change subject: sampling: Rename stats_flags variable
..


Abandoned

Based on Francesco comment, this is by design

-- 
To view, visit https://gerrit.ovirt.org/60862
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: Iaca95656038489f1ce4286e32ab819d78b9524dd
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Restore watermark tracking

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 13:

(3 comments)

https://gerrit.ovirt.org/#/c/60889/13/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS13, Line 993: volUUID
> As Francesco suggested, let's use vol_id instead of volUUID.
Done


PS13, Line 1020: pathToVolID
> Looking forward to you reusing the other function that Francesco pointed ou
Done


PS13, Line 4752: COW'
> Maybe we should also use the storage constant here too.
in a separate patch


-- 
To view, visit https://gerrit.ovirt.org/60889
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I52199bb38a2d3f439d33b9e78743c3e611349672
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: core: Use constant instead of text

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: core: Use constant instead of text
..

core: Use constant instead of text

Use storage constants instead of text when checking volume format

Change-Id: I8e3b4d519140d570103eef09e76776a7330c568c
Signed-off-by: Ala Hino 
---
M vdsm/virt/vm.py
1 file changed, 2 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/66/62366/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index d66e4cb..5f097d7 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -4749,7 +4749,8 @@
 self.untrackBlockJob(jobUUID)
 return response.error('mergeErr')
 
-if drive.chunked and baseInfo['format'] == 'COW':
+vol_format = sc.name2type(baseInfo['format'])
+if drive.chunked and vol_format == sc.COW_FORMAT:
 # We try to get volume alloc value from libvirt
 alloc = self._getVolumeWriteWatermarks(drive, baseVolUUID)
 if not alloc:


-- 
To view, visit https://gerrit.ovirt.org/62366
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8e3b4d519140d570103eef09e76776a7330c568c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sampling: Retrieve backing chain stats

[ahino]

Ala Hino has posted comments on this change.

Change subject: sampling: Retrieve backing chain stats
..


Patch Set 4: Verified-1

-1 for visibility:
once this patch is +1ed one, I will run performance tests in order to verify no 
performance degradation introduced with this change

-- 
To view, visit https://gerrit.ovirt.org/60888
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3bbb8643d1c86e90d1e2de7cb2a5b00116c71453
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sampling: Retrieve backing chain stats

[ahino]

Ala Hino has posted comments on this change.

Change subject: sampling: Retrieve backing chain stats
..


Patch Set 3:

(7 comments)

https://gerrit.ovirt.org/#/c/60888/3/lib/vdsm/virt/sampling.py
File lib/vdsm/virt/sampling.py:

Line 30: import re
Line 31: import threading
Line 32: import time
Line 33: 
Line 34: # 3rd party libs imports
> this comment is redundant. I don't mind that much, but plase kill it if you
Done
Line 35: import libvirt
Line 36: 
Line 37: from vdsm import numa
Line 38: from vdsm import utils


PS3, Line 502: extra_flags
> we need some data about the performance cost of this call.
sure thing. Once all comments are addressed and the patches +1ed I will run 
performance tests and provide data


PS3, Line 511: extra_flags
> I think this is going to blow up with UnboundLocalError or NameError. Just 
good catch; fixed


https://gerrit.ovirt.org/#/c/60888/1/lib/vdsm/virt/vmstats.py
File lib/vdsm/virt/vmstats.py:

Line 43: cpu(stats, first_sample, last_sample, interval)
Line 44: networks(vm, stats, first_sample, last_sample, interval)
Line 45: disks(vm, stats, first_sample, last_sample, interval)
Line 46: balloon(vm, stats, last_sample)
Line 47: cpu_count(stats, last_sample)
> not sure this fits here. We should use this only in the extension flow, not
removed to vm.py (separate patch that calculates live merge extend candidates)
Line 48: tune_io(vm, stats)
Line 49: 
Line 50: return stats
Line 51: 


Line 353: 
Line 354: 
Line 355: def _disk_rate(first_sample, first_index, last_sample, last_index, 
interval):
Line 356: stats = {}
Line 357: 
> please add a test (or few :) about this, mostly to let us see how it will l
removed
Line 358: for name, mode in (("readRate", "rd"), ("writeRate", "wr")):
Line 359: first_key = 'block.%d.%s.bytes' % (first_index, mode)
Line 360: last_key = 'block.%d.%s.bytes' % (last_index, mode)
Line 361: try:


PS1, Line 357: 
> you use only 'last_sample' and 'stats'. This is fine, but then you should d
removed


PS1, Line 372: stats = {}
> if we extract this from the reporting flow (e.g. not call this in produce()
removed


-- 
To view, visit https://gerrit.ovirt.org/60888
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3bbb8643d1c86e90d1e2de7cb2a5b00116c71453
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vm: refactor: Move pathToVolID method

[ahino]

Ala Hino has posted comments on this change.

Change subject: vm: refactor: Move pathToVolID method
..


Patch Set 1:

(3 comments)

https://gerrit.ovirt.org/#/c/62180/1//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2016-08-08 18:27:21 +0300
Line 4: Commit: Ala Hino 
Line 5: CommitDate: 2016-08-08 20:35:12 +0300
Line 6: 
Line 7: core: refactor: Move pathToVolID method
> vm: refactor: move...
Done
Line 8: 
Line 9: Move pathToVolID method from _diskXMLGetVolumeChainInfo method to
Line 10: module scope in order to reuse it in commit 8c52769(Live Merge:
Line 11: Restore watermark tracking)


PS1, Line 10: 8c52769
> commit ID are going to change, courtesy of how gerrit works.
Done


https://gerrit.ovirt.org/#/c/62180/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS1, Line 156: pathToVolID
> we can afford the un-shortened name, and to use pep8 names:
Done


-- 
To view, visit https://gerrit.ovirt.org/62180
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d8ef439120b8dff79cda9fcff7c42040f75e35e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Restore watermark tracking

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 10:

(10 comments)

https://gerrit.ovirt.org/#/c/60889/10/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS10, Line 197: _pathToVolumeID
> this is a duplicate of _pathToVolID, look in _diskXMLGetVolumeChainInfo.
Looks same logic. I wasn't aware of that method.
In a separate patch, I will move pathToVolID so I can reuse it here


PS10, Line 199: if vol["path"].split('/')[-1] == path.split('/')[-1]:
> This makes assumptions about the format of path strings which is not allowe
This comment isn't relevant anymore because I will remove this method and use 
pathToVolID


Line 935: except libvirt.libvirtError as e:
Line 936: self.log.error("Unable to get watermarks for drive 
%s: %s",
Line 937:drive.name, e)
Line 938: continue
Line 939: 
> Nice :)
:)
Line 940: return ret
Line 941: 
Line 942: def _getLiveMergeExtendCandidates(self):
Line 943: candidates = []


PS10, Line 977: 'cow'
> I think there are constants for this somewhere in the storage subtree
used this instead:
if sc.name2type(volInfo['format']) == sc.COW_FORMAT


Line 991:  volUUID)
Line 992: 
Line 993: return candidates
Line 994: 
Line 995: def _getVolumeWriteWatermarks(self, drive, volUUID):
> Looks good!  Simpler to follow since we're looking for just one volume now.
Done
Line 996: vmSample = sampling.stats_cache.get(self.id)
Line 997: lastSample = vmSample.last_value
Line 998: keys = [key for key in lastSample if key.endswith('path')]
Line 999: for key in keys:


PS10, Line 996: vmSample
> minor nit: let's use lower_case_identifiers for new code. Same for lastSamp
Done


PS10, Line 998: keys = [key for key in lastSample if 
key.endswith('path')]
  : for key in keys:
> minor:
Done


PS10, Line 1000: prefix, index, attr = key.split(".", 2)
   : try:
   : name = lastSample['block.%s.name' % index]
   : except KeyError:
   : self.log.warning("Drive %s is not in bulk 
stats, skipping",
   :  name)
   : continue
   : 
   : if name != drive.name:
   : continue
> would it be feasible and safe to just call
done as suggested


PS10, Line 4744: drive.imageID
> I'm baffled by the use of imageID.  Isn't that supposed to take a volumeID?
changed to baseVolUUID


PS10, Line 4744: drive.imageID
> Same questions here
changed to baseVolUUID


-- 
To view, visit https://gerrit.ovirt.org/60889
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I52199bb38a2d3f439d33b9e78743c3e611349672
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: core: refactor: Move pathToVolID method

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: core: refactor: Move pathToVolID method
..

core: refactor: Move pathToVolID method

Move pathToVolID method from _diskXMLGetVolumeChainInfo method to
module scope in order to reuse it in commit 8c52769(Live Merge:
Restore watermark tracking)

Change-Id: I8d8ef439120b8dff79cda9fcff7c42040f75e35e
Signed-off-by: Ala Hino 
---
M vdsm/virt/vm.py
1 file changed, 7 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/80/62180/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 4dd7596..3e949cc 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -153,6 +153,13 @@
 return "Unknown (%i)" % event
 
 
+def pathToVolID(drive, path):
+for vol in drive.volumeChain:
+if os.path.realpath(vol['path']) == os.path.realpath(path):
+return vol['volumeID']
+raise LookupError("Unable to find VolumeID for path '%s'", path)
+
+
 class SetLinkAndNetworkError(Exception):
 pass
 
@@ -4694,12 +4701,6 @@
 if child.nodeName == name:
 return child
 return None
-
-def pathToVolID(drive, path):
-for vol in drive.volumeChain:
-if os.path.realpath(vol['path']) == os.path.realpath(path):
-return vol['volumeID']
-raise LookupError("Unable to find VolumeID for path '%s'", path)
 
 volChain = []
 while True:


-- 
To view, visit https://gerrit.ovirt.org/62180
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8d8ef439120b8dff79cda9fcff7c42040f75e35e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Restore watermark tracking

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 6:

(9 comments)

https://gerrit.ovirt.org/#/c/60889/6/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 198: for vol in drive.volumeChain:
Line 199: if vol["path"] == path:
Line 200: return vol["volumeID"]
Line 201: 
Line 202: raise LookupError("Unable to find a volume matching path:%s"
> Need a space after %s
Done
Line 203:   "in drive:%s",
Line 204:   path, drive.name)
Line 205: 
Line 206: 


Line 926: self.conf['timeOffset'] = newTimeOffset
Line 927: 
Line 928: def _getExtendCandidates(self):
Line 929: ret = []
Line 930: mergeCandidates = self._getLiveMergeExtendCandidates()
> If _getLiveMergeExtendCandidates() returns a list of tuple(drive, vol_id, c
Nice! Done
Line 931: 
Line 932: for drive in self._chunkedDrives():
Line 933: try:
Line 934: capacity, alloc, physical = self._getExtendInfo(drive)


Line 950:mergeCandidate['physical']))
Line 951: return ret
Line 952: 
Line 953: def _getLiveMergeExtendCandidates(self):
Line 954: candidates = {}
> Return a list of tuples.
Done
Line 955: for job in self.conf['_blockJobs'].values():
Line 956: try:
Line 957: drive = self._findDriveByUUIDs(job['disk'])
Line 958: except LookupError:


Line 979: try:
Line 980: volInfo = self._getVolumeInfo(drive.domainID, 
drive.poolID,
Line 981:   drive.imageID, volUUID)
Line 982: except StorageUnavailableError:
Line 983: continue
> We should still log a warning here since this may cause us to not issue an 
Done
Line 984: 
Line 985: if volInfo['format'].lower() != 'cow':
Line 986: continue
Line 987: 


Line 993: candidates[drive.imageID] = {
Line 994: 'alloc': watermarks[volumeID],
Line 995: 'physical': int(volInfo['truesize']),
Line 996: 'capacity': int(volInfo['apparentsize']),
Line 997: 'volumeID': volUUID}
> Just make a tuple and append it to candidates list.
Done
Line 998: else:
Line 999: self.log.warning("No watermark info available for %s",
Line 1000:  volUUID)
Line 1001: 


Line 1005: vmSample = sampling.stats_cache.get(self.id)
Line 1006: lastSample = vmSample.last_value
Line 1007: for key in lastSample:
Line 1008: if not key.endswith("path"):
Line 1009: continue
> Really nice way to shorten this:
I like it :)
Line 1010: 
Line 1011: _, index, _ = key.split(".", 2)
Line 1012: try:
Line 1013: name = lastSample['block.%s.name' % index]


Line 1007: for key in lastSample:
Line 1008: if not key.endswith("path"):
Line 1009: continue
Line 1010: 
Line 1011: _, index, _ = key.split(".", 2)
> In this case I would prefer you use short descriptive names for the unused 
Done
Line 1012: try:
Line 1013: name = lastSample['block.%s.name' % index]
Line 1014: except KeyError:
Line 1015: self.log.warning("Drive %s is not in bulk stats, 
skipping"


Line 1012: try:
Line 1013: name = lastSample['block.%s.name' % index]
Line 1014: except KeyError:
Line 1015: self.log.warning("Drive %s is not in bulk stats, 
skipping"
Line 1016:  " it",
> Get rid it " it" from the message and bring the line with name up one line.
Done
Line 1017:  name)
Line 1018: continue
Line 1019: 
Line 1020: if name != drive.name:


Line 1023: path = lastSample[key]
Line 1024: if volUUID == _pathToVolumeID(drive, path):
Line 1025: allocKey = 'block.%s.allocation' % index
Line 1026: return lastSample[allocKey]
Line 1027: 
> Best log a warning message here too that the volume could not be found.
Done
Line 1028: return None
Line 1029: 
Line 1030: def _chunkedDrives(self):
Line 1031: """


-- 
To view, visit https://gerrit.ovirt.org/60889
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I52199bb38a2d3f439d33b9e78743c3e611349672
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vds

Change in vdsm[master]: Live Merge: Restore watermark tracking

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 5:

(18 comments)

https://gerrit.ovirt.org/#/c/60889/5/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS5, Line 200: st = os.stat(vol["path"])
> Is there a reason you can't just compare the path strings and return the vo
Done


PS5, Line 946: ret.append((drive, mergeCandidate['volumeID'],
 :mergeCandidate['capacity'],
 :mergeCandidate['alloc'],
 :mergeCandidate['physical']))
> This should be outside the try block (or in an else clause of the try block
Done


PS5, Line 956: # The common case is that there are no active jobs.
 : if not self.conf['_blockJobs']:
 : return {}
> This might not be needed if we move the call to self._getWriteWatermarks() 
Done


PS5, Line 961: watermarks = self._getWriteWatermarks()
> Looking at this closely, I think we can optimize this code further... see b
Done


PS5, Line 966: # After an active layer merge completes the vdsm metadata will
 : # be out of sync for a brief period.  If we 
cannot find the old
 : # disk then it's safe to skip it.
> Let's update this comment a bit:
Done


PS5, Line 969: self.log.debug("Couldn't find drive %s. After an active"
 :" layer merge completes the vdsm 
metadata"
 :" will be out of sync for a brief 
period."
 :" If we cannot find the old disk 
then it's"
 :" safe to skip it",
 :job['disk'])
> And we can make this log message more concise:
Done


PS5, Line 987: if volumeID not in watermarks:
 : self.log.warning("No watermark info available 
for %s",
 :  volumeID)
 : continue
> Move this block after the volumeInfo call and check for format == 'cow'.  T
Done


PS5, Line 992: res = self.cif.irs.getVolumeInfo(drive.domainID, drive.poolID,
 :  drive.imageID, 
volumeID)
 : if res['status']['code'] != 0:
 : self.log.error("Unable to get the info of volume 
%s (domain: "
 :"%s image: %s)", volumeID, 
drive.domainID,
 :drive.imageID)
 : continue
 : volInfo = res['info']
> please use the self._getVolumeInfo() helper for this.  You'll have to catch
Done


Line 998: continue
Line 999: volInfo = res['info']
Line 1000: if volInfo['format'].lower() != 'cow':
Line 1001: continue
Line 1002: 
> Here is where you can finally get the watermark...
Done
Line 1003: self.log.debug("Adding live merge extension candidate: "
Line 1004:"volume=%s allocation=%i", volumeID,
Line 1005:watermarks[volumeID])
Line 1006: candidates[drive.imageID] = {


PS5, Line 1014: self
> Let's rename this to _getVolumeWriteWatermark(self, drive, volUUID) and sea
Done


PS5, Line 1019: if key.endswith("path"):
> If you write this as:
Done


PS5, Line 1023: xcept KeyError:
> This is an unexpected situation.  You should probably add a warning message
Done


Line 1020: prefix, index, attr = key.split(".", 2)
Line 1021: try:
Line 1022: name = lastSample['block.%s.name' % index]
Line 1023: except KeyError:
Line 1024: continue
> If name doesn't match the drive name we can move to the next one.
Done
Line 1025: 
Line 1026: try:
Line 1027: drive = self._findDriveByName(name)
Line 1028: except LookupError:


PS5, Line 1026: try:
  : drive = self._findDriveByName(name)
  : except LookupError:
  : self.log.error("Unable to find drive '%s'", 
name)
  : continue
> We don't need to look up the drive anymore since we were already passed it.
Done


PS5, Line 1032: if not drive.chunked:
  : continue
> This function doesn't care about this.  It just looks up watermarks for thi
Done


Line 1034: 
Line 1035: path = lastSample[key]
Line 1036: allocKey = 'block.%s.allocation' % index
Line 1037: allocation = lastSample[allocKey]
Line 1038: volumeID = _pathToVolumeID(drive, path)
> 

Change in vdsm[master]: Live Merge: Restore watermark tracking

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 3:

(6 comments)

https://gerrit.ovirt.org/#/c/60889/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS3, Line 928: else:
> A single vm drive could require extensions to a merging volume and the leaf
Done


PS3, Line 961: blockDev
> Should this be drive.chuncked?
Done


PS3, Line 984: if volInfo['format'].lower() != 'cow':
 : continue
> using drive.chuncked above may mitigate the need for this check.
removed


PS3, Line 1008: vol_uuid = os.path.basename(path)
> This is not allowed in virt code.  We should either add a helper in HSM to 
Done


PS3, Line 4738: maxAlloc = 1
> This isn't quite right.  self.extendDriveVolume expects a unit in bytes for
Done


PS3, Line 4740: if drive.imageID in candidates.keys():
  : mergeCandidate = candidates[drive.imageID]
  : maxAlloc = mergeCandidate['alloc']
> This would look nicer as a try/except block:
Done


-- 
To view, visit https://gerrit.ovirt.org/60889
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I52199bb38a2d3f439d33b9e78743c3e611349672
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Restore watermark tracking

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 2:

(6 comments)

https://gerrit.ovirt.org/#/c/60889/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS2, Line 920: mergeCandidates
> how do you use this?
Done


PS2, Line 936: if not self.conf['_blockJobs'].values():
 : return {}
> why not just:
Done


PS2, Line 941: stats_cache
> please note that this is updated each 15s - and it is modifiable by the use
Good input; 15s is good enough for live merge. 
Do users update this very often?


PS2, Line 981: getVolumeInfo
> how costly is this?
We'll check this when running performance tests


PS2, Line 4725: extendDrivesIfNeeded
> Let me calrify a bit. In the commit message you state:
Done


PS2, Line 4725: extendDrivesIfNeeded
> why do you want to run all over all the drives (like extendDrivesIfNeeded()
Done


-- 
To view, visit https://gerrit.ovirt.org/60889
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I52199bb38a2d3f439d33b9e78743c3e611349672
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sampling: Retrieve backing chain stats

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: sampling: Retrieve backing chain stats
..

sampling: Retrieve backing chain stats

Update sampling mechanism to retrieve backing chain stats. This is
required to get the allocation value of all volumes in the chain.
These stats will be cachd in a volume UUID to allocatoin value
dictionary.
Based on the allocation value, we will find out how to extend volumes
during live merge. The logic of getting live merge ectend candidates
will be done in a separate patch.

Change-Id: I3bbb8643d1c86e90d1e2de7cb2a5b00116c71453
Signed-off-by: Ala Hino 
---
M lib/vdsm/virt/sampling.py
M lib/vdsm/virt/vmstats.py
2 files changed, 27 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/88/60888/1

diff --git a/lib/vdsm/virt/sampling.py b/lib/vdsm/virt/sampling.py
index 8feaf37..c92fc4a 100644
--- a/lib/vdsm/virt/sampling.py
+++ b/lib/vdsm/virt/sampling.py
@@ -31,6 +31,9 @@
 import threading
 import time
 
+# 3rd party libs imports
+import libvirt
+
 from vdsm import numa
 from vdsm import utils
 from vdsm.constants import P_VDSM_RUN, P_VDSM_CLIENT_LOG
@@ -493,17 +496,19 @@
 fast_path = acquired and not self._skip_doms
 doms = []  # whitelist, meaningful only in the slow path
 try:
+flags = libvirt.VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING
 if fast_path:
 # This is expected to be the common case.
 # If everything's ok, we can skip all the costly checks.
-bulk_stats = self._conn.getAllDomainStats(self._stats)
+bulk_stats = self._conn.getAllDomainStats(
+self._stats, flags)
 else:
 # A previous call got stuck, or not every domain
 # has properly recovered. Thus we must whitelist domains.
 doms = self._get_responsive_doms()
 if doms:
 bulk_stats = self._conn.domainListGetStats(
-doms, self._stats)
+doms, self._stats, flags)
 else:
 bulk_stats = []
 except Exception:
diff --git a/lib/vdsm/virt/vmstats.py b/lib/vdsm/virt/vmstats.py
index 177e2a9..2f06a66 100644
--- a/lib/vdsm/virt/vmstats.py
+++ b/lib/vdsm/virt/vmstats.py
@@ -21,6 +21,7 @@
 
 import contextlib
 import logging
+import os
 
 import six
 
@@ -43,6 +44,7 @@
 cpu(stats, first_sample, last_sample, interval)
 networks(vm, stats, first_sample, last_sample, interval)
 disks(vm, stats, first_sample, last_sample, interval)
+watermarks(stats, first_sample, last_sample, interval)
 balloon(vm, stats, last_sample)
 cpu_count(stats, last_sample)
 tune_io(vm, stats)
@@ -352,6 +354,24 @@
 return stats
 
 
+def watermarks(stats, first_sample, last_sample, interval):
+if first_sample is None or last_sample is None:
+return None
+
+watermarks = {}
+for key in last_sample:
+if key.endswith("path"):
+prefix, index, attr = key.split(".", 2)
+path = last_sample[key]
+alloc_key = "block" + "." + index + ".allocation"
+allocation = last_sample[alloc_key]
+vol_uuid = os.path.basename(path)
+watermarks[vol_uuid] = allocation
+
+if watermarks:
+stats['watermarks'] = watermarks
+
+
 def _disk_rate(first_sample, first_index, last_sample, last_index, interval):
 stats = {}
 


-- 
To view, visit https://gerrit.ovirt.org/60888
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3bbb8643d1c86e90d1e2de7cb2a5b00116c71453
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Live Merge: Restore watermark tracking

[ahino]

Hello Adam Litke,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/60889

to review the following change.

Change subject: Live Merge: Restore watermark tracking
..

Live Merge: Restore watermark tracking

Since libvirt 1.2.13, the virConnectGetAllDomainStats API will return
write watermark information for all volumes in the chain.  We can use
this information during an active live merge operation to perform
on-demand extension of the merge target (just as we already do for the
active layer).  When libvirt does not provide the necessary information,
use a preemptive extension instead.

Change-Id: I52199bb38a2d3f439d33b9e78743c3e611349672
Bug-Url: https://bugzilla.redhat.com/1168327
Signed-off-by: Adam Litke 
Signed-off-by: Ala Hino 
---
M vdsm/virt/vm.py
1 file changed, 72 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/89/60889/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 4dd7596..cc50ba7 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -917,6 +917,7 @@
 
 def _getExtendCandidates(self):
 ret = []
+mergeCandidates = self._getLiveMergeExtendCandidates()
 
 for drive in self._chunkedDrives():
 try:
@@ -929,6 +930,76 @@
 ret.append((drive, drive.volumeID, capacity, alloc, physical))
 
 return ret
+
+def _getLiveMergeExtendCandidates(self):
+# The common case is that there are no active jobs.
+if not self.conf['_blockJobs'].values():
+return {}
+
+candidates = {}
+try:
+vm_sample = sampling.stats_cache.get(self.id)
+stats = vmstats.produce(self,
+vm_sample.first_value,
+vm_sample.last_value,
+vm_sample.interval)
+watermarks = stats['watermarks']
+    self.log.debug("AHINO: watermarks %s:", watermarks)
+except Exception:
+self.log.exception("Error fetching volumes watermark")
+return {}
+
+for job in self.conf['_blockJobs'].values():
+try:
+drive = self._findDriveByUUIDs(job['disk'])
+except LookupError:
+# After an active layer merge completes the vdsm metadata will
+# be out of sync for a brief period.  If we cannot find the old
+# disk then it's safe to skip it.
+self.log.debug("Couldn't find drive %s. After an active"
+   " layer merge completes the vdsm metadata"
+   " will be out of sync for a brief period."
+   " If we cannot find the old disk then it's"
+   " safe to skip it",
+   job['disk'])
+continue
+
+if not drive.blockDev:
+continue
+
+if job['strategy'] == 'commit':
+volumeID = job['baseVolume']
+else:
+self.log.debug("Unrecognized merge strategy '%s'",
+   job['strategy'])
+continue
+
+if volumeID not in watermarks:
+self.log.warning("No watermark info available for %s",
+ volumeID)
+continue
+
+res = self.cif.irs.getVolumeInfo(drive.domainID, drive.poolID,
+ drive.imageID, volumeID)
+if res['status']['code'] != 0:
+self.log.error("Unable to get the info of volume %s (domain: "
+   "%s image: %s)", volumeID, drive.domainID,
+   drive.imageID)
+continue
+volInfo = res['info']
+if volInfo['format'].lower() != 'cow':
+continue
+
+self.log.debug("Adding live merge extension candidate: "
+   "volume=%s allocation=%i", volumeID,
+   watermarks[volumeID])
+candidates[drive.imageID] = {
+'alloc': watermarks[volumeID],
+'physical': int(volInfo['truesize']),
+'capacity': int(volInfo['apparentsize']),
+'volumeID': volumeID}
+
+return candidates
 
 def _chunkedDrives(self):
 """
@@ -4651,20 +4722,8 @@
 self.untrackBlockJob(jobUUID)
 return response.er

Change in vdsm[master]: sampling: Rename stats_flags variable

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: sampling: Rename stats_flags variable
..

sampling: Rename stats_flags variable

Rename 'stats_flags' to 'stats' to reflect the exact meaning of this
varaiable based on libvirt documentation. There is an additional
'falg' variable that is used to retrieve additionals stats, for
example: VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING that will be used
in a future patch.

Change-Id: Iaca95656038489f1ce4286e32ab819d78b9524dd
Signed-off-by: Ala Hino 
---
M lib/vdsm/virt/sampling.py
1 file changed, 4 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/62/60862/1

diff --git a/lib/vdsm/virt/sampling.py b/lib/vdsm/virt/sampling.py
index 4cfd3b1..8feaf37 100644
--- a/lib/vdsm/virt/sampling.py
+++ b/lib/vdsm/virt/sampling.py
@@ -475,11 +475,11 @@
 
 class VMBulkSampler(object):
 def __init__(self, conn, get_vms, stats_cache,
- stats_flags=0, ttl=_TTL):
+ stats=0, ttl=_TTL):
 self._conn = conn
 self._get_vms = get_vms
 self._stats_cache = stats_cache
-self._stats_flags = stats_flags
+self._stats = stats
 self._skip_doms = ExpiringCache(ttl)
 self._sampling = threading.Semaphore()  # used as glorified counter
 self._log = logging.getLogger("virt.sampling.VMBulkSampler")
@@ -496,14 +496,14 @@
 if fast_path:
 # This is expected to be the common case.
 # If everything's ok, we can skip all the costly checks.
-bulk_stats = self._conn.getAllDomainStats(self._stats_flags)
+bulk_stats = self._conn.getAllDomainStats(self._stats)
 else:
 # A previous call got stuck, or not every domain
 # has properly recovered. Thus we must whitelist domains.
 doms = self._get_responsive_doms()
 if doms:
 bulk_stats = self._conn.domainListGetStats(
-doms, self._stats_flags)
+doms, self._stats)
 else:
 bulk_stats = []
 except Exception:


-- 
To view, visit https://gerrit.ovirt.org/60862
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaca95656038489f1ce4286e32ab819d78b9524dd
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: core: Unlink volume run path when deleting a snapshot

[ahino]

Ala Hino has posted comments on this change.

Change subject: core: Unlink volume run path when deleting a snapshot
..


Patch Set 1:

(4 comments)

https://gerrit.ovirt.org/#/c/59725/1/vdsm/storage/blockVolume.py
File vdsm/storage/blockVolume.py:

Line 610:self.sdUUID, self.volUUID, vol_path, 
exc_info=True)
Line 611: 
Line 612: try:
Line 613: imgRundir = os.path.join(constants.P_VDSM_STORAGE, 
self.sdUUID,
Line 614:  self.imgUUID, self.volUUID)
> This is the volume run path, not the image directory
Done
Line 615: self.log.debug("Unlinking %s", imgRundir)
Line 616: os.unlink(imgRundir)
Line 617: return True
Line 618: except Exception as e:


Line 611: 
Line 612: try:
Line 613: imgRundir = os.path.join(constants.P_VDSM_STORAGE, 
self.sdUUID,
Line 614:  self.imgUUID, self.volUUID)
Line 615: self.log.debug("Unlinking %s", imgRundir)
> Use %r
Done
Line 616: os.unlink(imgRundir)
Line 617: return True
Line 618: except Exception as e:
Line 619: eFound = e


Line 614:  self.imgUUID, self.volUUID)
Line 615: self.log.debug("Unlinking %s", imgRundir)
Line 616: os.unlink(imgRundir)
Line 617: return True
Line 618: except Exception as e:
> This will log errors when the run link path does not exists - very bad. Sho
Will do this: check if os.path.islink and then unlink it
Line 619: eFound = e
Line 620: self.log.error("cannot delete volume's %s/%s link path: 
%s",
Line 621:self.sdUUID, self.volUUID, imgRundir, 
exc_info=True)
Line 622: 


Line 616: os.unlink(imgRundir)
Line 617: return True
Line 618: except Exception as e:
Line 619: eFound = e
Line 620: self.log.error("cannot delete volume's %s/%s link path: 
%s",
> This is the volume run link
Done
Line 621:self.sdUUID, self.volUUID, imgRundir, 
exc_info=True)
Line 622: 
Line 623: raise eFound
Line 624: 


-- 
To view, visit https://gerrit.ovirt.org/59725
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: core: Unlink image run directory when deleting a snapshot

[ahino]

Ala Hino has uploaded a new change for review.

Change subject: core: Unlink image run directory when deleting a snapshot
..

core: Unlink image run directory when deleting a snapshot

Unlink image run directory,
/run/vdsm/storage/sdUUID/imgUUID/volUUID, when removing a
snapshot.

Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Bug-Url: https://bugzilla.redhat.com/1321018
Signed-off-by: Ala Hino 
---
M vdsm/storage/blockVolume.py
1 file changed, 11 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/25/59725/1

diff --git a/vdsm/storage/blockVolume.py b/vdsm/storage/blockVolume.py
index 4476a9e..080a858 100644
--- a/vdsm/storage/blockVolume.py
+++ b/vdsm/storage/blockVolume.py
@@ -604,12 +604,22 @@
 try:
 self.log.debug("Unlinking %s", vol_path)
 os.unlink(vol_path)
-return True
 except Exception as e:
 eFound = e
 self.log.error("cannot delete volume's %s/%s link path: %s",
self.sdUUID, self.volUUID, vol_path, exc_info=True)
 
+try:
+imgRundir = os.path.join(constants.P_VDSM_STORAGE, self.sdUUID,
+ self.imgUUID, self.volUUID)
+self.log.debug("Unlinking %s", imgRundir)
+os.unlink(imgRundir)
+return True
+except Exception as e:
+eFound = e
+self.log.error("cannot delete volume's %s/%s link path: %s",
+   self.sdUUID, self.volUUID, imgRundir, exc_info=True)
+
 raise eFound
 
 def extend(self, newSize):


-- 
To view, visit https://gerrit.ovirt.org/59725
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: Move blockVolume constants to storage constants

[ahino]

Ala Hino has posted comments on this change.

Change subject: Move blockVolume constants to storage constants
..


Patch Set 3: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/57583
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I59e6d14b0a5a0a6144403add43748b5e3d95f8b0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: Fix FakeDomainManifest implementation

[ahino]

Ala Hino has posted comments on this change.

Change subject: tests: Fix FakeDomainManifest implementation
..


Patch Set 2: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/57686
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1174ebb4b0b7ba1afe19e0d52700e7ec94cca02f
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Move VolumeMetadata to vsdm.storage

[ahino]

Ala Hino has posted comments on this change.

Change subject: Move VolumeMetadata to vsdm.storage
..


Patch Set 2: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/57685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I29e910915680e2f68d25d2a896e5e3c3802e6273
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Move volume type constants to constants module

[ahino]

Ala Hino has posted comments on this change.

Change subject: Move volume type constants to constants module
..


Patch Set 3: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/57581
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6a8db6de344d85029cb8fa2da302c88f4d605991
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches