Change in vdsm[master]: storage: Introduction to imagetickets.py
aaviram at redhat.com
aaviram at redhat.com
Thu Jan 14 11:37:01 UTC 2016
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 <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