Francesco Romani has uploaded a new change for review.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
vm: check stats timeout only for monitorable VMs
We should adjust responsiveness for stats too old only if a VM is monitorable, not inconditionally. Otherwise we can have misreports on slow startup or slow shutdowns.
Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600 Backport-To: 4.0 Backport-To: 3.6 Bug-Url: https://bugzilla.redhat.com/1382578 Bug-Url: https://bugzilla.redhat.com/1382583 Signed-off-by: Francesco Romani fromani@redhat.com --- M tests/vmTests.py M vdsm/virt/vm.py 2 files changed, 19 insertions(+), 3 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/27/65727/1
diff --git a/tests/vmTests.py b/tests/vmTests.py index 603f4af..99a870c 100644 --- a/tests/vmTests.py +++ b/tests/vmTests.py @@ -1343,6 +1343,7 @@ 'vmType': 'kvm', 'memSize': 1024}
+@expandPermutations class TestVmStats(TestCaseBase):
DEV_BALLOON = [{'type': 'balloon', 'specParams': {'model': 'virtio'}}] @@ -1414,8 +1415,14 @@
@MonkeyPatch(vm, 'config', make_config([('vars', 'vm_command_timeout', '10')])) - def testMonitorTimeoutResponsive(self): + @permutations([ + # monitorable + [True], + [False], + ]) + def testMonitorTimeoutResponsive(self, monitorable): with fake.VM(_VM_PARAMS) as testvm: + testvm._monitorable = monitorable # accessing private field self.assertFalse(testvm.isMigrating()) stats = {'monitorResponse': '0'} testvm._setUnresponsiveIfTimeout(stats, 1) # any value < timeout @@ -1423,13 +1430,20 @@
@MonkeyPatch(vm, 'config', make_config([('vars', 'vm_command_timeout', '1')])) - def testMonitorTimeoutUnresponsive(self): + @permutations([ + # monitorable + [True], + [False], + ]) + def testMonitorTimeoutUnresponsive(self, monitorable): with fake.VM(_VM_PARAMS) as testvm: + testvm._monitorable = monitorable # accessing private field self.assertEqual(testvm._monitorResponse, 0) self.assertFalse(testvm.isMigrating()) stats = {'monitorResponse': '0'} testvm._setUnresponsiveIfTimeout(stats, 10) # any value > timeout - self.assertEqual(stats['monitorResponse'], '-1') + self.assertEqual(stats['monitorResponse'], + '0' if not monitorable else '-1')
@MonkeyPatch(vm, 'config', make_config([('vars', 'vm_command_timeout', '10')])) diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py index 6dedd20..ba19d7f 100644 --- a/vdsm/virt/vm.py +++ b/vdsm/virt/vm.py @@ -4686,6 +4686,8 @@ def _setUnresponsiveIfTimeout(self, stats, stats_age): if self.isMigrating(): return + if not self._monitorable: + return # we don't care about decimals here if stats_age < config.getint('vars', 'vm_command_timeout'): return
gerrit-hooks has posted comments on this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Patch Set 1:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Update Tracker::#1382583::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::#1382578::OK, public bug * Check Public Bug::#1382583::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Patch Set 2:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Update Tracker::#1382583::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::#1382578::OK, public bug * Check Public Bug::#1382583::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Patch Set 3:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Update Tracker::#1382583::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::#1382578::OK, public bug * Check Public Bug::#1382583::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Francesco Romani has posted comments on this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Patch Set 3: Verified+1
verified injecting sleep() in the domDependentInit method, and checked that monitorResponse was reported '0' - thus OK- while the thread was sleeping.
Jenkins CI has posted comments on this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Patch Set 3: Continuous-Integration+1
Propagate review hook: Continuous Integration value inherited from patch 1
Jenkins CI has posted comments on this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Patch Set 3:
Propagate review hook: Continuous Integration value inherited from patch 2
Francesco Romani has posted comments on this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/65727/3//COMMIT_MSG Commit Message:
PS3, Line 21: <= 3.6. wrong: either one of the following
* < 3.6 * <= 3.5
Milan Zamazal has posted comments on this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/65727/3/vdsm/virt/vm.py File vdsm/virt/vm.py:
Line 4683: Line 4684: def _setUnresponsiveIfTimeout(self, stats, stats_age): Line 4685: if self.isMigrating(): Line 4686: return Line 4687: if not self._monitorable: Don't we have still a minor race here (I'm not sure, just asking)? May it happen that stats_age "0" is retrieved just before stats cache is initialized with the given VM (in an independent creation thread?) and we reach this line after _monitorable is set? If yes, do we care? Line 4688: return Line 4689: # we don't care about decimals here Line 4690: if stats_age < config.getint('vars', 'vm_command_timeout'): Line 4691: return
gerrit-hooks has posted comments on this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Patch Set 4:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Update Tracker::#1382583::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for 'Red Hat' classification * Check Bug-Url::IGNORE, not relevant for 'Red Hat' classification * Check Public Bug::#1382578::OK, public bug * Check Public Bug::#1382583::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Francesco Romani has posted comments on this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/65727/3/vdsm/virt/vm.py File vdsm/virt/vm.py:
Line 4683: Line 4684: def _setUnresponsiveIfTimeout(self, stats, stats_age): Line 4685: if self.isMigrating(): Line 4686: return Line 4687: if not self._monitorable:
Don't we have still a minor race here (I'm not sure, just asking)? May it h
I think such race exists, and since preventing it looks feasible, let's try to do so. Line 4688: return Line 4689: # we don't care about decimals here Line 4690: if stats_age < config.getint('vars', 'vm_command_timeout'): Line 4691: return
Martin Polednik has posted comments on this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Patch Set 4:
(1 comment)
Please consider the comment just as an unfortunate remark. :) Postponing rating for the race.
https://gerrit.ovirt.org/#/c/65727/4/tests/vmTests.py File tests/vmTests.py:
PS4, Line 1425: # accessing private field Sure! But I believe that saying this is equivalent to having '_' in front of attribute's name and the only reason for this comment is to avoid code review comments stating "accessing private field".
We shouldn't need those comments (that's unfortunately not saying we don't need them).
Francesco Romani has posted comments on this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Patch Set 4: Code-Review-1
After the changes made to address the race pointed out by Milan, this patche makes little sense on its own; will squash into 65590
gerrit-hooks has posted comments on this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Patch Set 4:
* update_tracker: OK
Francesco Romani has abandoned this change.
Change subject: vm: check stats timeout only for monitorable VMs ......................................................................
Abandoned
squashed into 65590
vdsm-patches@lists.fedorahosted.org
-
automation＠ovirt.org -
fromani＠redhat.com -
Jenkins CI RO -
Martin Polednik -
mzamazal＠redhat.com