Change in vdsm[master]: vm: Cleaner vm stats thread initialization

[Nir Soffer]

Nir Soffer has uploaded a new change for review.

Change subject: vm: Cleaner vm stats thread initialization
......................................................................

vm: Cleaner vm stats thread initialization

The vm stats thread was initialized too late, leading to unneeded checks
and unclear try except blocks when trying to stop the thread.

This patch changes AdvancedStatsThread so it keeps a thread instance
instead of inheriting from Thread, and initialize it in Vm.__init__, so
it is always available when we access it.

Change-Id: I2917de42b76ee3dc8b27bdc23b33f3c984a7cc68
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt;
---
M vdsm/virt/sampling.py
M vdsm/virt/vm.py
2 files changed, 11 insertions(+), 18 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/99/39299/1

diff --git a/vdsm/virt/sampling.py b/vdsm/virt/sampling.py
index 3206b97..848bb36 100644
--- a/vdsm/virt/sampling.py
+++ b/vdsm/virt/sampling.py
@@ -401,7 +401,7 @@
         return self._samples.last()
 
 
-class AdvancedStatsThread(threading.Thread):
+class AdvancedStatsThread(object):
     """
     A thread that runs the registered AdvancedStatsFunction objects
     for statistic and monitoring purpose.
@@ -412,9 +412,8 @@
         """
         Initialize an AdvancedStatsThread object
         """
-        threading.Thread.__init__(self)
         self.daemon = daemon
-
+        self._thread = None
         self._log = log
         self._stopEvent = threading.Event()
         self._contEvent = threading.Event()
@@ -426,7 +425,7 @@
         """
         Register the functions listed as arguments
         """
-        if self.isAlive():
+        if self._thread is not None:
             raise RuntimeError("AdvancedStatsThread is started")
 
         for statsFunction in args:
@@ -437,7 +436,9 @@
         Start the execution of the thread and exit
         """
         self._log.debug("Start statistics collection")
-        threading.Thread.start(self)
+        self._thread = threading.Thread(target=self._run)
+        self._thread.daemon = self.daemon
+        self._thread.start()
 
     def stop(self):
         """
@@ -464,7 +465,7 @@
     def getLastSampleTime(self):
         return self._statsTime
 
-    def run(self):
+    def _run(self):
         self._log.debug("Stats thread started")
         self._contEvent.set()
 
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 28054bf..1bc04e7 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -779,7 +779,7 @@
         self._initTimeRTC = long(self.conf.get('timeOffset', 0))
         self._guestEvent = vmstatus.POWERING_UP
         self._guestEventTime = 0
-        self._vmStats = None
+        self._vmStats = VmStatsThread(self)
         self._guestCpuRunning = False
         self._guestCpuLock = threading.Lock()
         self._startTime = time.time() - \
@@ -1269,7 +1269,7 @@
             return
         toSave = self.status()
         toSave['startTime'] = self._startTime
-        if self.lastStatus != vmstatus.DOWN and self._vmStats:
+        if self.lastStatus != vmstatus.DOWN:
             guestInfo = self.guestAgent.getGuestInfo()
             toSave['username'] = guestInfo['username']
             toSave['guestIPs'] = guestInfo['guestIPs']
@@ -1758,7 +1758,7 @@
 
         decStats = {}
         try:
-            if self._vmStats and self._vmStats.getLastSampleTime() is not None:
+            if self._vmStats.getLastSampleTime() is not None:
                 decStats = self._vmStats.get()
                 self._setUnresponsiveIfTimeout(
                     stats,
@@ -2001,19 +2001,11 @@
         return domxml.toxml()
 
     def startVmStats(self):
-        self._vmStats = VmStatsThread(self)
         self._vmStats.start()
         self._guestEventTime = self._startTime
 
     def stopVmStats(self):
-        # this is less clean that it could be, but we can get here from
-        # many flows and with various locks held
-        # (_releaseLock, _shutdownLock)
-        # _vmStats may be None already, and we're good with that.
-        try:
-            self._vmStats.stop()
-        except AttributeError:
-            pass
+        self._vmStats.stop()
 
     @staticmethod
     def _guestSockCleanup(sock):


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2917de42b76ee3dc8b27bdc23b33f3c984a7cc68
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;

automation(a)ovirt.org has posted comments on this change.

Change subject: vm: Cleaner vm stats thread initialization
......................................................................


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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2917de42b76ee3dc8b27bdc23b33f3c984a7cc68
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Cleaner vm stats thread initialization
......................................................................


Patch Set 1:

Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17268/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2917de42b76ee3dc8b27bdc23b33f3c984a7cc68
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Cleaner vm stats thread initialization
......................................................................


Patch Set 1:

Build Started (2/2) ->
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17442/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2917de42b76ee3dc8b27bdc23b33f3c984a7cc68
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Cleaner vm stats thread initialization
......................................................................


Patch Set 1:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17268/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17442/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2917de42b76ee3dc8b27bdc23b33f3c984a7cc68
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm: Cleaner vm stats thread initialization
......................................................................


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/39299/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1268:         if self._destroyed:
Line 1269:             return
Line 1270:         toSave = self.status()
Line 1271:         toSave['startTime'] = self._startTime
Line 1272:         if self.lastStatus != vmstatus.DOWN:
It was not clear why we check self._vmStats here do we want to know if we started it?
Line 1273:             guestInfo = self.guestAgent.getGuestInfo()
Line 1274:             toSave['username'] = guestInfo['username']
Line 1275:             toSave['guestIPs'] = guestInfo['guestIPs']
Line 1276:             toSave['guestFQDN'] = guestInfo['guestFQDN']


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2917de42b76ee3dc8b27bdc23b33f3c984a7cc68
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm: Cleaner vm stats thread initialization
......................................................................


Patch Set 1:

ping

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2917de42b76ee3dc8b27bdc23b33f3c984a7cc68
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Jenkins CI RO]

Jenkins CI RO has abandoned this change.

Change subject: vm: Cleaner vm stats thread initialization
......................................................................


Abandoned

Abandoned due to no activity - please restore if still relevant

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I2917de42b76ee3dc8b27bdc23b33f3c984a7cc68
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Jenkins CI RO
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;

gerrit-hooks has posted comments on this change.

Change subject: vm: Cleaner vm stats thread initialization
......................................................................


Patch Set 1:

* Update tracker: IGNORE, no Bug-Url found

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2917de42b76ee3dc8b27bdc23b33f3c984a7cc68
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Jenkins CI RO
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No