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

[e-mail] 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'])

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

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

[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

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 1:

ping

[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

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