Change in vdsm[master]: downloadFromStream

nsoffer at redhat.com nsoffer at redhat.com
Tue Jan 14 22:54:47 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: downloadFromStream
......................................................................


Patch Set 4:

(20 comments)

http://gerrit.ovirt.org/#/c/23131/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 feature adds an option to upload files - I don't think that donwloadFromStream is good name. The only reason this is implemented like download, is reusing code that use write data to images.

The best option would be to replace the current upload method which is not clear what it does :-)
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/23131/4/vdsm/BindingXMLRPC.py
File vdsm/BindingXMLRPC.py:

Line 134:             def setup(self):
Line 135:                 threadLocal.client = self.client_address[0]
Line 136:                 return basehandler.setup(self)
Line 137: 
Line 138:             def do_PUT(self):
It would be nice to limit this handler to specific path - xmlrpc is using /RPC2, this should use something like /DATA.
Line 139:                 ctype, pt = cgi.parse_header(self.headers.
Line 140:                                              getheader('content-type'))
Line 141: 
Line 142:                 if ctype == 'application/octet-stream':


Line 143:                     try:
Line 144:                         # NEED TO CHECK THOSE
Line 145:                         #self.rfile._sock.settimeout(5)
Line 146:                         #self.rfile._sock.set_socket_read_timeout
Line 147:                         spUUID = self.headers.getheader('X-Storage-Pool-Id')
Why not use the same terms we already use?

   X-Pool-Id
   X-Domain-Id
   X-Image-Id
   X-Volume-Id
Line 148:                         sdUUID = self.headers.getheader('X-Storage-Domain-Id')
Line 149:                         imgUUID = self.headers.getheader('X-Image-Id')
Line 150:                         volUUID = self.headers.getheader('X-Volume-Id')
Line 151:                         bufferSize = self.headers.getheader('X-Buffer-Size')


Line 147:                         spUUID = self.headers.getheader('X-Storage-Pool-Id')
Line 148:                         sdUUID = self.headers.getheader('X-Storage-Domain-Id')
Line 149:                         imgUUID = self.headers.getheader('X-Image-Id')
Line 150:                         volUUID = self.headers.getheader('X-Volume-Id')
Line 151:                         bufferSize = self.headers.getheader('X-Buffer-Size')
Uneeded
Line 152:                         contentLength = self.headers.\
Line 153:                             getheader('content-length')
Line 154: 
Line 155:                         if contentLength is None:


Line 149:                         imgUUID = self.headers.getheader('X-Image-Id')
Line 150:                         volUUID = self.headers.getheader('X-Volume-Id')
Line 151:                         bufferSize = self.headers.getheader('X-Buffer-Size')
Line 152:                         contentLength = self.headers.\
Line 153:                             getheader('content-length')
For consistency, use "Content-Length".

Finally, all those strings should be constants, so if you make a typo, you get NameError instead of silent failure when misspelled key is not found.
Line 154: 
Line 155:                         if contentLength is None:
Line 156:                             self.send_response(411)
Line 157:                             self.finish()


http://gerrit.ovirt.org/#/c/23131/4/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 1668:         and methodArgs.
Line 1669:         """
Line 1670:         sdCache.produce(sdUUID)
Line 1671:         pool = self.getPool(spUUID)
Line 1672: 
Unrelated change
Line 1673:         # NOTE: this could become an hsm task, in such case the LV extension
Line 1674:         # required to prepare the destination should go through the mailbox.
Line 1675:         self._spmSchedule(spUUID, "downloadImage", pool.downloadImage,
Line 1676:                           methodArgs, sdUUID, imgUUID, volUUID)


Line 1677: 
Line 1678:     @public
Line 1679:     def downloadImageFromStream(self, methodArgs, spUUID, sdUUID, imgUUID,
Line 1680:                                 volUUID=None):
Line 1681:         """
Looks like this should be merged in downloadImage, passing the right parameters to poll.downloadImage using methodArgs.

Ohterwise we don't have a task, and spm may be stopped while an upload is writing to storage.
Line 1682:         Download an image from a stream synchronously.
Line 1683:         """
Line 1684:         sdCache.produce(sdUUID)
Line 1685:         pool = self.getPool(spUUID)


http://gerrit.ovirt.org/#/c/23131/4/vdsm/storage/imageSharing.py
File vdsm/storage/imageSharing.py:

Line 50: 
Line 51: 
Line 52: def streamGetSize(methodArgs):
Line 53:     size = None
Line 54:     if 'Content-Length' in methodArgs:
Should be a constant
Line 55:         size = int(methodArgs['Content-Length'])
Line 56:     return size
Line 57: 
Line 58: 


Line 55:         size = int(methodArgs['Content-Length'])
Line 56:     return size
Line 57: 
Line 58: 
Line 59: def streamGetBufferSize(methodArgs):
We don't need this value. vdsm does not care about the buffer size used by engine to upload data, and engine does not care about the buffer size used by vdsm to buffer data in the way to the dd command.
Line 60:     size = None
Line 61:     if 'X-Buffer-Size' in methodArgs:
Line 62:         size = int(methodArgs['X-Buffer-Size'])
Line 63:     return size


Line 74: 
Line 75: 
Line 76: def streamDownloadImage(dstImgPath, methodArgs):
Line 77:     bytes_left = streamGetSize(methodArgs)
Line 78:     buffer_size = streamGetBufferSize(methodArgs)
This should be a constant of this module.
Line 79:     stream = methodArgs.get('fileObj')
Line 80: 
Line 81:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" %
Line 82:            str(storageConstants.BLOCK_SIZE), "count=%d" % bytes_left]


Line 77:     bytes_left = streamGetSize(methodArgs)
Line 78:     buffer_size = streamGetBufferSize(methodArgs)
Line 79:     stream = methodArgs.get('fileObj')
Line 80: 
Line 81:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" %
We may need other dd options - see how other code is using dd to write data.
Line 82:            str(storageConstants.BLOCK_SIZE), "count=%d" % bytes_left]
Line 83:     p = utils.execCmd(cmd, sudo=False, sync=False)
Line 84:     log.debug(str(stream._getclosed()))
Line 85: 


Line 78:     buffer_size = streamGetBufferSize(methodArgs)
Line 79:     stream = methodArgs.get('fileObj')
Line 80: 
Line 81:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" %
Line 82:            str(storageConstants.BLOCK_SIZE), "count=%d" % bytes_left]
This should be a constant of this module - this is not the storage block size, but the buffer size used by dd when writing data.
Line 83:     p = utils.execCmd(cmd, sudo=False, sync=False)
Line 84:     log.debug(str(stream._getclosed()))
Line 85: 
Line 86:     while bytes_left > 0:


Line 80: 
Line 81:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" %
Line 82:            str(storageConstants.BLOCK_SIZE), "count=%d" % bytes_left]
Line 83:     p = utils.execCmd(cmd, sudo=False, sync=False)
Line 84:     log.debug(str(stream._getclosed()))
You are calling here a private method of stream - why?
Line 85: 
Line 86:     while bytes_left > 0:
Line 87:         i = buffer_size
Line 88:         if not i:


Line 83:     p = utils.execCmd(cmd, sudo=False, sync=False)
Line 84:     log.debug(str(stream._getclosed()))
Line 85: 
Line 86:     while bytes_left > 0:
Line 87:         i = buffer_size
The name i is confusing name in this context. "bytes_to_read" would more clear.
Line 88:         if not i:
Line 89:             i = storageConstants.BLOCK_SIZE
Line 90:         if (bytes_left < i):
Line 91:             i = bytes_left


Line 93:         if not data:
Line 94:             break
Line 95:         p.stdin.write(data)
Line 96:         p.stdin.flush()
Line 97:         bytes_left = bytes_left - i
You must assume that you can get partial read.
Line 98: 
Line 99:     p.stdin.close()
Line 100:     p.wait()
Line 101: 


Line 96:         p.stdin.flush()
Line 97:         bytes_left = bytes_left - i
Line 98: 
Line 99:     p.stdin.close()
Line 100:     p.wait()
You must check the returncode of the dd process - otherwise engine may think that the upload succeeded, while dd failed to write some data.
Line 101: 
Line 102: 
Line 103: _METHOD_IMPLEMENTATIONS = {
Line 104:     'http': (httpGetSize, httpDownloadImage, httpUploadImage),


Line 98: 
Line 99:     p.stdin.close()
Line 100:     p.wait()
Line 101: 
Line 102: 
In summary, this should be something like this:

    # Should be a good start value for reading from a socket
    # and writing to a pipe.
    BUFFER_SIZE = 64 * 1024

    while bytes_left > 0:
        bytes_to_read = min(BUFFER_SIZE, bytes_left)
        data = stream.read(bytes_to_read)
        if not data:
            # socket was closed too early
            p.kill()
            raise DownloadError("description...")
        bytes_left -= len(data)
        p.stdin.write(data)
        p.stdin.flush()

    p.stdin.close()
    p.wait()
    if p.returncode != 0:
        raise DownloadError("dd process exited with status: %d", p.returncode)
Line 103: _METHOD_IMPLEMENTATIONS = {
Line 104:     'http': (httpGetSize, httpDownloadImage, httpUploadImage),
Line 105:     'stream': (streamGetSize, streamDownloadImage, None),
Line 106: }


Line 105:     'stream': (streamGetSize, streamDownloadImage, None),
Line 106: }
Line 107: 
Line 108: 
Line 109: def getSharingMethod(methodArgs):
Why is this public now?
Line 110:     try:
Line 111:         return methodArgs['method']
Line 112:     except KeyError:
Line 113:         raise RuntimeError("Sharing method not specified")


Line 112:     except KeyError:
Line 113:         raise RuntimeError("Sharing method not specified")
Line 114: 
Line 115: 
Line 116: def _getSharingMethodsImplementation(methodArgs):
> 1. the word 'Implementation' here is not adding any information.  This look
I agree with Ayal, and even worse, this change is not needed for this patch.
Line 117:     method = getSharingMethod(methodArgs)
Line 118: 
Line 119:     try:
Line 120:         return _METHOD_IMPLEMENTATIONS[method]


http://gerrit.ovirt.org/#/c/23131/4/vdsm/storage/storageConstants.py
File vdsm/storage/storageConstants.py:

Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: #
Line 20: 
Line 21: STORAGE = "Storage"
Line 22: BLOCK_SIZE = 512
This constant does not belong here - it is private implementation details of the new upload feature. If we find that we need the same constant in other places, we can move it here.


-- 
To view, visit http://gerrit.ovirt.org/23131
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I85bc63365273adf306c73ef1de7fea578ec42662
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: Nir Soffer <nsoffer 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