Change in vdsm[master]: introducing downloadImageFromStream

Liron Ar has uploaded a new change for review.

Change subject: introducing downloadImageFromStream
......................................................................

introducing downloadImageFromStream

This patch introduces the downloadImageFromStream verb in vdsm which
allows to upload using streaming content to vdsm images.
XML-RPC spec doesn't support streaming, as we don't want to use HTTP
server in addition to the current XML-RPC server (to not use another
port) we will threat some requests as http requests instead of xml-rpc
requests.

General upload information:
---------------------------------------------------------------
PUT requests arriving to the server with content type of
application/octet-stream to default paths that we use
today for request handling ('/', '/RPC2') will be threated as
upload requests.

The PUT stream upload request inspected headers are:
-- Mandatory headers:
        *content-length

-- Mandatory custom headers:
        *X-Storage-Pool-Id : mandatory for succesfull execution
        *X-Storage-Domain-Id: mandatory for succesfull execution
        *X-Image-Id: mandatory for succesfull execution
        *X-Volume-Id: mandatory for succesfull execution

-- Optional custom headers;
        *X-Buffer-Size: optional, indicates the passed buffer

The server will return answers with the following codes:
200 - request completed, the response body will contain the execution
results, the returned content type is 'application/json'
500 - internal error
406 - missing content-length header

in case that a task was created, its id will be passed in the response
using the custom header 'X-Task-Id' for polling by the engine.

Implementation details
--------------------------------------------------------------
Currently for any spm task (besides delete image) the operation is
performed through a spm task.
We need to have a task to indicate that "there's something" going on -
for example, if we had no task during the upload the spm could be
stopped and be started on other host which might be hazardous.

For having a task we can do two different things -
1. schedule a task to perform the whole download process, that means that
the request thread will be blocked, the read from the stream and writes
to the destination image will be done through the task and then the
request thread will be released and response would be sent.

2. schedule a "dummy" task with "blank" function that will just be used to
indicate that there's an downloadFromStream execution going on while the
request thread will be used for executing the actual download.

Option 2 is implemented with this patch.

needed:
*socket timeout inspection
*tests?

Change-Id: I768b84799ed9fb2769c6d4240519d036f8988b99
Signed-off-by: Liron Aravot 
---
M vdsm/API.py
M vdsm/BindingXMLRPC.py
M vdsm/storage/hsm.py
M vdsm/storage/imageSharing.py
4 files changed, 153 insertions(+), 3 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/81/23281/1

diff --git a/vdsm/API.py b/vdsm/API.py
index e7a550c..37802e1 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -838,6 +838,10 @@
         return self._irs.downloadImage(
             methodArgs, self._spUUID, self._sdUUID, self._UUID, volUUID)

+    def downloadFromStream(self, methodArgs, volUUID=None):
+        return self._irs.downloadImageFromStream(
+            methodArgs, self._spUUID, self._sdUUID, self._UUID, volUUID)
+

class LVMVolumeGroup(APIBase):
     ctorArgs = ['lvmvolumegroupID']
diff --git a/vdsm/BindingXMLRPC.py b/vdsm/BindingXMLRPC.py
index a340ae7..8b7d9fa 100644
--- a/vdsm/BindingXMLRPC.py
+++ b/vdsm/BindingXMLRPC.py
@@ -25,11 +25,14 @@
 import logging
 import libvirt
 import threading
+import cgi
+import json

from vdsm import constants
 from vdsm import utils
 from vdsm.define import doneCode, errCode
 from vdsm.netinfo import getRouteDeviceTo
+from storage.threadLocal import vars
 import API
 from vdsm.exception import VdsmException
 try:
@@ -128,10 +131,91 @@
         else:
             basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler

-        class LoggingHandler(LoggingMixIn, basehandler):
+        class RequestHandler(LoggingMixIn, basehandler):
+
+            log = logging.getLogger("BindingXMLRPC.RequestHandler")
+            CUSTOM_HEADER_POOL = 'X-Storage-Pool-Id'
+            CUSTOM_HEADER_DOMAIN = 'X-Storage-Domain-Id'
+            CUSTOM_HEADER_IMAGE = 'X-Image-Id'
+            CUSTOM_HEADER_VOLUME = 'X-Volume-Id'
+            CUSTOM_HEADER_BUFFER = 'X-Buffer-Size'
+            CUSTOM_HEADER_TASK_ID = 'X-Task-Id'
+            HEADER_CONTENT_LENGTH = 'content-length'
+            HEADER_CONTENT_TYPE = 'content-type'
+            RESPONSE_LENGTH_REQUIRED_ = 411
+            RESPONSE_NOT_ACCEPTABLE = 406
+            RESPONSE_REQUEST_SUCCEEDED = 200
+            RESPONSE_INTERNAL_ERROR = 500
+
             def setup(self):
                 threadLocal.client = self.client_address[0]
                 return basehandler.setup(self)
+
+            def addTaskIdHeaderIfPresent(self):
+                if hasattr(vars, 'task'):
+                    self.send_header(self.CUSTOM_HEADER_TASK_ID,
+                                     vars.task.getID())
+
+            def do_PUT(self):
+                ctype, pt = cgi.parse_header(self.headers.getheader(self.
+                                             HEADER_CONTENT_TYPE))
+                try:
+                    if ctype == 'application/octet-stream':
+                        contentLength = self.headers. \
+                            getheader(self.HEADER_CONTENT_LENGTH)
+                        if not contentLength:
+                            self.send_response(411)
+                            self.finish()
+                            return
+
+                        # NEED TO DECIDE ABOUT THOSE
+                        #self.rfile._sock.settimeout(5)
+                        #self.rfile._sock.set_socket_read_timeout
+                        spUUID = self.headers. \
+                            getheader(self.CUSTOM_HEADER_POOL)
+                        sdUUID = self.headers. \
+                            getheader(self.CUSTOM_HEADER_DOMAIN)
+                        imgUUID = self.headers. \
+                            getheader(self.CUSTOM_HEADER_IMAGE)
+                        volUUID = self.headers. \
+                            getheader(self.CUSTOM_HEADER_VOLUME)
+                        bufferSize = self.headers. \
+                            getheader(self.CUSTOM_HEADER_BUFFER)
+                        contentLength = self.headers. \
+                            getheader(self.HEADER_CONTENT_LENGTH)
+
+                        methodArgs = {'method': 'stream',
+                                      'fileObj': self.rfile,
+                                      'Content-Length': contentLength,
+                                      'Buffer-Size': bufferSize}
+                        image = API.Image(imgUUID, spUUID, sdUUID)
+                        response = image.downloadFromStream(methodArgs,
+                                                            volUUID)
+                        json_response = json.dumps(response)
+                        self.send_response(200)
+                        self.send_header(self.HEADER_CONTENT_TYPE,
+                                         'application/json')
+                        self.send_header(self.HEADER_CONTENT_LENGTH,
+                                         len(json_response))
+                        self.addTaskIdHeaderIfPresent()
+                        self.end_headers()
+                        self.wfile.write(json_response)
+                        self.finish()
+                    else:
+                        self.send_response(406)
+                        self.finish()
+                # shouldn't happen
+                except Exception:
+                    self.log.error("execution exception",
+                                   exc_info=True)
+                    try:
+                        self.send_response(500)
+                        self.addTaskIdHeaderIfPresent()
+                        self.end_headers()
+                        self.finish()
+                    except Exception:
+                        self.log.error("failed to return response",
+                                       exc_info=True)

def parse_request(self):
                 r = (SecureXMLRPCServer.SecureXMLRPCRequestHandler.
@@ -145,11 +229,11 @@
                 server_address,
                 keyfile=KEYFILE, certfile=CERTFILE, ca_certs=CACERT,
                 timeout=self.serverRespTimeout,
-                requestHandler=LoggingHandler)
+                requestHandler=RequestHandler)
         else:
             server = utils.SimpleThreadedXMLRPCServer(
                 server_address,
-                requestHandler=LoggingHandler, logRequests=True)
+                requestHandler=RequestHandler, logRequests=True)
         utils.closeOnExec(server.socket.fileno())

server.lastClientTime = 0
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 31a6e1c..42b54ce 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -1674,6 +1674,18 @@
         self._spmSchedule(spUUID, "downloadImage", pool.downloadImage,
                           methodArgs, sdUUID, imgUUID, volUUID)

+    @public
+    def downloadImageFromStream(self, methodArgs, spUUID, sdUUID, imgUUID,
+                                volUUID=None):
+        """
+        Download an image from a stream synchronously.
+        """
+        sdCache.produce(sdUUID)
+        pool = self.getPool(spUUID)
+        self._spmSchedule(spUUID, "downloadImageFromStream %s" %
+                          imgUUID, lambda: True)
+        pool.downloadImage(methodArgs, sdUUID, imgUUID, volUUID)
+
     @deprecated
     @public
     def moveMultipleImages(self, spUUID, srcDomUUID, dstDomUUID, imgDict,
diff --git a/vdsm/storage/imageSharing.py b/vdsm/storage/imageSharing.py
index 26f299a..fa2080d 100644
--- a/vdsm/storage/imageSharing.py
+++ b/vdsm/storage/imageSharing.py
@@ -20,6 +20,9 @@
 import logging

import curlImgWrap
+import storage_exception as se
+from vdsm import constants
+from vdsm import utils

log = logging.getLogger("Storage.ImageSharing")
@@ -46,6 +49,20 @@
     return size

+def streamGetSize(methodArgs):
+    size = None
+    if 'Content-Length' in methodArgs:
+        size = int(methodArgs['Content-Length'])
+    return size
+
+
+def streamGetBufferSize(methodArgs):
+    size = None
+    if 'X-Buffer-Size' in methodArgs:
+        size = int(methodArgs['X-Buffer-Size'])
+    return size
+
+
 def httpDownloadImage(dstImgPath, methodArgs):
     curlImgWrap.download(methodArgs.get('url'), dstImgPath,
                          methodArgs.get("headers", {}))
@@ -56,8 +73,41 @@
                        methodArgs.get("headers", {}))

+def streamDownloadImage(dstImgPath, methodArgs):
+    bytes_left = streamGetSize(methodArgs)
+    buffer_size = streamGetBufferSize(methodArgs)
+    stream = methodArgs.get('fileObj')
+
+    BS = constants.MEGAB
+    count = bytes_left / BS
+    cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" %
+           constants.MEGAB, "count=%d" % count]
+    p = utils.execCmd(cmd, sudo=False, sync=False)
+
+    default_read_size = buffer_size
+    if default_read_size is None:
+        default_read_size = BS
+
+    while bytes_left > 0:
+        i = min(default_read_size, bytes_left)
+        data = stream.read(i)
+        if not data:
+            break
+        p.stdin.write(data)
+        p.stdin.flush()
+        bytes_left = bytes_left - i
+
+    p.stdin.close()
+    p.wait()
+    if p.returncode != 0:
+        log.error("streamDownloadImage failed",
+                  p.returncode, p.stderr.read(1000))
+        raise se.MiscFileWriteException()
+
+
 _METHOD_IMPLEMENTATIONS = {
     'http': (httpGetSize, httpDownloadImage, httpUploadImage),
+    'stream': (streamGetSize, streamDownloadImage, None),
 }

Liron Ar has abandoned this change.

Change subject: introducing downloadImageFromStream
......................................................................

Abandoned

Liron Ar has restored this change.

Change subject: introducing downloadImageFromStream
......................................................................

Restored

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: introducing downloadImageFromStream
......................................................................

Patch Set 1:

(20 comments)

Partial review: BindingXMLRPC.py

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

Line 25: import logging
Line 26: import libvirt
Line 27: import threading
Line 28: import cgi
Line 29: import json
These imports should be sorted
Line 30: 
Line 31: from vdsm import constants
Line 32: from vdsm import utils
Line 33: from vdsm.define import doneCode, errCode

Line 131:         else:
Line 132:             basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler
Line 133: 
Line 134:         class RequestHandler(LoggingMixIn, basehandler):
Line 135: 
For timeout, you can put here a class variable:

timeout = 60  # Don't wait more then 60 seconds for data
Line 136:             log = logging.getLogger("BindingXMLRPC.RequestHandler")
Line 137:             CUSTOM_HEADER_POOL = 'X-Storage-Pool-Id'
Line 138:             CUSTOM_HEADER_DOMAIN = 'X-Storage-Domain-Id'
Line 139:             CUSTOM_HEADER_IMAGE = 'X-Image-Id'

Line 132:             basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler
Line 133: 
Line 134:         class RequestHandler(LoggingMixIn, basehandler):
Line 135: 
Line 136:             log = logging.getLogger("BindingXMLRPC.RequestHandler")
It would be nice to separate constants from the log.
Line 137:             CUSTOM_HEADER_POOL = 'X-Storage-Pool-Id'
Line 138:             CUSTOM_HEADER_DOMAIN = 'X-Storage-Domain-Id'
Line 139:             CUSTOM_HEADER_IMAGE = 'X-Image-Id'
Line 140:             CUSTOM_HEADER_VOLUME = 'X-Volume-Id'

Line 133: 
Line 134:         class RequestHandler(LoggingMixIn, basehandler):
Line 135: 
Line 136:             log = logging.getLogger("BindingXMLRPC.RequestHandler")
Line 137:             CUSTOM_HEADER_POOL = 'X-Storage-Pool-Id'
CUSTOM_HEADER_ prefix is not needed and only make the constants too long, leading to unwanted line breaks.

Do we need the "Storage" word in X-Storage-Pool-Id and X-Storage-Domain-Id?

Same for HEADER_ prefix, since we have only one CONTENT_LENGTH constant, it help nobody to add a prefix.
Line 138:             CUSTOM_HEADER_DOMAIN = 'X-Storage-Domain-Id'
Line 139:             CUSTOM_HEADER_IMAGE = 'X-Image-Id'
Line 140:             CUSTOM_HEADER_VOLUME = 'X-Volume-Id'
Line 141:             CUSTOM_HEADER_BUFFER = 'X-Buffer-Size'

Line 137:             CUSTOM_HEADER_POOL = 'X-Storage-Pool-Id'
Line 138:             CUSTOM_HEADER_DOMAIN = 'X-Storage-Domain-Id'
Line 139:             CUSTOM_HEADER_IMAGE = 'X-Image-Id'
Line 140:             CUSTOM_HEADER_VOLUME = 'X-Volume-Id'
Line 141:             CUSTOM_HEADER_BUFFER = 'X-Buffer-Size'
Not needed as we discussed
Line 142:             CUSTOM_HEADER_TASK_ID = 'X-Task-Id'
Line 143:             HEADER_CONTENT_LENGTH = 'content-length'
Line 144:             HEADER_CONTENT_TYPE = 'content-type'
Line 145:             RESPONSE_LENGTH_REQUIRED_ = 411

Line 144:             HEADER_CONTENT_TYPE = 'content-type'
Line 145:             RESPONSE_LENGTH_REQUIRED_ = 411
Line 146:             RESPONSE_NOT_ACCEPTABLE = 406
Line 147:             RESPONSE_REQUEST_SUCCEEDED = 200
Line 148:             RESPONSE_INTERNAL_ERROR = 500
For http status codes, please use httplib:
> > > import httplib
> > > httplib.OK

200
> > > httplib.LENGTH_REQUIRED

411
> > > httplib.UNSUPPORTED_MEDIA_TYPE

415
Line 149: 
Line 150:             def setup(self):
Line 151:                 threadLocal.client = self.client_address[0]
Line 152:                 return basehandler.setup(self)

Line 150:             def setup(self):
Line 151:                 threadLocal.client = self.client_address[0]
Line 152:                 return basehandler.setup(self)
Line 153: 
Line 154:             def addTaskIdHeaderIfPresent(self):
This should be sendTaskHeaderIfPresent, since it sends a header.
Line 155:                 if hasattr(vars, 'task'):
Line 156:                     self.send_header(self.CUSTOM_HEADER_TASK_ID,
Line 157:                                      vars.task.getID())
Line 158:

Line 157:                                      vars.task.getID())
Line 158: 
Line 159:             def do_PUT(self):
Line 160:                 ctype, pt = cgi.parse_header(self.headers.getheader(self.
Line 161:                                              HEADER_CONTENT_TYPE))
Why this check is out of the try-except block?
Line 162:                 try:
Line 163:                     if ctype == 'application/octet-stream':
Line 164:                         contentLength = self.headers. \
Line 165:                             getheader(self.HEADER_CONTENT_LENGTH)

Line 159:             def do_PUT(self):
Line 160:                 ctype, pt = cgi.parse_header(self.headers.getheader(self.
Line 161:                                              HEADER_CONTENT_TYPE))
Line 162:                 try:
Line 163:                     if ctype == 'application/octet-stream':
The code would be more clear if we bail out as soon as we find an error:

check content type
 if wrong:
     send error
     return

check content length
 if wrong:
     send error
     return

continue with valid request...

As done here, we have unneeded indentation that hamper readability, and when we finally get to the else: block related to the content type check, we don't have any idea what if this else corresponds to.
Line 164:                         contentLength = self.headers. \
Line 165:                             getheader(self.HEADER_CONTENT_LENGTH)
Line 166:                         if not contentLength:
Line 167:                             self.send_response(411)

Line 163:                     if ctype == 'application/octet-stream':
Line 164:                         contentLength = self.headers. \
Line 165:                             getheader(self.HEADER_CONTENT_LENGTH)
Line 166:                         if not contentLength:
Line 167:                             self.send_response(411)
Use constant
Line 168:                             self.finish()
Line 169:                             return
Line 170: 
Line 171:                         # NEED TO DECIDE ABOUT THOSE

Line 164:                         contentLength = self.headers. \
Line 165:                             getheader(self.HEADER_CONTENT_LENGTH)
Line 166:                         if not contentLength:
Line 167:                             self.send_response(411)
Line 168:                             self.finish()
This pattern:

self.send_response(n)
    self.finish()

Appear 2 times here. It would be nice to add a send_error method - maybe even the original class has such method.

Such method can accept an error code and message and log the message before returning an error response.
Line 169:                             return
Line 170: 
Line 171:                         # NEED TO DECIDE ABOUT THOSE
Line 172:                         #self.rfile._sock.settimeout(5)

Line 169:                             return
Line 170: 
Line 171:                         # NEED TO DECIDE ABOUT THOSE
Line 172:                         #self.rfile._sock.settimeout(5)
Line 173:                         #self.rfile._sock.set_socket_read_timeout
We don't need it as we discussed, timeout class variable will handle this.
Line 174:                         spUUID = self.headers. \
Line 175:                             getheader(self.CUSTOM_HEADER_POOL)
Line 176:                         sdUUID = self.headers. \
Line 177:                             getheader(self.CUSTOM_HEADER_DOMAIN)

Line 171:                         # NEED TO DECIDE ABOUT THOSE
Line 172:                         #self.rfile._sock.settimeout(5)
Line 173:                         #self.rfile._sock.set_socket_read_timeout
Line 174:                         spUUID = self.headers. \
Line 175:                             getheader(self.CUSTOM_HEADER_POOL)
This hard to read lines are caused by too much indentation and too long constant names.
Line 176:                         sdUUID = self.headers. \
Line 177:                             getheader(self.CUSTOM_HEADER_DOMAIN)
Line 178:                         imgUUID = self.headers. \
Line 179:                             getheader(self.CUSTOM_HEADER_IMAGE)

Line 185:                             getheader(self.HEADER_CONTENT_LENGTH)
Line 186: 
Line 187:                         methodArgs = {'method': 'stream',
Line 188:                                       'fileObj': self.rfile,
Line 189:                                       'Content-Length': contentLength,
Use constant
Line 190:                                       'Buffer-Size': bufferSize}
Line 191:                         image = API.Image(imgUUID, spUUID, sdUUID)
Line 192:                         response = image.downloadFromStream(methodArgs,
Line 193:                                                             volUUID)

Line 186: 
Line 187:                         methodArgs = {'method': 'stream',
Line 188:                                       'fileObj': self.rfile,
Line 189:                                       'Content-Length': contentLength,
Line 190:                                       'Buffer-Size': bufferSize}
Not needed
Line 191:                         image = API.Image(imgUUID, spUUID, sdUUID)
Line 192:                         response = image.downloadFromStream(methodArgs,
Line 193:                                                             volUUID)
Line 194:                         json_response = json.dumps(response)

Line 191:                         image = API.Image(imgUUID, spUUID, sdUUID)
Line 192:                         response = image.downloadFromStream(methodArgs,
Line 193:                                                             volUUID)
Line 194:                         json_response = json.dumps(response)
Line 195:                         self.send_response(200)
Use constant
Line 196:                         self.send_header(self.HEADER_CONTENT_TYPE,
Line 197:                                          'application/json')
Line 198:                         self.send_header(self.HEADER_CONTENT_LENGTH,
Line 199:                                          len(json_response))

Line 201:                         self.end_headers()
Line 202:                         self.wfile.write(json_response)
Line 203:                         self.finish()
Line 204:                     else:
Line 205:                         self.send_response(406)
This status code is not correct, it should be 415 Unsupported media type:

http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.16
Line 206:                         self.finish()
Line 207:                 # shouldn't happen
Line 208:                 except Exception:
Line 209:                     self.log.error("execution exception",

Line 202:                         self.wfile.write(json_response)
Line 203:                         self.finish()
Line 204:                     else:
Line 205:                         self.send_response(406)
Line 206:                         self.finish()
If we use a timeout, we catch socket.timeout and handle it properly - we should not raise an exception in this case.

except socket.timeout:
        log error message (without exception)
        send error
Line 207:                 # shouldn't happen
Line 208:                 except Exception:
Line 209:                     self.log.error("execution exception",
Line 210:                                    exc_info=True)

Line 205:                         self.send_response(406)
Line 206:                         self.finish()
Line 207:                 # shouldn't happen
Line 208:                 except Exception:
Line 209:                     self.log.error("execution exception",
execution?
Line 210:                                    exc_info=True)
Line 211:                     try:
Line 212:                         self.send_response(500)
Line 213:                         self.addTaskIdHeaderIfPresent()

Line 208:                 except Exception:
Line 209:                     self.log.error("execution exception",
Line 210:                                    exc_info=True)
Line 211:                     try:
Line 212:                         self.send_response(500)
Use constant
Line 213:                         self.addTaskIdHeaderIfPresent()
Line 214:                         self.end_headers()
Line 215:                         self.finish()
Line 216:                     except Exception:

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: introducing downloadImageFromStream
......................................................................

Patch Set 1:

(22 comments)

Partial review: imageSharing, commit message

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

Line 3: AuthorDate: 2014-01-12 13:52:24 +0200
Line 4: Commit:     Liron Aravot 
Line 5: CommitDate: 2014-01-15 16:43:38 +0200
Line 6: 
Line 7: introducing downloadImageFromStream
Introducing

I think this feature should have better title then the twisted "downloadFromStream". Download from stream does not make sense and is just a hack to get this working with minimal changes in the code.
Line 8: 
Line 9: This patch introduces the downloadImageFromStream verb in vdsm which
Line 10: allows to upload using streaming content to vdsm images.
Line 11: XML-RPC spec doesn't support streaming, as we don't want to use HTTP

Line 10: allows to upload using streaming content to vdsm images.
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.
Streaming content is not correct - no streaming done here. This is simply adding upload support without the size limits implied by xmlrpc.
Line 15: 
Line 16: General upload information:
Line 17: ---------------------------------------------------------------
Line 18: PUT requests arriving to the server with content type of

Line 13: port) we will threat some requests as http requests instead of xml-rpc
Line 14: requests.
Line 15: 
Line 16: General upload information:
Line 17: ---------------------------------------------------------------
A more useful formatting would be:

General upload information
 --------------------------

content...
Line 18: PUT requests arriving to the server with content type of
Line 19: application/octet-stream to default paths that we use
Line 20: today for request handling ('/', '/RPC2') will be threated as
Line 21: upload requests.

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
Line 20: today for request handling ('/', '/RPC2') will be threated as
Line 21: upload requests.
I think it would be more correct to limit the upload support to certain path, for example, "/storage".

Later we may want to extend this to download data directly from storage, and we would like to add GET verb. This will not work if we use the same path of xmlrpc handlers.

Upload to storage:

PUT /stoarage

Download from storage:

GET /storage
Line 22: 
Line 23: The PUT stream upload request inspected headers are:
Line 24: -- Mandatory headers:
Line 25:         *content-length

Line 27: -- Mandatory custom headers:
Line 28:         *X-Storage-Pool-Id : mandatory for succesfull execution
Line 29:         *X-Storage-Domain-Id: mandatory for succesfull execution
Line 30:         *X-Image-Id: mandatory for succesfull execution
Line 31:         *X-Volume-Id: mandatory for succesfull execution
Consider passing these in the request path:

PUT /storage/pool-id/domain-id/image-id/volume-id

GET /storage/pool-id/domain-id/image-id/volume-id

Processing these is quite simple:
> > > '/data/p/d/i/v'[len('/data/'):].split('/')

['p', 'd', 'i', 'v']
Line 32: 
Line 33: -- Optional custom headers;
Line 34:         *X-Buffer-Size: optional, indicates the passed buffer
Line 35:

Line 30:         *X-Image-Id: mandatory for succesfull execution
Line 31:         *X-Volume-Id: mandatory for succesfull execution
Line 32: 
Line 33: -- Optional custom headers;
Line 34:         *X-Buffer-Size: optional, indicates the passed buffer
This is pointless as we discussed.
Line 35: 
Line 36: The server will return answers with the following codes:
Line 37: 200 - request completed, the response body will contain the execution
Line 38: results, the returned content type is 'application/json'

Line 36: The server will return answers with the following codes:
Line 37: 200 - request completed, the response body will contain the execution
Line 38: results, the returned content type is 'application/json'
Line 39: 500 - internal error
Line 40: 406 - missing content-length header
This list is missing some values, but do not fix it - we don't need to describe these details here, it just creates more work to synchronize the implementation with this message.
Line 41: 
Line 42: in case that a task was created, its id will be passed in the response
Line 43: using the custom header 'X-Task-Id' for polling by the engine.
Line 44:

Line 59: 2. schedule a "dummy" task with "blank" function that will just be used to
Line 60: indicate that there's an downloadFromStream execution going on while the
Line 61: request thread will be used for executing the actual download.
Line 62: 
Line 63: Option 2 is implemented with this patch.
I like the dummy task hack.
Line 64: 
Line 65: needed:
Line 66: *socket timeout inspection
Line 67: *tests?

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

Line 161:                                              HEADER_CONTENT_TYPE))
Line 162:                 try:
Line 163:                     if ctype == 'application/octet-stream':
Line 164:                         contentLength = self.headers. \
Line 165:                             getheader(self.HEADER_CONTENT_LENGTH)
It makes more sense to convert content-length to integer here:

try:
      contentLength = int(contentLength)
  except ValueError:
      send 400 error...

Then we can pass the integer down into the application, and we don't have to add functions such as streamGetSize().
Line 166:                         if not contentLength:
Line 167:                             self.send_response(411)
Line 168:                             self.finish()
Line 169:                             return

Line 185:                             getheader(self.HEADER_CONTENT_LENGTH)
Line 186: 
Line 187:                         methodArgs = {'method': 'stream',
Line 188:                                       'fileObj': self.rfile,
Line 189:                                       'Content-Length': contentLength,
> Use constant

Lets *not* use http constants to pass data to our implementation. This will only make our life harder, having to share the constant, while we can use our shorter and easier to work with term.

Lets use here "size" or "length" key for the content-length.
Line 190:                                       'Buffer-Size': bufferSize}
Line 191:                         image = API.Image(imgUUID, spUUID, sdUUID)
Line 192:                         response = image.downloadFromStream(methodArgs,
Line 193:                                                             volUUID)

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

Line 48: 
Line 49:     return size
Line 50: 
Line 51: 
Line 52: def streamGetSize(methodArgs):
Unneeded
Line 53:     size = None
Line 54:     if 'Content-Length' in methodArgs:
Line 55:         size = int(methodArgs['Content-Length'])
Line 56:     return size

Line 59: def streamGetBufferSize(methodArgs):
Line 60:     size = None
Line 61:     if 'X-Buffer-Size' in methodArgs:
Line 62:         size = int(methodArgs['X-Buffer-Size'])
Line 63:     return size
Not needed
Line 64: 
Line 65: 
Line 66: def httpDownloadImage(dstImgPath, methodArgs):
Line 67:     curlImgWrap.download(methodArgs.get('url'), dstImgPath,

Line 73:                        methodArgs.get("headers", {}))
Line 74: 
Line 75: 
Line 76: def streamDownloadImage(dstImgPath, methodArgs):
Line 77:     bytes_left = streamGetSize(methodArgs)
This will not work as streamGetSize may return None, and byte_left -= bytes_read does not work with None.

What you want here is simply:

bytes_left = int(methodArsgs['Content-Length'])

It should never raise a key error since we never call it without this key, and if the value is not an int, we would rather die now.
Line 78:     buffer_size = streamGetBufferSize(methodArgs)
Line 79:     stream = methodArgs.get('fileObj')
Line 80: 
Line 81:     BS = constants.MEGAB

Line 78:     buffer_size = streamGetBufferSize(methodArgs)
Line 79:     stream = methodArgs.get('fileObj')
Line 80: 
Line 81:     BS = constants.MEGAB
Line 82:     count = bytes_left / BS
count is wrong, since bytes_left is not divisible by BS. For example, if we get 234 bytes, count will be 0 and we will write no data.

This dd should copy all data it receive until you close the pipe, writing in blocks of 1MB. I guess that not specifying a count wold do what you want, please check.
Line 83:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" %
Line 84:            constants.MEGAB, "count=%d" % count]
Line 85:     p = utils.execCmd(cmd, sudo=False, sync=False)
Line 86:

Line 80: 
Line 81:     BS = constants.MEGAB
Line 82:     count = bytes_left / BS
Line 83:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" %
Line 84:            constants.MEGAB, "count=%d" % count]
Check if more dd flags are needed for writing to storage - check how copyImage is implemented.
Line 85:     p = utils.execCmd(cmd, sudo=False, sync=False)
Line 86: 
Line 87:     default_read_size = buffer_size
Line 88:     if default_read_size is None:

Line 81:     BS = constants.MEGAB
Line 82:     count = bytes_left / BS
Line 83:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" %
Line 84:            constants.MEGAB, "count=%d" % count]
Line 85:     p = utils.execCmd(cmd, sudo=False, sync=False)
Rest of this must be in a try-except block, to ensure that dd is killed on errors:

create process...
 try:
     copy data...
 except Exception:
     kill process
     raise
Line 86: 
Line 87:     default_read_size = buffer_size
Line 88:     if default_read_size is None:
Line 89:         default_read_size = BS

Line 83:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" %
Line 84:            constants.MEGAB, "count=%d" % count]
Line 85:     p = utils.execCmd(cmd, sudo=False, sync=False)
Line 86: 
Line 87:     default_read_size = buffer_size
Use constant
Line 88:     if default_read_size is None:
Line 89:         default_read_size = BS
Line 90: 
Line 91:     while bytes_left > 0:

Line 85:     p = utils.execCmd(cmd, sudo=False, sync=False)
Line 86: 
Line 87:     default_read_size = buffer_size
Line 88:     if default_read_size is None:
Line 89:         default_read_size = BS
There is no point to read from a socket and write to a pipe in 1MB chunks, unless you increased the socket and pipe buffers to similar size. Even if you do this, this is not effective usage of your memory. So using this value is just misleading the reader.

I guess that 64KB would be a nice start value for this task.
Line 90: 
Line 91:     while bytes_left > 0:
Line 92:         i = min(default_read_size, bytes_left)
Line 93:         data = stream.read(i)

Line 88:     if default_read_size is None:
Line 89:         default_read_size = BS
Line 90: 
Line 91:     while bytes_left > 0:
Line 92:         i = min(default_read_size, bytes_left)
i is not a good name for a size variable in this context. People expect i to be an iteration variable, increasing or decreasing on each iteration.

How about bytes_to_read?
Line 93:         data = stream.read(i)
Line 94:         if not data:
Line 95:             break
Line 96:         p.stdin.write(data)

Line 91:     while bytes_left > 0:
Line 92:         i = min(default_read_size, bytes_left)
Line 93:         data = stream.read(i)
Line 94:         if not data:
Line 95:             break
You must kill the process here. Also, this unexpected situation, so you should better raise an exception here.
Line 96:         p.stdin.write(data)
Line 97:         p.stdin.flush()
Line 98:         bytes_left = bytes_left - i
Line 99:

Line 93:         data = stream.read(i)
Line 94:         if not data:
Line 95:             break
Line 96:         p.stdin.write(data)
Line 97:         p.stdin.flush()
Maybe put a note here so we remember to check if flush is needed. In a perfect world, the file object keeps a limited buffer size and automatically flush the buffer when the buffer is full.
Line 98:         bytes_left = bytes_left - i
Line 99: 
Line 100:     p.stdin.close()
Line 101:     p.wait()

Line 94:         if not data:
Line 95:             break
Line 96:         p.stdin.write(data)
Line 97:         p.stdin.flush()
Line 98:         bytes_left = bytes_left - i
You must assume partial reads:

bytes_left -= len(data)
Line 99: 
Line 100:     p.stdin.close()
Line 101:     p.wait()
Line 102:     if p.returncode != 0:

Liron Ar has abandoned this change.

Change subject: introducing downloadImageFromStream
......................................................................

Abandoned

making related changes, will return.

Liron Ar has posted comments on this change.

Change subject: introducing downloadImageFromStream
......................................................................

Patch Set 1:

(36 comments)

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

Line 10: allows to upload using streaming content to vdsm images.
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.
> Streaming content is not correct - no streaming done here. This is simply a

it's not related to the size  but to the fact that we don't want to have all file passed at once/in memory - therefore the term streaming is correct imo. if you have better suggestion it'll be great.
Line 15: 
Line 16: General upload information:
Line 17: ---------------------------------------------------------------
Line 18: PUT requests arriving to the server with content type of

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
Line 20: today for request handling ('/', '/RPC2') will be threated as
Line 21: upload requests.
> I think it would be more correct to limit the upload support to certain pat

I don't really agree about that.
having a special path for it means that when executing other kind of requests we should block this path - let's say that we add /upload - we need now to add hanling on post methods to that path.
Line 22: 
Line 23: The PUT stream upload request inspected headers are:
Line 24: -- Mandatory headers:
Line 25:         *content-length

Line 27: -- Mandatory custom headers:
Line 28:         *X-Storage-Pool-Id : mandatory for succesfull execution
Line 29:         *X-Storage-Domain-Id: mandatory for succesfull execution
Line 30:         *X-Image-Id: mandatory for succesfull execution
Line 31:         *X-Volume-Id: mandatory for succesfull execution
> Consider passing these in the request path:

i'll stick with the headers, the supported paths are defined per all the handler instances and are verified when processing the requests. there's no need to add extra noise while the headers method seems better and simpler to me and we already use it.
Line 32: 
Line 33: -- Optional custom headers;
Line 34:         *X-Buffer-Size: optional, indicates the passed buffer
Line 35:

Line 30:         *X-Image-Id: mandatory for succesfull execution
Line 31:         *X-Volume-Id: mandatory for succesfull execution
Line 32: 
Line 33: -- Optional custom headers;
Line 34:         *X-Buffer-Size: optional, indicates the passed buffer
> This is pointless as we discussed.

Done
Line 35: 
Line 36: The server will return answers with the following codes:
Line 37: 200 - request completed, the response body will contain the execution
Line 38: results, the returned content type is 'application/json'

Line 36: The server will return answers with the following codes:
Line 37: 200 - request completed, the response body will contain the execution
Line 38: results, the returned content type is 'application/json'
Line 39: 500 - internal error
Line 40: 406 - missing content-length header
> This list is missing some values, but do not fix it - we don't need to desc

Done
Line 41: 
Line 42: in case that a task was created, its id will be passed in the response
Line 43: using the custom header 'X-Task-Id' for polling by the engine.
Line 44:

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

Line 131:         else:
Line 132:             basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler
Line 133: 
Line 134:         class RequestHandler(LoggingMixIn, basehandler):
Line 135:
> For timeout, you can put here a class variable:

the correct place for this is in the server and not in the handler.
Line 136:             log = logging.getLogger("BindingXMLRPC.RequestHandler")
Line 137:             CUSTOM_HEADER_POOL = 'X-Storage-Pool-Id'
Line 138:             CUSTOM_HEADER_DOMAIN = 'X-Storage-Domain-Id'
Line 139:             CUSTOM_HEADER_IMAGE = 'X-Image-Id'

Line 132:             basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler
Line 133: 
Line 134:         class RequestHandler(LoggingMixIn, basehandler):
Line 135: 
Line 136:             log = logging.getLogger("BindingXMLRPC.RequestHandler")
> It would be nice to separate constants from the log.

Done
Line 137:             CUSTOM_HEADER_POOL = 'X-Storage-Pool-Id'
Line 138:             CUSTOM_HEADER_DOMAIN = 'X-Storage-Domain-Id'
Line 139:             CUSTOM_HEADER_IMAGE = 'X-Image-Id'
Line 140:             CUSTOM_HEADER_VOLUME = 'X-Volume-Id'

Line 133: 
Line 134:         class RequestHandler(LoggingMixIn, basehandler):
Line 135: 
Line 136:             log = logging.getLogger("BindingXMLRPC.RequestHandler")
Line 137:             CUSTOM_HEADER_POOL = 'X-Storage-Pool-Id'
> CUSTOM_HEADER_ prefix is not needed and only make the constants too long, l

I removed the "custom" part, i prefer to keep the header in the name to indicate that those are headers.
Line 138:             CUSTOM_HEADER_DOMAIN = 'X-Storage-Domain-Id'
Line 139:             CUSTOM_HEADER_IMAGE = 'X-Image-Id'
Line 140:             CUSTOM_HEADER_VOLUME = 'X-Volume-Id'
Line 141:             CUSTOM_HEADER_BUFFER = 'X-Buffer-Size'

Line 137:             CUSTOM_HEADER_POOL = 'X-Storage-Pool-Id'
Line 138:             CUSTOM_HEADER_DOMAIN = 'X-Storage-Domain-Id'
Line 139:             CUSTOM_HEADER_IMAGE = 'X-Image-Id'
Line 140:             CUSTOM_HEADER_VOLUME = 'X-Volume-Id'
Line 141:             CUSTOM_HEADER_BUFFER = 'X-Buffer-Size'
> Not needed as we discussed

Done
Line 142:             CUSTOM_HEADER_TASK_ID = 'X-Task-Id'
Line 143:             HEADER_CONTENT_LENGTH = 'content-length'
Line 144:             HEADER_CONTENT_TYPE = 'content-type'
Line 145:             RESPONSE_LENGTH_REQUIRED_ = 411

Line 144:             HEADER_CONTENT_TYPE = 'content-type'
Line 145:             RESPONSE_LENGTH_REQUIRED_ = 411
Line 146:             RESPONSE_NOT_ACCEPTABLE = 406
Line 147:             RESPONSE_REQUEST_SUCCEEDED = 200
Line 148:             RESPONSE_INTERNAL_ERROR = 500
> For http status codes, please use httplib:

Done
Line 149: 
Line 150:             def setup(self):
Line 151:                 threadLocal.client = self.client_address[0]
Line 152:                 return basehandler.setup(self)

Line 150:             def setup(self):
Line 151:                 threadLocal.client = self.client_address[0]
Line 152:                 return basehandler.setup(self)
Line 153: 
Line 154:             def addTaskIdHeaderIfPresent(self):
> This should be sendTaskHeaderIfPresent, since it sends a header.

Done
Line 155:                 if hasattr(vars, 'task'):
Line 156:                     self.send_header(self.CUSTOM_HEADER_TASK_ID,
Line 157:                                      vars.task.getID())
Line 158:

Line 157:                                      vars.task.getID())
Line 158: 
Line 159:             def do_PUT(self):
Line 160:                 ctype, pt = cgi.parse_header(self.headers.getheader(self.
Line 161:                                              HEADER_CONTENT_TYPE))
> Why this check is out of the try-except block?

Done
Line 162:                 try:
Line 163:                     if ctype == 'application/octet-stream':
Line 164:                         contentLength = self.headers. \
Line 165:                             getheader(self.HEADER_CONTENT_LENGTH)

Line 159:             def do_PUT(self):
Line 160:                 ctype, pt = cgi.parse_header(self.headers.getheader(self.
Line 161:                                              HEADER_CONTENT_TYPE))
Line 162:                 try:
Line 163:                     if ctype == 'application/octet-stream':
> The code would be more clear if we bail out as soon as we find an error:

Done
Line 164:                         contentLength = self.headers. \
Line 165:                             getheader(self.HEADER_CONTENT_LENGTH)
Line 166:                         if not contentLength:
Line 167:                             self.send_response(411)

Line 161:                                              HEADER_CONTENT_TYPE))
Line 162:                 try:
Line 163:                     if ctype == 'application/octet-stream':
Line 164:                         contentLength = self.headers. \
Line 165:                             getheader(self.HEADER_CONTENT_LENGTH)
> It makes more sense to convert content-length to integer here:

Done
Line 166:                         if not contentLength:
Line 167:                             self.send_response(411)
Line 168:                             self.finish()
Line 169:                             return

Line 163:                     if ctype == 'application/octet-stream':
Line 164:                         contentLength = self.headers. \
Line 165:                             getheader(self.HEADER_CONTENT_LENGTH)
Line 166:                         if not contentLength:
Line 167:                             self.send_response(411)
> Use constant

Done
Line 168:                             self.finish()
Line 169:                             return
Line 170: 
Line 171:                         # NEED TO DECIDE ABOUT THOSE

Line 164:                         contentLength = self.headers. \
Line 165:                             getheader(self.HEADER_CONTENT_LENGTH)
Line 166:                         if not contentLength:
Line 167:                             self.send_response(411)
Line 168:                             self.finish()
> This pattern:

I don't think that its worth extracting to a method
Line 169:                             return
Line 170: 
Line 171:                         # NEED TO DECIDE ABOUT THOSE
Line 172:                         #self.rfile._sock.settimeout(5)

Line 169:                             return
Line 170: 
Line 171:                         # NEED TO DECIDE ABOUT THOSE
Line 172:                         #self.rfile._sock.settimeout(5)
Line 173:                         #self.rfile._sock.set_socket_read_timeout
> We don't need it as we discussed, timeout class variable will handle this.

replied before, the correct place for this is in the server and not in the handler.
as we agreed f2f, timeout will be added to the server generally and not only for this usecase.
Line 174:                         spUUID = self.headers. \
Line 175:                             getheader(self.CUSTOM_HEADER_POOL)
Line 176:                         sdUUID = self.headers. \
Line 177:                             getheader(self.CUSTOM_HEADER_DOMAIN)

Line 171:                         # NEED TO DECIDE ABOUT THOSE
Line 172:                         #self.rfile._sock.settimeout(5)
Line 173:                         #self.rfile._sock.set_socket_read_timeout
Line 174:                         spUUID = self.headers. \
Line 175:                             getheader(self.CUSTOM_HEADER_POOL)
> This hard to read lines are caused by too much indentation and too long con

Done
Line 176:                         sdUUID = self.headers. \
Line 177:                             getheader(self.CUSTOM_HEADER_DOMAIN)
Line 178:                         imgUUID = self.headers. \
Line 179:                             getheader(self.CUSTOM_HEADER_IMAGE)

Line 185:                             getheader(self.HEADER_CONTENT_LENGTH)
Line 186: 
Line 187:                         methodArgs = {'method': 'stream',
Line 188:                                       'fileObj': self.rfile,
Line 189:                                       'Content-Length': contentLength,
> Use constant

I prefer to keep it as content-length, that's already the terminology that we use there.
Line 190:                                       'Buffer-Size': bufferSize}
Line 191:                         image = API.Image(imgUUID, spUUID, sdUUID)
Line 192:                         response = image.downloadFromStream(methodArgs,
Line 193:                                                             volUUID)

Line 186: 
Line 187:                         methodArgs = {'method': 'stream',
Line 188:                                       'fileObj': self.rfile,
Line 189:                                       'Content-Length': contentLength,
Line 190:                                       'Buffer-Size': bufferSize}
> Not needed

Done
Line 191:                         image = API.Image(imgUUID, spUUID, sdUUID)
Line 192:                         response = image.downloadFromStream(methodArgs,
Line 193:                                                             volUUID)
Line 194:                         json_response = json.dumps(response)

Line 191:                         image = API.Image(imgUUID, spUUID, sdUUID)
Line 192:                         response = image.downloadFromStream(methodArgs,
Line 193:                                                             volUUID)
Line 194:                         json_response = json.dumps(response)
Line 195:                         self.send_response(200)
> Use constant

Done
Line 196:                         self.send_header(self.HEADER_CONTENT_TYPE,
Line 197:                                          'application/json')
Line 198:                         self.send_header(self.HEADER_CONTENT_LENGTH,
Line 199:                                          len(json_response))

Line 201:                         self.end_headers()
Line 202:                         self.wfile.write(json_response)
Line 203:                         self.finish()
Line 204:                     else:
Line 205:                         self.send_response(406)
> This status code is not correct, it should be 415 Unsupported media type:

Done
Line 206:                         self.finish()
Line 207:                 # shouldn't happen
Line 208:                 except Exception:
Line 209:                     self.log.error("execution exception",

Line 202:                         self.wfile.write(json_response)
Line 203:                         self.finish()
Line 204:                     else:
Line 205:                         self.send_response(406)
Line 206:                         self.finish()
> If we use a timeout, we catch socket.timeout and handle it properly - we sh

i don't think that it's needed, we log the exc_info.
what we could do here is to re-raise to have the server default handling for it..but it doesn't seem to make a real diff from current handling.
Line 207:                 # shouldn't happen
Line 208:                 except Exception:
Line 209:                     self.log.error("execution exception",
Line 210:                                    exc_info=True)

Line 205:                         self.send_response(406)
Line 206:                         self.finish()
Line 207:                 # shouldn't happen
Line 208:                 except Exception:
Line 209:                     self.log.error("execution exception",
> execution?

changed to unexpected error during execution
Line 210:                                    exc_info=True)
Line 211:                     try:
Line 212:                         self.send_response(500)
Line 213:                         self.addTaskIdHeaderIfPresent()

Line 208:                 except Exception:
Line 209:                     self.log.error("execution exception",
Line 210:                                    exc_info=True)
Line 211:                     try:
Line 212:                         self.send_response(500)
> Use constant

Done
Line 213:                         self.addTaskIdHeaderIfPresent()
Line 214:                         self.end_headers()
Line 215:                         self.finish()
Line 216:                     except Exception:

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

Line 48: 
Line 49:     return size
Line 50: 
Line 51: 
Line 52: def streamGetSize(methodArgs):
> Unneeded

i prefer to keep the convenetion here, see 	_METHOD_IMPLEMENTATIONS and therefore i leave this method, it's actual implementation is other issue..but if we already define that there is "size" method, i want to implement it.
Line 53:     size = None
Line 54:     if 'Content-Length' in methodArgs:
Line 55:         size = int(methodArgs['Content-Length'])
Line 56:     return size

Line 59: def streamGetBufferSize(methodArgs):
Line 60:     size = None
Line 61:     if 'X-Buffer-Size' in methodArgs:
Line 62:         size = int(methodArgs['X-Buffer-Size'])
Line 63:     return size
> Not needed

Done
Line 64: 
Line 65: 
Line 66: def httpDownloadImage(dstImgPath, methodArgs):
Line 67:     curlImgWrap.download(methodArgs.get('url'), dstImgPath,

Line 73:                        methodArgs.get("headers", {}))
Line 74: 
Line 75: 
Line 76: def streamDownloadImage(dstImgPath, methodArgs):
Line 77:     bytes_left = streamGetSize(methodArgs)
> This will not work as streamGetSize may return None, and byte_left -= bytes

Right, it's a leftover from when this verb supported unspecified content length, revmoing.
Line 78:     buffer_size = streamGetBufferSize(methodArgs)
Line 79:     stream = methodArgs.get('fileObj')
Line 80: 
Line 81:     BS = constants.MEGAB

Line 78:     buffer_size = streamGetBufferSize(methodArgs)
Line 79:     stream = methodArgs.get('fileObj')
Line 80: 
Line 81:     BS = constants.MEGAB
Line 82:     count = bytes_left / BS
> count is wrong, since bytes_left is not divisible by BS. For example, if we

it's a copy copy-paste from other invocation that we have..i just removed the count.
Line 83:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" %
Line 84:            constants.MEGAB, "count=%d" % count]
Line 85:     p = utils.execCmd(cmd, sudo=False, sync=False)
Line 86:

Line 80: 
Line 81:     BS = constants.MEGAB
Line 82:     count = bytes_left / BS
Line 83:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" %
Line 84:            constants.MEGAB, "count=%d" % count]
> Check if more dd flags are needed for writing to storage - check how copyIm

will be checked.
Line 85:     p = utils.execCmd(cmd, sudo=False, sync=False)
Line 86: 
Line 87:     default_read_size = buffer_size
Line 88:     if default_read_size is None:

Line 81:     BS = constants.MEGAB
Line 82:     count = bytes_left / BS
Line 83:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" %
Line 84:            constants.MEGAB, "count=%d" % count]
Line 85:     p = utils.execCmd(cmd, sudo=False, sync=False)
> Rest of this must be in a try-except block, to ensure that dd is killed on

Done
Line 86: 
Line 87:     default_read_size = buffer_size
Line 88:     if default_read_size is None:
Line 89:         default_read_size = BS

Line 85:     p = utils.execCmd(cmd, sudo=False, sync=False)
Line 86: 
Line 87:     default_read_size = buffer_size
Line 88:     if default_read_size is None:
Line 89:         default_read_size = BS
> There is no point to read from a socket and write to a pipe in 1MB chunks,

in the python documentation it's written:
"For best match with hardware and network realities, the value of bufsize should be a relatively small power of 2, for example, 4096."

we can start with the value.
Line 90: 
Line 91:     while bytes_left > 0:
Line 92:         i = min(default_read_size, bytes_left)
Line 93:         data = stream.read(i)

Line 88:     if default_read_size is None:
Line 89:         default_read_size = BS
Line 90: 
Line 91:     while bytes_left > 0:
Line 92:         i = min(default_read_size, bytes_left)
> i is not a good name for a size variable in this context. People expect i t

Done
Line 93:         data = stream.read(i)
Line 94:         if not data:
Line 95:             break
Line 96:         p.stdin.write(data)

Line 91:     while bytes_left > 0:
Line 92:         i = min(default_read_size, bytes_left)
Line 93:         data = stream.read(i)
Line 94:         if not data:
Line 95:             break
> You must kill the process here. Also, this unexpected situation, so you sho

Done
Line 96:         p.stdin.write(data)
Line 97:         p.stdin.flush()
Line 98:         bytes_left = bytes_left - i
Line 99:

Line 93:         data = stream.read(i)
Line 94:         if not data:
Line 95:             break
Line 96:         p.stdin.write(data)
Line 97:         p.stdin.flush()
> Maybe put a note here so we remember to check if flush is needed. In a perf

I don't think that this note will somehow help, no real harm in having that here.
Line 98:         bytes_left = bytes_left - i
Line 99: 
Line 100:     p.stdin.close()
Line 101:     p.wait()

Line 94:         if not data:
Line 95:             break
Line 96:         p.stdin.write(data)
Line 97:         p.stdin.flush()
Line 98:         bytes_left = bytes_left - i
> You must assume partial reads:

Done
Line 99: 
Line 100:     p.stdin.close()
Line 101:     p.wait()
Line 102:     if p.returncode != 0:

Liron Ar has restored this change.

Change subject: introducing downloadImageFromStream
......................................................................

Restored

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: introducing downloadImageFromStream
......................................................................

Patch Set 2: Code-Review-1 Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5957/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6745/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6851/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests/28/ : FAILURE

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: introducing downloadImageFromStream
......................................................................

Patch Set 3: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5958/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6746/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6852/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests/29/ : FAILURE

Change in vdsm[master]: introducing downloadFromStream verb

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 4: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5980/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6768/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6874/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests/35/ : FAILURE

Change in vdsm[master]: introducing downloadFromStream verb

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 4:

(17 comments)

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

Line 6: 
Line 7: introducing downloadFromStream verb
Line 8: 
Line 9: This patch introduces the downloadFromStream verb in vdsm which
Line 10: allows to upload using streaming content to vdsm images.
"upload using streaming content" is not clear, and does not explain that you can upload data without size limit.
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 7: introducing downloadFromStream verb
Line 8: 
Line 9: This patch introduces the downloadFromStream verb in vdsm which
Line 10: allows to upload using streaming content to vdsm images.
Line 11: XML-RPC spec doesn't support streaming, as we don't want to use HTTP
Again "XML-RPC spec doesn't support streaming" is not clear, and does not explain that xml-rpc has limited payload size, so it cannot be used to upload big files.
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:

Line 9: This patch introduces the downloadFromStream verb in vdsm which
Line 10: allows to upload using streaming content to vdsm images.
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
Replace "threat" with "treat"
Line 14: requests.
Line 15: 
Line 16: General upload information:
Line 17: ---------------------------------------------------------------

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
Line 20: today for request handling ('/', '/RPC2') will be threated as
Replace "threated" with "treated"
Line 21: upload requests.
Line 22: 
Line 23: The PUT stream upload request inspected headers are:
Line 24: -- Mandatory headers:

Line 19: application/octet-stream to default paths that we use
Line 20: today for request handling ('/', '/RPC2') will be threated as
Line 21: upload requests.
Line 22: 
Line 23: The PUT stream upload request inspected headers are:
The PUT request inspected headers are:
Line 24: -- Mandatory headers:
Line 25:         *content-length
Line 26: 
Line 27: -- Mandatory custom headers:

http://gerrit.ovirt.org/#/c/23281/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 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/4/vdsm/BindingXMLRPC.py
File vdsm/BindingXMLRPC.py:

Line 27: from vdsm import SecureXMLRPCServer
Line 28: import logging
Line 29: import libvirt
Line 30: import threading
Line 31: import socket
Note for later patch: separate vdsm and Python imports and sort imports.
Line 32: 
Line 33: from vdsm import constants
Line 34: from vdsm import utils
Line 35: from vdsm.define import doneCode, errCode

Line 43:     _glusterEnabled = False
Line 44: 
Line 45: 
Line 46: class BindingXMLRPC(object):
Line 47: 
Not related but lets keep this.
Line 48:     def __init__(self, cif, log, ip, port, ssl, vds_resp_timeout,
Line 49:                  trust_store_path, default_bridge):
Line 50:         self.cif = cif
Line 51:         self.log = log

Line 132:             basehandler = SecureXMLRPCServer.SecureXMLRPCRequestHandler
Line 133:         else:
Line 134:             basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler
Line 135: 
Line 136:         class RequestHandler(LoggingMixIn, basehandler):
Empty line bellow the class would be nice.
Line 137:             # timeout for the request socket
Line 138:             timeout = 60
Line 139:             log = logging.getLogger("BindingXMLRPC.RequestHandler")
Line 140:

Line 134:             basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler
Line 135: 
Line 136:         class RequestHandler(LoggingMixIn, basehandler):
Line 137:             # timeout for the request socket
Line 138:             timeout = 60
Check that this timeout is good for xmlrpc requests.
Line 139:             log = logging.getLogger("BindingXMLRPC.RequestHandler")
Line 140: 
Line 141:             HEADER_POOL = 'X-Storage-Pool-Id'
Line 142:             HEADER_DOMAIN = 'X-Storage-Domain-Id'

Line 151: 
Line 152:             def do_PUT(self):
Line 153:                 try:
Line 154:                     ctype, pt = cgi.parse_header(self.headers.getheader(self.
Line 155:                                                  HEADER_CONTENT_TYPE))
Indenting the next line after break at the first "(" does not look good in this case, and *is not* required by pep8. In such case you can indent like this (verified with pep8):

ctype, pt = cgi.parse_header(self.headers.getheader(
        self.HEADER_CONTENT_TYPE))

Or you can use multiple lines - usually easier to read:

ct = self.headers.getheader(self.HEADER_CONTENT_TYPE)
    ctype, pt = cgi.parse_header(ct)
Line 156:                     if ctype != 'application/octet-stream':
Line 157:                         self.send_error(httplib.INTERNAL_SERVER_ERROR,
Line 158:                                         "unsupported content type: %r" % ctype)
Line 159:                         return

Line 153:                 try:
Line 154:                     ctype, pt = cgi.parse_header(self.headers.getheader(self.
Line 155:                                                  HEADER_CONTENT_TYPE))
Line 156:                     if ctype != 'application/octet-stream':
Line 157:                         self.send_error(httplib.INTERNAL_SERVER_ERROR,
This is a user error (4XX), not a server error (5XX). I think you use http.UNSUPPORTED_MEDIA_TYPE in a previous patch set.
Line 158:                                         "unsupported content type: %r" % ctype)
Line 159:                         return
Line 160: 
Line 161:                     contentLength = self.headers. \

Line 158:                                         "unsupported content type: %r" % ctype)
Line 159:                         return
Line 160: 
Line 161:                     contentLength = self.headers. \
Line 162:                         getheader(self.HEADER_CONTENT_LENGTH)
Breaking lines using '' is considered bad style. pep8 wrongly let you do this.

You can do this (verified with pep8):

contentLength = self.headers.getheader(
        self.HEADER_CONTENT_LENGTH)
Line 163:                     if not contentLength:
Line 164:                         self.send_error(httplib.LENGTH_REQUIRED,
Line 165:                                         "missing content length")
Line 166:                         return

Line 194:                     self.wfile.write(json_response)
Line 195:                     self.finish()
Line 196: 
Line 197:                 except socket.timeout:
Line 198:                     self.send_error(httplib.INTERNAL_SERVER_ERROR)
This is also not a server error - not sure what the proper http status code for this.

And there is a missing "message" argument here, which will fail in runtime with TypeError.
Line 199: 
Line 200:                 except Exception:
Line 201:                     self.send_error(httplib.INTERNAL_SERVER_ERROR,
Line 202:                                     "error during execution", exc_info=True)

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

Line 1675:                           methodArgs, sdUUID, imgUUID, volUUID)
Line 1676: 
Line 1677:     @public
Line 1678:     def downloadImageFromStream(self, methodArgs, spUUID, sdUUID, imgUUID,
Line 1679:                                 volUUID=None):
Same issue with the name - should match name used in API.Image class.
Line 1680:         """
Line 1681:         Download an image from a stream synchronously.
Line 1682: 
Line 1683:         Warning: Internal use only.

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

Line 28: 
Line 29: 
Line 30: log = logging.getLogger("Storage.ImageSharing")
Line 31: WAIT_TIMEOUT = 30
Line 32: BUFFER_READ_SIZE = 65536
This is also the write size, so better call it BUFFER_SIZE.
Line 33: 
Line 34: 
Line 35: def httpGetSize(methodArgs):
Line 36:     headers = curlImgWrap.head(methodArgs.get('url'),

Line 90:                           total_size - bytes_left, total_size)
Line 91:                 raise se.MiscFileReadException()
Line 92: 
Line 93:             p.stdin.write(data)
Line 94:             p.stdin.flush()
A comment explaining why we flush here would be nice.
Line 95:             bytes_left = bytes_left - len(data)
Line 96: 
Line 97:         p.stdin.close()
Line 98:         if not p.wait(WAIT_TIMEOUT):

Change in vdsm[master]: introducing downloadFromStream verb

Liron Ar has posted comments on this change.

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

Patch Set 4:

(15 comments)

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

Line 6: 
Line 7: introducing downloadFromStream verb
Line 8: 
Line 9: This patch introduces the downloadFromStream verb in vdsm which
Line 10: allows to upload using streaming content to vdsm images.
> "upload using streaming content" is not clear, and does not explain that yo

please phrase however you prefer it and i'll replace it.
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 7: introducing downloadFromStream verb
Line 8: 
Line 9: This patch introduces the downloadFromStream verb in vdsm which
Line 10: allows to upload using streaming content to vdsm images.
Line 11: XML-RPC spec doesn't support streaming, as we don't want to use HTTP
> Again "XML-RPC spec doesn't support streaming" is not clear, and does not e

It's not related to XML-RPC payload limit, it relates that we want to stream rather then send all the payload in the request rather then xml-rpc payload size limits.
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:

Line 9: This patch introduces the downloadFromStream verb in vdsm which
Line 10: allows to upload using streaming content to vdsm images.
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
> Replace "threat" with "treat"

Done
Line 14: requests.
Line 15: 
Line 16: General upload information:
Line 17: ---------------------------------------------------------------

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
Line 20: today for request handling ('/', '/RPC2') will be threated as
> Replace "threated" with "treated"

Done
Line 21: upload requests.
Line 22: 
Line 23: The PUT stream upload request inspected headers are:
Line 24: -- Mandatory headers:

Line 19: application/octet-stream to default paths that we use
Line 20: today for request handling ('/', '/RPC2') will be threated as
Line 21: upload requests.
Line 22: 
Line 23: The PUT stream upload request inspected headers are:
> The PUT request inspected headers are:

Done
Line 24: -- Mandatory headers:
Line 25:         *content-length
Line 26: 
Line 27: -- Mandatory custom headers:

http://gerrit.ovirt.org/#/c/23281/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 name is confusing:

i'm not really in favour of dump/write. let's wait for more name suggestions and pick one.
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/4/vdsm/BindingXMLRPC.py
File vdsm/BindingXMLRPC.py:

Line 132:             basehandler = SecureXMLRPCServer.SecureXMLRPCRequestHandler
Line 133:         else:
Line 134:             basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler
Line 135: 
Line 136:         class RequestHandler(LoggingMixIn, basehandler):
> Empty line bellow the class would be nice.

Done
Line 137:             # timeout for the request socket
Line 138:             timeout = 60
Line 139:             log = logging.getLogger("BindingXMLRPC.RequestHandler")
Line 140:

Line 134:             basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler
Line 135: 
Line 136:         class RequestHandler(LoggingMixIn, basehandler):
Line 137:             # timeout for the request socket
Line 138:             timeout = 60
> Check that this timeout is good for xmlrpc requests.

any other check then execute xml-rpc requests?

In the meanwhile let's go with that for all handlers and i'll test normal workflows between engine-vdsm.

Another option would be to set the timeout for the socket only  for the upload  as i suggested in the first patchset-
#self.rfile._sock.settimeout(5)

and then in later patch we can apply it the handler so it will be for all requests.
Line 139:             log = logging.getLogger("BindingXMLRPC.RequestHandler")
Line 140: 
Line 141:             HEADER_POOL = 'X-Storage-Pool-Id'
Line 142:             HEADER_DOMAIN = 'X-Storage-Domain-Id'

Line 151: 
Line 152:             def do_PUT(self):
Line 153:                 try:
Line 154:                     ctype, pt = cgi.parse_header(self.headers.getheader(self.
Line 155:                                                  HEADER_CONTENT_TYPE))
> Indenting the next line after break at the first "(" does not look good in

Done
Line 156:                     if ctype != 'application/octet-stream':
Line 157:                         self.send_error(httplib.INTERNAL_SERVER_ERROR,
Line 158:                                         "unsupported content type: %r" % ctype)
Line 159:                         return

Line 153:                 try:
Line 154:                     ctype, pt = cgi.parse_header(self.headers.getheader(self.
Line 155:                                                  HEADER_CONTENT_TYPE))
Line 156:                     if ctype != 'application/octet-stream':
Line 157:                         self.send_error(httplib.INTERNAL_SERVER_ERROR,
> This is a user error (4XX), not a server error (5XX). I think you use http.

returned, copy mistake.
Line 158:                                         "unsupported content type: %r" % ctype)
Line 159:                         return
Line 160: 
Line 161:                     contentLength = self.headers. \

Line 158:                                         "unsupported content type: %r" % ctype)
Line 159:                         return
Line 160: 
Line 161:                     contentLength = self.headers. \
Line 162:                         getheader(self.HEADER_CONTENT_LENGTH)
> Breaking lines using '' is considered bad style. pep8 wrongly let you do t

Done
Line 163:                     if not contentLength:
Line 164:                         self.send_error(httplib.LENGTH_REQUIRED,
Line 165:                                         "missing content length")
Line 166:                         return

Line 194:                     self.wfile.write(json_response)
Line 195:                     self.finish()
Line 196: 
Line 197:                 except socket.timeout:
Line 198:                     self.send_error(httplib.INTERNAL_SERVER_ERROR)
> This is also not a server error - not sure what the proper http status code

1. the proper one seems to be REQUEST_TIMEOUT
2. Done
Line 199: 
Line 200:                 except Exception:
Line 201:                     self.send_error(httplib.INTERNAL_SERVER_ERROR,
Line 202:                                     "error during execution", exc_info=True)

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

Line 1675:                           methodArgs, sdUUID, imgUUID, volUUID)
Line 1676: 
Line 1677:     @public
Line 1678:     def downloadImageFromStream(self, methodArgs, spUUID, sdUUID, imgUUID,
Line 1679:                                 volUUID=None):
> Same issue with the name - should match name used in API.Image class.

same, let's wait for better name suggestions - i really don't mind which one we'll pick
Line 1680:         """
Line 1681:         Download an image from a stream synchronously.
Line 1682: 
Line 1683:         Warning: Internal use only.

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

Line 28: 
Line 29: 
Line 30: log = logging.getLogger("Storage.ImageSharing")
Line 31: WAIT_TIMEOUT = 30
Line 32: BUFFER_READ_SIZE = 65536
> This is also the write size, so better call it BUFFER_SIZE.

I don't really agree - we never use it for writing, just for reading. the size of the write depends on what we received..but i don't mind. Done.
Line 33: 
Line 34: 
Line 35: def httpGetSize(methodArgs):
Line 36:     headers = curlImgWrap.head(methodArgs.get('url'),

Line 90:                           total_size - bytes_left, total_size)
Line 91:                 raise se.MiscFileReadException()
Line 92: 
Line 93:             p.stdin.write(data)
Line 94:             p.stdin.flush()
> A comment explaining why we flush here would be nice.

what comment would you suggest?
Line 95:             bytes_left = bytes_left - len(data)
Line 96: 
Line 97:         p.stdin.close()
Line 98:         if not p.wait(WAIT_TIMEOUT):

Change in vdsm[master]: introducing downloadFromStream verb

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 5: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6008/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6796/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6902/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests/43/ : FAILURE

Change in vdsm[master]: introducing downloadFromStream verb

[Nir Soffer]

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,

Change in vdsm[master]: introducing downloadFromStream verb

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 4:

(3 comments)

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

Line 7: introducing downloadFromStream verb
Line 8: 
Line 9: This patch introduces the downloadFromStream verb in vdsm which
Line 10: allows to upload using streaming content to vdsm images.
Line 11: XML-RPC spec doesn't support streaming, as we don't want to use HTTP
> It's not related to XML-RPC payload limit, it relates that we want to strea

We are sending the whole payload in the request, just like xml rpc. The difference is that xmlrpc payload is limited in size, but sending data using bare http is not.
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:

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

Line 28: 
Line 29: 
Line 30: log = logging.getLogger("Storage.ImageSharing")
Line 31: WAIT_TIMEOUT = 30
Line 32: BUFFER_READ_SIZE = 65536
> I don't really agree - we never use it for writing, just for reading. the s

What we read from the socket, we write to the pipe, don't we?
Line 33: 
Line 34: 
Line 35: def httpGetSize(methodArgs):
Line 36:     headers = curlImgWrap.head(methodArgs.get('url'),

Line 90:                           total_size - bytes_left, total_size)
Line 91:                 raise se.MiscFileReadException()
Line 92: 
Line 93:             p.stdin.write(data)
Line 94:             p.stdin.flush()
> what comment would you suggest?

This:

Process stdin is not a real file object but a wrapper using StringIO buffer. To ensure that we don't use more memory if we get data faster then dd read it from the pipe, we flush on every write. We can remove if we can limit the buffer size used by this stdin wrapper.
Line 95:             bytes_left = bytes_left - len(data)
Line 96: 
Line 97:         p.stdin.close()
Line 98:         if not p.wait(WAIT_TIMEOUT):

Change in vdsm[master]: introducing downloadFromStream verb

Antoni Segura Puimedon has posted comments on this change.

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

Patch Set 5: Code-Review-1

(3 comments)

Some minor nits.

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

Line 43:     _glusterEnabled = False
Line 44: 
Line 45: 
Line 46: class BindingXMLRPC(object):
Line 47: 
Adding this blank line is just noise in the git diff.
Line 48:     def __init__(self, cif, log, ip, port, ssl, vds_resp_timeout,
Line 49:                  trust_store_path, default_bridge):
Line 50:         self.cif = cif
Line 51:         self.log = log

Line 151:                 return basehandler.setup(self)
Line 152: 
Line 153:             def do_PUT(self):
Line 154:                 try:
Line 155:                     ctype, pt = cgi.parse_header(self.headers.getheader(
ctype(s) has a very specific python meaning that differs by leaps and bounds from the semantics of this variable. I'd very much recommend naming it contentType.
Line 156:                         self.HEADER_CONTENT_TYPE))
Line 157:                     if ctype != 'application/octet-stream':
Line 158:                         self.send_error(httplib.UNSUPPORTED_MEDIA_TYPE,
Line 159:                                         "unsupported content type: %r" % ctype)

Line 153:             def do_PUT(self):
Line 154:                 try:
Line 155:                     ctype, pt = cgi.parse_header(self.headers.getheader(
Line 156:                         self.HEADER_CONTENT_TYPE))
Line 157:                     if ctype != 'application/octet-stream':
this literal should probably be also a constant.
Line 158:                         self.send_error(httplib.UNSUPPORTED_MEDIA_TYPE,
Line 159:                                         "unsupported content type: %r" % ctype)
Line 160:                         return
Line 161:

Change in vdsm[master]: introducing downloadFromStream verb

Antoni Segura Puimedon has posted comments on this change.

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

Patch Set 5:

the -1 is just for visibility of my comments.

Change in vdsm[master]: introducing downloadFromStream verb

Liron Ar has posted comments on this change.

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

Patch Set 5:

(6 comments)

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:

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

Line 43:     _glusterEnabled = False
Line 44: 
Line 45: 
Line 46: class BindingXMLRPC(object):
Line 47:
> Adding this blank line is just noise in the git diff.

Done
Line 48:     def __init__(self, cif, log, ip, port, ssl, vds_resp_timeout,
Line 49:                  trust_store_path, default_bridge):
Line 50:         self.cif = cif
Line 51:         self.log = log

Line 151:                 return basehandler.setup(self)
Line 152: 
Line 153:             def do_PUT(self):
Line 154:                 try:
Line 155:                     ctype, pt = cgi.parse_header(self.headers.getheader(
> ctype(s) has a very specific python meaning that differs by leaps and bound

Done
Line 156:                         self.HEADER_CONTENT_TYPE))
Line 157:                     if ctype != 'application/octet-stream':
Line 158:                         self.send_error(httplib.UNSUPPORTED_MEDIA_TYPE,
Line 159:                                         "unsupported content type: %r" % ctype)

Line 153:             def do_PUT(self):
Line 154:                 try:
Line 155:                     ctype, pt = cgi.parse_header(self.headers.getheader(
Line 156:                         self.HEADER_CONTENT_TYPE))
Line 157:                     if ctype != 'application/octet-stream':
> this literal should probably be also a constant.

Done
Line 158:                         self.send_error(httplib.UNSUPPORTED_MEDIA_TYPE,
Line 159:                                         "unsupported content type: %r" % ctype)
Line 160:                         return
Line 161:

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:

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

Done
Line 210:                     self.send_response(error)
Line 211:                     self.end_headers()
Line 212:                     self.finish()
Line 213:                 except Exception:

Change in vdsm[master]: introducing downloadFromStream verb

Liron Ar has posted comments on this change.

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

Patch Set 4:

(3 comments)

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

Line 7: introducing downloadFromStream verb
Line 8: 
Line 9: This patch introduces the downloadFromStream verb in vdsm which
Line 10: allows to upload using streaming content to vdsm images.
Line 11: XML-RPC spec doesn't support streaming, as we don't want to use HTTP
> We are sending the whole payload in the request, just like xml rpc. The dif

sending in XMLRPC will automatically read the whole message body, opposed to the streaming which is implemented here - that's the motivation, not the size of one request.
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:

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

Line 28: 
Line 29: 
Line 30: log = logging.getLogger("Storage.ImageSharing")
Line 31: WAIT_TIMEOUT = 30
Line 32: BUFFER_READ_SIZE = 65536
> What we read from the socket, we write to the pipe, don't we?

depends on the length of what we got.
Line 33: 
Line 34: 
Line 35: def httpGetSize(methodArgs):
Line 36:     headers = curlImgWrap.head(methodArgs.get('url'),

Line 90:                           total_size - bytes_left, total_size)
Line 91:                 raise se.MiscFileReadException()
Line 92: 
Line 93:             p.stdin.write(data)
Line 94:             p.stdin.flush()
> This:

Done
Line 95:             bytes_left = bytes_left - len(data)
Line 96: 
Line 97:         p.stdin.close()
Line 98:         if not p.wait(WAIT_TIMEOUT):

Change in vdsm[master]: introducing downloadFromStream verb

Antoni Segura Puimedon has posted comments on this change.

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

Patch Set 7: Code-Review+1

Change in vdsm[master]: introducing downloadFromStream verb

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 7: Code-Review+1

Change in vdsm[master]: introducing downloadFromStream verb

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 6: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6022/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6810/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6916/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_localfs/49/ : FAILURE

Change in vdsm[master]: introducing downloadFromStream verb

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 7: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6024/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6812/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6918/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_localfs/50/ : FAILURE

Change in vdsm[master]: introducing downloadFromStream verb

Yaniv Bronhaim has posted comments on this change.

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

Patch Set 7: Code-Review-1

(9 comments)

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

Line 8: 
Line 9: This patch adds the downloadFromStream internal verb, allowing efficient
Line 10: upload of data in any size and format and storing it directly on an
Line 11: image. As the  XML-RPC spec doesn't support streaming and to avoid requiring
Line 12: another port by using dedicated http server, in this patch we use the existing
please don't pass 60 chars in line
Line 13: xmlrpc server to handle upload requests.
Line 14: 
Line 15: Previously we could send ovf data using XMLRPC, which limit the size of
Line 16: the data, having to encode the payload into the xml, and make it hard an

Line 11: image. As the  XML-RPC spec doesn't support streaming and to avoid requiring
Line 12: another port by using dedicated http server, in this patch we use the existing
Line 13: xmlrpc server to handle upload requests.
Line 14: 
Line 15: Previously we could send ovf data using XMLRPC, which limit the size of
limits?
Line 16: the data, having to encode the payload into the xml, and make it hard an
Line 17: inefficient to upload lot of data and store it on some image.
Line 18: 
Line 19: General upload information:

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

Line 28: import logging
Line 29: import libvirt
Line 30: import threading
Line 31: import socket
Line 32: 
put "import socket" before vdsm imports
Line 33: from vdsm import constants
Line 34: from vdsm import utils
Line 35: from vdsm.define import doneCode, errCode
Line 36: from vdsm.netinfo import getRouteDeviceTo

Line 144:             HEADER_VOLUME = 'X-Volume-Id'
Line 145:             HEADER_CONTENT_LENGTH = 'content-length'
Line 146:             HEADER_CONTENT_TYPE = 'content-type'
Line 147:             CONTENT_STREAM_TYPE = 'application/octet-stream'
Line 148: 
you use those only once.. why constants? just write the strings explicitly
Line 149:             def setup(self):
Line 150:                 threadLocal.client = self.client_address[0]
Line 151:                 return basehandler.setup(self)
Line 152:

http://gerrit.ovirt.org/#/c/23281/7/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 1679:                                 volUUID=None):
Line 1680:         """
Line 1681:         Download an image from a stream synchronously.
Line 1682: 
Line 1683:         Warning: Internal use only.
explain this warning. is it only for redhatter? :)
Line 1684:         """
Line 1685:         sdCache.produce(sdUUID)
Line 1686:         pool = self.getPool(spUUID)
Line 1687:         pool.downloadImage(methodArgs, sdUUID, imgUUID, volUUID)

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

Line 32: 65536
why this size and timeout? can you add short explanation above?

Line 57: methodArgs
This function is redundant. just get the value directly

you do it with all the other args you get from this dict.. so why this one is different?

Line 76: deathSignal
not sure if it fits, but you have misc.ddWatchCopy that you can use here

Line 109:                       p.returncode, p.stderr.read(1000))
Line 110:             raise se.MiscFileWriteException()
Line 111: 
Line 112:     except Exception:
Line 113:         if p.returncode is None:
p is an AsyncProc (you sent sync=False), it doesn't contain returncode data-member if i didn't miss anything... (maybe p._proc.returncode). I think you'll get here AttributeError
Line 114:             p.kill()
Line 115:         raise
Line 116: 
Line 117:

Change in vdsm[master]: introducing downloadFromStream verb

Liron Ar has posted comments on this change.

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

Patch Set 7:

(6 comments)

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

Line 28: import logging
Line 29: import libvirt
Line 30: import threading
Line 31: import socket
Line 32:
> put "import socket" before vdsm imports

Done
Line 33: from vdsm import constants
Line 34: from vdsm import utils
Line 35: from vdsm.define import doneCode, errCode
Line 36: from vdsm.netinfo import getRouteDeviceTo

Line 144:             HEADER_VOLUME = 'X-Volume-Id'
Line 145:             HEADER_CONTENT_LENGTH = 'content-length'
Line 146:             HEADER_CONTENT_TYPE = 'content-type'
Line 147:             CONTENT_STREAM_TYPE = 'application/octet-stream'
Line 148:
> you use those only once.. why constants? just write the strings explicitly

It was asked by Nir and Antoni
Line 149:             def setup(self):
Line 150:                 threadLocal.client = self.client_address[0]
Line 151:                 return basehandler.setup(self)
Line 152:

http://gerrit.ovirt.org/#/c/23281/7/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 1679:                                 volUUID=None):
Line 1680:         """
Line 1681:         Download an image from a stream synchronously.
Line 1682: 
Line 1683:         Warning: Internal use only.
> explain this warning. is it only for redhatter? :)

that's the comment that we use for other verbs for internal use, take a look in the methods : appropriateDevice, inappropriateDevices in this file.
Line 1684:         """
Line 1685:         sdCache.produce(sdUUID)
Line 1686:         pool = self.getPool(spUUID)
Line 1687:         pool.downloadImage(methodArgs, sdUUID, imgUUID, volUUID)

http://gerrit.ovirt.org/#/c/23281/7/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['contentLength']
> This function is redundant. just get the value directly

the convention in this class is to define a method for getting a size, see getSharingMethods, and it's used by the extension outside of this class.
Line 58: 
Line 59: 
Line 60: def httpDownloadImage(dstImgPath, methodArgs):
Line 61:     curlImgWrap.download(methodArgs.get('url'), dstImgPath,

Line 72:     stream = methodArgs['fileObj']
Line 73: 
Line 74:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" % constants.MEGAB]
Line 75:     p = utils.execCmd(cmd, sudo=False, sync=False,
Line 76:                       deathSignal=signal.SIGKILL)
> not sure if it fits, but you have misc.ddWatchCopy that you can use here

ddWatchCopy has other use.
Line 77:     try:
Line 78:         while bytes_left > 0:
Line 79:             to_read = min(BUFFER_SIZE, bytes_left)
Line 80:

Line 109:                       p.returncode, p.stderr.read(1000))
Line 110:             raise se.MiscFileWriteException()
Line 111: 
Line 112:     except Exception:
Line 113:         if p.returncode is None:
> p is an AsyncProc (you sent sync=False), it doesn't contain returncode data

it contains it afaik (it was tested), but i'll reverify that.
Line 114:             p.kill()
Line 115:         raise
Line 116: 
Line 117:

Change in vdsm[master]: introducing downloadFromStream verb

Liron Ar has posted comments on this change.

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

Patch Set 7:

(2 comments)

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

Line 8: 
Line 9: This patch adds the downloadFromStream internal verb, allowing efficient
Line 10: upload of data in any size and format and storing it directly on an
Line 11: image. As the  XML-RPC spec doesn't support streaming and to avoid requiring
Line 12: another port by using dedicated http server, in this patch we use the existing
> please don't pass 60 chars in line

Done
Line 13: xmlrpc server to handle upload requests.
Line 14: 
Line 15: Previously we could send ovf data using XMLRPC, which limit the size of
Line 16: the data, having to encode the payload into the xml, and make it hard an

Line 11: image. As the  XML-RPC spec doesn't support streaming and to avoid requiring
Line 12: another port by using dedicated http server, in this patch we use the existing
Line 13: xmlrpc server to handle upload requests.
Line 14: 
Line 15: Previously we could send ovf data using XMLRPC, which limit the size of
> limits?

Done
Line 16: the data, having to encode the payload into the xml, and make it hard an
Line 17: inefficient to upload lot of data and store it on some image.
Line 18: 
Line 19: General upload information:

Change in vdsm[master]: introducing downloadFromStream verb

Yaniv Bronhaim has posted comments on this change.

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

Patch Set 7:

(5 comments)

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

Line 144:             HEADER_VOLUME = 'X-Volume-Id'
Line 145:             HEADER_CONTENT_LENGTH = 'content-length'
Line 146:             HEADER_CONTENT_TYPE = 'content-type'
Line 147:             CONTENT_STREAM_TYPE = 'application/octet-stream'
Line 148:
> It was asked by Nir and Antoni

redundant. ask for a reason.. if you use it only on do_PUT, put it there at least. you really don't need it golably
Line 149:             def setup(self):
Line 150:                 threadLocal.client = self.client_address[0]
Line 151:                 return basehandler.setup(self)
Line 152:

http://gerrit.ovirt.org/#/c/23281/7/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 1679:                                 volUUID=None):
Line 1680:         """
Line 1681:         Download an image from a stream synchronously.
Line 1682: 
Line 1683:         Warning: Internal use only.
> that's the comment that we use for other verbs for internal use, take a loo

I still don't understand that and I prefer to understand what "internal use only" means.. if you understand why "appropriateDevice, inappropriateDevices" are also for internal use, be my guess to explain. but this is your part, so why internal? you call it from API like all the other verbs
Line 1684:         """
Line 1685:         sdCache.produce(sdUUID)
Line 1686:         pool = self.getPool(spUUID)
Line 1687:         pool.downloadImage(methodArgs, sdUUID, imgUUID, volUUID)

http://gerrit.ovirt.org/#/c/23281/7/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['contentLength']
> the convention in this class is to define a method for getting a size, see

you can't compare. getSharingMethods catches KeyError and translates the output . here you do nothing , just getting the value
Line 58: 
Line 59: 
Line 60: def httpDownloadImage(dstImgPath, methodArgs):
Line 61:     curlImgWrap.download(methodArgs.get('url'), dstImgPath,

Line 72:     stream = methodArgs['fileObj']
Line 73: 
Line 74:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" % constants.MEGAB]
Line 75:     p = utils.execCmd(cmd, sudo=False, sync=False,
Line 76:                       deathSignal=signal.SIGKILL)
> ddWatchCopy has other use.

what do you mean? it does dd from source to destination . i prefer using utilities than calling the command explicitly, reuse leads to less bugs.
Line 77:     try:
Line 78:         while bytes_left > 0:
Line 79:             to_read = min(BUFFER_SIZE, bytes_left)
Line 80:

Line 109:                       p.returncode, p.stderr.read(1000))
Line 110:             raise se.MiscFileWriteException()
Line 111: 
Line 112:     except Exception:
Line 113:         if p.returncode is None:
> it contains it afaik (it was tested), but i'll reverify that.

please do. i read the code and it doesn't. see in class AsyncProc in utils.py
Line 114:             p.kill()
Line 115:         raise
Line 116: 
Line 117:

Change in vdsm[master]: introducing downloadFromStream verb

Liron Ar has posted comments on this change.

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

Patch Set 7:

(6 comments)

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

Line 144:             HEADER_VOLUME = 'X-Volume-Id'
Line 145:             HEADER_CONTENT_LENGTH = 'content-length'
Line 146:             HEADER_CONTENT_TYPE = 'content-type'
Line 147:             CONTENT_STREAM_TYPE = 'application/octet-stream'
Line 148:
> redundant. ask for a reason.. if you use it only on do_PUT, put it there at

there's no ned to move it to do_PUT and to have it defined everytime the method is called.
I prefer to keep it as constants as Nir and Anotoni suggested.
Line 149:             def setup(self):
Line 150:                 threadLocal.client = self.client_address[0]
Line 151:                 return basehandler.setup(self)
Line 152:

http://gerrit.ovirt.org/#/c/23281/7/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 1679:                                 volUUID=None):
Line 1680:         """
Line 1681:         Download an image from a stream synchronously.
Line 1682: 
Line 1683:         Warning: Internal use only.
> I still don't understand that and I prefer to understand what "internal use

internal use only means that theres no meaning to use it besides our use from the server. you can't pass the socket.fileObject parameter and therefore this verb is for internal use only.
Line 1684:         """
Line 1685:         sdCache.produce(sdUUID)
Line 1686:         pool = self.getPool(spUUID)
Line 1687:         pool.downloadImage(methodArgs, sdUUID, imgUUID, volUUID)

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

Line 28: 
Line 29: 
Line 30: log = logging.getLogger("Storage.ImageSharing")
Line 31: WAIT_TIMEOUT = 30
Line 32: BUFFER_SIZE = 65536
> why this size and timeout? can you add short explanation above?

what explanation would you suggest? those are arbitrary values that can always be tuned.
Line 33: 
Line 34: 
Line 35: def httpGetSize(methodArgs):
Line 36:     headers = curlImgWrap.head(methodArgs.get('url'),

Line 53:     return size
Line 54: 
Line 55: 
Line 56: def streamGetSize(methodArgs):
Line 57:     return methodArgs['contentLength']
> you can't compare. getSharingMethods catches KeyError and translates the ou

that's internal implementation, and as i said - it's called from sp.py, take a look there.
Line 58: 
Line 59: 
Line 60: def httpDownloadImage(dstImgPath, methodArgs):
Line 61:     curlImgWrap.download(methodArgs.get('url'), dstImgPath,

Line 72:     stream = methodArgs['fileObj']
Line 73: 
Line 74:     cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" % constants.MEGAB]
Line 75:     p = utils.execCmd(cmd, sudo=False, sync=False,
Line 76:                       deathSignal=signal.SIGKILL)
> what do you mean? it does dd from source to destination . i prefer using ut

ddWatchCopy use to copy from file, using specified path, offset , seek, etc.
here we don't copy from a file but piping data to the dd process,
therefore there's no meaning to use that method.
the resuse here is to use the utils.execCmd verb like we do execute dd by from other places when needed, we need a specific dd here.
Line 77:     try:
Line 78:         while bytes_left > 0:
Line 79:             to_read = min(BUFFER_SIZE, bytes_left)
Line 80:

Line 109:                       p.returncode, p.stderr.read(1000))
Line 110:             raise se.MiscFileWriteException()
Line 111: 
Line 112:     except Exception:
Line 113:         if p.returncode is None:
> please do. i read the code and it doesn't. see in class AsyncProc in utils.

see there
 @property
    def returncode(self):
Line 114:             p.kill()
Line 115:         raise
Line 116: 
Line 117:

Change in vdsm[master]: introducing downloadFromStream verb

Yaniv Bronhaim has posted comments on this change.

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

Patch Set 7:

(5 comments)

the -1 is mostly about the commit message. please upload your fixed version with your preferred code. no score from my side after that

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

Line 144:             HEADER_VOLUME = 'X-Volume-Id'
Line 145:             HEADER_CONTENT_LENGTH = 'content-length'
Line 146:             HEADER_CONTENT_TYPE = 'content-type'
Line 147:             CONTENT_STREAM_TYPE = 'application/octet-stream'
Line 148:
> there's no ned to move it to do_PUT and to have it defined everytime the me

you can also write it in global.py with no need. but as you don't use it anywhere else this will be redundant.. I'm just suggesting.
Line 149:             def setup(self):
Line 150:                 threadLocal.client = self.client_address[0]
Line 151:                 return basehandler.setup(self)
Line 152:

http://gerrit.ovirt.org/#/c/23281/7/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 1679:                                 volUUID=None):
Line 1680:         """
Line 1681:         Download an image from a stream synchronously.
Line 1682: 
Line 1683:         Warning: Internal use only.
> internal use only means that theres no meaning to use it besides our use fr

ok.. doesn't mean anything to me. its public. but if you understand this label in the other methods, be my guess to put it also here
Line 1684:         """
Line 1685:         sdCache.produce(sdUUID)
Line 1686:         pool = self.getPool(spUUID)
Line 1687:         pool.downloadImage(methodArgs, sdUUID, imgUUID, volUUID)

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

Line 28: 
Line 29: 
Line 30: log = logging.getLogger("Storage.ImageSharing")
Line 31: WAIT_TIMEOUT = 30
Line 32: BUFFER_SIZE = 65536
> what explanation would you suggest? those are arbitrary values that can alw

write it
Line 33: 
Line 34: 
Line 35: def httpGetSize(methodArgs):
Line 36:     headers = curlImgWrap.head(methodArgs.get('url'),

Line 53:     return size
Line 54: 
Line 55: 
Line 56: def streamGetSize(methodArgs):
Line 57:     return methodArgs['contentLength']
> that's internal implementation, and as i said - it's called from sp.py, tak

what is called from sp.py? whatever, if you insist to have a method that does nothing ..
Line 58: 
Line 59: 
Line 60: def httpDownloadImage(dstImgPath, methodArgs):
Line 61:     curlImgWrap.download(methodArgs.get('url'), dstImgPath,

Line 109:                       p.returncode, p.stderr.read(1000))
Line 110:             raise se.MiscFileWriteException()
Line 111: 
Line 112:     except Exception:
Line 113:         if p.returncode is None:
> see there

it does... my bad
Line 114:             p.kill()
Line 115:         raise
Line 116: 
Line 117:

Change in vdsm[master]: introducing downloadFromStream verb

Liron Ar has abandoned this change.

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

Abandoned

I've thought about problematic scenario,
abandoning and will re-upload it.

Change in vdsm[master]: introducing downloadFromStream verb

Liron Ar has restored this change.

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

Restored

Change in vdsm[master]: introducing downloadFromStream verb

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 8: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6107/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7000/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6894/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_localfs/75/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_nfs/10/ : FAILURE

Change in vdsm[master]: introducing downloadFromStream verb

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 8:

(7 comments)

This patch is even nicer now - less code and more correct.

Needs little cleanup in the commit message and the comments about constants that Yaniv asked in the previous patch set.

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

Line 3: AuthorDate: 2014-01-12 13:52:24 +0200
Line 4: Commit:     Liron Aravot 
Line 5: CommitDate: 2014-01-29 14:06:14 +0200
Line 6: 
Line 7: introducing downloadFromStream verb
Seems that this is wrong now, as we don't have new verb now.

So this should describe now the new feature, like:

Add upload to image feature
Line 8: 
Line 9: This patch introduces the downloadFromStream verb in vdsm
Line 10: which allows to upload using streaming content to vdsm
Line 11: images.

Line 5: CommitDate: 2014-01-29 14:06:14 +0200
Line 6: 
Line 7: introducing downloadFromStream verb
Line 8: 
Line 9: This patch introduces the downloadFromStream verb in vdsm
There is no such verb now.
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 7: introducing downloadFromStream verb
Line 8: 
Line 9: This patch introduces the downloadFromStream verb in vdsm
Line 10: which allows to upload using streaming content to vdsm
Line 11: images.
The previous text explained this better:
http://gerrit.ovirt.org/#/c/23281/7//COMMIT_MSG
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 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 downloadFromStream internal verb,
No such verb now
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

Line 17: 
Line 18: This patch adds the downloadFromStream internal verb,
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
We already discussed that in previous patche sets. There is no difference between sending xmlrpc payload and sending data using pure http. Both stream data to the server in the same way.

What makes xmlrpc less useful for uploading data is the need to encode the data into the xml, and the need to parse the xml containing the data. The problem is not streaming support in xmlrpc.
Line 22: to avoid requiring another port by using dedicated http
Line 23: server, in this patch we use the existing xmlrpc server to
Line 24: handle upload requests.
Line 25:

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.
Maybe explain that having a task protect from stopping the spm by the engine.
Line 35: 
Line 36: The PUT request inspected headers are:
Line 37: -- Mandatory headers:
Line 38:         *content-length

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

Line 28: 
Line 29: 
Line 30: log = logging.getLogger("Storage.ImageSharing")
Line 31: WAIT_TIMEOUT = 30
Line 32: BUFFER_SIZE = 65536
Lets add comments as Yaniv asked:

# Time to wait from finishing writing data to dd, until dd exists,
 # Ensure that we don't keep the task active forever if dd cannot
 # access the storage.
 WAIT_TIMEOUT = 30

# Number of bytes to read from the socket and write
 # to dd stdin trough the pipe. Based on default socket buffer  
 # size(~80KB) and default pipe buffer size (64K), this should
 # minimize system call overhead without consuming too much
 # memory.
 BUFFER_SIZE = 65536
Line 33: 
Line 34: 
Line 35: def httpGetSize(methodArgs):
Line 36:     headers = curlImgWrap.head(methodArgs.get('url'),

Change in vdsm[master]: introducing downloadFromStream verb

Liron Ar has posted comments on this change.

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

Patch Set 8:

(6 comments)

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

Line 3: AuthorDate: 2014-01-12 13:52:24 +0200
Line 4: Commit:     Liron Aravot 
Line 5: CommitDate: 2014-01-29 14:06:14 +0200
Line 6: 
Line 7: introducing downloadFromStream verb
> Seems that this is wrong now, as we don't have new verb now.

Done
Line 8: 
Line 9: This patch introduces the downloadFromStream verb in vdsm
Line 10: which allows to upload using streaming content to vdsm
Line 11: images.

Line 5: CommitDate: 2014-01-29 14:06:14 +0200
Line 6: 
Line 7: introducing downloadFromStream verb
Line 8: 
Line 9: This patch introduces the downloadFromStream verb in vdsm
> There is no such verb now.

Done
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 7: introducing downloadFromStream verb
Line 8: 
Line 9: This patch introduces the downloadFromStream verb in vdsm
Line 10: which allows to upload using streaming content to vdsm
Line 11: images.
> The previous text explained this better:

Done
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 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 downloadFromStream internal verb,
> No such verb now

Done
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

Line 17: 
Line 18: This patch adds the downloadFromStream internal verb,
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
> We already discussed that in previous patche sets. There is no difference b

Already commented about that before, there is a difference - xmlrpc spec doesn't support streaming (when the request arrives to the server all the body is read),.
The reason that we use http here is the lack of the streaming support- to provide support for uploading big payloads.
Line 22: to avoid requiring another port by using dedicated http
Line 23: server, in this patch we use the existing xmlrpc server to
Line 24: handle upload requests.
Line 25:

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.
> Maybe explain that having a task protect from stopping the spm by the engin

I'll add it.
Line 35: 
Line 36: The PUT request inspected headers are:
Line 37: -- Mandatory headers:
Line 38:         *content-length

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 9: Code-Review-1 Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_localfs/130/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6259/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7038/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7149/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_nfs/69/ : FAILURE

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 9: Code-Review+1

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 9:

Please fix pep8 error:
vdsm/storage/imageSharing.py:42:1: E302 expected 2 blank lines, found 1

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

Yaniv Bronhaim has posted comments on this change.

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

Patch Set 9: Code-Review+1

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 9:

(1 comment)

Minor cleanup.

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

Line 152: 
Line 153:             def do_PUT(self):
Line 154:                 try:
Line 155:                     contentType, pt = cgi.parse_header(self.headers.getheader(
Line 156:                         self.HEADER_CONTENT_TYPE))
You can replace these lines with:

contentType = self.headers.gettype()

And remove the cgi import.

See http://docs.python.org/2/library/mimetools.html#mimetools.Message.gettype
Line 157:                     if contentType != self.CONTENT_STREAM_TYPE:
Line 158:                         self.send_error(httplib.UNSUPPORTED_MEDIA_TYPE,
Line 159:                                         "unsupported content type: %r" %
Line 160:                                         contentType)

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

[Dan Kenigsberg]

Dan Kenigsberg 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 not having hsm tasks, why must we be the spm in order to receive streams? It could have been much nicer if the SPM could create a volume, and any other host could write into it.

Anyway, the comment promised to Nir is missing.
Line 35: 
Line 36: The PUT request inspected headers are:
Line 37: -- Mandatory headers:
Line 38:         *content-length

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

Line 123:                 # file (rather than an event) because editNetwork is
Line 124:                 # performed by a separate, root process.  To clean this
Line 125:                 # up we need to move this to an API wrapper that is only
Line 126:                 # run for real clients (not vdsm internal API calls).
Line 127:                 file(constants.P_VDSM_CLIENT_LOG, 'w')
Context has considerably changed, this patch requires a manual rebase.
Line 128: 
Line 129:         server_address = (self.serverIP, int(self.serverPort))
Line 130:         if self.enableSSL:
Line 131:             basehandler = SecureXMLRPCServer.SecureXMLRPCRequestHandler

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:

http://tools.ietf.org/search/rfc6648
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?
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.
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()*
Line 105:             # this stdin wrapper.
Line 106:             p.stdin.flush()
Line 107:             bytes_left = bytes_left - len(data)
Line 108:

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

[Nir Soffer]

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:

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

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

Patch Set 9:

(3 comments)

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

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 is scheduling a task in hsm and return immediately with a task id.

Thanks, I got tangled between stroage.image and API.image.
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
> This is unrelated to this patch, can be cleanup later to keep this patch mi

I'm referring only to newly added imports.
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
> Did you check the underlying file object wrapper?

my comment was only about the comment english. Sorry about the ambiguity.
Line 105:             # this stdin wrapper.
Line 106:             p.stdin.flush()
Line 107:             bytes_left = bytes_left - len(data)
Line 108:

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

Liron Ar has posted comments on this change.

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

Patch Set 9:

(5 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.
> The purpose of the upload/download task is to prevent spm replacement durin

Dan,
basically yes - we could have an implementation that perform needed extend from the spm and executes the retrieval of the stream to the image through an hsm - but that's a general change which is relevant for the current "downloadImage" verb (which could extend through the spm and download through hsm).

As we currently use the download verb that already exists and the fact that we're on the way to remove the spm entirely combined with fact that we don't have any support for hsm tasks (engine and vdsm wise) - its better that we'll "use" (with adaptation) the current download verb as it's good enough for our usecase and as we use on the current "downloadImage", it'll be changed all together.

Regards the created task, right now all the spm tasks are perform through a task to indicate to the engine that something is going on, if you take a look in spmStop vdsm side  you'll notice that we don't have any check there on ongoing operations, the engine is responsible to decide when it's fine to stop the spm or not based on the tasks.
With that said, it makes more sense IMO to perform the actual writes to the storage through the task thread, otherwise we won't know what's going on, can pass the allocated number of "task" threads that we wanted for storage writes, etc and it may also cause issues with future engine versions that'll have to know that there might be changes to the storage without a task because it'll be the first time that we perform such operations thorugh a host outside of a task .

Nir,
The comment you made is wrong -
lock for the image isn't an issue, unless when you'll want to stop the spm you'll attempt to lock all the images in the relevant pool.
the issue is persistent spm lock, i don't see a reason to add such a mechanism while we are about to remove the spm or reduce it's responsibilities massively, the current flow as it is will suffice for now till we "get rid" of it :)
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 bel

I don't agree with removing it from here, when looking for/in the patch it's much clearer to see the useage rather than start searching for it in the patch files.
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'
> +1 for removing the "X-" prefix.

Done
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 152: 
Line 153:             def do_PUT(self):
Line 154:                 try:
Line 155:                     contentType, pt = cgi.parse_header(self.headers.getheader(
Line 156:                         self.HEADER_CONTENT_TYPE))
> You can replace these lines with:

Done
Line 157:                     if contentType != self.CONTENT_STREAM_TYPE:
Line 158:                         self.send_error(httplib.UNSUPPORTED_MEDIA_TYPE,
Line 159:                                         "unsupported content type: %r" %
Line 160:                                         contentType)

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

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
> my comment was only about the comment english. Sorry about the ambiguity.

Dan, how do you suggest to write the comment without "flush"? or did you mean to remove that comment entirely?
Line 105:             # this stdin wrapper.
Line 106:             p.stdin.flush()
Line 107:             bytes_left = bytes_left - len(data)
Line 108:

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 9:

(1 comment)

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

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
> I don't agree with removing it from here, when looking for/in the patch it'

The headers required by the put request are uninteresting implementation details, so they do not add any value to the commit message. They do add value to when they are located near the implementation.

Think about someone trying to understand what headers are required for the PUT method. What would be the best place for this documentation?
Line 45: 
Line 46: Change-Id: I768b84799ed9fb2769c6d4240519d036f8988b99

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 9:

(1 comment)

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

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
> Dan, how do you suggest to write the comment without "flush"? or did you me

Seems like replace "We can remove if" with "We can remove flush() if"
Line 105:             # this stdin wrapper.
Line 106:             p.stdin.flush()
Line 107:             bytes_left = bytes_left - len(data)
Line 108:

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

[Federico Simoncelli]

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

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

Yaniv Bronhaim has posted comments on this change.

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

Patch Set 9:

(1 comment)

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

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
> The headers required by the put request are uninteresting implementation de

agree. move it above the implementation
Line 45: 
Line 46: Change-Id: I768b84799ed9fb2769c6d4240519d036f8988b99

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

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

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

Patch Set 11:

(1 comment)

http://gerrit.ovirt.org/#/c/23281/11/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 1685:         pool = self.getPool(spUUID)
Line 1686:         # NOTE: this could become an hsm task, in such case the LV extension
Line 1687:         # required to prepare the destination should go through the mailbox.
Line 1688:         self._spmSchedule(spUUID, "downloadImageFromStream",
Line 1689:                           pool.downloadImageFromStream, methodArgs, callback,
Regardless of the patch we'll pick... we'll need Saggi's opinion here. I am not sure if passing complex objects (callback function) is allowed... since this is persisted. For sure we can't recover but we don't care about it.

(It wouldn't be different if the callback was in methodArgs anyway).
Line 1690:                           sdUUID, imgUUID, volUUID)
Line 1691: 
Line 1692:     @deprecated
Line 1693:     @public

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 10:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7229/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6339/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7123/ : SUCCESS

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 11:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7231/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6341/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7125/ : SUCCESS

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 11: Code-Review-1

(7 comments)

Need to fix the code duplication introduced in patches 10 and 11.

And address Federico and Dan comments from patch 9.

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

Line 175: 
Line 176:                     spUUID = self.headers.getheader(self.HEADER_POOL)
Line 177:                     sdUUID = self.headers.getheader(self.HEADER_DOMAIN)
Line 178:                     imgUUID = self.headers.getheader(self.HEADER_IMAGE)
Line 179:                     volUUID = self.headers.getheader(self.HEADER_VOLUME)
Please address Federico comment in http://gerrit.ovirt.org/#/c/23281/9/vdsm/BindingXMLRPC.py
Line 180: 
Line 181:                     uploadFinishedEvent = threading.Event()
Line 182: 
Line 183:                     def upload_finished():

http://gerrit.ovirt.org/#/c/23281/11/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 1678:     def downloadImageFromStream(self, methodArgs, callback, spUUID, sdUUID,
Line 1679:                                 imgUUID, volUUID=None):
Line 1680:         """
Line 1681:         Download an image from a remote endpoint using the specified method
Line 1682:         and methodArgs.
Please add a note that this is for internal use only.
Line 1683:         """
Line 1684:         sdCache.produce(sdUUID)
Line 1685:         pool = self.getPool(spUUID)
Line 1686:         # NOTE: this could become an hsm task, in such case the LV extension

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

Line 1163:         finally:
Line 1164:             domain.deactivateImage(imgUUID)
Line 1165: 
Line 1166:     def downloadFromStream(self, methodArgs, callback, sdUUID, imgUUID,
Line 1167:                            volUUID=None):
Do we need this or we can add optional callback argument to download()?
Line 1168:         try:
Line 1169:             self._downloadFromStream(methodArgs, sdUUID, imgUUID, volUUID)
Line 1170:         finally:
Line 1171:             if callback is not None:

Line 1167:                            volUUID=None):
Line 1168:         try:
Line 1169:             self._downloadFromStream(methodArgs, sdUUID, imgUUID, volUUID)
Line 1170:         finally:
Line 1171:             if callback is not None:
We don't need to check the callback - if this is not an optional argument, it is the caller problem if it send None here. Just call it.

If you want to make the callback optional, make it an optional argument:

def downloadFromStream(self, methodArgs, sdUUID, imgUUID, volUUID=None, callback=None):
        ...
Line 1172:                 callback()
Line 1173: 
Line 1174:     def download(self, methodArgs, sdUUID, imgUUID, volUUID=None):
Line 1175:         domain = sdCache.produce(sdUUID)

Line 1181:             imageSharing.download(vol.getVolumePath(), methodArgs)
Line 1182:         finally:
Line 1183:             domain.deactivateImage(imgUUID)
Line 1184: 
Line 1185:     def _downloadFromStream(self, methodArgs, sdUUID, imgUUID, volUUID=None):
This is a duplicate of download() - use download.
Line 1186:         domain = sdCache.produce(sdUUID)
Line 1187: 
Line 1188:         vol = self._activateVolumeForImportExport(domain, imgUUID, volUUID)
Line 1189:         try:

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

Line 102:             # Process stdin is not a real file object but a wrapper using
Line 103:             # StringIO buffer. To ensure that we don't use more memory if we
Line 104:             # get data faster then dd read it from the pipe, we flush on every
Line 105:             # write. We can remove if we can limit the buffer size used by
Line 106:             # this stdin wrapper.
Please address Dan comment in http://gerrit.ovirt.org/#/c/23281/9/vdsm/storage/imageSharing.py
Line 107:             p.stdin.flush()
Line 108:             bytes_left = bytes_left - len(data)
Line 109: 
Line 110:         p.stdin.close()

http://gerrit.ovirt.org/#/c/23281/11/vdsm/storage/sp.py
File vdsm/storage/sp.py:

Line 1774:             return image.Image(self.poolPath) \
Line 1775:                 .download(methodArgs, sdUUID, imgUUID, volUUID)
Line 1776: 
Line 1777:     def downloadImageFromStream(self, methodArgs, callback, sdUUID, imgUUID,
Line 1778:                                 volUUID=None):
This is duplicate of downloadImage. This is not acceptable.
Line 1779:         """
Line 1780:         Download an image from a remote endpoint using the specified method
Line 1781:         and methodArgs.
Line 1782:         """

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

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

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

Patch Set 11: Code-Review+1

I like the direction, but yes, there are few things to address. General feeling is positive.

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 10:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7241/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6351/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7135/ : SUCCESS

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 10:

(1 comment)

Minor text issue in imageSharing, otherwise it is good enough considering the desire to get this into 3.4.

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

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

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

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

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

Patch Set 10: Code-Review+2

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

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

Patch Set 10: Code-Review-1

(2 comments)

two very minor comments

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

Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: 
Line 20: import logging
Line 21: import curlImgWrap
you've introduced new imports - please introduce them properly sorted.
Line 22: import signal
Line 23: import socket
Line 24: 
Line 25: import storage_exception as se

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

right. sorry for not being clear in my request.
Line 106:             # this stdin wrapper.
Line 107:             p.stdin.flush()
Line 108:             bytes_left = bytes_left - len(data)
Line 109:

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

Liron Ar has posted comments on this change.

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

Patch Set 10:

(2 comments)

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

Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: 
Line 20: import logging
Line 21: import curlImgWrap
> you've introduced new imports - please introduce them properly sorted.

Done
Line 22: import signal
Line 23: import socket
Line 24: 
Line 25: import storage_exception as se

Line 101:             p.stdin.write(data)
Line 102:             # Process stdin is not a real file object but a wrapper using
Line 103:             # StringIO buffer. To ensure that we don't use more memory if we
Line 104:             # get data faster then dd read it from the pipe, we flush on every
Line 105:             # write. We can remove if we can limit the buffer size used by
> right. sorry for not being clear in my request.

Done
Line 106:             # this stdin wrapper.
Line 107:             p.stdin.flush()
Line 108:             bytes_left = bytes_left - len(data)
Line 109:

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 11:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7260/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6370/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7154/ : SUCCESS

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 11: Code-Review-1

(1 comment)

Missing return after header was missing.

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

Line 147:                     if not all((spUUID, sdUUID, imgUUID)):
Line 148:                         self.send_error(httplib.BAD_REQUEST,
Line 149:                                         "missing or empty required header(s):"
Line 150:                                         " spUUID=%s sdUUID=%s imgUUID=%s"
Line 151:                                         % (spUUID, sdUUID, imgUUID))
You must return after sending error.
Line 152: 
Line 153:                     # Optional headers
Line 154:                     volUUID = self.headers.getheader(self.HEADER_VOLUME)
Line 155:

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

Liron Ar has posted comments on this change.

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

Patch Set 11:

(1 comment)

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

Line 147:                     if not all((spUUID, sdUUID, imgUUID)):
Line 148:                         self.send_error(httplib.BAD_REQUEST,
Line 149:                                         "missing or empty required header(s):"
Line 150:                                         " spUUID=%s sdUUID=%s imgUUID=%s"
Line 151:                                         % (spUUID, sdUUID, imgUUID))
> You must return after sending error.

good catch, done.
Line 152: 
Line 153:                     # Optional headers
Line 154:                     volUUID = self.headers.getheader(self.HEADER_VOLUME)
Line 155:

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 12: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7264/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6374/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7158/ : SUCCESS

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 12: Verified+1

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 12: -Verified Code-Review+1

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

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

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

Patch Set 12: Code-Review+2

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 12:

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6374/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7158/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7265/ : FAILURE

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 12:

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6374/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7158/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7266/ : FAILURE

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 12:

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6374/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7158/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7268/ : FAILURE

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 12:

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6374/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7158/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7269/ : FAILURE

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 12:

I triggered few jenkins builds with this patch and the patch is rebased on http://gerrit.ovirt.org/#/c/24487.

The testLeakFd fails consistently with this patch - see jenkins reports above. There is such no failure with the patch 24487.

We should look into this.

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

Liron Ar has posted comments on this change.

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

Patch Set 12:

@Nir, the failure doesn't seem related to this patch - take a look here.
we should look at this regardless, but it doesn't seem cause or affected by this change.

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/

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

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

Patch Set 12: Code-Review-1

(1 comment)

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

Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: 
Line 20: import logging
Line 21: import curlImgWrap
Liron, I do not quite get why you keep the curlImgWrap with the stdlib imports, and not with the vdsm imports, as it should.

It's not a big deal, it's just following standard, to help reviewers and make future rebases easier.
Line 22: import signal
Line 23: import socket
Line 24: 
Line 25: from vdsm import constants

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

Liron Ar has posted comments on this change.

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

Patch Set 12:

(1 comment)

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

Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: 
Line 20: import logging
Line 21: import curlImgWrap
> Liron, I do not quite get why you keep the curlImgWrap with the stdlib impo

Done
Line 22: import signal
Line 23: import socket
Line 24: 
Line 25: from vdsm import constants

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 12:

(1 comment)

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

Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: 
Line 20: import logging
Line 21: import curlImgWrap
> Liron, I do not quite get why you keep the curlImgWrap with the stdlib impo

Do you suggest to import signal and socket after logging, and add a blank line before curlImgWrap?
Line 22: import signal
Line 23: import socket
Line 24: 
Line 25: from vdsm import constants

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 13: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7275/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6377/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7161/ : SUCCESS

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 13: Code-Review+1

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

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

Patch Set 13: Code-Review+2

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

Liron Ar has posted comments on this change.

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

Patch Set 13: Verified+1

verified - tested upload of 1000 ovfs, both ISCSI and NFS. vdsm - rhel 6.5, engine - rhel 6.4

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

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

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

introducing capability to stream data to image

This patch introduces the a new capabaility to vdsm
which allows to upload using streaming content to vdsm
images.

Previously we sent ovf data using XMLRPC (UpdateVM verb),
which limits the size of the data, having to encode the
payload into the xml, and make it hard and inefficient
to upload lot of data and store it on some image.

This patch adds the capabillity to stream data to image,
allowing efficient upload of data in any size and format
and storing it directly on an
image. As the  XML-RPC spec doesn't support streaming and
to avoid requiring another port by using dedicated http
server, in this patch we use the existing xmlrpc server to
handle upload requests.

General upload information:
-----------------------------------------------------------
PUT requests arriving to the server with content type of
application/octet-stream to default paths that we use
today for request handling ('/', '/RPC2') will be treated
as upload requests.
The upload itself is being executed within a task, that's
needed to indicate that there's an operation executed by
the host.

Change-Id: I768b84799ed9fb2769c6d4240519d036f8988b99
Signed-off-by: Liron Aravot 
Reviewed-on: http://gerrit.ovirt.org/23281
Reviewed-by: Nir Soffer 
Reviewed-by: Dan Kenigsberg 
---
M vdsm/API.py
M vdsm/BindingXMLRPC.py
M vdsm/storage/hsm.py
M vdsm/storage/image.py
M vdsm/storage/imageSharing.py
M vdsm/storage/sp.py
6 files changed, 211 insertions(+), 3 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved
  Liron Ar: Verified