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

aaviram at redhat.com aaviram at redhat.com
Tue Jan 12 08:42:04 UTC 2016


Amit Aviram 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
Done
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
Done
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
Done
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"
Done
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)
Done
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 ar
Done
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>
Done
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
Done
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.
a python dict. what is its type then?
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 ti
Done
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 
Done
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 d
Done
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?
Done
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
Ok, I will change that when imaged's change will be merged.
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
Done
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
Done
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
How about:
ImageTicketsError("Imaged is not installed!") ?
otherwise it means creating an error class just for an unsupported scenario
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:
Done
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
Done
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:
Done
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 htt
Done
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 
Done
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)
Done
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 im
It will be helpful in later requests to imaged and makes the communication between engine and imaged to work in two directions instead of one. I think we should keep it and define the return type in the schema in case the method we are using does expect to return anything (the current 3 does not)
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
Done
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
Done
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):
Done
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.
Done
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