Change in vdsm[master]: netwiring: [1/3] Add API definitions.

danken at redhat.com danken at redhat.com
Thu Nov 29 19:09:04 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: netwiring: [1/3] Add API definitions.
......................................................................


Patch Set 2: I would prefer that you didn't submit this

(5 inline comments)

....................................................
File vdsm_api/vdsmapi-schema.json
Line 4852: # @updateVmDeviceParams:
Line 4853: #
Line 4854: # Parameters for @VM.updateVmDevice
Line 4855: #
Line 4856: # @type:      The type of device to update. For example 'interface' for network
I think this can be made more similar to VmInterfaceDevice - I think this is @device.
Line 4857: #             devices or 'disk' for disk storage devices.
Line 4858: #
Line 4859: # @alias:     Alias used to identify this device in commands.
Line 4860: #


Line 4866: #
Line 4867: # @promisc:   If present, indicates the traffic of the network which should be
Line 4868: #             mirrored to the interface identified by alias.
Line 4869: #
Line 4870: # Since 4.11.0
this is going to be available in 4.10.3 (I hope)
Line 4871: ##
Line 4872: {'type': 'updateVmDeviceParams',
Line 4873:  'data': {'type': 'str', 'network': 'str',
Line 4874:           'linkState': 'NetworkInterfaceState', 'alias': 'str', 'promisc': ''}}


Line 4870: # Since 4.11.0
Line 4871: ##
Line 4872: {'type': 'updateVmDeviceParams',
Line 4873:  'data': {'type': 'str', 'network': 'str',
Line 4874:           'linkState': 'NetworkInterfaceState', 'alias': 'str', 'promisc': ''}}
I'd follow libvirt's naming: it's the state of the link, not of the network interface.

I think linkState should be optional here (default is up) , as well as in VmInterfaceDevice.

promisc should not be an empty string. Actually, it is called "portMirroring", it's optional, and is a list of network that should be mirrored into this iface. This should be put in VmInterfaceDevice as well - preferably in a preceding patch.
Line 4875: 
Line 4876: ##
Line 4877: # @VM.updateVmDevice:
Line 4878: #


....................................................
File vdsm/BindingXMLRPC.py
Line 263:         return vm.hotunplugNic(params)
Line 264: 
Line 265:     def updateVmDevice(self, vmId, params):
Line 266:         vm = API.VM(vmId)
Line 267:         return vm.updateDevice(params)
I'm not the biggest fan of introducing partial commits, that ends up in AttributeError.

Maybe it simplifies the review process - I should probably first see how complex is the implementation.
Line 268: 
Line 269:     def vmSnapshot(self, vmId, snapDrives):
Line 270:         vm = API.VM(vmId)
Line 271:         return vm.snapshot(snapDrives)


Line 793:                 (self.vmHotplugDisk, 'hotplugDisk'),
Line 794:                 (self.vmHotunplugDisk, 'hotunplugDisk'),
Line 795:                 (self.vmHotplugNic, 'hotplugNic'),
Line 796:                 (self.vmHotunplugNic, 'hotunplugNic'),
Line 797:                 (self.updateVmDevice, 'updateVmDevice'))
Please keep the name convention for vm-related commands: vmUpdateDevice seems better.
Line 798: 
Line 799:     def getIrsMethods(self):
Line 800:         return ((self.domainActivate, 'activateStorageDomain'),
Line 801:                 (self.domainAttach, 'attachStorageDomain'),


--
To view, visit http://gerrit.ovirt.org/9560
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9d8352f237dbe0229dff368f7c1dfa4f5f8fc766
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkaplan at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer at redhat.com>
Gerrit-Reviewer: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list