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

nsoffer at redhat.com nsoffer at redhat.com
Wed Dec 9 22:23:30 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 2:

(21 comments)

https://gerrit.ovirt.org/#/c/50014/2/vdsm/storage/Makefile.am
File vdsm/storage/Makefile.am:

Line 34: 	fileSD.py \
Line 35: 	fileUtils.py \
Line 36: 	fileVolume.py \
Line 37: 	fuser.py \
Line 38: 	transfer.py \
Lets call this imaged.py

Then we can have nice names like imaged.start_session().
Line 39: 	glusterSD.py \
Line 40: 	glusterVolume.py \
Line 41: 	hba.py \
Line 42: 	hsm.py \


https://gerrit.ovirt.org/#/c/50014/2/vdsm/storage/transfer.py
File vdsm/storage/transfer.py:

Line 12:     _imagedInstalled = True
Line 13: except ImportError:
Line 14:     _imagedInstalled = False
Line 15: 
Line 16: log = logging.getLogger('Storage.Transfer')
Use lowercase logger names.
Line 17: 
Line 18: 
Line 19: def prepare_session(ticket_uuid, ops, expiration, path):
Line 20:     ops = list([s.strip() for s in ops.split(',')])


Line 15: 
Line 16: log = logging.getLogger('Storage.Transfer')
Line 17: 
Line 18: 
Line 19: def prepare_session(ticket_uuid, ops, expiration, path):
Lets use the same terms we use in the public api, start_session and stop_session, instead of prepare and teardown.

Or, change the public api to prepare and teardown if you think it is better.
Line 20:     ops = list([s.strip() for s in ops.split(',')])
Line 21:     ticket = Ticket(ticket_uuid, ops=ops, expires=expiration,
Line 22:                     path=path, size=2**64)
Line 23:     ImagedHandler.ticket_request(ImagedHandler.Actions.PUT, ticket)


Line 16: log = logging.getLogger('Storage.Transfer')
Line 17: 
Line 18: 
Line 19: def prepare_session(ticket_uuid, ops, expiration, path):
Line 20:     ops = list([s.strip() for s in ops.split(',')])
We don't need this, engine should send list of ops and we should pass it as is.
Line 21:     ticket = Ticket(ticket_uuid, ops=ops, expires=expiration,
Line 22:                     path=path, size=2**64)
Line 23:     ImagedHandler.ticket_request(ImagedHandler.Actions.PUT, ticket)
Line 24:     return True


Line 17: 
Line 18: 
Line 19: def prepare_session(ticket_uuid, ops, expiration, path):
Line 20:     ops = list([s.strip() for s in ops.split(',')])
Line 21:     ticket = Ticket(ticket_uuid, ops=ops, expires=expiration,
Engine should prepare this dict, and we can pass it to imaged as is.
Line 22:                     path=path, size=2**64)
Line 23:     ImagedHandler.ticket_request(ImagedHandler.Actions.PUT, ticket)
Line 24:     return True
Line 25: 


Line 18: 
Line 19: def prepare_session(ticket_uuid, ops, expiration, path):
Line 20:     ops = list([s.strip() for s in ops.split(',')])
Line 21:     ticket = Ticket(ticket_uuid, ops=ops, expires=expiration,
Line 22:                     path=path, size=2**64)
unlimited size should be no size parameter, or special value like -1, not some huge number.
Line 23:     ImagedHandler.ticket_request(ImagedHandler.Actions.PUT, ticket)
Line 24:     return True
Line 25: 
Line 26: 


Line 19: def prepare_session(ticket_uuid, ops, expiration, path):
Line 20:     ops = list([s.strip() for s in ops.split(',')])
Line 21:     ticket = Ticket(ticket_uuid, ops=ops, expires=expiration,
Line 22:                     path=path, size=2**64)
Line 23:     ImagedHandler.ticket_request(ImagedHandler.Actions.PUT, ticket)
Use a put function instead.
Line 24:     return True
Line 25: 
Line 26: 
Line 27: def extend_session(ticket_uuid, expiration):


Line 24:     return True
Line 25: 
Line 26: 
Line 27: def extend_session(ticket_uuid, expiration):
Line 28:     ticket = Ticket(ticket_uuid, expires=expiration)
We don't need a ticket here, imaged needs only the id and the new expire time.
Line 29:     ImagedHandler.ticket_request(ImagedHandler.Actions.PATCH, ticket)
Line 30:     return True
Line 31: 
Line 32: 


Line 29:     ImagedHandler.ticket_request(ImagedHandler.Actions.PATCH, ticket)
Line 30:     return True
Line 31: 
Line 32: 
Line 33: def get_transfer_session_stats(ticket_uuid):
Not needed now, separate to another patch, or drop.
Line 34:     ticket = Ticket(ticket_uuid)
Line 35:     retval = ImagedHandler.ticket_request(ImagedHandler.Actions.GET, ticket)
Line 36:     return dict(bytesWritePosition=retval["bytes"])
Line 37: 


Line 41:     ImagedHandler.ticket_request(ImagedHandler.Actions.DELETE, ticket)
Line 42:     return True
Line 43: 
Line 44: 
Line 45: class Ticket(object):
We don't need this object, it does nothing useful, and we can use a regular dict instead.
Lets drop it, create a map in engine, and pass the map as is to imaged (converting it to json).
Line 46:     def __init__(self, uuid, expires=None, ops=None, path=None, size=None):
Line 47:         if not uuid:
Line 48:             raise se.TransferTicketError("Ticket UUID must not be empty!")
Line 49:         self.attr_dict = {


Line 54:             "path": path
Line 55:         }
Line 56: 
Line 57:     def get_existing_dict_attr(self):
Line 58:         return {attr: val for attr, val in self.attr_dict.items() if val}
You are re-implementing dict.copy()

    >>> {'a': 'b'}.copy()
    {'a': 'b'}
Line 59: 
Line 60:     def get_uuid(self):
Line 61:         return self.attr_dict["uuid"]
Line 62: 


Line 60:     def get_uuid(self):
Line 61:         return self.attr_dict["uuid"]
Line 62: 
Line 63: 
Line 64: class ImagedHandler(object):
No classes if you are not going to create an instance.
Line 65:     class Actions:
Line 66:         PUT = "PUT"
Line 67:         DELETE = "DELETE"
Line 68:         PATCH = "PATCH"


Line 65:     class Actions:
Line 66:         PUT = "PUT"
Line 67:         DELETE = "DELETE"
Line 68:         PATCH = "PATCH"
Line 69:         GET = "GET"
These constants are standard http constants. If they are not defined in Python standard library, lets defined them in utthp.py.
Line 70: 
Line 71:     @staticmethod
Line 72:     def ticket_request(method, ticket):
Line 73:         body = json.dumps(ticket.get_existing_dict_attr())


Line 67:         DELETE = "DELETE"
Line 68:         PATCH = "PATCH"
Line 69:         GET = "GET"
Line 70: 
Line 71:     @staticmethod
staticmethod are not allowed in vdsm. Use modules function instead.
Line 72:     def ticket_request(method, ticket):
Line 73:         body = json.dumps(ticket.get_existing_dict_attr())
Line 74:         res = ImagedHandler.request(method, "/tickets/%s" % ticket.get_uuid(),
Line 75:                                     body)


Line 68:         PATCH = "PATCH"
Line 69:         GET = "GET"
Line 70: 
Line 71:     @staticmethod
Line 72:     def ticket_request(method, ticket):
Lets call this session_request, and accept session id and dict.

    def session_request(method, id='', body=None):
        path = "/sessions/" + id
        return request(method, path, body)

There is no need to manipulate the response here, request should always return a response.
Line 73:         body = json.dumps(ticket.get_existing_dict_attr())
Line 74:         res = ImagedHandler.request(method, "/tickets/%s" % ticket.get_uuid(),
Line 75:                                     body)
Line 76:         resbody = res.read()


Line 73:         body = json.dumps(ticket.get_existing_dict_attr())
Line 74:         res = ImagedHandler.request(method, "/tickets/%s" % ticket.get_uuid(),
Line 75:                                     body)
Line 76:         resbody = res.read()
Line 77:         return json.loads(resbody) if resbody else True
We should always have a response.
Line 78: 
Line 79:     @staticmethod
Line 80:     def request(method, path, body=None):
Line 81:         if not _imagedInstalled:


Line 78: 
Line 79:     @staticmethod
Line 80:     def request(method, path, body=None):
Line 81:         if not _imagedInstalled:
Line 82:             raise se.TransferTicketError("Imaged is not installed!")
This should be done in the public verbs, maybe using validate_supported() utility function.
Line 83:         log.info("Imaged request: method:%s, path:%s, body:%s" %
Line 84:                  (method, path, body))
Line 85:         config = imgdServer.Config()
Line 86:         try:


Line 79:     @staticmethod
Line 80:     def request(method, path, body=None):
Line 81:         if not _imagedInstalled:
Line 82:             raise se.TransferTicketError("Imaged is not installed!")
Line 83:         log.info("Imaged request: method:%s, path:%s, body:%s" %
Use:

    "Sending request (methdo=%r, path=%r, body=%r)"
Line 84:                  (method, path, body))
Line 85:         config = imgdServer.Config()
Line 86:         try:
Line 87:             res = ImagedHandler.unix_request(config, method, path, body)


Line 86:         try:
Line 87:             res = ImagedHandler.unix_request(config, method, path, body)
Line 88:         except socket.error as e:
Line 89:             raise se.TransferTicketError("Error connecting to imaged: {} - {}"
Line 90:                                          .format(e.errno, e.strerror))
We should initialize TransferTicketError with the error, and it should format the error string.
Line 91:         if res.status != 200 and res.status != 204:
Line 92:             raise se.TransferTicketRequestError(
Line 93:                 res.status, res.reason, res.read())
Line 94: 


Line 94: 
Line 95:         return res
Line 96: 
Line 97:     @staticmethod
Line 98:     def unix_request(config, method, uri, body=None, headers=None):
Make it a function
Line 99:         con = uhttp.UnixHTTPConnection(config.socket)
Line 100:         with closing(con):
Line 101:             log.info("request: %s %s", method, uri)
Line 102:             log.debug("request body: %r, headers: %r", body, headers)


Line 98:     def unix_request(config, method, uri, body=None, headers=None):
Line 99:         con = uhttp.UnixHTTPConnection(config.socket)
Line 100:         with closing(con):
Line 101:             log.info("request: %s %s", method, uri)
Line 102:             log.debug("request body: %r, headers: %r", body, headers)
This duplicates the logs in request()
Line 103:             con.request(method, uri, body, headers=headers or {})
Line 104:             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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke 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