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

nsoffer at redhat.com nsoffer at redhat.com
Tue Feb 11 09:06:13 UTC 2014


Nir Soffer 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 30: today for request handling ('/', '/RPC2') will be treated
Line 31: as upload requests.
Line 32: The upload itself is being executed within a task, that's
Line 33: needed to indicate that there's an operation executed by
Line 34: the host.
> Liron, Nir: I did not understand the spm-ness issue: beyond the problem of 
The purpose of the upload/download task is to prevent spm replacement during the operation. When and pool has tasks on the vdsm side, the engine will not let you stop the spm before the task are completed.

We can probably remove this requirement if the engine manage a persistent lock for the ovf image, so only one host is permitted to write to this image. Until we have such infrastructure, the SPM lease is used to synchronize access to the ovf image.

This text can probably used in the commit message.
Line 35: 
Line 36: The PUT request inspected headers are:
Line 37: -- Mandatory headers:
Line 38:         *content-length


Line 40: -- Mandatory custom headers:
Line 41:         *X-Storage-Pool-Id : mandatory for succesfull execution
Line 42:         *X-Storage-Domain-Id: mandatory for succesfull execution
Line 43:         *X-Image-Id: mandatory for succesfull execution
Line 44:         *X-Volume-Id: mandatory for succesfull execution
Lets remove the details from the commit message, and put them were they belong, in the do_PUT docstring.
Line 45: 
Line 46: Change-Id: I768b84799ed9fb2769c6d4240519d036f8988b99


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

Line 137:             # timeout for the request socket
Line 138:             timeout = 60
Line 139:             log = logging.getLogger("BindingXMLRPC.RequestHandler")
Line 140: 
Line 141:             HEADER_POOL = 'X-Storage-Pool-Id'
> Deprecating the "X-" Prefix and Similar Constructs in Application Protocols
+1 for removing the "X-" prefix.
Line 142:             HEADER_DOMAIN = 'X-Storage-Domain-Id'
Line 143:             HEADER_IMAGE = 'X-Image-Id'
Line 144:             HEADER_VOLUME = 'X-Volume-Id'
Line 145:             HEADER_CONTENT_LENGTH = 'content-length'


Line 189:                                   'fileObj': self.rfile,
Line 190:                                   'callback': upload_finished,
Line 191:                                   'contentLength': contentLength}
Line 192:                     image = API.Image(imgUUID, spUUID, sdUUID)
Line 193:                     response = image.download(methodArgs, volUUID)
> download() seems synchronous to me - why is it needed to have a callback?
download is scheduling a task in hsm and return immediately with a task id.
Line 194: 
Line 195:                     while not uploadFinishedEvent.is_set():
Line 196:                         uploadFinishedEvent.wait()
Line 197: 


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

Line 18: #
Line 19: 
Line 20: import logging
Line 21: import curlImgWrap
Line 22: import signal
> please import stlib modules first, and Vdsm constructs only later.
This is unrelated to this patch, can be cleanup later to keep this patch minimal.
Line 23: import socket
Line 24: 
Line 25: import storage_exception as se
Line 26: from vdsm import constants


Line 100:             p.stdin.write(data)
Line 101:             # Process stdin is not a real file object but a wrapper using
Line 102:             # StringIO buffer. To ensure that we don't use more memory if we
Line 103:             # get data faster then dd read it from the pipe, we flush on every
Line 104:             # write. We can remove if we can limit the buffer size used by
> We can remove *flush()*
Did you check the underlying file object wrapper?
Line 105:             # this stdin wrapper.
Line 106:             p.stdin.flush()
Line 107:             bytes_left = bytes_left - len(data)
Line 108: 


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