Change in vdsm[master]: introducing downloadFromStream verb

nsoffer at redhat.com nsoffer at redhat.com
Tue Jan 21 15:01:13 UTC 2014


Nir Soffer has posted comments on this change.

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


Patch Set 5: Code-Review+1

(5 comments)

I think this is a really nice hack. There are some minor text and naming issues but I think it is good enough as is. We can always beautify it later.

http://gerrit.ovirt.org/#/c/23281/5//COMMIT_MSG
Commit Message:

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 15: 
Trying to rephrase this to make it more clear:

Previously we could send ovf data using XMLRPC, which limit the size of the data, having to encode the payload into the xml, and make it hard an inefficient to upload lot of data and store it on some image.

This patch adds the downloadFromStream internal verb, allowing efficient upload of data in any size and format and storing it directly on an image. To avoid requiring another port, we use the existing xmlrpc server for handling upload requests.
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


http://gerrit.ovirt.org/#/c/23281/5/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):
(Copied from patch set 4)

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/5/vdsm/BindingXMLRPC.py
File vdsm/BindingXMLRPC.py:

Line 180:                     volUUID = self.headers.getheader(self.HEADER_VOLUME)
Line 181: 
Line 182:                     methodArgs = {'method': 'stream',
Line 183:                                   'fileObj': self.rfile,
Line 184:                                   'Content-Length': contentLength}
This key should use the same style as other keys:

    'contentLength': contentLength
Line 185:                     image = API.Image(imgUUID, spUUID, sdUUID)
Line 186:                     response = image.downloadFromStream(methodArgs,
Line 187:                                                         volUUID)
Line 188:                     json_response = json.dumps(response)


Line 205: 
Line 206:             def send_error(self, error, message, exc_info=False):
Line 207:                 try:
Line 208:                     self.log.error(message,
Line 209:                                    exc_info=exc_info)
Seems that this line break is not needed.
Line 210:                     self.send_response(error)
Line 211:                     self.end_headers()
Line 212:                     self.finish()
Line 213:                 except Exception:


http://gerrit.ovirt.org/#/c/23281/5/vdsm/storage/imageSharing.py
File vdsm/storage/imageSharing.py:

Line 53:     return size
Line 54: 
Line 55: 
Line 56: def streamGetSize(methodArgs):
Line 57:     return methodArgs['Content-Length']
Note that 'Content-Length' is not symmetric to the same key in httpGetSize. In httpGetSize, the content length is extracted from the headers dictionary, while here is is passed in the methodArgs dict.

The fact that we got this data using the http protocol is not relevant to this code, which is focus on writing data to images from a file object. We can change the implementation of the server, using plain socket for sending data, and this interface will not change.
Line 58: 
Line 59: 
Line 60: def httpDownloadImage(dstImgPath, methodArgs):
Line 61:     curlImgWrap.download(methodArgs.get('url'), dstImgPath,


-- 
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: 5
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