Change in vdsm[master]: storage: Introduction to imagetickets.py

nsoffer at redhat.com nsoffer at redhat.com
Tue Jan 12 22:40:36 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: storage: Introduction to imagetickets.py
......................................................................


Patch Set 8:

(21 comments)

Much nicer now

https://gerrit.ovirt.org/#/c/50014/8/client/vdsClient.py
File client/vdsClient.py:

Line 1620
Line 1621
Line 1622
Line 1623
Line 1624
We are not adding new features to vdsClient. If you want to add this now for testing, please separate the vdsClient and xmlrpc changes to separate patch, so we can revert them easily when we merge the replacement jsonrpc client (patch in review).


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

Line 524: {'command': {'class': 'Host', 'name': 'setHaMaintenanceMode'},
Line 525:  'data': {'mode': 'HaMaintenanceMode', 'enabled': 'bool'}}
Line 526: 
Line 527: ##
Line 528: # @Host.addImageTicket:
Adam started to add lowercase_verbs (see SDM.create_volume), and I like this style better. Lets rename to verbs to add_image_ticket, remove_imge_ticket and extend_image_ticket
Line 529: #
Line 530: # Start a session to expose an image via imaged daemon https api for
Line 531: # reading and writing image data.
Line 532: #


Line 534: #
Line 535: # Since: 4.18.0
Line 536: ##
Line 537: {'command': {'class': 'Host', 'name': 'addImageTicket'},
Line 538:  'data': {'ticket': 'dict'}}
You need to define type for ticket instead of "dict". For example, see VolumeInfo
Line 539: 
Line 540: ##
Line 541: # @Host.removeImageTicket:
Line 542: #


Line 555: # Extend an image upload data transfer session.
Line 556: #
Line 557: # @ticket_uuid: uuid of the ticket to be extended
Line 558: #
Line 559: # @expires:     new expiration time for the ticket
in seconds since 1970-01-01 00:00:00 UTC.

Hopfully this is what you send from engine?
Line 560: #
Line 561: # Since: 4.18.0
Line 562: ##
Line 563: {'command': {'class': 'Host', 'name': 'extendImageTicket'},


Line 560: #
Line 561: # Since: 4.18.0
Line 562: ##
Line 563: {'command': {'class': 'Host', 'name': 'extendImageTicket'},
Line 564:  'data': {'ticket_uuid': 'UUID', 'expires': 'int'}}
expires should be 'uint'
Line 565: 
Line 566: 
Line 567: ##
Line 568: # @TaskDetails:


https://gerrit.ovirt.org/#/c/50014/8/vdsm.spec.in
File vdsm.spec.in:

Line 907: %{_datadir}/%{vdsm_name}/storage/fileSD.py*
Line 908: %{_datadir}/%{vdsm_name}/storage/fileUtils.py*
Line 909: %{_datadir}/%{vdsm_name}/storage/fileVolume.py*
Line 910: %{_datadir}/%{vdsm_name}/storage/fuser.py*
Line 911: %{_datadir}/%{vdsm_name}/storage/imagetickets.py*
We need to add this also in debian/vdsm.install - same file were other storage modules are specified.
Line 912: %{_datadir}/%{vdsm_name}/storage/glusterSD.py*
Line 913: %{_datadir}/%{vdsm_name}/storage/glusterVolume.py*
Line 914: %{_datadir}/%{vdsm_name}/storage/hba.py*
Line 915: %{_datadir}/%{vdsm_name}/storage/hsm.py*


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

Line 1622:             self.log.exception("error setting HA maintenance mode")
Line 1623:             return errCode['haErr']
Line 1624:         return {'status': doneCode}
Line 1625: 
Line 1626:     def addImageTicket(self, ticket):
If we rename the apis to lower_case, we should rename also here.
Line 1627:         return self._irs.addImageTicket(ticket)
Line 1628: 
Line 1629:     def removeImageTicket(self, ticket_uuid):
Line 1630:         return self._irs.removeImageTicket(ticket_uuid)


https://gerrit.ovirt.org/#/c/50014/8/vdsm/rpc/bindingxmlrpc.py
File vdsm/rpc/bindingxmlrpc.py:

Line 638
Line 639
Line 640
Line 641
Line 642
Separate this change with vdsClient change if you need it now, otherwise remove it.


https://gerrit.ovirt.org/#/c/50014/8/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 3102:             sdUUID=sdUUID).produceVolume(imgUUID=imgUUID,
Line 3103:                                          volUUID=volUUID).refreshVolume()
Line 3104: 
Line 3105:     @public
Line 3106:     def addImageTicket(self, ticket):
If we rename to apis to lower_case, rename also here.
Line 3107:         return imagetickets.add_ticket(ticket)
Line 3108: 
Line 3109:     @public
Line 3110:     def removeImageTicket(self, ticket_uuid):


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

Line 1: from contextlib import closing
You need the standard copyright block, copy from another module.
Line 2: import logging
Line 3: import socket
Line 4: import json
Line 5: 


Line 1: from contextlib import closing
Line 2: import logging
Line 3: import socket
Line 4: import json
sort imports
Line 5: 
Line 6: import storage_exception as se
Line 7: 
Line 8: 


Line 5: 
Line 6: import storage_exception as se
Line 7: 
Line 8: 
Line 9: _imaged_installed = False
Lets call it _have_imaged, we don't check if it is installed, only if the python module is available.
Line 10: 
Line 11: try:
Line 12:     import imaged.uhttp as uhttp
Line 13:     import imaged.server as imgdServer


Line 8: 
Line 9: _imaged_installed = False
Line 10: 
Line 11: try:
Line 12:     import imaged.uhttp as uhttp
Use from imaged import uhttp
Line 13:     import imaged.server as imgdServer
Line 14:     _imaged_installed = True
Line 15: except ImportError:
Line 16:     pass


Line 9: _imaged_installed = False
Line 10: 
Line 11: try:
Line 12:     import imaged.uhttp as uhttp
Line 13:     import imaged.server as imgdServer
Since we are  interested only the Config class, and we really do not want anything from the server module, lets do:

    from imaged.server import Config
Line 14:     _imaged_installed = True
Line 15: except ImportError:
Line 16:     pass
Line 17: 


Line 19: 
Line 20: 
Line 21: def validate_supported():
Line 22:     if not _imaged_installed:
Line 23:         raise se.ImageTicketsError("Imaged is not installed!")
Repeating this in all verbs works, but it would be little nicer to add a decorator instead:

    def requires_imaged(func):
        @functools.wraps(func)
        def wrapper(*args, **kw):
            if not _have_imaged:
                raise se.ImageTicketError("Cannot import imaged")
            return func(*args, **kw)

Now we can do:

    @requires_imaged
    def add_ticket(ticket):
        ...
Line 24: 
Line 25: 
Line 26: def add_ticket(ticket):
Line 27:     validate_supported()


Line 46:     return json.loads(resbody) if resbody else None
Line 47: 
Line 48: 
Line 49: def request(method, path, body=None):
Line 50:     log.info("Sending request (method=%r, path=%r, body=%r)" %
For logging, we don't format the message, we provide a format string and arguuments. Logging formatter will format the message for us:

    log.info("format string %s %r", arg1, arg2)
Line 51:              (method, path, body))
Line 52: 
Line 53:     con = uhttp.UnixHTTPConnection(imgdServer.Config().socket)
Line 54:     try:


Line 54:     try:
Line 55:         res = unix_request(con, method, path, body)
Line 56:     except socket.error as e:
Line 57:         raise se.ImageTicketsError("Error connecting to imaged: {} - {}"
Line 58:                                    .format(e.errno, e.strerror))
This is no how this error should be used. Looking in storage_excpetion, it expects code, reason and body, but you provide only a string.

Maybe we need different errors based on type of error - error sending request to imaged, error from imaged. These errors have different information - first have errno and description, second have http error code, and some other fields from imaged.

Don't use bare {} for formating, use names:

   "{foo} - {bar}".format(foo="...", bar="...")
Line 59:     if res.status not in (200, 204):
Line 60:         raise se.ImageTicketsError(
Line 61:             res.status, res.reason, res.read())
Line 62: 


Line 57:         raise se.ImageTicketsError("Error connecting to imaged: {} - {}"
Line 58:                                    .format(e.errno, e.strerror))
Line 59:     if res.status not in (200, 204):
Line 60:         raise se.ImageTicketsError(
Line 61:             res.status, res.reason, res.read())
I commented about this in the previous version, you should read exactly content-legnth bytes.

After you read, you should parse the error, and initiialize the error with the fields image is sending.

If parsing failed, you may generate the error fileds yourself, and add the body as is.
Line 62: 
Line 63:     return res
Line 64: 
Line 65: 


Line 62: 
Line 63:     return res
Line 64: 
Line 65: 
Line 66: def unix_request(con, method, uri, body=None, headers=None):
This is strange - how is this related to unix socket request?

Not clear why this is not part of request(). I think you should inline this code into request() and rename request() to unix_request.
Line 67:     with closing(con):
Line 68:         con.request(method, uri, body, headers=headers or {})
Line 69:         res = con.getresponse()


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

Line 1207: class ImageTicketsError(StorageException):
Line 1208:     code = 481
Line 1209:     message = "Unsuccessful request to imaged"
Line 1210: 
Line 1211:     def __init__(self, code, reason, body):
Having code here is confusing, since exception have a code attribute.

We should parse body, and initialize the error with the info return from imaged errors.
Line 1212:         self.value = "result: %d %s - %r" % (code, reason, body)
Line 1213: 
Line 1214: 
Line 1215: class TransferTicketError(StorageException):


Line 1211:     def __init__(self, code, reason, body):
Line 1212:         self.value = "result: %d %s - %r" % (code, reason, body)
Line 1213: 
Line 1214: 
Line 1215: class TransferTicketError(StorageException):
Not clear why do we need this error, and what is the difference between the two.
Line 1216:     code = 482
Line 1217:     message = "Unexpected error initiating transfer session"
Line 1218: 
Line 1219:     def __init__(self, reason):


-- 
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke 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