Change in vdsm[master]: introducing capabillity to stream data to image

Federico Simoncelli fsimonce at redhat.com
Wed Feb 12 13:56:45 UTC 2014


Federico Simoncelli has posted comments on this change.

Change subject: introducing capabillity to stream data to image
......................................................................


Patch Set 9:

(6 comments)

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

Line 3: AuthorDate: 2014-01-12 13:52:24 +0200
Line 4: Commit:     Liron Aravot <laravot at redhat.com>
Line 5: CommitDate: 2014-02-09 15:08:28 +0200
Line 6: 
Line 7: introducing capabillity to stream data to image
typo: capability
Line 8: 
Line 9: This patch introduces the a new capabaillity to vdsm
Line 10: which allows to upload using streaming content to vdsm
Line 11: images.


Line 5: CommitDate: 2014-02-09 15:08:28 +0200
Line 6: 
Line 7: introducing capabillity to stream data to image
Line 8: 
Line 9: This patch introduces the a new capabaillity to vdsm
typo: capability
Line 10: which allows to upload using streaming content to vdsm
Line 11: images.
Line 12: 
Line 13: Previously we sent ovf data using XMLRPC (UpdateVM verb),


Line 14: which limits the size of the data, having to encode the
Line 15: payload into the xml, and make it hard and inefficient
Line 16: to upload lot of data and store it on some image.
Line 17: 
Line 18: This patch adds the capabillity to stream data to image,
typo: capability
Line 19: allowing efficient upload of data in any size and format
Line 20: and storing it directly on an
Line 21: image. As the  XML-RPC spec doesn't support streaming and
Line 22: to avoid requiring another port by using dedicated http


http://gerrit.ovirt.org/#/c/23281/9/vdsm/BindingXMLRPC.py
File vdsm/BindingXMLRPC.py:

Line 177: 
Line 178:                     spUUID = self.headers.getheader(self.HEADER_POOL)
Line 179:                     sdUUID = self.headers.getheader(self.HEADER_DOMAIN)
Line 180:                     imgUUID = self.headers.getheader(self.HEADER_IMAGE)
Line 181:                     volUUID = self.headers.getheader(self.HEADER_VOLUME)
We generally validate these when received from the client. We should at least check that they're provided (except volUUID probably).
Line 182: 
Line 183:                     uploadFinishedEvent = threading.Event()
Line 184: 
Line 185:                     def upload_finished():


Line 187: 
Line 188:                     methodArgs = {'method': 'stream',
Line 189:                                   'fileObj': self.rfile,
Line 190:                                   'callback': upload_finished,
Line 191:                                   'contentLength': contentLength}
methodArgs is an external API that shouldn't be stretched to our internal purposes.
I prefer the previous downloadFromStream method implementation (one of the previous abandoned patches) for two reasons:

* there's nothing that prevents a client from using "stream" from the regular upload/download API (which is wrong, it just exposes something potentially dangerous)
* also the additional "callback" and "contentLength" values are dangerous and shouldn't be exposed in the API.
Line 192:                     image = API.Image(imgUUID, spUUID, sdUUID)
Line 193:                     response = image.download(methodArgs, volUUID)
Line 194: 
Line 195:                     while not uploadFinishedEvent.is_set():


http://gerrit.ovirt.org/#/c/23281/9/vdsm/storage/image.py
File vdsm/storage/image.py:

Line 1164:             domain.deactivateImage(imgUUID)
Line 1165: 
Line 1166:     def download(self, methodArgs, sdUUID, imgUUID, volUUID=None):
Line 1167:         try:
Line 1168:             self._download(methodArgs, sdUUID, imgUUID, volUUID=None)
volUUID=None ? 

Anyway I think this shouldn't reuse the old download. It should be the new downloadFromStream.
Line 1169:         finally:
Line 1170:             callback = imageSharing.getCallback(methodArgs)
Line 1171:             if callback is not None:
Line 1172:                 callback()


-- 
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <laravot at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken 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: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list