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

[Jenkins CI RO]

Jenkins CI RO has abandoned this change.

Change subject: Live Merge: Restore watermark tracking
..


Abandoned

Abandoned due to no activity - please restore if still relevant

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Jenkins CI RO
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
___
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: Restore watermark tracking

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 4:

* Update tracker: IGNORE, no Bug-Url found

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Jenkins CI RO
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: Restore watermark tracking

[alitke]

Adam Litke has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 15: Code-Review+2

Looks good!  Let's verify the underlying patches so we can merge this.

-- 
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: 15
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]: Live Merge: Restore watermark tracking

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 15:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 15
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]: 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]: Live Merge: Restore watermark tracking

[alitke]

Adam Litke has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 14:

(1 comment)

couple of minor things and this will be ready for merge.

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?


-- 
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]: Live Merge: Restore watermark tracking

[alitke]

Adam Litke has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 13:

(2 comments)

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

PS13, Line 931: ret.append((drive, drive.volumeID, capacity, alloc,
  : physical))
This does not need to be moved inside the try block.  You can keep it after 
(like it was originally) and minimize changes.


PS13, Line 4752: COW'
> in a separate patch
please fix in this patch.  Otherwise we're introducing too much noise into the 
commit history.


-- 
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]: 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]: Live Merge: Restore watermark tracking

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 14:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 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: Restore watermark tracking

[alitke]

Adam Litke 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.


PS13, Line 1020: pathToVolID
Looking forward to you reusing the other function that Francesco pointed out.


PS13, Line 4752: COW'
Maybe we should also use the storage constant here too.


-- 
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]: Live Merge: Restore watermark tracking

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 13:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 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: Restore watermark tracking

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 12:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 12
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]: 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]: Live Merge: Restore watermark tracking

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 11:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 11
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]: Live Merge: Restore watermark tracking

[fromani]

Francesco Romani has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 10:

(6 comments)

-1 for visibility of inline questions

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.

Is this intentional? In this case sorry I missed the explanation, please point 
me to it.
Otherwise we should avoid this duplication


PS10, Line 977: 'cow'
I think there are constants for this somewhere in the storage subtree


PS10, Line 996: vmSample
minor nit: let's use lower_case_identifiers for new code. Same for lastSample, 
allocKey


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

why not just

  for key in last_sample:
 if not key.endswith('path'):
   continue
 # ...

?


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

_pathToVolumeID()
(or _pathToVolID() :)),
and just continue should we get LookupError?

or, put differently, what's the benefit of the extra check by name?


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


-- 
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]: Live Merge: Restore watermark tracking

[alitke]

Adam Litke has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 10:

(4 comments)

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

PS10, Line 199: if vol["path"].split('/')[-1] == path.split('/')[-1]:
This makes assumptions about the format of path strings which is not allowed in 
virt code.  Is there a reason you can't just compare path and vol["path"} 
directly?


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 = []


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.

It might be helpful to add a comment here explaining how we look up the alloc 
value including an example of the keys and values from the bulk stats that are 
involved in the lookup.
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 4744: drive.imageID
I'm baffled by the use of imageID.  Isn't that supposed to take a volumeID?

Do you think it would be a good idea to just trigger the top-level 
extendDrivesIfNeeded flow from here?


-- 
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]: Live Merge: Restore watermark tracking

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 10:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 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: Restore watermark tracking

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 9:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 9
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]: Live Merge: Restore watermark tracking

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 8:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 8
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]: 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 

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

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 7:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 7
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]: Live Merge: Restore watermark tracking

[alitke]

Adam Litke has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 6:

(9 comments)

Nice!

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
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, 
capacity, alloc, physical) values then you can simplify this code:

 ret = self._getLiveMergeExtendCandidates()
 for drive in self._chunckedDrives():
... append this drive's info to ret ...
 return ret
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.
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 
extend request and the vm could be later paused.
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.
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:
 keys = [key for key in lastSample if key.endswith('path')]
 for key in keys:
...

Use it if you 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 
parts of the split.
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.
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.
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: 

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

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 6:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 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: 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

[alitke]

Adam Litke has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 5:

(18 comments)

Great patch!  I gave this a pretty hands-on review because I saw some room for 
making it a bit cleaner and more efficient.

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 
volUUID if they match?


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.  
We don't want to ignore KeyErrors from this line.


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() 
inside the for loop.  See below.


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


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:

 # After an active layer merge completes the volume ID of the drive is updated 
and it no longer matches the original volume ID specified in the saved block 
jobs.  If we can't find the drive we assume this is the reason and that it's 
safe to skip it.


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:
 self.log.debug("Cannot find drive for block job %s.  Skipping extend check", 
job['id'])


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'.  This 
way we delay watermark parsing until absolutely necessary.  Usually watermark 
info should not be missing so this condition should almost never occur.


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 
StorageUnavailableError and continue


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...

This is the first time you actually need the watermarks.  So in all the above 
cases we can avoid calling it and instead here we can do:
 alloc = self._getVolumeWatermark(drive, volUUID)
 if alloc:
 ... add candidate ...
 else:
 ... Warn about missing info ...
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 search 
for just one watermark.


PS5, Line 1019: if key.endswith("path"):
If you write this as:
 if not key.endswith('path'):
 continue

Then you can save one level of indentation for the rest of the function.


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


Line 1020: prefix, index, attr = key.split(".", 2)
Line 1021: try:
Line 1022: name = lastSample['block.%s.name' % 

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

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 5:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 5
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]: Live Merge: Restore watermark tracking

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 4:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 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]: 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

[alitke]

Adam Litke 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 at 
the same time.


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


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


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 
convert a path into imgUUID and volUUID or iterate over the VM devices looking 
for one that contains a matching path.


PS3, Line 4738: maxAlloc = 1
This isn't quite right.  self.extendDriveVolume expects a unit in bytes for the 
curSize parameter.  However, 1GB is not the right answer when you have no 
watermark info because extendDriveVolume expects to receive the current size, 
not the amount by which you want to increase.  I'm wodering if it makes sense 
to use baseInfo['truesize'].  For block volumes this is the LV size and would 
automatically cause the LV to grow by 1GB.  This isn't ideal but the 
alternatives aren't much better.


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:
 alloc = 1
 try:
 candidate = self._getLiveMergeExtendCandidates()[drive.imageID]
 alloc = candidate['alloc']
 except KeyError:
 pass


-- 
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]: Live Merge: Restore watermark tracking

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 3:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 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]: Live Merge: Restore watermark tracking

[fromani]

Francesco Romani has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 2:

(1 comment)

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

PS2, Line 4725: extendDrivesIfNeeded
> why do you want to run all over all the drives (like extendDrivesIfNeeded()
Let me calrify a bit. In the commit message you state:

"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."

and this is fine, but I think we know exactly which drives we need to extend, 
no need to run over all of them - right?

To summarize, I think it is better to avoid to run too generic code in this 
flow, and be more specific, avoiding unneeded work.


-- 
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: 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

[fromani]

Francesco Romani has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 2:

(1 comment)

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

PS2, Line 4725: extendDrivesIfNeeded
why do you want to run all over all the drives (like extendDrivesIfNeeded() 
does) and not just the specific drive like the old code did?
What's the new need?


-- 
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: 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

[fromani]

Francesco Romani has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 2: Code-Review-1

-- 
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: 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]: Live Merge: Restore watermark tracking

[fromani]

Francesco Romani has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 2:

(4 comments)

-1 for visibility

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?


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

  if not self.conf['_blockJobs']:
return {}

plus, more important: let's check carefully about the locking here.


PS2, Line 941: stats_cache
please note that this is updated each 15s - and it is modifiable by the users. 
Is this good enough for live merge?


PS2, Line 981: getVolumeInfo
how costly is this?


-- 
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: 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

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 2:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 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]: Live Merge: Restore watermark tracking

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 1:

* #1168327::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1168327::OK, public bug
* Check Product::#1168327::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
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: 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.error('mergeErr')
 
-# blockCommit will cause data to be written into the base volume.
-# Perform an initial extension to ensure there is enough space to
-# copy all the required data.  Normally we'd use monitoring to extend
-# the volume on-demand but internal watermark information is not being
-# 

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

[alitke]

Adam Litke has restored this change.

Change subject: Live Merge: Restore watermark tracking
..


Restored

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

Gerrit-MessageType: restore
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Jenkins CI RO
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 4:

* Update tracker: IGNORE, no Bug-Url found

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Jenkins CI RO
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]: Live Merge: Restore watermark tracking

[Jenkins CI RO]

Jenkins CI RO has abandoned this change.

Change subject: Live Merge: Restore watermark tracking
..


Abandoned

Abandoned due to no activity - please restore if still relevant

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Jenkins CI RO
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[tnisan]

Tal Nisan has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 4:

Ping?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 4:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[fromani]

Francesco Romani has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 4:

(4 comments)

sorry for going out of sync. Seen lot of action between versions 3 and 4, 
waiting to see how version 5 becomes before to add more comments.

http://gerrit.ovirt.org/#/c/36924/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 723: 'flushLatency': str(compute_latency('flush'))}
Line 724: 
Line 725: 
Line 726: def _getAllVMsBulkStats(conn, vmList, statsTypes=0, statsFlags=0):
Line 727: domList = [x._dom._dom for x in vmList]
 x would be more clear as vm.
+1
Line 728: if statsTypes == 0:
Line 729: statsTypes = (libvirt.VIR_DOMAIN_STATS_STATE |
Line 730:   libvirt.VIR_DOMAIN_STATS_CPU_TOTAL |
Line 731:   libvirt.VIR_DOMAIN_STATS_BALLOON |


Line 730:   libvirt.VIR_DOMAIN_STATS_CPU_TOTAL |
Line 731:   libvirt.VIR_DOMAIN_STATS_BALLOON |
Line 732:   libvirt.VIR_DOMAIN_STATS_VCPU |
Line 733:   libvirt.VIR_DOMAIN_STATS_INTERFACE |
Line 734:   libvirt.VIR_DOMAIN_STATS_BLOCK)
Are you interested in this specific subset here and in the time being or you 
just want them all?
Because if you want all it is safe to just use statsTypes == 0:

Using 0 for @stats returns all stats groups supported by the given hypervisor.

both in
http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainListGetStats
and
http://libvirt.org/html/libvirt-libvirt-domain.html#virConnectGetAllDomainStats
Line 735: statsList = conn.domainListGetStats(domList, statsTypes, 
statsFlags)
Line 736: return dict([(x[0].UUIDString(), x[1]) for x in statsList])
Line 737: 
Line 738: 


Line 732:   libvirt.VIR_DOMAIN_STATS_VCPU |
Line 733:   libvirt.VIR_DOMAIN_STATS_INTERFACE |
Line 734:   libvirt.VIR_DOMAIN_STATS_BLOCK)
Line 735: statsList = conn.domainListGetStats(domList, statsTypes, 
statsFlags)
Line 736: return dict([(x[0].UUIDString(), x[1]) for x in statsList])
 The x variable is not very helpful, specially when you don't know what are 
+1 for the genexp
Line 737: 
Line 738: 
Line 739: class TimeoutError(libvirt.libvirtError):
Line 740: pass


Line 1436: volAllocMap = {}
Line 1437: bulkStats = 
self._getBulkStats(libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1438:
_LIBVIRT_BACKING_CHAIN_STATS_FLAG)
Line 1439: for i in xrange(bulkStats['block.count']):
Line 1440: name = bulkStats['block.%i.name' % i]
 What is this expected key is missing?
Yes, this is possible, especially when dealing with backing chains (and not 
with simple block devices).
Line 1441: try:
Line 1442: drive = self._findDriveByName(name)
Line 1443: except LookupError:
Line 1444: self.log.error(Unable to find drive '%s', name)


-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[alitke]

Adam Litke has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 3:

(5 comments)

http://gerrit.ovirt.org/#/c/36924/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 169: 
Line 170: 
Line 171: # Since libvirt 1.2.13, the virConnectGetAllDomainStats API will 
return
Line 172: # block statistics for all volumes in the chain when using a new flag.
Line 173: _libvirtBackingChainStatsFlag = getattr(
 Why do you a constant which looks like a variable? Can this value change af
No it cannot.  I'll capitalize it.
Line 174: libvirt, VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING, 0)
Line 175: 
Line 176: 
Line 177: class VmStatsThread(AdvancedStatsThread):


Line 712: 'writeLatency': str(compute_latency('wr')),
Line 713: 'flushLatency': str(compute_latency('flush'))}
Line 714: 
Line 715: 
Line 716: def _vmsGetBulkStats(conn, vmList, statsTypes=0, statsFlags=0):
 I think _getAllVMsBulkStats would be more clear
ok.
Line 717: domList = [x._dom._dom for x in vmList]
Line 718: if statsTypes == 0:
Line 719: statsTypes = (libvirt.VIR_DOMAIN_STATS_STATE |
Line 720:   libvirt.VIR_DOMAIN_STATS_CPU_TOTAL |


Line 1419: self.conf['timeOffset'] = newTimeOffset
Line 1420: 
Line 1421: def _getBulkStats(self, statsTypes, statsFlags):
Line 1422: return _vmsGetBulkStats(self._connection, [self], statsTypes,
Line 1423: statsFlags)[self.id]
 So we must get stats for all vms for getting single vm stats?
Yes this is a good long-term idea but I am not sure I want to commit to it 
here.  Getting all of the stats for all VMs could be expensive inside libvirt 
since it involves multiple qemu monitor calls to each VM.  A 2 second sampling 
interval may not be appropriate for all of these things. 

Further, we cannot replace the current call to dom.getBlockInfo (for the active 
layer monitoring because the bulk stats API only provides values for capacity 
and allocation.  We need also the physical value which would then require calls 
down to the storage getVolumeInfo which won't end up making this any more 
efficient.
Line 1424: 
Line 1425: def _getWriteWatermarks(self):
Line 1426: def pathToVolID(drive, path):
Line 1427: for vol in drive.volumeChain:


Line 1424: 
Line 1425: def _getWriteWatermarks(self):
Line 1426: def pathToVolID(drive, path):
Line 1427: for vol in drive.volumeChain:
Line 1428: if os.path.realpath(vol['path']) == 
os.path.realpath(path):
 Finding the real path should be done once.
Sure I can add a comment.  Thanks for your suggestion on how to look it up 
using os.stat.  I'll adopt that and move the logic info a global helper 
function.
Line 1429: return vol['volumeID']
Line 1430: raise LookupError(Unable to find VolumeID for path 
'%s', path)
Line 1431: 
Line 1432: volAllocMap = {}


Line 1436: name = bulkStats['block.%i.name' % i]
Line 1437: try:
Line 1438: drive = self._findDriveByName(name)
Line 1439: except LookupError:
Line 1440: continue
 Is this expected? I think we like at least a debug log here to understand w
No, not expected.  I'll add a log.error message.
Line 1441: if not drive.blockDev or drive.format != 'cow':
Line 1442: continue
Line 1443: 
Line 1444: try:


-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 3:

(2 comments)

http://gerrit.ovirt.org/#/c/36924/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 712: 'writeLatency': str(compute_latency('wr')),
Line 713: 'flushLatency': str(compute_latency('flush'))}
Line 714: 
Line 715: 
Line 716: def _vmsGetBulkStats(conn, vmList, statsTypes=0, statsFlags=0):
 ok.
Previously I missed the fact that this returns stats for *some* vms and not all 
of them, so this should not be _getAllVMsBulkStats but _getVMsBulkStats.
Line 717: domList = [x._dom._dom for x in vmList]
Line 718: if statsTypes == 0:
Line 719: statsTypes = (libvirt.VIR_DOMAIN_STATS_STATE |
Line 720:   libvirt.VIR_DOMAIN_STATS_CPU_TOTAL |


Line 1419: self.conf['timeOffset'] = newTimeOffset
Line 1420: 
Line 1421: def _getBulkStats(self, statsTypes, statsFlags):
Line 1422: return _vmsGetBulkStats(self._connection, [self], statsTypes,
Line 1423: statsFlags)[self.id]
 Yes this is a good long-term idea but I am not sure I want to commit to it 
I see now that this get the stats for only one vm ([self]), so this fine.
Line 1424: 
Line 1425: def _getWriteWatermarks(self):
Line 1426: def pathToVolID(drive, path):
Line 1427: for vol in drive.volumeChain:


-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 4:

(13 comments)

http://gerrit.ovirt.org/#/c/36924/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 722: 'writeLatency': str(compute_latency('wr')),
Line 723: 'flushLatency': str(compute_latency('flush'))}
Line 724: 
Line 725: 
Line 726: def _getAllVMsBulkStats(conn, vmList, statsTypes=0, statsFlags=0):
This returns stats for *some* vms, not all. Sorry for asking for  the bad name 
:-)
Line 727: domList = [x._dom._dom for x in vmList]
Line 728: if statsTypes == 0:
Line 729: statsTypes = (libvirt.VIR_DOMAIN_STATS_STATE |
Line 730:   libvirt.VIR_DOMAIN_STATS_CPU_TOTAL |


Line 723: 'flushLatency': str(compute_latency('flush'))}
Line 724: 
Line 725: 
Line 726: def _getAllVMsBulkStats(conn, vmList, statsTypes=0, statsFlags=0):
Line 727: domList = [x._dom._dom for x in vmList]
x would be more clear as vm.
Line 728: if statsTypes == 0:
Line 729: statsTypes = (libvirt.VIR_DOMAIN_STATS_STATE |
Line 730:   libvirt.VIR_DOMAIN_STATS_CPU_TOTAL |
Line 731:   libvirt.VIR_DOMAIN_STATS_BALLOON |


Line 732:   libvirt.VIR_DOMAIN_STATS_VCPU |
Line 733:   libvirt.VIR_DOMAIN_STATS_INTERFACE |
Line 734:   libvirt.VIR_DOMAIN_STATS_BLOCK)
Line 735: statsList = conn.domainListGetStats(domList, statsTypes, 
statsFlags)
Line 736: return dict([(x[0].UUIDString(), x[1]) for x in statsList])
The x variable is not very helpful, specially when you don't know what are x[0] 
and x[1]. Better unpack the tuple/list.

And we can avoid the temporary list using generator expression, which is also 
little more clean:

return dict((dom.UUIDString(), domStats) for dom, domStats in statsList)
Line 737: 
Line 738: 
Line 739: class TimeoutError(libvirt.libvirtError):
Line 740: pass


Line 1429: self.conf['timeOffset'] = newTimeOffset
Line 1430: 
Line 1431: def _getBulkStats(self, statsTypes, statsFlags):
Line 1432: return _getAllVMsBulkStats(self._connection, [self], 
statsTypes,
Line 1433:statsFlags)[self.id]
This is little too hard to read as one line. It would be nicer as:

vmsStats = _getVMsBulkStats(self._connection, [self], statsTypes, 
statsFlags)
return vmsStats[self.id]
Line 1434: 
Line 1435: def _getWriteWatermarks(self):
Line 1436: volAllocMap = {}
Line 1437: bulkStats = 
self._getBulkStats(libvirt.VIR_DOMAIN_STATS_BLOCK,


Line 1431: def _getBulkStats(self, statsTypes, statsFlags):
Line 1432: return _getAllVMsBulkStats(self._connection, [self], 
statsTypes,
Line 1433:statsFlags)[self.id]
Line 1434: 
Line 1435: def _getWriteWatermarks(self):
Should be documented - what stats do we get, for which volumes.
Line 1436: volAllocMap = {}
Line 1437: bulkStats = 
self._getBulkStats(libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1438:
_LIBVIRT_BACKING_CHAIN_STATS_FLAG)
Line 1439: for i in xrange(bulkStats['block.count']):


Line 1432: return _getAllVMsBulkStats(self._connection, [self], 
statsTypes,
Line 1433:statsFlags)[self.id]
Line 1434: 
Line 1435: def _getWriteWatermarks(self):
Line 1436: volAllocMap = {}
Since this returns watermarks, it would be little bit nicer if you call this 
map watermarks instead of volAllocMap.

This is also more consistent with other code such as 
_getLiveMergeExtendCandidates.
Line 1437: bulkStats = 
self._getBulkStats(libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1438:
_LIBVIRT_BACKING_CHAIN_STATS_FLAG)
Line 1439: for i in xrange(bulkStats['block.count']):
Line 1440: name = bulkStats['block.%i.name' % i]


Line 1436: volAllocMap = {}
Line 1437: bulkStats = 
self._getBulkStats(libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1438:
_LIBVIRT_BACKING_CHAIN_STATS_FLAG)
Line 1439: for i in xrange(bulkStats['block.count']):
Line 1440: name = bulkStats['block.%i.name' % i]
What is this expected key is missing?
Line 1441: try:
Line 1442: drive = self._findDriveByName(name)
Line 1443: except LookupError:
Line 1444: self.log.error(Unable to find drive '%s', name)


Line 1450: path = bulkStats['block.%i.path' % i]
Line 1451: alloc = bulkStats['block.%i.allocation' % i]
Line 1452: except KeyError as e:
Line 1453: self.log.debug(Block stats are missing expected key 
'%s', 
Line 1454:skipping volume, e.args[0])
Adding volume name would 

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

[oVirt Jenkins CI Server]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 4:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/15582/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/15414/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/2314/ : 
There was an infra issue, please contact in...@ovirt.org

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/14610/ : FAILURE

-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 3:

(5 comments)

Added few more comments, I still need more time to finish the review.

http://gerrit.ovirt.org/#/c/36924/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 169: 
Line 170: 
Line 171: # Since libvirt 1.2.13, the virConnectGetAllDomainStats API will 
return
Line 172: # block statistics for all volumes in the chain when using a new flag.
Line 173: _libvirtBackingChainStatsFlag = getattr(
Why do you a constant which looks like a variable? Can this value change after 
it was set?
Line 174: libvirt, VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING, 0)
Line 175: 
Line 176: 
Line 177: class VmStatsThread(AdvancedStatsThread):


Line 712: 'writeLatency': str(compute_latency('wr')),
Line 713: 'flushLatency': str(compute_latency('flush'))}
Line 714: 
Line 715: 
Line 716: def _vmsGetBulkStats(conn, vmList, statsTypes=0, statsFlags=0):
I think _getAllVMsBulkStats would be more clear
Line 717: domList = [x._dom._dom for x in vmList]
Line 718: if statsTypes == 0:
Line 719: statsTypes = (libvirt.VIR_DOMAIN_STATS_STATE |
Line 720:   libvirt.VIR_DOMAIN_STATS_CPU_TOTAL |


Line 1419: self.conf['timeOffset'] = newTimeOffset
Line 1420: 
Line 1421: def _getBulkStats(self, statsTypes, statsFlags):
Line 1422: return _vmsGetBulkStats(self._connection, [self], statsTypes,
Line 1423: statsFlags)[self.id]
So we must get stats for all vms for getting single vm stats?

I think we should get bulkstats for all vms in the sampling thread, and use 
cached data here.

Today perform virDomainBlockInfo for each disk on each vm every 2 seconds (and 
some other calls every 15 and 60 seconds). We can replace this with one call to 
get bulk stats every 2 seconds, and use cached values everywhere else.
Line 1424: 
Line 1425: def _getWriteWatermarks(self):
Line 1426: def pathToVolID(drive, path):
Line 1427: for vol in drive.volumeChain:


Line 1424: 
Line 1425: def _getWriteWatermarks(self):
Line 1426: def pathToVolID(drive, path):
Line 1427: for vol in drive.volumeChain:
Line 1428: if os.path.realpath(vol['path']) == 
os.path.realpath(path):
Finding the real path should be done once.

Can you explain which values we get from libvirt and why we need to normalize 
them accessing the file system?

Why not use os.stat instead of the complex and expensive dance that is 
os.path.realpath()?

it = os.stat(path)

for vol in drive.volumeChain:
st = os.stat(vol[path])
if st.st_ino, st.st_dev == it.st_ino, it.st_dev:
found it...

Last, having inline function make it harder to read the code and increase the 
chance for duplicate code doing the same thing. I think we should avoid these 
unless we must (e.g. decorators).
Line 1429: return vol['volumeID']
Line 1430: raise LookupError(Unable to find VolumeID for path 
'%s', path)
Line 1431: 
Line 1432: volAllocMap = {}


Line 1436: name = bulkStats['block.%i.name' % i]
Line 1437: try:
Line 1438: drive = self._findDriveByName(name)
Line 1439: except LookupError:
Line 1440: continue
Is this expected? I think we like at least a debug log here to understand why 
this happens.
Line 1441: if not drive.blockDev or drive.format != 'cow':
Line 1442: continue
Line 1443: 
Line 1444: try:


-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[oVirt Jenkins CI Server]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 3:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/15459/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/15290/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/2275/ : 
There was an infra issue, please contact in...@ovirt.org

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/14502/ : FAILURE

-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[alitke]

Adam Litke has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 2:

(5 comments)

http://gerrit.ovirt.org/#/c/36924/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 172: # block statistics for all volumes in the chain when using a new 
flag.
Line 173: _libvirtBackingChainStatsFlag = \
Line 174: libvirt.VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING
Line 175: except AttributeError:
Line 176: _libvirtBackingChainStatsFlag = 0
 This will be little nicer:
Done
Line 177: 
Line 178: 
Line 179: class VmStatsThread(AdvancedStatsThread):
Line 180: MBPS_TO_BPS = 10 ** 6 / 8


Line 1431: return vol['volumeID']
Line 1432: raise LookupError(Unable to find VolumeID for path 
'%s', path)
Line 1433: 
Line 1434: volAllocMap = {}
Line 1435: blkStats = self._getBulkStats(libvirt.VIR_DOMAIN_STATS_BLOCK,
 Can we call this bulkStats?
Done
Line 1436:   _libvirtBackingChainStatsFlag)
Line 1437: for i in xrange(blkStats['block.count']):
Line 1438: name = blkStats['block.%i.name' % i]
Line 1439: try:


Line 4763: startCleanup(storedJob, drive, doPivot)
Line 4764: jobsRet[jobID] = entry
Line 4765: return jobsRet
Line 4766: 
Line 4767: def _libvirtBackingChainStatsFlag(self):
 Can be deleted
Done
Line 4768: # Since libvirt 1.2.13, the virConnectGetAllDomainStats API 
will return
Line 4769: # block statistics for all volumes in the chain when using a 
new flag.
Line 4770: try:
Line 4771: return libvirt.VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING


Line 4873: # during the rest of the operation.  Otherwise, extend now to
Line 4874: # accomodate the worst case scenario: no intersection 
between the
Line 4875: # allocated blocks in the base volume and the top volume.
Line 4876: if drive.blockDev and baseCow:
Line 4877: if self._libvirtBackingChainStatsFlag():
 Replace with the constant
Done
Line 4878: self.extendDrivesIfNeeded()
Line 4879: else:
Line 4880: extendSize = baseSize + topSize
Line 4881: self.log.debug(Preemptively extending volume %s 
with size %i


Line 4874: # accomodate the worst case scenario: no intersection 
between the
Line 4875: # allocated blocks in the base volume and the top volume.
Line 4876: if drive.blockDev and baseCow:
Line 4877: if self._libvirtBackingChainStatsFlag():
Line 4878: self.extendDrivesIfNeeded()
 This will not extend by one chunk as you describe, but try to extend all dr
I think an adjustment to the language of the comment is all that's required.  I 
personally prefer to keep the extension logic for the common case in 
extendDrivesIfNeeded().  Since the block of code above registered the block 
job, this call to extendDrivesIfNeeded() will be able to add the live merge 
candidate volume.  We're simply choosing to try an extension right now rather 
than wait for the next stats collection interval.
Line 4879: else:
Line 4880: extendSize = baseSize + topSize
Line 4881: self.log.debug(Preemptively extending volume %s 
with size %i
Line 4882:(job: %s), baseVolUUID, extendSize, 
jobUUID)


-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[alitke]

Adam Litke has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 2: Verified+1

-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[oVirt Jenkins CI Server]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 2:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/15401/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/15232/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/2260/ : 
There was an infra issue, please contact in...@ovirt.org

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/1/ : FAILURE

-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 2:

(5 comments)

Mostly ok, but will have to invest more time in this. Added few comments and 
questions.

http://gerrit.ovirt.org/#/c/36924/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 172: # block statistics for all volumes in the chain when using a new 
flag.
Line 173: _libvirtBackingChainStatsFlag = \
Line 174: libvirt.VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING
Line 175: except AttributeError:
Line 176: _libvirtBackingChainStatsFlag = 0
This will be little nicer:

_STATS_BACKING_FLAG = getattr(
libvirt, VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING, 0)
Line 177: 
Line 178: 
Line 179: class VmStatsThread(AdvancedStatsThread):
Line 180: MBPS_TO_BPS = 10 ** 6 / 8


Line 1431: return vol['volumeID']
Line 1432: raise LookupError(Unable to find VolumeID for path 
'%s', path)
Line 1433: 
Line 1434: volAllocMap = {}
Line 1435: blkStats = self._getBulkStats(libvirt.VIR_DOMAIN_STATS_BLOCK,
Can we call this bulkStats?
Line 1436:   _libvirtBackingChainStatsFlag)
Line 1437: for i in xrange(blkStats['block.count']):
Line 1438: name = blkStats['block.%i.name' % i]
Line 1439: try:


Line 4763: startCleanup(storedJob, drive, doPivot)
Line 4764: jobsRet[jobID] = entry
Line 4765: return jobsRet
Line 4766: 
Line 4767: def _libvirtBackingChainStatsFlag(self):
Can be deleted
Line 4768: # Since libvirt 1.2.13, the virConnectGetAllDomainStats API 
will return
Line 4769: # block statistics for all volumes in the chain when using a 
new flag.
Line 4770: try:
Line 4771: return libvirt.VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING


Line 4873: # during the rest of the operation.  Otherwise, extend now to
Line 4874: # accomodate the worst case scenario: no intersection 
between the
Line 4875: # allocated blocks in the base volume and the top volume.
Line 4876: if drive.blockDev and baseCow:
Line 4877: if self._libvirtBackingChainStatsFlag():
Replace with the constant
Line 4878: self.extendDrivesIfNeeded()
Line 4879: else:
Line 4880: extendSize = baseSize + topSize
Line 4881: self.log.debug(Preemptively extending volume %s 
with size %i


Line 4874: # accomodate the worst case scenario: no intersection 
between the
Line 4875: # allocated blocks in the base volume and the top volume.
Line 4876: if drive.blockDev and baseCow:
Line 4877: if self._libvirtBackingChainStatsFlag():
Line 4878: self.extendDrivesIfNeeded()
This will not extend by one chunk as you describe, but try to extend all 
drives, and will probably do not extend this drive.

I think we should calculate the requested size in the if, and extend out of the 
if.
Line 4879: else:
Line 4880: extendSize = baseSize + topSize
Line 4881: self.log.debug(Preemptively extending volume %s 
with size %i
Line 4882:(job: %s), baseVolUUID, extendSize, 
jobUUID)


-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[fromani]

Francesco Romani has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 1:

(1 comment)

Thanks for the answer!

Do you plan for a tighter integration of this code with followup patches? By 
looking at the methods you added and how they are going to play with 
_getExtendCandidates, it _seems_ to me that there is some room for further 
improvement.

If softdeps are properly handled (I just need to read the updated commit 
message)it seems to me (IIUC all the details of the flow) that we could have 
just one loop in _getExtendCandidates and issue domainListGetStats here and 
once, and do all checks per-drive.
This could make the code easier to follow and spare us a lot of find*() and 
loops.

http://gerrit.ovirt.org/#/c/36924/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1551: return {}
Line 1552: 
Line 1553: candidates = {}
Line 1554: watermarks = self._getWriteWatermarks()
Line 1555: for job in self.conf['_blockJobs'].values():
I wonder if it makes any sense to iterate not on blockJobs but on watermarks. 
IIUC the flow, this should save us to do again checks at line 1564 and 1582 
below, since the not interesting drives are been ruled out by check at line 
1534 above.

This could make the code a bit easier to follow.
Line 1556: try:
Line 1557: drive = self._findDriveByUUIDs(job['disk'])
Line 1558: except LookupError:
Line 1559: # After an active layer merge completes the vdsm 
metadata will


-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[alitke]

Adam Litke has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 1:

(1 comment)

Francesco,
Regarding dependency on libvirt: We have a soft dependency built into the code. 
 If libvirt does not support the backing chain stats then we detect that and 
apply the current, overzealous, preemptive extension of internal volumes.  This 
deserves to be mentioned in the commit message that I will update in the next 
submission.

Thank you for your review!

http://gerrit.ovirt.org/#/c/36924/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1523: statsFlags = self._libvirtBackingChainStatsFlag()
Line 1524: conn = libvirtconnection.get()
Line 1525: blkStats = conn.domainListGetStats([self._dom._dom],
Line 1526:
libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1527:statsFlags)[0][1]
 I prefer to avoid the double indexing.
Done
Line 1528: for i in xrange(0, blkStats['block.count']):
Line 1529: name = blkStats['block.%i.name' % i]
Line 1530: try:
Line 1531: drive = self._findDriveByName(name)


-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[alitke]

Adam Litke has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 1:

(5 comments)

http://gerrit.ovirt.org/#/c/36924/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1520: raise LookupError(Unable to find VolumeID for path 
'%s', path)
Line 1521: 
Line 1522: volAllocMap = {}
Line 1523: statsFlags = self._libvirtBackingChainStatsFlag()
Line 1524: conn = libvirtconnection.get()
 unless you need a specific different connection (then please explain why), 
Done
Line 1525: blkStats = conn.domainListGetStats([self._dom._dom],
Line 1526:
libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1527:statsFlags)[0][1]
Line 1528: for i in xrange(0, blkStats['block.count']):


Line 1523: statsFlags = self._libvirtBackingChainStatsFlag()
Line 1524: conn = libvirtconnection.get()
Line 1525: blkStats = conn.domainListGetStats([self._dom._dom],
Line 1526:
libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1527:statsFlags)[0][1]
 I prefer to avoid the double indexing.
Done
Line 1528: for i in xrange(0, blkStats['block.count']):
Line 1529: name = blkStats['block.%i.name' % i]
Line 1530: try:
Line 1531: drive = self._findDriveByName(name)


Line 1523: statsFlags = self._libvirtBackingChainStatsFlag()
Line 1524: conn = libvirtconnection.get()
Line 1525: blkStats = conn.domainListGetStats([self._dom._dom],
Line 1526:
libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1527:statsFlags)[0][1]
 about the bulk stats, we need a common shared API/pattern to use them. We'r
I've headed down this direction in the next series.  I hope you like the 
changes.
Line 1528: for i in xrange(0, blkStats['block.count']):
Line 1529: name = blkStats['block.%i.name' % i]
Line 1530: try:
Line 1531: drive = self._findDriveByName(name)


Line 1524: conn = libvirtconnection.get()
Line 1525: blkStats = conn.domainListGetStats([self._dom._dom],
Line 1526:
libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1527:statsFlags)[0][1]
Line 1528: for i in xrange(0, blkStats['block.count']):
 why the explicit start? Isn't xrange(blkStats['block.count']) enough?
Just a personal preference but am happy to change it if it's important enough 
to mention here.
Line 1529: name = blkStats['block.%i.name' % i]
Line 1530: try:
Line 1531: drive = self._findDriveByName(name)
Line 1532: except LookupError:


Line 4858: # block statistics for all volumes in the chain when using a 
new flag.
Line 4859: try:
Line 4860: return libvirt.VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING
Line 4861: except AttributeError:
Line 4862: return 0
 This could be detected once per VDSM run, no need to check it for every ope
Done
Line 4863: 
Line 4864: def merge(self, driveSpec, baseVolUUID, topVolUUID, bandwidth, 
jobUUID):
Line 4865: if not caps.getLiveMergeSupport():
Line 4866: self.log.error(Live merge is not supported on this 
host)


-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[fromani]

Francesco Romani has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/36924/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1523: statsFlags = self._libvirtBackingChainStatsFlag()
Line 1524: conn = libvirtconnection.get()
Line 1525: blkStats = conn.domainListGetStats([self._dom._dom],
Line 1526:
libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1527:statsFlags)[0][1]
 I prefer to avoid the double indexing.
about the bulk stats, we need a common shared API/pattern to use them. We're 
(hopefully) going to use them for sampling as well and I want to avoid 
duplication
Line 1528: for i in xrange(0, blkStats['block.count']):
Line 1529: name = blkStats['block.%i.name' % i]
Line 1530: try:
Line 1531: drive = self._findDriveByName(name)


-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[fromani]

Francesco Romani has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 1: Code-Review-1

(4 comments)

A few initial remarks. Most important: we need some form of dependency from new 
libvirt, either hard or soft, don't we?

http://gerrit.ovirt.org/#/c/36924/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1520: raise LookupError(Unable to find VolumeID for path 
'%s', path)
Line 1521: 
Line 1522: volAllocMap = {}
Line 1523: statsFlags = self._libvirtBackingChainStatsFlag()
Line 1524: conn = libvirtconnection.get()
unless you need a specific different connection (then please explain why), just 
use self._connection
Line 1525: blkStats = conn.domainListGetStats([self._dom._dom],
Line 1526:
libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1527:statsFlags)[0][1]
Line 1528: for i in xrange(0, blkStats['block.count']):


Line 1523: statsFlags = self._libvirtBackingChainStatsFlag()
Line 1524: conn = libvirtconnection.get()
Line 1525: blkStats = conn.domainListGetStats([self._dom._dom],
Line 1526:
libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1527:statsFlags)[0][1]
I prefer to avoid the double indexing.
Why we need [0] is sufficiently clear, but I prefer

_, blkStats = conn...

only for clarity
Line 1528: for i in xrange(0, blkStats['block.count']):
Line 1529: name = blkStats['block.%i.name' % i]
Line 1530: try:
Line 1531: drive = self._findDriveByName(name)


Line 1524: conn = libvirtconnection.get()
Line 1525: blkStats = conn.domainListGetStats([self._dom._dom],
Line 1526:
libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1527:statsFlags)[0][1]
Line 1528: for i in xrange(0, blkStats['block.count']):
why the explicit start? Isn't xrange(blkStats['block.count']) enough?
Line 1529: name = blkStats['block.%i.name' % i]
Line 1530: try:
Line 1531: drive = self._findDriveByName(name)
Line 1532: except LookupError:


Line 4858: # block statistics for all volumes in the chain when using a 
new flag.
Line 4859: try:
Line 4860: return libvirt.VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING
Line 4861: except AttributeError:
Line 4862: return 0
This could be detected once per VDSM run, no need to check it for every 
operation.

Probably better as module level private constant.
Line 4863: 
Line 4864: def merge(self, driveSpec, baseVolUUID, topVolUUID, bandwidth, 
jobUUID):
Line 4865: if not caps.getLiveMergeSupport():
Line 4866: self.log.error(Live merge is not supported on this 
host)


-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[oVirt Jenkins CI Server]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
..


Patch Set 1:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/15044/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14875/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/2179/ : 
There was an infra issue, please contact in...@ovirt.org

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/14087/ : FAILURE

-- 
To view, visit http://gerrit.ovirt.org/36924
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke ali...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

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

[alitke]

Adam Litke has uploaded a new change for review.

Change subject: Live Merge: Restore watermark tracking
..

Live Merge: Restore watermark tracking

Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f
Signed-off-by: Adam Litke ali...@redhat.com
---
M vdsm/virt/vm.py
1 file changed, 108 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/24/36924/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index f22610d..09080b9 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -1512,12 +1512,94 @@
 with self._confLock:
 self.conf['timeOffset'] = newTimeOffset
 
+def _getWriteWatermarks(self):
+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)
+
+volAllocMap = {}
+statsFlags = self._libvirtBackingChainStatsFlag()
+conn = libvirtconnection.get()
+blkStats = conn.domainListGetStats([self._dom._dom],
+   libvirt.VIR_DOMAIN_STATS_BLOCK,
+   statsFlags)[0][1]
+for i in xrange(0, blkStats['block.count']):
+name = blkStats['block.%i.name' % i]
+try:
+drive = self._findDriveByName(name)
+except LookupError:
+continue
+if not drive.blockDev or drive.format != 'cow':
+continue
+
+try:
+path = blkStats['block.%i.path' % i]
+alloc = blkStats['block.%i.allocation' % i]
+except KeyError as e:
+self.log.debug(Block stats are missing expected key '%s', 
+   skipping volume, e.args[0])
+continue
+volID = pathToVolID(drive, path)
+volAllocMap[volID] = alloc
+return volAllocMap
+
+def _getLiveMergeExtendCandidates(self):
+# The common case is that there are no active jobs.
+if not self.conf['_blockJobs'].values():
+return {}
+
+candidates = {}
+watermarks = self._getWriteWatermarks()
+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.
+continue
+
+if not drive.blockDev:
+continue
+
+if job['strategy'] == 'commit':
+volumeID = job['baseVolume']
+else:
+self.log.debug(Unrecognized merge strategy '%s',
+   job['strategy'])
+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
+
+if volumeID in watermarks:
+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}
+else:
+self.log.warning(No watermark info available for %s,
+ volumeID)
+return candidates
+
 def _getExtendCandidates(self):
 ret = []
 
-# FIXME: mergeCandidates should be a dictionary of candidate volumes
-# once libvirt starts reporting watermark information for all volumes.
-mergeCandidates = {}
+mergeCandidates = self._getLiveMergeExtendCandidates()
 for drive in self._devices[hwclass.DISK]:
 if not drive.blockDev or drive.format != 'cow':
 continue
@@ -4771,6 +4853,14 @@
 jobsRet[jobID] = entry
 return jobsRet
 
+def _libvirtBackingChainStatsFlag(self):
+# Since libvirt 1.2.13, the virConnectGetAllDomainStats API will return
+# block statistics for all volumes in the chain when using