Change in vdsm[master]: vm: use the new response handling

nsoffer at redhat.com nsoffer at redhat.com
Fri Mar 25 10:14:03 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: vm: use the new response handling
......................................................................


Patch Set 9:

(14 comments)

https://gerrit.ovirt.org/#/c/54799/9/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 3937
Line 3938
Line 3939
Line 3940
Line 3941
ValueError is raised form int(target) of from libvirt?

If it si fro int(), please separate the error handlers.


Line 2535:             # Find the proper device object
Line 2536:             found_device = self._findDeviceByNameOrPath(device_name,
Line 2537:                                                         device_path)
Line 2538:             if found_device is None:
Line 2539:                 raise exception.UpdateIOTuneError(
Device not found is a generic error that should be raised by _findDeviceByNameOrPath. We should check if the engine can accept such error instead of UpdateIOTuneError.
Line 2540:                     "Device {} not found".format(device_name))
Line 2541: 
Line 2542:             # Merge the update with current values
Line 2543:             dom = found_device.getXML()


Line 2551:             # Verify the ioTune params
Line 2552:             try:
Line 2553:                 found_device._validateIoTuneParams(io_tune)
Line 2554:             except ValueError:
Line 2555:                 raise exception.UpdateIOTuneError('Invalid ioTune value')
This error should be raised by _validateIoTuneParams, so this code would be:

    found_device._validateIoTuneParams(io_tune)

Without any error checking.

We can also make this change later, but if work on single verbs level, I think we should do the fix properly now.

Also the error here should be InvalidParameterError, not IOTune specifc error, since we did not try to update iotune parameters. Should check with engine guys if this is possible.
Line 2556: 
Line 2557:             try:
Line 2558:                 self._dom.setBlockIoTune(found_device.name, io_tune,
Line 2559:                                          libvirt.VIR_DOMAIN_AFFECT_LIVE)


Line 2559:                                          libvirt.VIR_DOMAIN_AFFECT_LIVE)
Line 2560:             except libvirt.libvirtError as e:
Line 2561:                 self.log.exception("setVmIoTune failed")
Line 2562:                 if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 2563:                     raise exception.NoSuchVM()
It would be nice to have wrap the domain object so libvirt.VIR_ERR_NO_DOMAIN will raise always NoSuchVM() without having to duplicate this code everywhere.
Line 2564:                 else:
Line 2565:                     raise exception.UpdateIOTuneError(e.message)
Line 2566: 
Line 2567:             # Update both the ioTune arguments and device xml DOM


Line 2561:                 self.log.exception("setVmIoTune failed")
Line 2562:                 if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 2563:                     raise exception.NoSuchVM()
Line 2564:                 else:
Line 2565:                     raise exception.UpdateIOTuneError(e.message)
It would be nice to raise here LibvirtError from the domain object wrapper, and remove this specific iotune error. Should check with engine guys if this is possible.
Line 2566: 
Line 2567:             # Update both the ioTune arguments and device xml DOM
Line 2568:             # so we are still up-to-date
Line 2569:             # TODO: improve once libvirt gets support for iotune events


Line 3933:     @api.method
Line 3934:     def setBalloonTarget(self, target):
Line 3935: 
Line 3936:         if not self._dom.connected:
Line 3937:             raise exception.BalloonError()
This should raise something like VMNotRunning, not balloon specific error, so same error handling can be reused everywhere.
Line 3938:         try:
Line 3939:             target = int(target)
Line 3940:             self._dom.setMemory(target)
Line 3941:         except ValueError:


Line 3938:         try:
Line 3939:             target = int(target)
Line 3940:             self._dom.setMemory(target)
Line 3941:         except ValueError:
Line 3942:             raise exception.BalloonError('an integer is required for target')
Again, this should be InvalidParameterError
Line 3943:         except libvirt.libvirtError as e:
Line 3944:             if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 3945:                 raise exception.NoSuchVM()
Line 3946:             raise exception.BalloonError(e.message)


Line 3941:         except ValueError:
Line 3942:             raise exception.BalloonError('an integer is required for target')
Line 3943:         except libvirt.libvirtError as e:
Line 3944:             if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 3945:                 raise exception.NoSuchVM()
No such vm again, should not be duplicated in any verb.
Line 3946:             raise exception.BalloonError(e.message)
Line 3947:         else:
Line 3948:             for dev in self.conf['devices']:
Line 3949:                 if dev['type'] == hwclass.BALLOON and \


Line 3942:             raise exception.BalloonError('an integer is required for target')
Line 3943:         except libvirt.libvirtError as e:
Line 3944:             if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 3945:                 raise exception.NoSuchVM()
Line 3946:             raise exception.BalloonError(e.message)
LibvirtError
Line 3947:         else:
Line 3948:             for dev in self.conf['devices']:
Line 3949:                 if dev['type'] == hwclass.BALLOON and \
Line 3950:                         dev['specParams']['model'] != 'none':


Line 3956:     def setCpuTuneQuota(self, quota):
Line 3957:         try:
Line 3958:             self._dom.setSchedulerParameters({'vcpu_quota': int(quota)})
Line 3959:         except ValueError:
Line 3960:             raise exception.CpuTuneError('an integer is required for quota')
InvalidParameterError
Line 3961:         except libvirt.libvirtError as e:
Line 3962:             raise exception.CpuTuneError(e.message)
Line 3963:         else:
Line 3964:             # libvirt may change the value we set, so we must get fresh data


Line 3958:             self._dom.setSchedulerParameters({'vcpu_quota': int(quota)})
Line 3959:         except ValueError:
Line 3960:             raise exception.CpuTuneError('an integer is required for quota')
Line 3961:         except libvirt.libvirtError as e:
Line 3962:             raise exception.CpuTuneError(e.message)
LibvirtError
Line 3963:         else:
Line 3964:             # libvirt may change the value we set, so we must get fresh data
Line 3965:             return self._updateVcpuTuneInfo()
Line 3966: 


Line 3968:     def setCpuTunePeriod(self, period):
Line 3969:         try:
Line 3970:             self._dom.setSchedulerParameters({'vcpu_period': int(period)})
Line 3971:         except ValueError:
Line 3972:             raise exception.CpuTuneError('an integer is required for period')
InvalidParameterError
Line 3973:         except libvirt.libvirtError as e:
Line 3974:             raise exception.CpuTuneError(e.message)
Line 3975:         else:
Line 3976:             # libvirt may change the value we set, so we must get fresh data


Line 3970:             self._dom.setSchedulerParameters({'vcpu_period': int(period)})
Line 3971:         except ValueError:
Line 3972:             raise exception.CpuTuneError('an integer is required for period')
Line 3973:         except libvirt.libvirtError as e:
Line 3974:             raise exception.CpuTuneError(e.message)
LibvirtError
Line 3975:         else:
Line 3976:             # libvirt may change the value we set, so we must get fresh data
Line 3977:             return self._updateVcpuTuneInfo()
Line 3978: 


Line 3979:     def _updateVcpuTuneInfo(self):
Line 3980:         try:
Line 3981:             self._vcpuTuneInfo = self._dom.schedulerParameters()
Line 3982:         except libvirt.libvirtError as e:
Line 3983:             raise exception.CpuTuneError(e.message)
LibvirtError?
Line 3984: 
Line 3985:     def _setWriteWatermarks(self):
Line 3986:         """
Line 3987:         Define when to receive an event about high write to guest image


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib594a5cc6cd8945c24cfcb3704ad92d02102993b
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
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