Amit Aviram has posted comments on this change. Change subject: storage: Introduction to imagetickets.py ......................................................................
Patch Set 8: (18 comments) https://gerrit.ovirt.org/#/c/50014/8/client/vdsClient.py File client/vdsClient.py: Line 1620 Line 1621 Line 1622 Line 1623 Line 1624 > We are not adding new features to vdsClient. If you want to add this now fo Done https://gerrit.ovirt.org/#/c/50014/8/lib/api/vdsmapi-schema.json File lib/api/vdsmapi-schema.json: Line 524: {'command': {'class': 'Host', 'name': 'setHaMaintenanceMode'}, Line 525: 'data': {'mode': 'HaMaintenanceMode', 'enabled': 'bool'}} Line 526: Line 527: ## Line 528: # @Host.addImageTicket: > Adam started to add lowercase_verbs (see SDM.create_volume), and I like thi Done Line 529: # Line 530: # Start a session to expose an image via imaged daemon https api for Line 531: # reading and writing image data. Line 532: # Line 534: # Line 535: # Since: 4.18.0 Line 536: ## Line 537: {'command': {'class': 'Host', 'name': 'addImageTicket'}, Line 538: 'data': {'ticket': 'dict'}} > You need to define type for ticket instead of "dict". For example, see Volu Done Line 539: Line 540: ## Line 541: # @Host.removeImageTicket: Line 542: # Line 555: # Extend an image upload data transfer session. Line 556: # Line 557: # @ticket_uuid: uuid of the ticket to be extended Line 558: # Line 559: # @expires: new expiration time for the ticket > in seconds since 1970-01-01 00:00:00 UTC. Will use timeouts. Line 560: # Line 561: # Since: 4.18.0 Line 562: ## Line 563: {'command': {'class': 'Host', 'name': 'extendImageTicket'}, Line 560: # Line 561: # Since: 4.18.0 Line 562: ## Line 563: {'command': {'class': 'Host', 'name': 'extendImageTicket'}, Line 564: 'data': {'ticket_uuid': 'UUID', 'expires': 'int'}} > expires should be 'uint' Done Line 565: Line 566: Line 567: ## Line 568: # @TaskDetails: https://gerrit.ovirt.org/#/c/50014/8/vdsm.spec.in File vdsm.spec.in: Line 907: %{_datadir}/%{vdsm_name}/storage/fileSD.py* Line 908: %{_datadir}/%{vdsm_name}/storage/fileUtils.py* Line 909: %{_datadir}/%{vdsm_name}/storage/fileVolume.py* Line 910: %{_datadir}/%{vdsm_name}/storage/fuser.py* Line 911: %{_datadir}/%{vdsm_name}/storage/imagetickets.py* > We need to add this also in debian/vdsm.install - same file were other stor Done Line 912: %{_datadir}/%{vdsm_name}/storage/glusterSD.py* Line 913: %{_datadir}/%{vdsm_name}/storage/glusterVolume.py* Line 914: %{_datadir}/%{vdsm_name}/storage/hba.py* Line 915: %{_datadir}/%{vdsm_name}/storage/hsm.py* https://gerrit.ovirt.org/#/c/50014/8/vdsm/API.py File vdsm/API.py: Line 1622: self.log.exception("error setting HA maintenance mode") Line 1623: return errCode['haErr'] Line 1624: return {'status': doneCode} Line 1625: Line 1626: def addImageTicket(self, ticket): > If we rename the apis to lower_case, we should rename also here. Done Line 1627: return self._irs.addImageTicket(ticket) Line 1628: Line 1629: def removeImageTicket(self, ticket_uuid): Line 1630: return self._irs.removeImageTicket(ticket_uuid) https://gerrit.ovirt.org/#/c/50014/8/vdsm/storage/imagetickets.py File vdsm/storage/imagetickets.py: Line 1: from contextlib import closing > You need the standard copyright block, copy from another module. Done Line 2: import logging Line 3: import socket Line 4: import json Line 5: Line 1: from contextlib import closing Line 2: import logging Line 3: import socket Line 4: import json > sort imports Done Line 5: Line 6: import storage_exception as se Line 7: Line 8: Line 5: Line 6: import storage_exception as se Line 7: Line 8: Line 9: _imaged_installed = False > Lets call it _have_imaged, we don't check if it is installed, only if the p Done Line 10: Line 11: try: Line 12: import imaged.uhttp as uhttp Line 13: import imaged.server as imgdServer Line 8: Line 9: _imaged_installed = False Line 10: Line 11: try: Line 12: import imaged.uhttp as uhttp > Use from imaged import uhttp Done Line 13: import imaged.server as imgdServer Line 14: _imaged_installed = True Line 15: except ImportError: Line 16: pass Line 9: _imaged_installed = False Line 10: Line 11: try: Line 12: import imaged.uhttp as uhttp Line 13: import imaged.server as imgdServer > Since we are interested only the Config class, and we really do not want a Done Line 14: _imaged_installed = True Line 15: except ImportError: Line 16: pass Line 17: Line 19: Line 20: Line 21: def validate_supported(): Line 22: if not _imaged_installed: Line 23: raise se.ImageTicketsError("Imaged is not installed!") > Repeating this in all verbs works, but it would be little nicer to add a de Awesome- Thanks, Done. Line 24: Line 25: Line 26: def add_ticket(ticket): Line 27: validate_supported() Line 22: if not _imaged_installed: Line 23: raise se.ImageTicketsError("Imaged is not installed!") Line 24: Line 25: Line 26: def add_ticket(ticket): > We should have tests for all the apis - the easier way should be mocking uh Will be in another patch Line 27: validate_supported() Line 28: body = json.dumps(ticket) Line 29: ticket_request(uhttp.PUT, ticket["uuid"], body) Line 30: Line 46: return json.loads(resbody) if resbody else None Line 47: Line 48: Line 49: def request(method, path, body=None): Line 50: log.info("Sending request (method=%r, path=%r, body=%r)" % > For logging, we don't format the message, we provide a format string and ar Done Line 51: (method, path, body)) Line 52: Line 53: con = uhttp.UnixHTTPConnection(imgdServer.Config().socket) Line 54: try: Line 54: try: Line 55: res = unix_request(con, method, path, body) Line 56: except socket.error as e: Line 57: raise se.ImageTicketsError("Error connecting to imaged: {} - {}" Line 58: .format(e.errno, e.strerror)) > This is no how this error should be used. Looking in storage_excpetion, it Done Line 59: if res.status not in (200, 204): Line 60: raise se.ImageTicketsError( Line 61: res.status, res.reason, res.read()) Line 62: Line 57: raise se.ImageTicketsError("Error connecting to imaged: {} - {}" Line 58: .format(e.errno, e.strerror)) Line 59: if res.status not in (200, 204): Line 60: raise se.ImageTicketsError( Line 61: res.status, res.reason, res.read()) > I commented about this in the previous version, you should read exactly con Done Line 62: Line 63: return res Line 64: Line 65: Line 62: Line 63: return res Line 64: Line 65: Line 66: def unix_request(con, method, uri, body=None, headers=None): > This is strange - how is this related to unix socket request? Done Line 67: with closing(con): Line 68: con.request(method, uri, body, headers=headers or {}) Line 69: 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: 8 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
