Change in vdsm[ovirt-3.6]: vmstats: take in account missing bulk stats fields

ibarkan at redhat.com ibarkan at redhat.com
Wed Oct 28 08:08:32 UTC 2015


Hello Dan Kenigsberg, Francesco Romani, Milan Zamazal,

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

    https://gerrit.ovirt.org/47800

to review the following change.

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 <fromani at redhat.com>
Bug-Url: https://bugzilla.redhat.com/1273837
Reviewed-on: https://gerrit.ovirt.org/47760
Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
Continuous-Integration: Jenkins CI
Reviewed-by: Dan Kenigsberg <danken at redhat.com>
---
M tests/vmStatsTests.py
M vdsm/virt/vmstats.py
2 files changed, 80 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/00/47800/1

diff --git a/tests/vmStatsTests.py b/tests/vmStatsTests.py
index 9259335..44fe296 100644
--- a/tests/vmStatsTests.py
+++ b/tests/vmStatsTests.py
@@ -160,6 +160,68 @@
     ),
 }
 
+# on SR-IOV we seen unexpected net.count == 2 but data only for one nic.
+_FAKE_BULK_STATS_SRIOV = {
+    'f3243a90-2e9e-4061-b7b3-a6c585e14857': (
+        {
+            'state.state': 1,
+            'state.reason': 1,
+            'cpu.time': 13755069120,
+            'cpu.user': 3370000000,
+            'cpu.system': 6320000000,
+            'balloon.current': 4194304,
+            'balloon.maximum': 4194304,
+            'vcpu.current': 2,
+            'vcpu.maximum': 16,
+            'vcpu.0.state': 1,
+            'vcpu.0.time': 10910000000,
+            'vcpu.1.state': 1,
+            'vcpu.1.time': 0,
+            'net.count': 2,
+            'net.1.name': 'vnet1',
+            'net.1.rx.bytes': 0,
+            'net.1.rx.pkts': 0,
+            'net.1.rx.errs': 0,
+            'net.1.rx.drop': 0,
+            'net.1.tx.bytes': 0,
+            'net.1.tx.pkts': 0,
+            'net.1.tx.errs': 0,
+            'net.1.tx.drop': 0,
+            'block.count': 2,
+            'block.0.name': 'hdc',
+            'block.0.rd.reqs': 0,
+            'block.0.rd.bytes': 0,
+            'block.0.rd.times': 0,
+            'block.0.wr.reqs': 0,
+            'block.0.wr.bytes': 0,
+            'block.0.wr.times': 0,
+            'block.0.fl.reqs': 0,
+            'block.0.fl.times': 0,
+            'block.0.allocation': 0,
+            'block.1.name': 'vda',
+            'block.1.path': (
+                '/rhev'
+                '/data-center'
+                '/00000001-0001-0001-0001-0000000001e8'
+                '/bbed5784-b0ee-4a0a-aff2-801da0bcf39e'
+                '/images'
+                '/cbe82d1f-a0ba-4af2-af2f-788d15eef043'
+                '/7ba49d31-4fa7-49df-8df4-37a22de79f62'
+            ),
+            'block.1.rd.reqs': 1,
+            'block.1.rd.bytes': 512,
+            'block.1.rd.times': 58991,
+            'block.1.wr.reqs': 0,
+            'block.1.wr.bytes': 0,
+            'block.1.wr.times': 0,
+            'block.1.fl.reqs': 0,
+            'block.1.fl.times': 0,
+            'block.1.allocation': 0,
+            'block.1.capacity': 42949672960,
+        },
+    )
+}
+
 
 class VmStatsTestCase(TestCaseBase):
 
@@ -216,6 +278,14 @@
         # and indeed indexes must change
         self.assertEqual(len(all_indexes), len(self.samples))
 
+    def test_network_missing(self):
+        # seen using SR-IOV
+
+        bulk_stats = _FAKE_BULK_STATS_SRIOV.values()[0]
+        indexes = vmstats._find_bulk_stats_reverse_map(
+            bulk_stats[0], 'net')
+        self.assertTrue(indexes)
+
 
 class NetworkStatsTests(VmStatsTestCase):
 
diff --git a/vdsm/virt/vmstats.py b/vdsm/virt/vmstats.py
index 455586c..1f89c3f 100644
--- a/vdsm/virt/vmstats.py
+++ b/vdsm/virt/vmstats.py
@@ -337,5 +337,14 @@
 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 fundamental
+            # 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


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5a0eae867305dead8a96a23801ff2429605522ea
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>


More information about the vdsm-patches mailing list