Change in vdsm[master]: vmstats: take in account missing bulk stats fields

Francesco Romani has uploaded a new change for review.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

vmstats: take in account missing bulk stats fields

One of the cornerstones of how libvirt's bulk stats work is
accumulate as much as we can, raise errors only on the most
critical paths.

This was well understood for actual sampled data, e.g. disk
traffic, network traffic, but was unexptected for static (meta)
data, like interface/block name.

We discovered in the SR-IOV case that this can indeed be true,
so we add defensive code for this scenario, taking into account
that also the static data may be absent.
This also means that the '<group>.count' attribute
(e.g. net.count, block.count) must be considered an upper bound,
not an exact amount.

It is worth checking if the observed behaviour it is actually OK
inside libvirt; anyway, this added defense fits nicely in the
bulk stats consuming code.

Change-Id: I5a0eae867305dead8a96a23801ff2429605522ea
Signed-off-by: Francesco Romani 
---
M vdsm/virt/vmstats.py
1 file changed, 10 insertions(+), 1 deletion(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/60/47760/1

diff --git a/vdsm/virt/vmstats.py b/vdsm/virt/vmstats.py
index 73c1d98..ead99e7 100644
--- a/vdsm/virt/vmstats.py
+++ b/vdsm/virt/vmstats.py
@@ -359,7 +359,16 @@
 def _find_bulk_stats_reverse_map(stats, group):
     name_to_idx = {}
     for idx in six.moves.xrange(stats.get('%s.count' % group, 0)):
-        name_to_idx[stats['%s.%d.name' % (group, idx)]] = idx
+        try:
+            name = stats['%s.%d.name' % (group, idx)]
+        except KeyError:
+            # bulk stats accumulate what they can get, raising errors
+            # only in the critical cases. This includes fundamntal
+            # attributes like names, so count has to be considered
+            # an upper bound more like a precise indicator.
+            pass
+        else:
+            name_to_idx[name] = idx
     return name_to_idx

[e-mail] has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

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'])

Ido Barkan has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

Patch Set 1: Code-Review-1

I think it might be a good idea to fix _FAKE_BULK_STATS to simulate this in a unit test

Ido Barkan has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

Patch Set 1:

an example would be:
vcpu.0.time=22080000000
  net.count=2
  net.1.name=vnet0
  net.1.rx.bytes=648
  net.1.rx.pkts=8
  net.1.rx.errs=0
  net.1.rx.drop=0
  net.1.tx.bytes=1992
  net.1.tx.pkts=12
  net.1.tx.errs=0
  net.1.tx.drop=0
  block.count=2

[e-mail] has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

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'])

Ido Barkan has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

Patch Set 2: Code-Review+1

Ido Barkan has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

Patch Set 2:

please add Bug-Url: https://bugzilla.redhat.com/1273837

[e-mail] has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

Patch Set 3:

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

Francesco Romani has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

Patch Set 3: Verified+1

this change is very simple and covered by tests. Verified running the testsuite.

Milan Zamazal has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

Patch Set 3: Code-Review-1

(2 comments)

Fine, just typos in the comment.

https://gerrit.ovirt.org/#/c/47760/3/vdsm/virt/vmstats.py
File vdsm/virt/vmstats.py:

Line 361:     for idx in six.moves.xrange(stats.get('%s.count' % group, 0)):
Line 362:         try:
Line 363:             name = stats['%s.%d.name' % (group, idx)]
Line 364:         except KeyError:
Line 365:             # bulk stats accumulate what they can get, raising errors
Bulk ...
Line 366:             # only in the critical cases. This includes fundamntal
Line 367:             # attributes like names, so count has to be considered
Line 368:             # an upper bound more like a precise indicator.
Line 369:             pass

Line 362:         try:
Line 363:             name = stats['%s.%d.name' % (group, idx)]
Line 364:         except KeyError:
Line 365:             # bulk stats accumulate what they can get, raising errors
Line 366:             # only in the critical cases. This includes fundamntal
... fundamental
Line 367:             # attributes like names, so count has to be considered
Line 368:             # an upper bound more like a precise indicator.
Line 369:             pass
Line 370:         else:

Francesco Romani has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

Patch Set 3:

(2 comments)

https://gerrit.ovirt.org/#/c/47760/3/vdsm/virt/vmstats.py
File vdsm/virt/vmstats.py:

Line 361:     for idx in six.moves.xrange(stats.get('%s.count' % group, 0)):
Line 362:         try:
Line 363:             name = stats['%s.%d.name' % (group, idx)]
Line 364:         except KeyError:
Line 365:             # bulk stats accumulate what they can get, raising errors
> Bulk ...

Done
Line 366:             # only in the critical cases. This includes fundamntal
Line 367:             # attributes like names, so count has to be considered
Line 368:             # an upper bound more like a precise indicator.
Line 369:             pass

Line 362:         try:
Line 363:             name = stats['%s.%d.name' % (group, idx)]
Line 364:         except KeyError:
Line 365:             # bulk stats accumulate what they can get, raising errors
Line 366:             # only in the critical cases. This includes fundamntal
> ... fundamental

Done
Line 367:             # attributes like names, so count has to be considered
Line 368:             # an upper bound more like a precise indicator.
Line 369:             pass
Line 370:         else:

[e-mail] has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

Patch Set 4:

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

Francesco Romani has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

Patch Set 4: Verified+1

changes in the comments -> copying score

Milan Zamazal has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

Patch Set 4: Code-Review+1

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

Patch Set 4: Code-Review+2

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

vmstats: take in account missing bulk stats fields

One of the cornerstones of how libvirt's bulk stats work is
accumulate as much as we can, raise errors only on the most
critical paths.

This was well understood for actual sampled data, e.g. disk
traffic, network traffic, but was unexptected for static (meta)
data, like interface/block name.

We discovered in the SR-IOV case that this can indeed be true,
so we add defensive code for this scenario, taking into account
that also the static data may be absent.
This also means that the '<group>.count' attribute
(e.g. net.count, block.count) must be considered an upper bound,
not an exact amount.

It is worth checking if the observed behaviour it is actually OK
inside libvirt; anyway, this added defense fits nicely in the
bulk stats consuming code.

Change-Id: I5a0eae867305dead8a96a23801ff2429605522ea
Signed-off-by: Francesco Romani 
Bug-Url: https://bugzilla.redhat.com/1273837
Reviewed-on: https://gerrit.ovirt.org/47760
Reviewed-by: Milan Zamazal 
Continuous-Integration: Jenkins CI
Reviewed-by: Dan Kenigsberg 
---
M tests/vmStatsTests.py
M vdsm/virt/vmstats.py
2 files changed, 80 insertions(+), 1 deletion(-)

Approvals:
  Jenkins CI: Passed CI tests
  Dan Kenigsberg: Looks good to me, approved
  Francesco Romani: Verified
  Milan Zamazal: Looks good to me, but someone else must approve

[e-mail] has posted comments on this change.

Change subject: vmstats: take in account missing bulk stats fields
......................................................................

Patch Set 5:

* Update tracker::#1273837::OK
* Set MODIFIED::bug 1273837::::#1273837::::IGNORE, not oVirt prod but vdsm