Change in vdsm[master]: introducing downloadFromStream verb
nsoffer at redhat.com
nsoffer at redhat.com
Mon Jan 20 15:34:25 UTC 2014
Nir Soffer has posted comments on this change.
Change subject: introducing downloadFromStream verb
......................................................................
Patch Set 4:
(17 comments)
http://gerrit.ovirt.org/#/c/23281/4//COMMIT_MSG
Commit Message:
Line 6:
Line 7: introducing downloadFromStream verb
Line 8:
Line 9: This patch introduces the downloadFromStream verb in vdsm which
Line 10: allows to upload using streaming content to vdsm images.
"upload using streaming content" is not clear, and does not explain that you can upload data without size limit.
Line 11: XML-RPC spec doesn't support streaming, as we don't want to use HTTP
Line 12: server in addition to the current XML-RPC server (to not use another
Line 13: port) we will threat some requests as http requests instead of xml-rpc
Line 14: requests.
Line 7: introducing downloadFromStream verb
Line 8:
Line 9: This patch introduces the downloadFromStream verb in vdsm which
Line 10: allows to upload using streaming content to vdsm images.
Line 11: XML-RPC spec doesn't support streaming, as we don't want to use HTTP
Again "XML-RPC spec doesn't support streaming" is not clear, and does not explain that xml-rpc has limited payload size, so it cannot be used to upload big files.
Line 12: server in addition to the current XML-RPC server (to not use another
Line 13: port) we will threat some requests as http requests instead of xml-rpc
Line 14: requests.
Line 15:
Line 9: This patch introduces the downloadFromStream verb in vdsm which
Line 10: allows to upload using streaming content to vdsm images.
Line 11: XML-RPC spec doesn't support streaming, as we don't want to use HTTP
Line 12: server in addition to the current XML-RPC server (to not use another
Line 13: port) we will threat some requests as http requests instead of xml-rpc
Replace "threat" with "treat"
Line 14: requests.
Line 15:
Line 16: General upload information:
Line 17: ---------------------------------------------------------------
Line 16: General upload information:
Line 17: ---------------------------------------------------------------
Line 18: PUT requests arriving to the server with content type of
Line 19: application/octet-stream to default paths that we use
Line 20: today for request handling ('/', '/RPC2') will be threated as
Replace "threated" with "treated"
Line 21: upload requests.
Line 22:
Line 23: The PUT stream upload request inspected headers are:
Line 24: -- Mandatory headers:
Line 19: application/octet-stream to default paths that we use
Line 20: today for request handling ('/', '/RPC2') will be threated as
Line 21: upload requests.
Line 22:
Line 23: The PUT stream upload request inspected headers are:
The PUT request inspected headers are:
Line 24: -- Mandatory headers:
Line 25: *content-length
Line 26:
Line 27: -- Mandatory custom headers:
http://gerrit.ovirt.org/#/c/23281/4/vdsm/API.py
File vdsm/API.py:
Line 837: def download(self, methodArgs, volUUID=None):
Line 838: return self._irs.downloadImage(
Line 839: methodArgs, self._spUUID, self._sdUUID, self._UUID, volUUID)
Line 840:
Line 841: def downloadFromStream(self, methodArgs, volUUID=None):
This name is confusing:
- download: download data from other machine to an image
- upload upload data from an image to other machine
- xxx: write data from stream to an image
Seems that the most correct verb regarding the Image class is "write" or "dump"
Line 842: return self._irs.downloadImageFromStream(
Line 843: methodArgs, self._spUUID, self._sdUUID, self._UUID, volUUID)
Line 844:
Line 845:
http://gerrit.ovirt.org/#/c/23281/4/vdsm/BindingXMLRPC.py
File vdsm/BindingXMLRPC.py:
Line 27: from vdsm import SecureXMLRPCServer
Line 28: import logging
Line 29: import libvirt
Line 30: import threading
Line 31: import socket
Note for later patch: separate vdsm and Python imports and sort imports.
Line 32:
Line 33: from vdsm import constants
Line 34: from vdsm import utils
Line 35: from vdsm.define import doneCode, errCode
Line 43: _glusterEnabled = False
Line 44:
Line 45:
Line 46: class BindingXMLRPC(object):
Line 47:
Not related but lets keep this.
Line 48: def __init__(self, cif, log, ip, port, ssl, vds_resp_timeout,
Line 49: trust_store_path, default_bridge):
Line 50: self.cif = cif
Line 51: self.log = log
Line 132: basehandler = SecureXMLRPCServer.SecureXMLRPCRequestHandler
Line 133: else:
Line 134: basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler
Line 135:
Line 136: class RequestHandler(LoggingMixIn, basehandler):
Empty line bellow the class would be nice.
Line 137: # timeout for the request socket
Line 138: timeout = 60
Line 139: log = logging.getLogger("BindingXMLRPC.RequestHandler")
Line 140:
Line 134: basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler
Line 135:
Line 136: class RequestHandler(LoggingMixIn, basehandler):
Line 137: # timeout for the request socket
Line 138: timeout = 60
Check that this timeout is good for xmlrpc requests.
Line 139: log = logging.getLogger("BindingXMLRPC.RequestHandler")
Line 140:
Line 141: HEADER_POOL = 'X-Storage-Pool-Id'
Line 142: HEADER_DOMAIN = 'X-Storage-Domain-Id'
Line 151:
Line 152: def do_PUT(self):
Line 153: try:
Line 154: ctype, pt = cgi.parse_header(self.headers.getheader(self.
Line 155: HEADER_CONTENT_TYPE))
Indenting the next line after break at the first "(" does not look good in this case, and *is not* required by pep8. In such case you can indent like this (verified with pep8):
ctype, pt = cgi.parse_header(self.headers.getheader(
self.HEADER_CONTENT_TYPE))
Or you can use multiple lines - usually easier to read:
ct = self.headers.getheader(self.HEADER_CONTENT_TYPE)
ctype, pt = cgi.parse_header(ct)
Line 156: if ctype != 'application/octet-stream':
Line 157: self.send_error(httplib.INTERNAL_SERVER_ERROR,
Line 158: "unsupported content type: %r" % ctype)
Line 159: return
Line 153: try:
Line 154: ctype, pt = cgi.parse_header(self.headers.getheader(self.
Line 155: HEADER_CONTENT_TYPE))
Line 156: if ctype != 'application/octet-stream':
Line 157: self.send_error(httplib.INTERNAL_SERVER_ERROR,
This is a user error (4XX), not a server error (5XX). I think you use http.UNSUPPORTED_MEDIA_TYPE in a previous patch set.
Line 158: "unsupported content type: %r" % ctype)
Line 159: return
Line 160:
Line 161: contentLength = self.headers. \
Line 158: "unsupported content type: %r" % ctype)
Line 159: return
Line 160:
Line 161: contentLength = self.headers. \
Line 162: getheader(self.HEADER_CONTENT_LENGTH)
Breaking lines using '\' is considered bad style. pep8 wrongly let you do this.
You can do this (verified with pep8):
contentLength = self.headers.getheader(
self.HEADER_CONTENT_LENGTH)
Line 163: if not contentLength:
Line 164: self.send_error(httplib.LENGTH_REQUIRED,
Line 165: "missing content length")
Line 166: return
Line 194: self.wfile.write(json_response)
Line 195: self.finish()
Line 196:
Line 197: except socket.timeout:
Line 198: self.send_error(httplib.INTERNAL_SERVER_ERROR)
This is also not a server error - not sure what the proper http status code for this.
And there is a missing "message" argument here, which will fail in runtime with TypeError.
Line 199:
Line 200: except Exception:
Line 201: self.send_error(httplib.INTERNAL_SERVER_ERROR,
Line 202: "error during execution", exc_info=True)
http://gerrit.ovirt.org/#/c/23281/4/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:
Line 1675: methodArgs, sdUUID, imgUUID, volUUID)
Line 1676:
Line 1677: @public
Line 1678: def downloadImageFromStream(self, methodArgs, spUUID, sdUUID, imgUUID,
Line 1679: volUUID=None):
Same issue with the name - should match name used in API.Image class.
Line 1680: """
Line 1681: Download an image from a stream synchronously.
Line 1682:
Line 1683: Warning: Internal use only.
http://gerrit.ovirt.org/#/c/23281/4/vdsm/storage/imageSharing.py
File vdsm/storage/imageSharing.py:
Line 28:
Line 29:
Line 30: log = logging.getLogger("Storage.ImageSharing")
Line 31: WAIT_TIMEOUT = 30
Line 32: BUFFER_READ_SIZE = 65536
This is also the write size, so better call it BUFFER_SIZE.
Line 33:
Line 34:
Line 35: def httpGetSize(methodArgs):
Line 36: headers = curlImgWrap.head(methodArgs.get('url'),
Line 90: total_size - bytes_left, total_size)
Line 91: raise se.MiscFileReadException()
Line 92:
Line 93: p.stdin.write(data)
Line 94: p.stdin.flush()
A comment explaining why we flush here would be nice.
Line 95: bytes_left = bytes_left - len(data)
Line 96:
Line 97: p.stdin.close()
Line 98: if not p.wait(WAIT_TIMEOUT):
--
To view, visit http://gerrit.ovirt.org/23281
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I768b84799ed9fb2769c6d4240519d036f8988b99
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <laravot at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Liron Ar <laravot at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgotliv at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list