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