Change in vdsm[master]: api: introducing short parameter for getVMList verb

fromani at redhat.com fromani at redhat.com
Fri Mar 6 12:10:02 UTC 2015


Francesco Romani has posted comments on this change.

Change subject: api: introducing short parameter for getVMList verb
......................................................................


Patch Set 2:

(3 comments)

I think we can't do much better considering the timeline. But I think we should improve a bit the docs in this patch, to make loud and clear that we must remove this and do things properly and safely for 3.6.x.

Let's just improve docs then. This way current verification will still be valid.

https://gerrit.ovirt.org/#/c/38449/2/vdsm/API.py
File vdsm/API.py:

Line 1388:                           for v in self._cif.vmContainer.values()
Line 1389:                           if not vmSet or v.id in vmSet]}
Line 1390:         if short:
Line 1391:             return {'status': doneCode,
Line 1392:                     'vmList': [v['vmId'] for v in ret['vmList']]}
I believe I see why you do that here and why you do it this way -  we are extremely tight on time, and basically just moving the code which was in Brdige.py seems the safer way.

I can't offer better alternatives given the current constraints, but I think it is fair enough to ask to
add a comment here mentioning the relevant BZs, to
make very loud and clear why we found ourselves in need to add such an ugly workaround.

Something like

"BZ#XYZABC workaround to ensure backward compatibility with
 3.5.x Engines" or whatever wording you prefer
Line 1393:         return ret
Line 1394: 
Line 1395:     def getExternalVMs(self, uri, username, password):
Line 1396:         """


https://gerrit.ovirt.org/#/c/38449/2/vdsm/rpc/vdsmapi-schema.json
File vdsm/rpc/vdsmapi-schema.json:

Line 3605: # @fullStatus:  #optional When set to False return limited info
Line 3606: #
Line 3607: # @vmList:      #optional Filter the results by a list of UUIDs
Line 3608: #
Line 3609: # @short:       #optional determines whether we need short of full status
this documentation could be improved, because we need to reduce the future risk. We need to convey that this applies only if fullStatus=False. So, what about something like:

"if returning limited info, limits the output only to the bare minimum information, a list of UUIDs of the running VMs"

wording can be improved of course.
Line 3610: #
Line 3611: # Returns:
Line 3612: # A list of VmInfos
Line 3613: #


Line 3614: # Since: 4.10.0
Line 3615: ##
Line 3616: {'command': {'class': 'Host', 'name': 'getVMList'},
Line 3617:  'data': {'*fullStatus': 'bool','*vmList': ['UUID'], '*short': 'bool'},
Line 3618:  'returns': ['VmInfo']}
please note that this is now a lie, because if
fullStatus=False and short=True the output will be an undocumented list of strings.

We know why, we know why it is hard to fix, but we should document this to avoid further damage, IMO.
Line 3619: 
Line 3620: ##
Line 3621: # @GuestDiskMappingInfo:
Line 3622: #


-- 
To view, visit https://gerrit.ovirt.org/38449
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d9eb359f6995c09d61a3159e733498fa3b55780
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofrenkel at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list