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

nsoffer at redhat.com nsoffer at redhat.com
Wed Jan 6 21:59:08 UTC 2016


Nir Soffer has posted comments on this change.

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


Patch Set 6:

(28 comments)

https://gerrit.ovirt.org/#/c/50014/6//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2015-12-13 11:33:57 +0200
Line 4: Commit:     Amit Aviram <aaviram at redhat.com>
Line 5: CommitDate: 2016-01-06 20:06:55 +0200
Line 6: 
Line 7: storage: Introduction to transfer.py
Lets update it to describe the new imagetickets module
Line 8: 
Line 9: The introduced module is part of the image upload implementation, which
Line 10: lets the end user of oVirt transfer images to and from oVirt storage
Line 11: devices.


Line 12: 
Line 13: Specifically, this VDSM module is responsible for being an intermediary
Line 14: between the engine and imaged- for initiating actions regarding
Line 15: the transfer process. This kinds of actions can be generating sessions,
Line 16: extending sessions, deleting sessions, etc.
sessions -> tickets
Line 17: 
Line 18: Change-Id: I6b9ded4bde73b1ab504cae50d2cea726d4f77e51


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

Line 1621:         if stats['status']['code']:
Line 1622:             return stats['status']['code'], stats['status']['message']
Line 1623:         return 0, ''
Line 1624: 
Line 1625:     def do_addImageTicket(self, args):
do_ prefix is not needed
Line 1626:         ticket = args[0]
Line 1627:         stats = self.s.addImageTicket(ticket)
Line 1628:         if stats['status']['code']:
Line 1629:             return stats['status']['code'], stats['status']['message']


Line 1624: 
Line 1625:     def do_addImageTicket(self, args):
Line 1626:         ticket = args[0]
Line 1627:         stats = self.s.addImageTicket(ticket)
Line 1628:         if stats['status']['code']:
This is not stats, it is a result. In new code we are using "res"
Line 1629:             return stats['status']['code'], stats['status']['message']
Line 1630:         return 0, ''
Line 1631: 
Line 1632:     def do_removeImageTicket(self, args):


Line 1629:             return stats['status']['code'], stats['status']['message']
Line 1630:         return 0, ''
Line 1631: 
Line 1632:     def do_removeImageTicket(self, args):
Line 1633:         ticketId = args[0]
use lowercase names (e.g. ticket_id)
Line 1634:         stats = self.s.removeImageTicket(ticketId)
Line 1635:         if stats['status']['code']:
Line 1636:             return stats['status']['code'], stats['status']['message']
Line 1637:         return 0, ''


Line 2621:                                   ' <enabled = true/false>',
Line 2622:                                   'Enable or disable Hosted Engine HA'
Line 2623:                                   ' maintenance')),
Line 2624:         'addImageTicket': (serv.do_addImageTicket,
Line 2625:                            ('<ticket>', 'Add a ticket for an image')),
Need to document what is a ticket - what are the fields and which values are valid, etc.

Is ticket an opaque string pass via vdsm, or a python dict?
Line 2626:         'removeImageTicket': (serv.do_removeImageTicket,
Line 2627:                               ('<ticket>', 'Remove a ticket for an image')),
Line 2628:         'extendImageTicket': (serv.do_extendImageTicket,
Line 2629:                               ('<ticket> ', 'Extend a ticket for an image')),


Line 2623:                                   ' maintenance')),
Line 2624:         'addImageTicket': (serv.do_addImageTicket,
Line 2625:                            ('<ticket>', 'Add a ticket for an image')),
Line 2626:         'removeImageTicket': (serv.do_removeImageTicket,
Line 2627:                               ('<ticket>', 'Remove a ticket for an image')),
<ticket> -> <ticket_id>
Line 2628:         'extendImageTicket': (serv.do_extendImageTicket,
Line 2629:                               ('<ticket> ', 'Extend a ticket for an image')),
Line 2630:         'deleteImage': (serv.deleteImage,
Line 2631:                         ('<sdUUID> <spUUID> <imgUUID> [<postZero>] [<force>]',


Line 2625:                            ('<ticket>', 'Add a ticket for an image')),
Line 2626:         'removeImageTicket': (serv.do_removeImageTicket,
Line 2627:                               ('<ticket>', 'Remove a ticket for an image')),
Line 2628:         'extendImageTicket': (serv.do_extendImageTicket,
Line 2629:                               ('<ticket> ', 'Extend a ticket for an image')),
<ticket> -> <ticket_id>, document timeout
Line 2630:         'deleteImage': (serv.deleteImage,
Line 2631:                         ('<sdUUID> <spUUID> <imgUUID> [<postZero>] [<force>]',
Line 2632:                          'Delete Image folder with all volumes.',
Line 2633:                          )),


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

Line 534: #
Line 535: # Since: 4.18.0
Line 536: ##
Line 537: {'command': {'class': 'Host', 'name': 'addImageTicket'},
Line 538:  'data': {'ticket': 'dict'}}
We don't have "dict" type.

What do you send from engine today?
Line 539: 
Line 540: ##
Line 541: # @Host.removeImageTicket:
Line 542: #


Line 546: #
Line 547: # Since: 4.18.0
Line 548: ##
Line 549: {'command': {'class': 'Host', 'name': 'removeImageTicket'},
Line 550:  'data': {'ticket': 'dict'}}
vdsmClient say that we pass here a ticket uuid. Why do we need to pass a ticket for removing it? a ticket id is enough.
Line 551: 
Line 552: ##
Line 553: # @Host.extendImageTicket:
Line 554: #


Line 558: #
Line 559: # Since: 4.18.0
Line 560: ##
Line 561: {'command': {'class': 'Host', 'name': 'extendImageTicket'},
Line 562:  'data': {'ticket': 'dict'}}
vdsClient says that we pass a ticket id and timeout. What is the real data passed?
Line 563: 
Line 564: 
Line 565: ##
Line 566: # @TaskDetails:


https://gerrit.ovirt.org/#/c/50014/6/vdsm/rpc/Bridge.py
File vdsm/rpc/Bridge.py:

Line 425:     'Host_startMonitoringDomain': {},
Line 426:     'Host_stopMonitoringDomain': {},
Line 427:     'Host_addImageTicket': {},
Line 428:     'Host_removeImageTicket': {},
Line 429:     'Host_extendImageTicket': {},
Do we really need this? we don't have a return value, so we don't need to define anything here.
Line 430:     'Host_getVMList': {'call': Host_getVMList_Call, 'ret': 'vmList'},
Line 431:     'Host_getVMFullList': {'call': Host_getVMFullList_Call, 'ret': 'vmList'},
Line 432:     'Host_getAllVmStats': {'ret': 'statsList'},
Line 433:     'Host_setupNetworks': {'ret': 'status'},


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

Line 3103:                                          volUUID=volUUID).refreshVolume()
Line 3104: 
Line 3105:     @public
Line 3106:     def addImageTicket(self, ticket):
Line 3107:         imagetickets.validate_supported()
embed the validation in the call, why hsm should care about this?
Line 3108:         return imagetickets.add_ticket(ticket)
Line 3109: 
Line 3110:     @public
Line 3111:     def removeImageTicket(self, ticket):


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

Line 9: _imagedInstalled = False
Line 10: 
Line 11: try:
Line 12:     import imaged.uhttp as uhttp
Line 13:     import imaged.server as imgdServer
We should import the server. I will move the configuration to config module so you can get the socket address from the config module, or maybe from the config file.
Line 14:     _imagedInstalled = True
Line 15: except ImportError:
Line 16:     pass
Line 17: 


Line 10: 
Line 11: try:
Line 12:     import imaged.uhttp as uhttp
Line 13:     import imaged.server as imgdServer
Line 14:     _imagedInstalled = True
no mixedCase in vdsm, only lower_case
Line 15: except ImportError:
Line 16:     pass
Line 17: 
Line 18: log = logging.getLogger('storage.transfer')


Line 14:     _imagedInstalled = True
Line 15: except ImportError:
Line 16:     pass
Line 17: 
Line 18: log = logging.getLogger('storage.transfer')
transfer -> imagetickets
Line 19: 
Line 20: 
Line 21: def validate_supported():
Line 22:     if not _imagedInstalled:


Line 19: 
Line 20: 
Line 21: def validate_supported():
Line 22:     if not _imagedInstalled:
Line 23:         raise se.TransferTicketError("Imaged is not installed!")
TransferTicketError -> ImageTicketsUnsupported
Line 24: 
Line 25: 
Line 26: def add_ticket(ticket):
Line 27:     ticket_request(uhttp.PUT, ticket)


Line 23:         raise se.TransferTicketError("Imaged is not installed!")
Line 24: 
Line 25: 
Line 26: def add_ticket(ticket):
Line 27:     ticket_request(uhttp.PUT, ticket)
We should create the body here:

    def add_ticket(ticket):
        body = json.dumps(ticket)
        ticket_request(utthp.PUT, ticket["uuid"], body)
Line 28:     return True
Line 29: 
Line 30: 
Line 31: def extend_ticket(ticket):


Line 24: 
Line 25: 
Line 26: def add_ticket(ticket):
Line 27:     ticket_request(uhttp.PUT, ticket)
Line 28:     return True
No return value is needed
Line 29: 
Line 30: 
Line 31: def extend_ticket(ticket):
Line 32:     ticket_request(uhttp.PATCH, ticket)


Line 28:     return True
Line 29: 
Line 30: 
Line 31: def extend_ticket(ticket):
Line 32:     ticket_request(uhttp.PATCH, ticket)
Here the request should be:

    PATCH /ticket/uuid

With json body:

    {"expires": 12345}

We can pass the dict from engine, but I think the api will be more clear if we do this instead:

   def extend_ticket(ticket_uuid, expires):
       body json.dumps({"expires": expires})
       ticket_request(uhttp.PATCH, ticket_uuid, body)
Line 33:     return True
Line 34: 
Line 35: 
Line 36: def remove_ticket(ticket):


Line 33:     return True
Line 34: 
Line 35: 
Line 36: def remove_ticket(ticket):
Line 37:     ticket_request(uhttp.DELETE, ticket)
This will send DELETE request with a body - I don't think this is valid http, and it does not make sense.

remove_ticket needs only a ticket_id, there no reason to send a ticket,
the the request should be:

    DELETE /tickets/uuid

Without any content.

This should work:

    def remove_ticket(ticket_uuid):
        ticket_request(uhttp.DELETE, ticket_uuid)
Line 38:     return True
Line 39: 
Line 40: 
Line 41: def ticket_request(method, ticket):


Line 38:     return True
Line 39: 
Line 40: 
Line 41: def ticket_request(method, ticket):
Line 42:     body = json.dumps(ticket)
Since we don't want body in all requests, the body should be passed by the caller.

We need here a ticket_id parameter, and optional body.
Line 43:     res = request(method, "/tickets/%s" % ticket["uuid"], body)
Line 44:     resbody = res.read()
Line 45:     return json.loads(resbody) if resbody else True
Line 46: 


Line 39: 
Line 40: 
Line 41: def ticket_request(method, ticket):
Line 42:     body = json.dumps(ticket)
Line 43:     res = request(method, "/tickets/%s" % ticket["uuid"], body)
res = request(method, "/tickets/" + ticket_id, body)
Line 44:     resbody = res.read()
Line 45:     return json.loads(resbody) if resbody else True
Line 46: 
Line 47: 


Line 41: def ticket_request(method, ticket):
Line 42:     body = json.dumps(ticket)
Line 43:     res = request(method, "/tickets/%s" % ticket["uuid"], body)
Line 44:     resbody = res.read()
Line 45:     return json.loads(resbody) if resbody else True
We don't need to return True. Do you want to pass the json returned from imaged to engine?

In this case we should define the return type in the schema, and always return the json from ticket requests.
Line 46: 
Line 47: 
Line 48: def request(method, path, body=None):
Line 49:     log.info("Sending request (method=%r, path=%r, body=%r)" %


Line 47: 
Line 48: def request(method, path, body=None):
Line 49:     log.info("Sending request (method=%r, path=%r, body=%r)" %
Line 50:              (method, path, body))
Line 51:     config = imgdServer.Config()
We don't need the entire config
Line 52:     try:
Line 53:         res = unix_request(config, method, path, body)
Line 54:     except socket.error as e:
Line 55:         raise se.TransferTicketError("Error connecting to imaged: {} - {}"


Line 51:     config = imgdServer.Config()
Line 52:     try:
Line 53:         res = unix_request(config, method, path, body)
Line 54:     except socket.error as e:
Line 55:         raise se.TransferTicketError("Error connecting to imaged: {} - {}"
TransferTicketError -> ImageTicketError
Line 56:                                      .format(e.errno, e.strerror))
Line 57:     if res.status != 200 and res.status != 204:
Line 58:         raise se.TransferTicketRequestError(
Line 59:             res.status, res.reason, res.read())


Line 53:         res = unix_request(config, method, path, body)
Line 54:     except socket.error as e:
Line 55:         raise se.TransferTicketError("Error connecting to imaged: {} - {}"
Line 56:                                      .format(e.errno, e.strerror))
Line 57:     if res.status != 200 and res.status != 204:
if res.status not in (200, 204):
        raise ...
Line 58:         raise se.TransferTicketRequestError(
Line 59:             res.status, res.reason, res.read())
Line 60: 
Line 61:     return res


Line 61:     return res
Line 62: 
Line 63: 
Line 64: def unix_request(config, method, uri, body=None, headers=None):
Line 65:     con = uhttp.UnixHTTPConnection(config.socket)
We need only the socket, so pass the socket to the request function.
Line 66:     with closing(con):
Line 67:         con.request(method, uri, body, headers=headers or {})
Line 68:         res = con.getresponse()


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