Change in vdsm[master]: introducing downloadImageFromStream

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