Change in vdsm[master]: sampling.ImagePathStatus: drop unused code, and only it

[Dan Kenigsberg]

Dan Kenigsberg has uploaded a new change for review.

Change subject: sampling.ImagePathStatus: drop unused code, and only it
......................................................................

sampling.ImagePathStatus: drop unused code, and only it

The ImagePathStatus is never started, but one of its methods is used by
HostStatsThread.get(). This patch moves the still-used bits of
ImagePathStatus code into HostStatsThread.

Change-Id: I530b6d83c6ab5d0cc52df797ae3e40998fb77435
Signed-off-by: Dan Kenigsberg 
---
M lib/vdsm/config.py.in
M vdsm/sampling.py
2 files changed, 20 insertions(+), 50 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/38/21038/1

diff --git a/lib/vdsm/config.py.in b/lib/vdsm/config.py.in
index c0bdd53..c8ce7e6 100644
--- a/lib/vdsm/config.py.in
+++ b/lib/vdsm/config.py.in
@@ -194,9 +194,6 @@

('irsd', '%(images)s/irsd', None),

-        ('images_check_times', '0',
-            'Image repository check period (seconds).'),
-
         ('volume_utilization_percent', '50', None),

('volume_utilization_chunk_mb', '1024', None),
diff --git a/vdsm/sampling.py b/vdsm/sampling.py
index 09e3dd4..04593de 100644
--- a/vdsm/sampling.py
+++ b/vdsm/sampling.py
@@ -36,7 +36,6 @@
 from vdsm import utils
 from vdsm import netinfo
 from vdsm.constants import P_VDSM_RUN
-from vdsm.config import config

_THP_STATE_PATH = '/sys/kernel/mm/transparent_hugepage/enabled'
 if not os.path.exists(_THP_STATE_PATH):
@@ -377,12 +376,11 @@
         self._lineRate = (sum(self._ifrates) or 1000) * (10 ** 6) / 8
         self._lastSampleTime = time.time()

-        self._imagesStatus = ImagePathStatus(cif)
+        self._cif = cif
         self._pid = os.getpid()
         self._ncpus = max(os.sysconf('SC_NPROCESSORS_ONLN'), 1)

def stop(self):
-        self._imagesStatus.stop()
         self._stopEvent.set()

def _updateIfidsIfrates(self):
@@ -418,19 +416,9 @@
     def get(self):
         stats = self._getInterfacesStats()
         stats['cpuSysVdsmd'] = stats['cpuUserVdsmd'] = 0.0
-        stats['storageDomains'] = {}
-        if self._imagesStatus._cif.irs:
-            self._imagesStatus._refreshStorageDomains()
+        stats['storageDomains'] = self._getStorageDomainsStats()
+
         now = time.time()
-        for sd, d in self._imagesStatus.storageDomains.iteritems():
-            stats['storageDomains'][sd] = {
-                'code': d['code'],
-                'delay': d['delay'],
-                'lastCheck': d['lastCheck'],
-                'valid': d['valid'],
-                'version': d['version'],
-                'acquired': d['acquired'],
-            }
         stats['elapsedTime'] = int(now - self.startTime)
         if len(self._samples) < 2:
             return stats
@@ -519,41 +507,26 @@
             stats['txRate'] = min(stats['txRate'], 100.0)
             stats['rxRate'] = min(stats['rxRate'], 100.0)
             logging.debug(stats)
-#        stats['rxBps'] = float(rx) / interval
-#        stats['txBps'] = float(tx) / interval
         stats['rxDropped'] = rxDropped
         stats['txDropped'] = txDropped

return stats

+    def _getStorageDomainsStats(self):
+        sds = {}
+        if self._cif.irs:
+            res = self._cif.irs.repoStats()
+            del res["status"]
+            if "args" in res:
+                del res["args"]

-class ImagePathStatus(threading.Thread):
-    def __init__(self, cif, interval=None):
-        if interval is None:
-            interval = config.getint('irs', 'images_check_times')
-        self._interval = interval
-        self._cif = cif
-        self.storageDomains = {}
-        self._stopEvent = threading.Event()
-        threading.Thread.__init__(self, name='ImagePathStatus')
-        if self._interval > 0:
-            self.start()
-
-    def stop(self):
-        self._stopEvent.set()
-
-    def _refreshStorageDomains(self):
-        self.storageDomains = self._cif.irs.repoStats()
-        del self.storageDomains["status"]
-        if "args" in self.storageDomains:
-            del self.storageDomains["args"]
-
-    def run(self):
-        try:
-            while not self._stopEvent.isSet():
-                if self._cif.irs:
-                    self._refreshStorageDomains()
-                self._stopEvent.wait(self._interval)
-        except:
-            logging.error("Error while refreshing storage domains",
-                          exc_info=True)
+            for sd, d in res.iteritems():
+                sds[sd] = {
+                    'code': d['code'],
+                    'delay': d['delay'],
+                    'lastCheck': d['lastCheck'],
+                    'valid': d['valid'],
+                    'version': d['version'],
+                    'acquired': d['acquired'],
+                }
+        return sds

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: sampling.ImagePathStatus: drop unused code, and only it
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4476/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5276/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5354/ : SUCCESS

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: sampling.ImagePathStatus: drop unused code, and only it
......................................................................

Patch Set 1:

So storageDomains key that was removed in http://gerrit.ovirt.org/#/c/20812 is still used by engine?

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: sampling.ImagePathStatus: drop unused code, and only it
......................................................................

Patch Set 1:

I am afraid so, Nir. Federico had to revert my patch, and now I am restoring much of it.

Petr Šebek has posted comments on this change.

Change subject: sampling.ImagePathStatus: drop unused code, and only it
......................................................................

Patch Set 1: Verified+1 Code-Review+1

I applied given patch to my host. In webadmin host smoothly went from maintenance to up state. I added VM to this host and some storage. When I ran

vdsClient -s 0 getVdsStats

there is correct content in storageDomains:

storageDomains = {'3cc3dc79-c40f-49a7-aaa7-33b8158b14a7': {'acquired': True,
                                                           'code': 0,
                                                           'delay': '0.000347731',
                                                           'lastCheck': '8.8',
                                                           'valid': True,
                                                           'version': 3}}

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: sampling.ImagePathStatus: drop unused code, and only it
......................................................................

Patch Set 1:

(4 comments)

Looks ok, can be cleaned up a bit.

....................................................
File vdsm/sampling.py
Line 363:     """
Line 364:     AVERAGING_WINDOW = 5
Line 365:     SAMPLE_INTERVAL_SEC = 2
Line 366: 
Line 367:     def __init__(self, cif, log):
I would accept irs instead of cif, since we do not use cif here - why depend on that class having an irs where we can get that irs directly?

Do we expect that cif.irs can become None while the application is running?
Line 368:         self.startTime = time.time()
Line 369: 
Line 370:         threading.Thread.__init__(self)
Line 371:         self._log = log

Line 416:     def get(self):
Line 417:         stats = self._getInterfacesStats()
Line 418:         stats['cpuSysVdsmd'] = stats['cpuUserVdsmd'] = 0.0
Line 419:         stats['storageDomains'] = self._getStorageDomainsStats()
Line 420: 
Is this extra line intended?
Line 421:         now = time.time()
Line 422:         stats['elapsedTime'] = int(now - self.startTime)
Line 423:         if len(self._samples) < 2:
Line 424:             return stats

Line 417:         stats = self._getInterfacesStats()
Line 418:         stats['cpuSysVdsmd'] = stats['cpuUserVdsmd'] = 0.0
Line 419:         stats['storageDomains'] = self._getStorageDomainsStats()
Line 420: 
Line 421:         now = time.time()
This temporary variable is pointless - I would inline it into line 422:

stats['elapsedTime'] = int(time.time() - self.startTime)

I think that this variable lead to the strange code in the previous version, where it seems like the code is trying to time the iteration over the dictionary, while it is not.
Line 422:         stats['elapsedTime'] = int(now - self.startTime)
Line 423:         if len(self._samples) < 2:
Line 424:             return stats
Line 425:         hs0, hs1 = self._samples[0], self._samples[-1]

Line 512: 
Line 513:         return stats
Line 514: 
Line 515:     def _getStorageDomainsStats(self):
Line 516:         sds = {}
This name "sds"  is little confusing together with "sd" and "d". i would rename it to "stats" like in _getInterfacesStats(). The fact that this is "storage domain stats" is not important since there are no other types of stats in this scope.
Line 517:         if self._cif.irs:
Line 518:             res = self._cif.irs.repoStats()
Line 519:             del res["status"]
Line 520:             if "args" in res:

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: sampling.ImagePathStatus: drop unused code, and only it
......................................................................

Patch Set 1:

(2 comments)

....................................................
File vdsm/sampling.py
Line 363:     """
Line 364:     AVERAGING_WINDOW = 5
Line 365:     SAMPLE_INTERVAL_SEC = 2
Line 366: 
Line 367:     def __init__(self, cif, log):
cif.irs is expected to transition from None to a real instance after the storage subsystem initializes itself (which can take a while).
Line 368:         self.startTime = time.time()
Line 369: 
Line 370:         threading.Thread.__init__(self)
Line 371:         self._log = log

Line 416:     def get(self):
Line 417:         stats = self._getInterfacesStats()
Line 418:         stats['cpuSysVdsmd'] = stats['cpuUserVdsmd'] = 0.0
Line 419:         stats['storageDomains'] = self._getStorageDomainsStats()
Line 420: 
it was, but I cannot defend its staying here.
Line 421:         now = time.time()
Line 422:         stats['elapsedTime'] = int(now - self.startTime)
Line 423:         if len(self._samples) < 2:
Line 424:             return stats

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: sampling.ImagePathStatus: drop unused code, and only it
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4662/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5462/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5541/ : SUCCESS

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: sampling.ImagePathStatus: drop unused code, and only it
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

For completeness, it would nice if the commit message say that it remove the thread.

....................................................
Commit Message
Line 7: sampling.ImagePathStatus: drop unused code, and only it
Line 8: 
Line 9: The ImagePathStatus is never started, but one of its methods is used by
Line 10: HostStatsThread.get(). This patch moves the still-used bits of
Line 11: ImagePathStatus code into HostStatsThread.
This patch moves the still-used bits of ImagePathStatus code into HostStatsThread and remove the unused ImagePathStatus thread.
Line 12: 
Line 13: Change-Id: I530b6d83c6ab5d0cc52df797ae3e40998fb77435

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: sampling.ImagePathStatus: drop unused code, and only it
......................................................................

Patch Set 3: Verified+1 Code-Review+2

Copying score: code has not changed since Nir's review, and practically unchanged since Petr's review and verification.

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: sampling.ImagePathStatus: drop unused code, and only it
......................................................................

sampling.ImagePathStatus: drop unused code, and only it

The ImagePathStatus is never started, but one of its methods is used by
HostStatsThread.get(). This patch moves the still-used bits of
ImagePathStatus code into HostStatsThread, and drops the unused
ImagePathStatus thread.

Change-Id: I530b6d83c6ab5d0cc52df797ae3e40998fb77435
Signed-off-by: Dan Kenigsberg 
Reviewed-on: http://gerrit.ovirt.org/21038
---
M lib/vdsm/config.py.in
M vdsm/sampling.py
2 files changed, 20 insertions(+), 52 deletions(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: sampling.ImagePathStatus: drop unused code, and only it
......................................................................

Patch Set 3:

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4671/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5471/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5550/ : FAILURE