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