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 <[email protected]> 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 <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
