Change in vdsm[master]: vm: check stats timeout only for monitorable VMs

[fromani]

Francesco Romani has abandoned this change.

Change subject: vm: check stats timeout only for monitorable VMs
..


Abandoned

squashed into 65590

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: vm: check stats timeout only for monitorable VMs

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check stats timeout only for monitorable VMs
..


Patch Set 4:

* update_tracker: OK

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: vm: check stats timeout only for monitorable VMs

[fromani]

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: vm: check stats timeout only for monitorable VMs

[mpolednik]

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


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
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]: vm: check stats timeout only for monitorable VMs

[fromani]

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


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
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]: vm: check stats timeout only for monitorable VMs

[automation]

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: vm: check stats timeout only for monitorable VMs

[mzamazal]

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


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
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]: vm: check stats timeout only for monitorable VMs

[fromani]

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


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
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]: vm: check stats timeout only for monitorable VMs

[automation]

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: vm: check stats timeout only for monitorable VMs

[Jenkins CI]

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: vm: check stats timeout only for monitorable VMs

[Jenkins CI]

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: vm: check stats timeout only for monitorable VMs

[fromani]

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.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: vm: check stats timeout only for monitorable VMs

[automation]

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
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]: vm: check stats timeout only for monitorable VMs

[automation]

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
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]: vm: check stats timeout only for monitorable VMs

[fromani]

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


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia60b98b5758adf9dddea8430db2378411372a600
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org