Change in vdsm[master]: imagetickets: Support image tickets

nsoffer at redhat.com nsoffer at redhat.com
Thu Jan 21 10:22:50 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: imagetickets: Support image tickets
......................................................................


Patch Set 11:

(19 comments)

https://gerrit.ovirt.org/#/c/50014/11/lib/api/vdsmapi-schema.json
File lib/api/vdsmapi-schema.json:

Line 526: 
Line 527: ##
Line 528: # @ImageTicketOps:
Line 529: #
Line 530: # An enumeration of operation types to request in an image ticket
An enumeration of operation types allowed by an image ticket
Line 531: #
Line 532: # @put:     a request for writing permissions to an image
Line 533: #
Line 534: # @get:     a request for reading permissions to an image


Line 528: # @ImageTicketOps:
Line 529: #
Line 530: # An enumeration of operation types to request in an image ticket
Line 531: #
Line 532: # @put:     a request for writing permissions to an image
permission to write data to image
Line 533: #
Line 534: # @get:     a request for reading permissions to an image
Line 535: #
Line 536: # Since: 4.18.0


Line 530: # An enumeration of operation types to request in an image ticket
Line 531: #
Line 532: # @put:     a request for writing permissions to an image
Line 533: #
Line 534: # @get:     a request for reading permissions to an image
permission to read data from image
Line 535: #
Line 536: # Since: 4.18.0
Line 537: ##
Line 538: {'enum': 'ImageTicketOps',


Line 535: #
Line 536: # Since: 4.18.0
Line 537: ##
Line 538: {'enum': 'ImageTicketOps',
Line 539:  'data': ['put', 'get']}
Should we use ['read', 'write']?

Seems that we are using the http method instead of the related image operation.
Line 540: 
Line 541: ##
Line 542: # @ImageTicket:
Line 543: #


Line 546: # @uuid:      A ticket UUID
Line 547: #
Line 548: # @timeout:   Timeout in seconds for the ticket to expire
Line 549: #
Line 550: # @ops:       operations list to be applied on the image
operations allowed by this ticket
Line 551: #
Line 552: # @size:      The image size
Line 553: #
Line 554: # @url:       The image url


Line 548: # @timeout:   Timeout in seconds for the ticket to expire
Line 549: #
Line 550: # @ops:       operations list to be applied on the image
Line 551: #
Line 552: # @size:      The image size
Maximum size (in bytes) that can be written or read from the image.

This happen to be the image size currently, but in the future we may have a reason to limit to part of the image. The daemon does not care about the image size, only about allowing or or forbidding a request.
Line 553: #
Line 554: # @url:       The image url
Line 555: #
Line 556: # Since: 4.18.0


Line 550: # @ops:       operations list to be applied on the image
Line 551: #
Line 552: # @size:      The image size
Line 553: #
Line 554: # @url:       The image url
Give an example to make this more clear: file:///path/to/image
Line 555: #
Line 556: # Since: 4.18.0
Line 557: ##
Line 558: {'type': 'ImageTicket',


Line 555: #
Line 556: # Since: 4.18.0
Line 557: ##
Line 558: {'type': 'ImageTicket',
Line 559:  'data': {'uuid': 'UUID', 'timeout': 'uint', 'ops': 'ImageTicketOps',
ops: ['ImageTicketOps']
Line 560:           'size': 'uint', 'url': 'str'}}
Line 561: 
Line 562: ##
Line 563: # @Host.add_image_ticket:


https://gerrit.ovirt.org/#/c/50014/11/vdsm/API.py
File vdsm/API.py:

Line 1624:         return {'status': doneCode}
Line 1625: 
Line 1626:     def add_image_ticket(self, ticket):
Line 1627:         self._irs.add_image_ticket(ticket)
Line 1628:         return {'status': doneCode}
This is deprecated in vdsm, new code must return response.success() or response.error().

But irs methods handlers this for you, since they are wrapped by storage.dispatcher.Dispatcher. Use:

    return self._irs.add_image_ticket(ticket)

Same for the other methods.

We will improve this in 4.0.
Line 1629: 
Line 1630:     def remove_image_ticket(self, uuid):
Line 1631:         self._irs.remove_image_ticket(uuid)
Line 1632:         return {'status': doneCode}


https://gerrit.ovirt.org/#/c/50014/11/vdsm/storage/imagetickets.py
File vdsm/storage/imagetickets.py:

Line 30: 
Line 31: 
Line 32: try:
Line 33:     from ovirt_image_daemon import uhttp
Line 34:     from ovirt_image_daemon.server import Config
Lets remove this import, and use hardconded path instead.
Line 35: 
Line 36:     _have_imaged = True
Line 37: except ImportError:
Line 38:     _have_imaged = False


Line 31: 
Line 32: try:
Line 33:     from ovirt_image_daemon import uhttp
Line 34:     from ovirt_image_daemon.server import Config
Line 35: 
Remove unneeded whitespace
Line 36:     _have_imaged = True
Line 37: except ImportError:
Line 38:     _have_imaged = False
Line 39: 


Line 32: try:
Line 33:     from ovirt_image_daemon import uhttp
Line 34:     from ovirt_image_daemon.server import Config
Line 35: 
Line 36:     _have_imaged = True
_have_image_daemon
Line 37: except ImportError:
Line 38:     _have_imaged = False
Line 39: 
Line 40: log = logging.getLogger('storage.imagetickets')


Line 37: except ImportError:
Line 38:     _have_imaged = False
Line 39: 
Line 40: log = logging.getLogger('storage.imagetickets')
Line 41: 
Add constant for the unix socket:

DAEMON_SOCK = os.path.join(constants.P_VDSM_RUN, "ovirt-image-daemon.sock")
Line 42: 
Line 43: def requires_imaged(func):
Line 44:     @functools.wraps(func)
Line 45:     def wrapper(*args, **kw):


Line 67:     request(uhttp.DELETE, uuid)
Line 68: 
Line 69: 
Line 70: def request(method, uuid, body=None):
Line 71:     log.debug("Sending request method=%r, body=%r", method, body)
Log also the ticket uuid
Line 72:     con = uhttp.UnixHTTPConnection(Config().socket)
Line 73:     with closing(con):
Line 74:         try:
Line 75:             con.request(method, "/tickets/%s" % uuid, body, {})


Line 77:             try:
Line 78:                 content_length = int(res.getheader("content-length", "0"))
Line 79:             except ValueError:
Line 80:                 raise se.ImageDeamonError("Invalid response without "
Line 81:                                           "content-length")
Getting the content length is needed only if we got an error, and we do't have any request with return value for now.

We are also hiding the response status.

Use:

    error_info = {"explanation": "invalid response without content-length"}
    raise se.ImageDaemonError(res.status, res.reason, error_info)
Line 82:             if res.status >= 300:
Line 83:                 try:
Line 84:                     error_info = json.loads(res.read(content_length))
Line 85:                 except ValueError:


Line 78:                 content_length = int(res.getheader("content-length", "0"))
Line 79:             except ValueError:
Line 80:                 raise se.ImageDeamonError("Invalid response without "
Line 81:                                           "content-length")
Line 82:             if res.status >= 300:
Get the content length here.
Line 83:                 try:
Line 84:                     error_info = json.loads(res.read(content_length))
Line 85:                 except ValueError:
Line 86:                     error_info = {"code": res.status, "reason": res.reason}


Line 83:                 try:
Line 84:                     error_info = json.loads(res.read(content_length))
Line 85:                 except ValueError:
Line 86:                     error_info = {"code": res.status, "reason": res.reason}
Line 87:                 raise se.ImageDeamonError(error_info)
Since we want to remove the code and title from the daemon, we should not use them here.

Use this instead:

    try:
        error_info = json.loads(res.read(content_length))
    except ValueError as e:
        error_info = {"explanation": "error parsing response from server",
                             "detail": str(e)}
        raise se.ImageDaemonError(res.status, res.reason, error_info)
Line 88:         except (httplib.HTTPException, socket.error) as e:
Line 89:             raise se.ImageTicketsError("{errtype} error when connecting to "
Line 90:                                        "imaged: {errno} - {strerror}"
Line 91:                                        .format(errtype=type(e).__name__,


Line 89:             raise se.ImageTicketsError("{errtype} error when connecting to "
Line 90:                                        "imaged: {errno} - {strerror}"
Line 91:                                        .format(errtype=type(e).__name__,
Line 92:                                                errno=e.errno,
Line 93:                                                strerror=e.strerror))
This is not error when connection, it may be also error when getting response.

HTTPException does not have errno or strerror attributes, so you will fail with AttributeError hiding the real error.

The best way to deal with multiple error is the include the string representation of the error, and not depend on the error attributes. 

Use:

    raise se.ImageTicketsError("Error communicating with ovirt-image-daemon: {error}".format(error=e))


https://gerrit.ovirt.org/#/c/50014/11/vdsm/storage/storage_exception.py
File vdsm/storage/storage_exception.py:

Line 1215: class ImageDeamonError(StorageException):
Line 1216:     code = 482
Line 1217:     message = "Error returned from Imaged"
Line 1218: 
Line 1219:     def __init__(self, error_info):
Since you want to remove error_info code and title, we should provide them to this error.

Also using dict.iteritems is unusual, and it does not work with Python 3, since iteritems was removed there. Since the item is list is small (3 or 4), better use items() which is supported in both Python 2 and 3.

Use:

    def __init__(self, status, reason, error_info):
        d = error_info.copy()
        d['status'] = status
        d['reason'] = reason
        self.value = ", ".join("%s=%s" % (k, v) for k, v in d.items())
Line 1220:         self.value = ", ".join("%s=%s" % (k, v)
Line 1221:                                for k, v in dict.iteritems(error_info))
Line 1222: 
Line 1223: 


-- 
To view, visit https://gerrit.ovirt.org/50014
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b9ded4bde73b1ab504cae50d2cea726d4f77e51
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Greg Padgett <gpadgett at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list