Change in vdsm[ovirt-3.6]: vmstats: handle known-missing stats

fromani at redhat.com fromani at redhat.com
Wed Feb 24 15:28:09 UTC 2016


Hello Dan Kenigsberg,

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

    https://gerrit.ovirt.org/53992

to review the following change.

Change subject: vmstats: handle known-missing stats
......................................................................

vmstats: handle known-missing stats

In some well-known and benign causes, libvirt will not
return the complete set of bulk stats.

It is dangerous to blindly ignore KeyErrors when some
fields are missing, because this can hide other bugs.
But it is safe to ignore them in the aforementioned cases.

This patch lays down the basic pieces to safely ignore
the errors, starting with the first loud and known case:
migration destination VM.

Backport-To: 3.6
Change-Id: I04ecbf98d36d576ede3c3cde36586245ea5e4218
Signed-off-by: Francesco Romani <fromani at redhat.com>
Reviewed-on: https://gerrit.ovirt.org/43777
Continuous-Integration: Jenkins CI
Reviewed-by: Dan Kenigsberg <danken at redhat.com>
---
M tests/vmStatsTests.py
M tests/vmTests.py
M vdsm/virt/vm.py
M vdsm/virt/vmstats.py
4 files changed, 81 insertions(+), 53 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/92/53992/1

diff --git a/tests/vmStatsTests.py b/tests/vmStatsTests.py
index f262f94..78e9998 100644
--- a/tests/vmStatsTests.py
+++ b/tests/vmStatsTests.py
@@ -310,8 +310,10 @@
     def test_nic_have_all_keys(self):
         nic = FakeNic(name='vnet0', model='virtio',
                       mac_addr='00:1a:4a:16:01:51')
+        testvm = FakeVM(nics=(nic,))
 
         stats = vmstats._nic_traffic(
+            testvm,
             nic.name, nic.nicModel, nic.macAddr,
             self.bulk_stats, 0,
             self.bulk_stats, 0,
diff --git a/tests/vmTests.py b/tests/vmTests.py
index fb8a89d..a0d7d0b 100644
--- a/tests/vmTests.py
+++ b/tests/vmTests.py
@@ -1439,27 +1439,29 @@
         GBPS = 10 ** 9 / 8
         MAC = '52:54:00:59:F5:3F'
         pretime = utils.monotonic_time()
-        res = vmstats._nic_traffic(
-            name='vnettest', model='virtio', mac=MAC,
-            start_sample={'net.0.rx.bytes': 2 ** 64 - 15 * GBPS,
-                          'net.0.rx.pkts': 1,
-                          'net.0.rx.errs': 2,
-                          'net.0.rx.drop': 3,
-                          'net.0.tx.bytes': 0,
-                          'net.0.tx.pkts': 4,
-                          'net.0.tx.errs': 5,
-                          'net.0.tx.drop': 6},
-            start_index=0,
-            end_sample={'net.0.rx.bytes': 0,
-                        'net.0.rx.pkts': 7,
-                        'net.0.rx.errs': 8,
-                        'net.0.rx.drop': 9,
-                        'net.0.tx.bytes': 5 * GBPS,
-                        'net.0.tx.pkts': 10,
-                        'net.0.tx.errs': 11,
-                        'net.0.tx.drop': 12},
-            end_index=0,
-            interval=15.0)
+        with fake.VM(_VM_PARAMS) as testvm:
+            res = vmstats._nic_traffic(
+                testvm,
+                name='vnettest', model='virtio', mac=MAC,
+                start_sample={'net.0.rx.bytes': 2 ** 64 - 15 * GBPS,
+                              'net.0.rx.pkts': 1,
+                              'net.0.rx.errs': 2,
+                              'net.0.rx.drop': 3,
+                              'net.0.tx.bytes': 0,
+                              'net.0.tx.pkts': 4,
+                              'net.0.tx.errs': 5,
+                              'net.0.tx.drop': 6},
+                start_index=0,
+                end_sample={'net.0.rx.bytes': 0,
+                            'net.0.rx.pkts': 7,
+                            'net.0.rx.errs': 8,
+                            'net.0.rx.drop': 9,
+                            'net.0.tx.bytes': 5 * GBPS,
+                            'net.0.tx.pkts': 10,
+                            'net.0.tx.errs': 11,
+                            'net.0.tx.drop': 12},
+                end_index=0,
+                interval=15.0)
         posttime = utils.monotonic_time()
         self.assertIn('sampleTime', res)
         self.assertTrue(pretime <= res['sampleTime'] <= posttime,
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index bb0c31c..890d328 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -759,7 +759,7 @@
                 self.log.exception("The vm start process failed")
                 self.setDownStatus(ERROR, vmexitreason.GENERIC_ERROR, str(e))
 
-    def _incomingMigrationPending(self):
+    def incomingMigrationPending(self):
         return 'migrationDest' in self.conf or 'restoreState' in self.conf
 
     def stopDisksStatsCollection(self):
diff --git a/vdsm/virt/vmstats.py b/vdsm/virt/vmstats.py
index a816ca4..93dd0e3 100644
--- a/vdsm/virt/vmstats.py
+++ b/vdsm/virt/vmstats.py
@@ -18,6 +18,7 @@
 # Refer to the README and COPYING files for full details of the license
 #
 
+import contextlib
 import logging
 
 import six
@@ -144,11 +145,16 @@
     # MOM will ignore VMs with missing balloon information instead
     # using incomplete data and computing wrong balloon targets
     if balloon_target is not None and sample is not None:
+
+        balloon_cur = 0
+        with _skip_if_missing_stats(vm):
+            balloon_cur = sample['balloon.current']
+
         stats['balloonInfo'].update({
             'balloon_max': str(max_mem),
             'balloon_min': str(
                 int(vm.conf.get('memGuaranteedSize', '0')) * 1024),
-            'balloon_cur': str(sample['balloon.current']),
+            'balloon_cur': str(balloon_cur),
             'balloon_target': str(balloon_target)
         })
 
@@ -166,7 +172,7 @@
             logging.error('Failed to get VM cpu count')
 
 
-def _nic_traffic(name, model, mac,
+def _nic_traffic(vm_obj, name, model, mac,
                  start_sample, start_index,
                  end_sample, end_index, interval):
     ifSpeed = [100, 1000][model in ('e1000', 'virtio')]
@@ -176,22 +182,25 @@
                'speed': str(ifSpeed),
                'state': 'unknown'}
 
-    ifStats['rxErrors'] = str(end_sample['net.%d.rx.errs' % end_index])
-    ifStats['rxDropped'] = str(end_sample['net.%d.rx.drop' % end_index])
-    ifStats['txErrors'] = str(end_sample['net.%d.tx.errs' % end_index])
-    ifStats['txDropped'] = str(end_sample['net.%d.tx.drop' % end_index])
+    with _skip_if_missing_stats(vm_obj):
+        ifStats['rxErrors'] = str(end_sample['net.%d.rx.errs' % end_index])
+        ifStats['rxDropped'] = str(end_sample['net.%d.rx.drop' % end_index])
+        ifStats['txErrors'] = str(end_sample['net.%d.tx.errs' % end_index])
+        ifStats['txDropped'] = str(end_sample['net.%d.tx.drop' % end_index])
 
-    rxDelta = (
-        end_sample['net.%d.rx.bytes' % end_index] -
-        start_sample['net.%d.rx.bytes' % start_index]
-    )
+    with _skip_if_missing_stats(vm_obj):
+        rxDelta = (
+            end_sample['net.%d.rx.bytes' % end_index] -
+            start_sample['net.%d.rx.bytes' % start_index]
+        )
+        txDelta = (
+            end_sample['net.%d.tx.bytes' % end_index] -
+            start_sample['net.%d.tx.bytes' % start_index]
+        )
+
     ifRxBytes = (100.0 *
                  (rxDelta % 2 ** 32) /
                  interval / ifSpeed / _MBPS_TO_BPS)
-    txDelta = (
-        end_sample['net.%d.tx.bytes' % end_index] -
-        start_sample['net.%d.tx.bytes' % start_index]
-    )
     ifTxBytes = (100.0 *
                  (txDelta % 2 ** 32) /
                  interval / ifSpeed / _MBPS_TO_BPS)
@@ -229,7 +238,7 @@
             continue
 
         stats['network'][nic.name] = _nic_traffic(
-            nic.name, nic.nicModel, nic.macAddr,
+            vm, nic.name, nic.nicModel, nic.macAddr,
             first_sample, first_indexes[nic.name],
             last_sample, last_indexes[nic.name],
             interval)
@@ -263,26 +272,28 @@
             if (vm_drive.name in first_indexes and
                vm_drive.name in last_indexes):
                 # will be None if sampled during recovery
-                if interval > 0:
-                    drive_stats.update(
-                        _disk_rate(
-                            first_sample, first_indexes[vm_drive.name],
-                            last_sample, last_indexes[vm_drive.name],
-                            interval))
-                    drive_stats.update(
-                        _disk_latency(
-                            first_sample, first_indexes[vm_drive.name],
-                            last_sample, last_indexes[vm_drive.name]))
-                else:
+                if interval <= 0:
                     logging.warning(
                         'invalid interval %i when calculating '
                         'stats for vm %s disk %s',
                         interval, vm.id, vm_drive.name)
-
-                drive_stats.update(
-                    _disk_iops_bytes(
-                        first_sample, first_indexes[vm_drive.name],
-                        last_sample, last_indexes[vm_drive.name]))
+                else:
+                    with _skip_if_missing_stats(vm):
+                        drive_stats.update(
+                            _disk_rate(
+                                first_sample, first_indexes[vm_drive.name],
+                                last_sample, last_indexes[vm_drive.name],
+                                interval))
+                with _skip_if_missing_stats(vm):
+                    drive_stats.update(
+                        _disk_latency(
+                            first_sample, first_indexes[vm_drive.name],
+                            last_sample, last_indexes[vm_drive.name]))
+                with _skip_if_missing_stats(vm):
+                    drive_stats.update(
+                        _disk_iops_bytes(
+                            first_sample, first_indexes[vm_drive.name],
+                            last_sample, last_indexes[vm_drive.name]))
 
         except AttributeError:
             logging.exception("Disk %s stats not available",
@@ -359,3 +370,16 @@
         else:
             name_to_idx[name] = idx
     return name_to_idx
+
+
+ at contextlib.contextmanager
+def _skip_if_missing_stats(vm_obj):
+    try:
+        yield
+    except KeyError:
+        if vm_obj.incomingMigrationPending():
+            # If a VM is migration destination,
+            # libvirt doesn't give any disk stat.
+            pass
+        else:
+            raise


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I04ecbf98d36d576ede3c3cde36586245ea5e4218
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>


More information about the vdsm-patches mailing list