Nir Soffer has posted comments on this change. Change subject: storage: Introduction to imagetickets.py ......................................................................
Patch Set 10: (19 comments) https://gerrit.ovirt.org/#/c/50014/10/lib/api/vdsmapi-schema.json File lib/api/vdsmapi-schema.json: Line 8411: 'legality': 'VolumeLegality', 'apparentsize': 'uint', Line 8412: 'truesize': 'uint', 'status': 'VolumeStatus', 'children': ['UUID']}} Line 8413: Line 8414: ## Line 8415: # @ImageTicket: Move above the imaged tickets methods, this is the convention in this file. Line 8416: # Line 8417: # Information about an image ticket. Line 8418: # Line 8419: # @uuid: A ticket ID Line 8415: # @ImageTicket: Line 8416: # Line 8417: # Information about an image ticket. Line 8418: # Line 8419: # @uuid: A ticket ID ID -> UUID Line 8420: # Line 8421: # @timeout: Timeout in seconds for the ticket to expire Line 8422: # Line 8423: # @ops: operations list to be applied on the image Line 8419: # @uuid: A ticket ID Line 8420: # Line 8421: # @timeout: Timeout in seconds for the ticket to expire Line 8422: # Line 8423: # @ops: operations list to be applied on the image We need and enum for the possible values. Line 8424: # Line 8425: # @size: block size of each transferred chunk Line 8426: # Line 8427: # @path: an image path Line 8421: # @timeout: Timeout in seconds for the ticket to expire Line 8422: # Line 8423: # @ops: operations list to be applied on the image Line 8424: # Line 8425: # @size: block size of each transferred chunk This is not the meaning of size. I think we should replace this with a range, which is the range in the file which you are allowed to perform ops. For example: ops = "get" range = (0, -1) You can get any byte in the file. ops = "get,put" range = (1000, 2000) You can read and write bytes 1000-2000 Line 8426: # Line 8427: # @path: an image path Line 8428: # Line 8429: # Since: 4.18.0 Line 8423: # @ops: operations list to be applied on the image Line 8424: # Line 8425: # @size: block size of each transferred chunk Line 8426: # Line 8427: # @path: an image path We will need also to support network disk, when path is a remote path accessed via rbd or glusgterfs protocols. I think we should use a url instead of path: - block or file - file:///<path/to/volume> - rdb - rbd:<poolname>/<volumename> - glusterfs - gluster://<server>/<vol-name>/<image> So we should name this img_url and use type URL (maybe we have to add a type for this, should be defined like UUID). Line 8428: # Line 8429: # Since: 4.18.0 Line 8430: ## Line 8431: {'type': 'ImageTicket', https://gerrit.ovirt.org/#/c/50014/10/vdsm/API.py File vdsm/API.py: Line 1623: return errCode['haErr'] Line 1624: return {'status': doneCode} Line 1625: Line 1626: def add_image_ticket(self, ticket): Line 1627: return self._irs.add_image_ticket(ticket) Same we don't have a return value, so not return anything. Line 1628: Line 1629: def remove_image_ticket(self, ticket_uuid): Line 1630: return self._irs.remove_image_ticket(ticket_uuid) Line 1631: https://gerrit.ovirt.org/#/c/50014/10/vdsm/rpc/bindingxmlrpc.py File vdsm/rpc/bindingxmlrpc.py: Line 638 Line 639 Line 640 Line 641 Line 642 Drop the changes in this file https://gerrit.ovirt.org/#/c/50014/10/vdsm/storage/hsm.py File vdsm/storage/hsm.py: Line 3103: volUUID=volUUID).refreshVolume() Line 3104: Line 3105: @public Line 3106: def add_image_ticket(self, ticket): Line 3107: return imagetickets.add_ticket(ticket) We don't return anything, so better use: def add_image_ticket(...): imagetickets.add_ticket(...) The behavior is the same since add_ticket returns None, but the code is more clear about the return value. Same for other methods. Line 3108: Line 3109: @public Line 3110: def remove_image_ticket(self, ticket_uuid): Line 3111: return imagetickets.remove_ticket(ticket_uuid) https://gerrit.ovirt.org/#/c/50014/10/vdsm/storage/imagetickets.py File vdsm/storage/imagetickets.py: Line 33: from imaged import uhttp Line 34: from imaged.server import Config Line 35: _have_imaged = True Line 36: except ImportError: Line 37: pass We can define _have_imaged here. Line 38: Line 39: log = logging.getLogger('storage.imagetickets') Line 40: Line 41: Line 65: request(uhttp.DELETE, ticket_uuid) Line 66: Line 67: Line 68: def request(method, ticket_uuid, body=None): Line 69: log.info("Sending request method=%r, body=%r", method, body) log.debug, the call and the arguments is already logged by hsm. Line 70: con = uhttp.UnixHTTPConnection(Config().socket) Line 71: try: Line 72: with closing(con): Line 73: con.request(method, "/tickets/%s" % ticket_uuid, body, {}) Line 66: Line 67: Line 68: def request(method, ticket_uuid, body=None): Line 69: log.info("Sending request method=%r, body=%r", method, body) Line 70: con = uhttp.UnixHTTPConnection(Config().socket) with closign(con) should be here, and everything in the function should be inside the context. Line 71: try: Line 72: with closing(con): Line 73: con.request(method, "/tickets/%s" % ticket_uuid, body, {}) Line 74: res = con.getresponse() Line 71: try: Line 72: with closing(con): Line 73: con.request(method, "/tickets/%s" % ticket_uuid, body, {}) Line 74: res = con.getresponse() Line 75: except socket.error as e: Are you sure that this can raise only socket.error? check httplib documentation. I thinks we may get httplib.HTTPException subclasses. You can various errors (close imaged before the request, drop the request in imaged, etc.) to check what errors are returned. Line 76: raise se.ImageTicketsError("Error connecting to imaged: {errno} - " Line 77: "{strerror}".format(errno=e.errno, Line 78: strerror=e.strerror)) Line 79: if res.status not in (200, 204): Line 75: except socket.error as e: Line 76: raise se.ImageTicketsError("Error connecting to imaged: {errno} - " Line 77: "{strerror}".format(errno=e.errno, Line 78: strerror=e.strerror)) Line 79: if res.status not in (200, 204): We may have other success codes. Better check for res.status >= 300, since we do not handle redirects, 4xx is client error, and 5xx is server error. Line 80: try: Line 81: ret_dict = json.loads(res.read(res.length)) Line 82: ret_code = ret_dict['code'] Line 83: ret_title = ret_dict['title'] Line 77: "{strerror}".format(errno=e.errno, Line 78: strerror=e.strerror)) Line 79: if res.status not in (200, 204): Line 80: try: Line 81: ret_dict = json.loads(res.read(res.length)) res.length? I don't see it documented in https://docs.python.org/2/library/httplib.html#httpresponse-objects I think you should do: try: content_length = int(res.getheader("content-length", "0")) except ValueError: raise ImageTicketError("Invalid response without content-length") And then you can read and parse the response body. Line 82: ret_code = ret_dict['code'] Line 83: ret_title = ret_dict['title'] Line 84: ret_expl = ret_dict['explanation'] Line 85: ret_detail = ret_dict['detail'] if 'detail' in ret_dict else None Line 79: if res.status not in (200, 204): Line 80: try: Line 81: ret_dict = json.loads(res.read(res.length)) Line 82: ret_code = ret_dict['code'] Line 83: ret_title = ret_dict['title'] code and title should be equal to res.status, and res.reason - check. Line 84: ret_expl = ret_dict['explanation'] Line 85: ret_detail = ret_dict['detail'] if 'detail' in ret_dict else None Line 86: except Exception as e: Line 87: raise se.ImageDeamonError(res.status, res.reason, Line 87: raise se.ImageDeamonError(res.status, res.reason, Line 88: ret_title="Unknown imaged Exception.", Line 89: ret_detail=res.read(res.length)) Line 90: raise se.ImageDeamonError(res.status, res.reason, ret_code, Line 91: ret_title, ret_expl, ret_detail) We are going to raise ImageDaemonError anyway, so we don't need to check for missing key. Instead, create the ImageDaemonErrror with the dict returned by vdsm-imaged as is, and log is as is: try: error_info = json.loads(res.read(content_length)) except ValueError: error_info = {"code": res.status, "reason": res.reason} raise ImageDaemonError(error_info) Line 92: elif res.length: Line 93: # Return the result for parsing if it has any content. Line 90: raise se.ImageDeamonError(res.status, res.reason, ret_code, Line 91: ret_title, ret_expl, ret_detail) Line 92: elif res.length: Line 93: # Return the result for parsing if it has any content. Line 94: return res Always return the res object to the caller. https://gerrit.ovirt.org/#/c/50014/10/vdsm/storage/storage_exception.py File vdsm/storage/storage_exception.py: Line 1208: code = 481 Line 1209: message = "Unsuccessful request to imaged" Line 1210: Line 1211: def __init__(self, reason): Line 1212: self.value = "reason: %s" % (reason) Use key=value, format, use in other errors: self.value = "reason=%s" % reason Line 1213: Line 1214: Line 1215: class ImageDeamonError(StorageException): Line 1216: code = 482 Line 1222: self.value = "result: res_code: %d, res_reason: %s, " \ Line 1223: "imaged_code: %d, imaged_title: %s, " \ Line 1224: "imaged_explanation: %s, imaged_detail: %s" \ Line 1225: % (res_code, res_reason, imaged_code, imaged_title, Line 1226: imaged_explanation, imaged_detail) Lets simplify: def __init__(self, error_info): self.value = ", ".join("%s=%s" % (k, v) for k, v in six.iteritems(error_info)) Now we don't care about the keys imaged returns, and we can add custom keys in imaged side, without changing vdsm side. Note also that we should use key=value, format, use by other errors. Line 1227: Line 1228: Line 1229: class ImageDeamonUnsupported(StorageException): Line 1230: code = 483 -- 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: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Amit Aviram <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Amit Aviram <[email protected]> Gerrit-Reviewer: Greg Padgett <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
