Change in vdsm[master]: introducing downloadFromStream verb

laravot at redhat.com laravot at redhat.com
Tue Jan 21 14:05:21 UTC 2014


Liron Ar has posted comments on this change.

Change subject: introducing downloadFromStream verb
......................................................................


Patch Set 4:

(15 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 yo
please phrase however you prefer it and i'll replace it.
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 e
It's not related to XML-RPC payload limit, it relates that we want to stream rather then send all the payload in the request rather then xml-rpc payload size limits.
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"
Done
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"
Done
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:
Done
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:
i'm not really in favour of dump/write. let's wait for more name suggestions and pick one.
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 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.
Done
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.
any other check then execute xml-rpc requests?

In the meanwhile let's go with that for all handlers and i'll test normal workflows between engine-vdsm.

Another option would be to set the timeout for the socket only  for the upload  as i suggested in the first patchset-
#self.rfile._sock.settimeout(5)

and then in later patch we can apply it the handler so it will be for all 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 
Done
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.
returned, copy mistake.
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 t
Done
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
1. the proper one seems to be REQUEST_TIMEOUT
2. Done
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.
same, let's wait for better name suggestions - i really don't mind which one we'll pick
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.
I don't really agree - we never use it for writing, just for reading. the size of the write depends on what we received..but i don't mind. Done.
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.
what comment would you suggest?
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