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