Change in vdsm[master]: hotplugDisk: Handle prepareVolumePath errors gracefully.

nsoffer at redhat.com nsoffer at redhat.com
Thu Mar 3 08:59:23 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: hotplugDisk: Handle prepareVolumePath errors gracefully.
......................................................................


Patch Set 2:

(7 comments)

https://gerrit.ovirt.org/#/c/54266/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2652
Line 2653
Line 2654
Line 2655
Line 2656
This may fail


Line 2664
Line 2665
Line 2666
Line 2667
Line 2668
This may fail


Line 2683
Line 2684
Line 2685
Line 2686
Line 2687
This may fail


Line 2684
Line 2685
Line 2686
Line 2687
Line 2688
This may fail


Line 2686
Line 2687
Line 2688
Line 2689
Line 2690
This may fail


Line 2649:         diskParams = params.get('drive', {})
Line 2650:         try:
Line 2651:             diskParams['path'] = self.cif.prepareVolumePath(diskParams)
Line 2652:         except VolumeError as e:
Line 2653:             self.log.exception("Cannot prepare path.")
Pleas avoid the trailing "." in vdsm log messages. Log messages are not complete sentences, they are fragment of text combined by logger.
Line 2654:             return response.error(
Line 2655:                 'hotplugDisk', "Cannot prepare path: %s" % e)
Line 2656: 
Line 2657:         if isVdsmImage(diskParams):


Line 2651:             diskParams['path'] = self.cif.prepareVolumePath(diskParams)
Line 2652:         except VolumeError as e:
Line 2653:             self.log.exception("Cannot prepare path.")
Line 2654:             return response.error(
Line 2655:                 'hotplugDisk', "Cannot prepare path: %s" % e)
So we cover now prepare path error, but what about errors in other parts of this verb?

Adding error handler for each line does not scale. What we need is a single error handler for the entire verb (actually for the entire virt subsystem), so you can just let the code fail, and have all errors reported in a useful way.

This is is just a partial solution that will make it harder to create the correct solution later by removing all error handlers in this code.

I think this bug cannot be fix with the current state of the code, we must first add proper error handling for the virt subsystem.

Francesco, what is you view on this?
Line 2656: 
Line 2657:         if isVdsmImage(diskParams):
Line 2658:             self._normalizeVdsmImg(diskParams)
Line 2659:             self._createTransientDisk(diskParams)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic049f388eb25127864edf5931ddb3b21b67f2dff
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland 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