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