oVirt Jenkins CI Server has posted comments on this change.
Change subject: This commit adds a man page for the vdsm-tool command line utility.
......................................................................
Patch Set 3:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/687/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged/1391/ : SUCCESS
--
To view, visit http://gerrit.ovirt.org/28196
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb3e4ab7947a151a30ccc1c1a41e8df9d02d1d65
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Andrew Dahms <andrewjdahms(a)gmail.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Nir Soffer has posted comments on this change.
Change subject: json-rpc: Protocol detection
......................................................................
Patch Set 33:
(6 comments)
protocol detection and xmlrpc looks fine, expect two issues:
- This patch exposes a bug we had in the non-secure xmlprc version, where http 1.1 is not used. This causes a regression in the secure xmlrpc version. Trivial fix, but requires some testing with ssl=false.
- None return value from SSLSocket.recv is not handled - trivial fix.
http://gerrit.ovirt.org/#/c/26300/33/vdsm/BindingXMLRPC.py
File vdsm/BindingXMLRPC.py:
Line 90: HTTP_HEADER_FLOWID = "FlowID"
Line 91:
Line 92: threadLocal = self.cif.threadLocal
Line 93:
Line 94: basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler
This change expose a bug in the non-secure xmlrpc version. SecureXMLRPCServer.SecureXMLRPCRequestHandler used before this patch is actually utils.IPXMLRPCRequestHandler, which is modified to support HTTP/1.1 since ovirt-3.4.
So the correct code here would to remove the basehandler variable and replace its usage with utils.IPXMLRPCRequestHandler.
This requires testing vdsm with ssl=false in vdsm.conf.
Line 95:
Line 96: class RequestHandler(basehandler):
Line 97:
Line 98: # Timeout for the request socket
Line 92: threadLocal = self.cif.threadLocal
Line 93:
Line 94: basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler
Line 95:
Line 96: class RequestHandler(basehandler):
basehandler -> utils.IPXMLRPCRequestHandler
Line 97:
Line 98: # Timeout for the request socket
Line 99: timeout = 60
Line 100: log = logging.getLogger("BindingXMLRPC.RequestHandler")
Line 115:
Line 116: def setup(self):
Line 117: threadLocal.client = self.client_address[0]
Line 118: threadLocal.server = self.request.getsockname()[0]
Line 119: return basehandler.setup(self)
basehandler -> utils.IPXMLRPCRequestHandler
Line 120:
Line 121: def do_GET(self):
Line 122: try:
Line 123: length = self._getIntHeader(self.HEADER_SIZE,
Line 258: self.end_headers()
Line 259: self.wfile.write(json_response)
Line 260:
Line 261: def parse_request(self):
Line 262: r = basehandler.parse_request(self)
basehandler -> utils.IPXMLRPCRequestHandler
Line 263: threadLocal.flowID = self.headers.get(HTTP_HEADER_FLOWID)
Line 264: return r
Line 265:
Line 266: def finish(self):
Line 263: threadLocal.flowID = self.headers.get(HTTP_HEADER_FLOWID)
Line 264: return r
Line 265:
Line 266: def finish(self):
Line 267: basehandler.finish(self)
basehandler -> utils.IPXMLRPCRequestHandler
Line 268: threadLocal.client = None
Line 269: threadLocal.server = None
Line 270: threadLocal.flowID = None
Line 271:
http://gerrit.ovirt.org/#/c/26300/33/vdsm/protocolDetector.py
File vdsm/protocolDetector.py:
Line 178: self.log.warning("Unable to read data")
Line 179: self._remove_connection(client_socket)
Line 180: client_socket.close()
Line 181: return
Line 182:
Looking in the betterAsyncore.py, we should handle here the case where data is None. This happens when using M2Crypto, if the SSLSocket want to read more data from the other side before it can return data.
I would add this for now:
if data is None:
# SSLSocket wants to read more data from the peer
return
Line 183: self._remove_connection(client_socket)
Line 184: try:
Line 185: handler = self.detect_protocol(data)
Line 186: except _CannotDetectProtocol:
--
To view, visit http://gerrit.ovirt.org/26300
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id739a40e2b37dcc175137ec91cd5ec166ad24a75
Gerrit-PatchSet: 33
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Barak Azulay <bazulay(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Liron Ar <laravot(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Dan Kenigsberg has submitted this change and it was merged.
Change subject: This commit adds a man page for the vdsm-tool command line utility.
......................................................................
This commit adds a man page for the vdsm-tool command line utility.
This commit adds the file vdsm-tool.1.in to the vdsm-tool directory,
updates the vdsm-tool make file and the vdsm spec file to include and
install the new man page for vdsm-tool to the /usr/share/man/man1 directory.
Change-Id: Ieb3e4ab7947a151a30ccc1c1a41e8df9d02d1d65
Signed-off-by: Andrew Dahms <andrewjdahms(a)gmail.com>
Reviewed-on: http://gerrit.ovirt.org/28196
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com>
Tested-by: Dan Kenigsberg <danken(a)redhat.com>
---
M vdsm-tool/Makefile.am
A vdsm-tool/vdsm-tool.1.in
M vdsm.spec.in
3 files changed, 198 insertions(+), 0 deletions(-)
Approvals:
Dan Kenigsberg: Verified; Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/28196
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ieb3e4ab7947a151a30ccc1c1a41e8df9d02d1d65
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Andrew Dahms <andrewjdahms(a)gmail.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: This commit adds a man page for the vdsm-tool command line utility.
......................................................................
Patch Set 2: Verified+1 Code-Review+2
Copying score, only a minor text change.
--
To view, visit http://gerrit.ovirt.org/28196
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb3e4ab7947a151a30ccc1c1a41e8df9d02d1d65
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Andrew Dahms <andrewjdahms(a)gmail.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Nir Soffer has uploaded a new change for review.
Change subject: misc: Faster version of readspeed
......................................................................
misc: Faster version of readspeed
After optimizing list2cmdline, we can see that the profile is dominated
by utils.execCmd(), used to invoke dd for checking reading speed. This
code is quite complex, using fancy classes such as AsyncProc and
StreamWrapper. These are useful when you read from multiple streams to
avoid deadlocks. However for checking reading speed, this seems like an
overkill; we want dd to read a block from storage and read 3 lines of
statistics from stderr. We don't care about the data read by dd, and
have no reason to read this data from dd stdout.
This patch uses cpopen.Popen directly, running dd reading from a path
and writing to /dev/null, and use Popen.communicate to read dd stderr.
Since we are communicating with dd using a single pipe, a deadlock is
not possible.
I tested this patch on two identical machines connected to 32 storage
domains. Each one run 5 minutes as spm, and 5 minutes as hsm during 10
minutes profiling session. The first system was running the previous
patch in this branch:
Sun May 11 02:23:31 2014 vdsmd-list2cmdline.prof
8758092 function calls (8816678 primitive calls) in 81.525 CPU seconds
Ordered by: internal time
List reduced from 1872 to 20 due to restriction <20>
ncalls tottime percall cumtime percall filename:lineno(function)
101/136 6.554 0.065 46.116 0.339 threading.py:481(Thread.run)
1352629 3.652 0.000 6.595 0.000 utils.py:557(AsyncProc._processStreams)
134289 2.296 0.000 2.296 0.000 pthread.py:98(Lock.unlock)
9764 2.282 0.000 3.617 0.000 __init__.py:226(LogRecord.__init__)
25631 2.202 0.000 10.448 0.000 utils.py:466(_streamWrapper._readNonBlock)
116992 2.145 0.000 2.157 0.000 pthread.py:95(Lock.lock)
6049 2.031 0.000 6.722 0.001 misc.py:125(findCaller)
18524 1.954 0.000 22.039 0.001 io.py:492(BufferedReader.readline)
58408 1.788 0.000 1.882 0.000 genericpath.py:85(_splitext)
2521 1.631 0.001 8.974 0.004 utils.py:623(AsyncProc.wait)
Ordered by: cumulative time
List reduced from 1872 to 20 due to restriction <20>
ncalls tottime percall cumtime percall filename:lineno(function)
33/36 0.001 0.000 69.017 1.917 utils.py:770(wrapper)
31/32 0.072 0.002 68.740 2.148 domainMonitor.py:163(DomainMonitorThread._monitorLoop)
1072/2105 0.720 0.001 68.122 0.032 domainMonitor.py:186(DomainMonitorThread._monitorDomain)
2519 0.494 0.000 56.573 0.022 utils.py:666(execCmd)
2041/2105 0.110 0.000 46.814 0.022 blockSD.py:594(BlockStorageDomain.getReadDelay)
2105 0.139 0.000 46.411 0.022 misc.py:222(readspeed)
2105 0.314 0.000 46.241 0.022 misc.py:192(_readfile)
101/136 6.554 0.065 46.116 0.339 threading.py:481(Thread.run)
2516 0.133 0.000 34.884 0.014 utils.py:633(AsyncProc.communicate)
18520/18524 0.255 0.000 22.294 0.001 io.py:532(BufferedReader.next)
The seconds system was running this branch:
Sun May 11 02:23:38 2014 vdsmd-optimize.prof
2113774 function calls (2168006 primitive calls) in 43.489 CPU seconds
Ordered by: internal time
List reduced from 1873 to 20 due to restriction <20>
ncalls tottime percall cumtime percall filename:lineno(function)
111/136 8.270 0.075 29.113 0.214 threading.py:481(Thread.run)
41106 0.912 0.000 0.912 0.000 pthread.py:95(Lock.lock)
2146 0.898 0.000 1.412 0.001 subprocess.py:1344(Popen._communicate_with_poll)
27318 0.866 0.000 1.873 0.000 utils.py:557(AsyncProc._processStreams)
1867 0.810 0.000 2.509 0.001 misc.py:126(findCaller)
43735 0.791 0.000 0.791 0.000 pthread.py:98(Lock.unlock)
8112 0.719 0.000 5.590 0.001 __init__.py:1204(Logger.callHandlers)
7326/7338 0.692 0.000 0.741 0.000 pthread.py:133(Cond.timedwait)
1553/2145 0.648 0.000 34.541 0.016 domainMonitor.py:186(DomainMonitorThread._monitorDomain)
Ordered by: cumulative time
List reduced from 1873 to 20 due to restriction <20>
ncalls tottime percall cumtime percall filename:lineno(function)
34/36 0.001 0.000 35.231 0.979 utils.py:770(wrapper)
32 0.075 0.002 35.099 1.097 domainMonitor.py:163(DomainMonitorThread._monitorLoop)
1553/2145 0.648 0.000 34.541 0.016 domainMonitor.py:186(DomainMonitorThread._monitorDomain)
111/136 8.270 0.075 29.113 0.214 threading.py:481(Thread.run)
13501 0.179 0.000 17.309 0.001 sdc.py:51(DomainProxy.getRealDomain)
13501 0.381 0.000 17.129 0.001 sdc.py:101(StorageDomainCache._realProduce)
97 0.009 0.000 15.431 0.159 sdc.py:133(StorageDomainCache._findDomain)
97 0.007 0.000 15.112 0.156 blockSD.py:1359(findDomain)
91 0.007 0.000 14.304 0.157 sdc.py:148(StorageDomainCache._findUnfetchedDomain)
97 0.042 0.000 14.060 0.145 blockSD.py:400(BlockStorageDomain.__init__)
...
2145 0.344 0.000 8.410 0.004 misc.py:223(readspeed)
Comparing the profiles show:
- Number of function calls dropped from 8758092 to 2113774 (75%)
- Number of cpu seconds dropped from 81 to 43 (46%)
- Time spent in _monitorDomain() dropped from 68 to 34 seconds (%50)
- Time spent in readspeed() dropped from 46.4 to 8.4 seconds (81%)
Change-Id: I7ecf1f27d8434aeae672e92ec7adb12e52e419a9
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
---
M vdsm/storage/misc.py
1 file changed, 20 insertions(+), 7 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/53/27553/1
diff --git a/vdsm/storage/misc.py b/vdsm/storage/misc.py
index 12d4d97..8520c37 100644
--- a/vdsm/storage/misc.py
+++ b/vdsm/storage/misc.py
@@ -32,6 +32,7 @@
from functools import wraps, partial
from itertools import chain, imap
import contextlib
+import cpopen
import errno
import glob
import logging
@@ -219,26 +220,38 @@
)
-def readspeed(path, buffersize=None):
+def readspeed(path, buffersize):
"""
Measures the amount of bytes transferred and the time elapsed
reading the content of the file/device
"""
- rc, _, err = _readfile(path, buffersize)
- if rc != 0:
- log.error("Unable to read file '%s'", path)
- raise se.MiscFileReadException(path)
+ # Read data from file to /dev/null, as we are not interested in the data
+ # itself, only in the read statistics provided by dd on stderr.
+ cmd = [constants.EXT_DD, "if=%s" % path, "iflag=%s" % DIRECTFLAG,
+ "of=/dev/null", "bs=%d" % buffersize, "count=1"]
+
+ log.debug("Running %s", utils.list2cmdline(cmd))
+ p = cpopen.CPopen(cmd, close_fds=True)
+ try:
+ out, err = p.communicate()
+ if p.returncode:
+ log.error("Unable to read file '%s'", path)
+ raise se.MiscFileReadException(path)
+ finally:
+ if p.returncode is None:
+ p.kill()
try:
# The statistics are located in the last line:
- m = _readspeed_regex.match(err[-1])
+ statsLine = err.splitlines()[-1]
+ m = _readspeed_regex.match(statsLine)
except IndexError:
log.error("Unable to find dd statistics to parse")
raise se.MiscFileReadException(path)
if m is None:
- log.error("Unable to parse dd output: '%s'", err[-1])
+ log.error("Unable to parse dd output: '%s'", statsLine)
raise se.MiscFileReadException(path)
return {
--
To view, visit http://gerrit.ovirt.org/27553
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7ecf1f27d8434aeae672e92ec7adb12e52e419a9
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Dan Kenigsberg has posted comments on this change.
Change subject: This commit adds a man page for the vdsm-tool command line utility.
......................................................................
Patch Set 1: Code-Review-1
(1 comment)
http://gerrit.ovirt.org/#/c/28196/1/vdsm-tool/vdsm-tool.1.in
File vdsm-tool/vdsm-tool.1.in:
Line 186: <Author list>
Line 187: .SH BUGS
Line 188: Report bugs to <http://bugzilla.redhat.com>
Line 189: .SH COPYRIGHT
Line 190: Copyright 2011 Red Hat, Inc. License GPLv2: GNU GPL Version 2 <http://gnu.org/licenses/gpl.html>.
> shouldn't be 2014?
that's the copyright of the code (not of the docs) so the years should be 2011-2014, I suppose.
--
To view, visit http://gerrit.ovirt.org/28196
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb3e4ab7947a151a30ccc1c1a41e8df9d02d1d65
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Andrew Dahms <andrewjdahms(a)gmail.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Nir Soffer has posted comments on this change.
Change subject: json-rpc: Protocol detection
......................................................................
Patch Set 32:
(1 comment)
http://gerrit.ovirt.org/#/c/26300/32/lib/yajsonrpc/__init__.py
File lib/yajsonrpc/__init__.py:
Line 488: res = True if res is None else res
Line 489:
Line 490: ctx.requestDone(JsonRpcResponse(res, None, req.id))
Line 491:
Line 492: @traceback(on=log.name)
> Nir suggested to use it.
I was suggesting this on the protocol detector, not here, because I did not review this file.
This decorator makes code simpler because you don't need to add try blocks around the body of threads target function, and it will not break when your error handling code is broken.
This does not replace proper error handling in the thread, but a last resort error handler, ensuring that when the thread dies in the field (because error handling was broken), it will be logged.
So instead of:
def mainloop(self):
try:
do stuff
and other stuff
even more
except Exception:
logging.exception("thread foo died")
You can do:
@traceback(on=log.name, msg="thread foo died")
def mainloop(self):
do stuff
and other stuff
even more
Looking in function body, there is no error handling in the while loop, so any unexpected failure in one of statements will cause the thread to abort silently, which makes it impossible to support this code in the field.
Line 493: def serve_requests(self):
Line 494: while True:
Line 495: self.log.debug("Waiting for request")
Line 496: obj = self._workQueue.get()
--
To view, visit http://gerrit.ovirt.org/26300
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id739a40e2b37dcc175137ec91cd5ec166ad24a75
Gerrit-PatchSet: 32
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Barak Azulay <bazulay(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Liron Ar <laravot(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes