Xavi Francisco has uploaded a new change for review.
Change subject: storage: ignore missing pv metadata ......................................................................
storage: ignore missing pv metadata
Ignore missing metadata when trying to parse PV information from a problematic LVM device.
When vdsm tries to read metadata from a LVM physical volume and there's an error, the lvm command exits normally but no pv metadata is retrieved and a message is printed in stderr. When this happens the method getPV() in LVM returns None. This has to be checked whenever the PV metadata is needed to avoid throwing an exception.
Change-Id: I9932b044c8b439dc8b1f09191a5d89f4bc44c38a Bug-Url: https://bugzilla.redhat.com/1048696 Signed-off-by: Xavi Francisco xfrancis@redhat.com --- M vdsm/storage/blockSD.py 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/42/27442/1
diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py index 377b132..c41e737 100644 --- a/vdsm/storage/blockSD.py +++ b/vdsm/storage/blockSD.py @@ -668,6 +668,10 @@ continue
pv = lvm.getPV(dev) + if not pv: + cls.log.warning("Could not get metadata from the device: %s", + dev) + continue pvInfo = {} pvInfo["guid"] = os.path.basename(pv.name) pvInfo["uuid"] = pv.uuid
oVirt Jenkins CI Server has posted comments on this change.
Change subject: storage: ignore missing pv metadata ......................................................................
Patch Set 1:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/8599/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/7809/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/8727/ : FAILURE
Allon Mureinik has posted comments on this change.
Change subject: storage: ignore missing pv metadata ......................................................................
Patch Set 1: Code-Review+1
Allon Mureinik has posted comments on this change.
Change subject: storage: ignore missing pv metadata ......................................................................
Patch Set 1:
Nir/Federico, can we proceed with this one?
Nir Soffer has posted comments on this change.
Change subject: storage: ignore missing pv metadata ......................................................................
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/27442/1/vdsm/storage/blockSD.py File vdsm/storage/blockSD.py:
Line 670: pv = lvm.getPV(dev) Line 671: if not pv: Line 672: cls.log.warning("Could not get metadata from the device: %s", Line 673: dev) Line 674: continue Why do we get into this corner, when a pv does not have meta data. can you reproduce this?
The solution, returning None from a function is not a good idea in general - this is something that we do in C when we have no other choice, but if this is not a normal condition, the right thing would be to raise an exception here.
I'm not sure that this is the right place to fix this, and I suspect that this should be handled in the point where we pvs fail and we get no metadata. Maybe we should remove this pv from our metadata cache that this point.
Since I don't have time to research this now, I cannot do a complete review of this change at this time. Line 675: pvInfo = {} Line 676: pvInfo["guid"] = os.path.basename(pv.name) Line 677: pvInfo["uuid"] = pv.uuid Line 678: # this is another trick, it's not the
Nir Soffer has posted comments on this change.
Change subject: storage: do not try to parse metadata from missing pv ......................................................................
Patch Set 2: Code-Review+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: storage: do not try to parse metadata from missing pv ......................................................................
Patch Set 2:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8571/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_storage_functional_tests_localfs_ge... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9505/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9359/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: blockSD: do not try to parse metadata from missing pv ......................................................................
Patch Set 3:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8574/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_storage_functional_tests_localfs_ge... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9508/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9362/ : SUCCESS
Allon Mureinik has posted comments on this change.
Change subject: blockSD: do not try to parse metadata from missing pv ......................................................................
Patch Set 3: Code-Review+1
Allon Mureinik has posted comments on this change.
Change subject: blockSD: do not try to parse metadata from missing pv ......................................................................
Patch Set 3:
Fede, can we move forward with this one?
Federico Simoncelli has posted comments on this change.
Change subject: blockSD: do not try to parse metadata from missing pv ......................................................................
Patch Set 3: Code-Review-1
(1 comment)
http://gerrit.ovirt.org/#/c/27442/3/vdsm/storage/blockSD.py File vdsm/storage/blockSD.py:
Line 670: pv = lvm.getPV(dev) Line 671: if not pv: Line 672: cls.log.warning("Could not get metadata from the device: %s", Line 673: dev) Line 674: continue You return an incomplete mapping swallowing the error? Why is it legal to create a volume storing an incomplete mapping? Line 675: pvInfo = {} Line 676: pvInfo["guid"] = os.path.basename(pv.name) Line 677: pvInfo["uuid"] = pv.uuid Line 678: # this is another trick, it's not the
Nir Soffer has posted comments on this change.
Change subject: blockSD: do not try to parse metadata from missing pv ......................................................................
Patch Set 3:
(1 comment)
http://gerrit.ovirt.org/#/c/27442/3/vdsm/storage/blockSD.py File vdsm/storage/blockSD.py:
Line 670: pv = lvm.getPV(dev) Line 671: if not pv: Line 672: cls.log.warning("Could not get metadata from the device: %s", Line 673: dev) Line 674: continue
You return an incomplete mapping swallowing the error? Why is it legal to c
This is not incomplete mapping - this is the complete list of pv available at the time of the check. This may be different from the list that was available at line 651 - unless you lock lvm module for the duration of the iteration. Line 675: pvInfo = {} Line 676: pvInfo["guid"] = os.path.basename(pv.name) Line 677: pvInfo["uuid"] = pv.uuid Line 678: # this is another trick, it's not the
Federico Simoncelli has posted comments on this change.
Change subject: blockSD: do not try to parse metadata from missing pv ......................................................................
Patch Set 3:
(1 comment)
http://gerrit.ovirt.org/#/c/27442/3/vdsm/storage/blockSD.py File vdsm/storage/blockSD.py:
Line 670: pv = lvm.getPV(dev) Line 671: if not pv: Line 672: cls.log.warning("Could not get metadata from the device: %s", Line 673: dev) Line 674: continue
This is not incomplete mapping - this is the complete list of pv available
You get the list of pvs and then you iterate over them to get the info. It doesn't mean that the info becomes irrelevant if something changes between the listing and the retrieval. Line 675: pvInfo = {} Line 676: pvInfo["guid"] = os.path.basename(pv.name) Line 677: pvInfo["uuid"] = pv.uuid Line 678: # this is another trick, it's not the
Nir Soffer has posted comments on this change.
Change subject: blockSD: do not try to parse metadata from missing pv ......................................................................
Patch Set 3:
(1 comment)
http://gerrit.ovirt.org/#/c/27442/3/vdsm/storage/blockSD.py File vdsm/storage/blockSD.py:
Line 670: pv = lvm.getPV(dev) Line 671: if not pv: Line 672: cls.log.warning("Could not get metadata from the device: %s", Line 673: dev) Line 674: continue
You get the list of pvs and then you iterate over them to get the info. It
Lets here from Xavi when this happens - I think we need to understand the context. Line 675: pvInfo = {} Line 676: pvInfo["guid"] = os.path.basename(pv.name) Line 677: pvInfo["uuid"] = pv.uuid Line 678: # this is another trick, it's not the
oVirt Jenkins CI Server has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 4:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9766/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10551/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10708/ : SUCCESS
Federico Simoncelli has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 4: Code-Review-1
Since you're now raising an exception instead of returning None, is there anything to do with the other getPV usages we have? (marking -1 for visibility)
Federico Simoncelli has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/27442/4/vdsm/storage/lvm.py File vdsm/storage/lvm.py:
Line 824: def getPV(pv): Line 825: pv = _lvminfo.getPv(_fqpvname(pv)) Line 826: if not pv: Line 827: raise se.InaccessiblePhysDev(pv) Line 828: else: Style: I wouldn't use an else block here. Line 829: return pv Line 830: Line 831: Line 832: def getAllPVs():
Nir Soffer has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 4: Code-Review-1
(2 comments)
http://gerrit.ovirt.org/#/c/27442/4/vdsm/storage/blockSD.py File vdsm/storage/blockSD.py:
Line 669: pv = lvm.getPV(dev) Line 670: except se.InaccessiblePhysDev: Line 671: cls.log.error("INTERNAL: Unable to get metadata from PV: %s", Line 672: dev) Line 673: raise Please remove this log and let this error bubble up the engine - we will have a clear traceback in the log showing this call, so this error is not needed.
We have this bad style of double errors in lot of places, lets not continue with this. Exceptions should simplify error handling, so you handle only stuff you *can* handle and letting others handle other errors. You clearly cannot handle this error as you raise. Line 674: Line 675: pvInfo = {} Line 676: pvInfo["guid"] = os.path.basename(pv.name) Line 677: pvInfo["uuid"] = pv.uuid
http://gerrit.ovirt.org/#/c/27442/4/vdsm/storage/lvm.py File vdsm/storage/lvm.py:
Line 824: def getPV(pv): Line 825: pv = _lvminfo.getPv(_fqpvname(pv)) Line 826: if not pv: Line 827: raise se.InaccessiblePhysDev(pv) Line 828: else:
Style: I wouldn't use an else block here.
+1 Line 829: return pv Line 830: Line 831: Line 832: def getAllPVs():
Xavi Francisco has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 4:
(2 comments)
There are two other methods that use getPV: __processVGInfos() and _getDeviceList() both in hsm.py. The second one already expects a StorageException when getPV should fail so this patch corrects this. In the first method there is no check whether the getPV returned None or not so in this case the added exception is needed as well.
http://gerrit.ovirt.org/#/c/27442/4/vdsm/storage/blockSD.py File vdsm/storage/blockSD.py:
Line 669: pv = lvm.getPV(dev) Line 670: except se.InaccessiblePhysDev: Line 671: cls.log.error("INTERNAL: Unable to get metadata from PV: %s", Line 672: dev) Line 673: raise
Please remove this log and let this error bubble up the engine - we will ha
OK, I just tried to give some insight in the VDSM logs about what happened and where. I can remove it but I added it for clarity purposes. Line 674: Line 675: pvInfo = {} Line 676: pvInfo["guid"] = os.path.basename(pv.name) Line 677: pvInfo["uuid"] = pv.uuid
http://gerrit.ovirt.org/#/c/27442/4/vdsm/storage/lvm.py File vdsm/storage/lvm.py:
Line 824: def getPV(pv): Line 825: pv = _lvminfo.getPv(_fqpvname(pv)) Line 826: if not pv: Line 827: raise se.InaccessiblePhysDev(pv) Line 828: else:
+1
I do not like it either, but I tried to be consistent with the same behaviour of getVG(). I'll change it but another patch should be added to change the style of the getVG call (to be consistent at least) Line 829: return pv Line 830: Line 831: Line 832: def getAllPVs():
Nir Soffer has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/27442/4/vdsm/storage/lvm.py File vdsm/storage/lvm.py:
Line 824: def getPV(pv): Line 825: pv = _lvminfo.getPv(_fqpvname(pv)) Line 826: if not pv: Line 827: raise se.InaccessiblePhysDev(pv) Line 828: else:
I do not like it either, but I tried to be consistent with the same behavio
You got my +1 for killing such code anywhere :-) Line 829: return pv Line 830: Line 831: Line 832: def getAllPVs():
oVirt Jenkins CI Server has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 5:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9778/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10563/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10720/ : SUCCESS
Nir Soffer has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 5:
(1 comment)
Nice, with more clear commit message this should be fine.
http://gerrit.ovirt.org/#/c/27442/5//COMMIT_MSG Commit Message:
Line 15: Line 16: This situation may happen as we cannot be sure that a given physical Line 17: volume will be accessible between the moment we first read the list of Line 18: physical volumes for a given volume group and the time we try to access Line 19: such physical volume's metadata. Since there is no special handling when reading the list of pvs and then getting the pv, the race in this operation is not interesting any more, so I think we can remove both of the paragraphs above.
Instead it would be better to describe the 3 places which are calling today lvm.getPV and how this patch effect them, as you described in reply to Federico. Line 20: Line 21: Change-Id: I9932b044c8b439dc8b1f09191a5d89f4bc44c38a Line 22: Bug-Url: https://bugzilla.redhat.com/1048696
Nir Soffer has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 7: Code-Review+1
(1 comment)
Suggested addition to the commit message that make make the benefit of the patch more clear.
http://gerrit.ovirt.org/#/c/27442/7//COMMIT_MSG Commit Message:
Line 16: This patch addresses these issues: Line 17: * The method _getDeviceList() in hsm.py already expects a StorageException Line 18: when getPV fails to read the physical volume. Line 19: * __processVGInfos in hsm.py and getMetadataMapping in blockSD.py don't Line 20: implement checks to assure the pv object returned by getPV is not None. ...so this patch replace a generic and confusing AttributeError (when trying to work with None) with relevant error that allows engine to display translated error message. Line 21: Line 22: Change-Id: I9932b044c8b439dc8b1f09191a5d89f4bc44c38a Line 23: Bug-Url: https://bugzilla.redhat.com/1048696
oVirt Jenkins CI Server has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 7:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9782/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10567/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10724/ : SUCCESS
Allon Mureinik has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 7:
Fede, what's your take on this? Can we proceed?
Allon Mureinik has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 7:
ping?
Allon Mureinik has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 7:
PING?
Federico Simoncelli has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 7:
(1 comment)
http://gerrit.ovirt.org/#/c/27442/7/vdsm/storage/lvm.py File vdsm/storage/lvm.py:
Line 824: def getPV(pv): Line 825: pv = _lvminfo.getPv(_fqpvname(pv)) Line 826: if not pv: Line 827: raise se.InaccessiblePhysDev(pv) Line 828: return pv In hsm.py we have:
try: pv = lvm.getPV(guid) except se.StorageException: self.log.warning("getPV failed for guid: %s", guid, exc_info=True) else: if pv is None: continue
I think it should be fixed (at least remove the "is None" check). Line 829: Line 830: Line 831: def getAllPVs(): Line 832: return _lvminfo.getAllPvs()
Nir Soffer has posted comments on this change.
Change subject: blockSD: raise an exception if missing physical volume ......................................................................
Patch Set 7: -Code-Review
(1 comment)
http://gerrit.ovirt.org/#/c/27442/7/vdsm/storage/lvm.py File vdsm/storage/lvm.py:
Line 824: def getPV(pv): Line 825: pv = _lvminfo.getPv(_fqpvname(pv)) Line 826: if not pv: Line 827: raise se.InaccessiblePhysDev(pv) Line 828: return pv
In hsm.py we have:
Looks like Xavi forgot to include that file in the patch, because he describes this change in the commit message. Line 829: Line 830: Line 831: def getAllPVs(): Line 832: return _lvminfo.getAllPvs()
automation@ovirt.org has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 8:
* Update tracker::#1048696::OK * Check Bug-Url::OK * Check Public Bug::#1048696::OK, public bug * Check Product::#1048696::OK, Correct product Red Hat Enterprise Virtualization Manager * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Idan Shaby has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 8: Verified+1
Nir Soffer has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 8:
(6 comments)
Commit message need rewrite.
https://gerrit.ovirt.org/#/c/27442/8//COMMIT_MSG Commit Message:
Line 6: Line 7: lvm: Raise an exception if missing physical volume Line 8: Line 9: Raise an exception when reading metatada from a missing or deactivated Line 10: physical volume. Raise InaccessiblePhysicalDev when getting a pv which is not in lvm cache.
There is no such thing as deactivated physical volume. Line 11: Line 12: When VDSM tries to read metadata from a physical volume using the getPV Line 13: methon in lvm.py, due to the possibility that that physical volume is not Line 14: accessible anymore, we should raise an exception.
Line 8: Line 9: Raise an exception when reading metatada from a missing or deactivated Line 10: physical volume. Line 11: Line 12: When VDSM tries to read metadata from a physical volume using the getPV VDSM does not try to read metadata from a physical volume, it tries to get a cached PV object from lvm cache.
Please use modulename.functionName() - lvm.getPV() Line 13: methon in lvm.py, due to the possibility that that physical volume is not Line 14: accessible anymore, we should raise an exception. Line 15: Line 16: This patch addresses these issues:
Line 9: Raise an exception when reading metatada from a missing or deactivated Line 10: physical volume. Line 11: Line 12: When VDSM tries to read metadata from a physical volume using the getPV Line 13: methon in lvm.py, due to the possibility that that physical volume is not methon -> function, but if you write lvm.getPV(), you don't need to write "getPV function in lvm.py". Line 14: accessible anymore, we should raise an exception. Line 15: Line 16: This patch addresses these issues: Line 17: * The method _getDeviceList() in hsm.py already expects a StorageException
Line 10: physical volume. Line 11: Line 12: When VDSM tries to read metadata from a physical volume using the getPV Line 13: methon in lvm.py, due to the possibility that that physical volume is not Line 14: accessible anymore, we should raise an exception. we should raise a relevant storage exception, instead of letting the code fail at some random point later when trying to access an attribute of None object. Line 15: Line 16: This patch addresses these issues: Line 17: * The method _getDeviceList() in hsm.py already expects a StorageException Line 18: when getPV fails to read the physical volume.
Line 14: accessible anymore, we should raise an exception. Line 15: Line 16: This patch addresses these issues: Line 17: * The method _getDeviceList() in hsm.py already expects a StorageException Line 18: when getPV fails to read the physical volume. There was no issue in this method, so this did not fix anything in _getDeviceList(). You patch just updated it to the new api (raise instead of returning None) Line 19: * __processVGInfos in hsm.py and getMetadataMapping in blockSD.py don't Line 20: implement checks to assure the pv object returned by getPV is not None. Line 21: Line 22: Change-Id: I9932b044c8b439dc8b1f09191a5d89f4bc44c38a
Line 16: This patch addresses these issues: Line 17: * The method _getDeviceList() in hsm.py already expects a StorageException Line 18: when getPV fails to read the physical volume. Line 19: * __processVGInfos in hsm.py and getMetadataMapping in blockSD.py don't Line 20: implement checks to assure the pv object returned by getPV is not None. Lets simplify - did not handle the case when pv was missing which cause the flow to fail with AttributeError. Now they do not fail and log warning about missing pvs. Line 21: Line 22: Change-Id: I9932b044c8b439dc8b1f09191a5d89f4bc44c38a Line 23: Bug-Url: https://bugzilla.redhat.com/1048696 Line 24: Signed-off-by: Xavi Francisco xfrancis@redhat.com
automation@ovirt.org has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 9:
* Update tracker::#1048696::OK * Check Bug-Url::OK * Check Public Bug::#1048696::OK, public bug * Check Product::#1048696::OK, Correct product Red Hat Enterprise Virtualization Manager * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Idan Shaby has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 8:
(6 comments)
https://gerrit.ovirt.org/#/c/27442/8//COMMIT_MSG Commit Message:
Line 6: Line 7: lvm: Raise an exception if missing physical volume Line 8: Line 9: Raise an exception when reading metatada from a missing or deactivated Line 10: physical volume.
Raise InaccessiblePhysicalDev when getting a pv which is not in lvm cache.
Done Line 11: Line 12: When VDSM tries to read metadata from a physical volume using the getPV Line 13: methon in lvm.py, due to the possibility that that physical volume is not Line 14: accessible anymore, we should raise an exception.
Line 8: Line 9: Raise an exception when reading metatada from a missing or deactivated Line 10: physical volume. Line 11: Line 12: When VDSM tries to read metadata from a physical volume using the getPV
VDSM does not try to read metadata from a physical volume, it tries to get
Done Line 13: methon in lvm.py, due to the possibility that that physical volume is not Line 14: accessible anymore, we should raise an exception. Line 15: Line 16: This patch addresses these issues:
Line 9: Raise an exception when reading metatada from a missing or deactivated Line 10: physical volume. Line 11: Line 12: When VDSM tries to read metadata from a physical volume using the getPV Line 13: methon in lvm.py, due to the possibility that that physical volume is not
methon -> function, but if you write lvm.getPV(), you don't need to write "
Done Line 14: accessible anymore, we should raise an exception. Line 15: Line 16: This patch addresses these issues: Line 17: * The method _getDeviceList() in hsm.py already expects a StorageException
Line 10: physical volume. Line 11: Line 12: When VDSM tries to read metadata from a physical volume using the getPV Line 13: methon in lvm.py, due to the possibility that that physical volume is not Line 14: accessible anymore, we should raise an exception.
we should raise a relevant storage exception, instead of letting the code f
Done Line 15: Line 16: This patch addresses these issues: Line 17: * The method _getDeviceList() in hsm.py already expects a StorageException Line 18: when getPV fails to read the physical volume.
Line 14: accessible anymore, we should raise an exception. Line 15: Line 16: This patch addresses these issues: Line 17: * The method _getDeviceList() in hsm.py already expects a StorageException Line 18: when getPV fails to read the physical volume.
There was no issue in this method, so this did not fix anything in _getDevi
Done Line 19: * __processVGInfos in hsm.py and getMetadataMapping in blockSD.py don't Line 20: implement checks to assure the pv object returned by getPV is not None. Line 21: Line 22: Change-Id: I9932b044c8b439dc8b1f09191a5d89f4bc44c38a
Line 16: This patch addresses these issues: Line 17: * The method _getDeviceList() in hsm.py already expects a StorageException Line 18: when getPV fails to read the physical volume. Line 19: * __processVGInfos in hsm.py and getMetadataMapping in blockSD.py don't Line 20: implement checks to assure the pv object returned by getPV is not None.
Lets simplify - did not handle the case when pv was missing which cause the
Done Line 21: Line 22: Change-Id: I9932b044c8b439dc8b1f09191a5d89f4bc44c38a Line 23: Bug-Url: https://bugzilla.redhat.com/1048696 Line 24: Signed-off-by: Xavi Francisco xfrancis@redhat.com
oVirt Jenkins CI Server has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 9:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16259/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/15459/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16429/ : SUCCESS
Nir Soffer has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 9:
(2 comments)
https://gerrit.ovirt.org/#/c/27442/9//COMMIT_MSG Commit Message:
Line 8: Line 9: Raise InaccessiblePhysDev when getting a pv which is not in lvm cache. Line 10: Line 11: When lvm.getPV() tries to get a cached PV object from its cache, there Line 12: is a possibility that the physical volume is not accessible anymore. In is not accessible -> is not in the cache
What why a missing device may happen:
Basically we do:
for pvName in lvm.listPVNames(vgName): pv = lvm.getPV(pvName)
While we iterate, lvm cache may change (another thread may invalidate the cache).
Typically we don't expect pvs to be removed, and if this happens we want to warn about it. Line 13: such a case, we should raise a relevant storage exception, instead of Line 14: letting the code fail at some random point later when trying to access Line 15: an attribute of None object. Line 16:
Line 18: * It updates hsm._getDeviceList() to work with the new API. Line 19: * hsm.__processVGInfos() and blockSD.getMetadataMapping() did not Line 20: handle the case of a missing pv which caused the flow to fail with Line 21: AttributeError. Now they do not fail and log a warning about missing Line 22: pvs. We do not need these details, we can see this in the code.
What we want to tell is:
- lvm.getPV() raises now InaccessiblePhysDev instead of returning None - Callers updated to log a warning when a device is not the cache, which is similar to the existing behavior of hsm._getDeviceList()
Lets separate error handling in hsm.__processVGInfos() and blockSD.getMetadataMapping() to another patch. Letting them fail with a relevant error is way better then the current behavior, and we may need to check if this failure is expected in these flows. Line 23: Line 24: Change-Id: I9932b044c8b439dc8b1f09191a5d89f4bc44c38a Line 25: Bug-Url: https://bugzilla.redhat.com/1048696 Line 26: Signed-off-by: Xavi Francisco xfrancis@redhat.com
automation@ovirt.org has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 10:
* Update tracker::#1048696::OK * Check Bug-Url::OK * Check Public Bug::#1048696::OK, public bug * Check Product::#1048696::OK, Correct product Red Hat Enterprise Virtualization Manager * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Idan Shaby has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 10: Verified+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 10:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16299/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/15499/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16469/ : SUCCESS
Nir Soffer has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 10: Code-Review+1
Dan Kenigsberg has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 10: Code-Review+1
Due to the delicacy of lvmcache, I'd like to obtain Federico's approval.
Federico Simoncelli has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 10:
(1 comment)
getPV looks good but I am worried about the change in _getDeviceList
https://gerrit.ovirt.org/#/c/27442/10/vdsm/storage/hsm.py File vdsm/storage/hsm.py:
Line 2007 Line 2008 Line 2009 Line 2010 Line 2011 Isn't this used by getDeviceList? Once we had this requirement of not returning None in XML-RPC (as it's not fully supported in the base protocol).
Also, is the engine able to deal with this API change?
Federico Simoncelli has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 10: Code-Review+2
(1 comment)
https://gerrit.ovirt.org/#/c/27442/10/vdsm/storage/hsm.py File vdsm/storage/hsm.py:
Line 2007 Line 2008 Line 2009 Line 2010 Line 2011
Isn't this used by getDeviceList? Once we had this requirement of not retur
Ah it's handled in line 2023, fine!
Federico Simoncelli has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 10:
(1 comment)
https://gerrit.ovirt.org/#/c/27442/10/vdsm/storage/hsm.py File vdsm/storage/hsm.py:
Line 2007 Line 2008 Line 2009 Line 2010 Line 2011
Ah it's handled in line 2023, fine!
Also we never reach here for the se.StorageException exception. Good.
Dan Kenigsberg has submitted this change and it was merged.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
lvm: Raise an exception if missing physical volume
Raise InaccessiblePhysDev when getting a pv which is not in lvm cache.
When lvm.getPV() tries to get a cached PV object from its cache, there is a possibility that the physical volume is not in the cache anymore. In such a case, we should raise a relevant storage exception, instead of letting the code fail at a random point later when trying to access an attribute of None object.
Change-Id: I9932b044c8b439dc8b1f09191a5d89f4bc44c38a Bug-Url: https://bugzilla.redhat.com/1048696 Signed-off-by: Xavi Francisco xfrancis@redhat.com Signed-off-by: Idan Shaby ishaby@redhat.com Reviewed-on: https://gerrit.ovirt.org/27442 Reviewed-by: Nir Soffer nsoffer@redhat.com Reviewed-by: Dan Kenigsberg danken@redhat.com Reviewed-by: Federico Simoncelli fsimonce@redhat.com --- M vdsm/storage/hsm.py M vdsm/storage/lvm.py 2 files changed, 5 insertions(+), 4 deletions(-)
Approvals: Nir Soffer: Looks good to me, but someone else must approve Federico Simoncelli: Looks good to me, approved Dan Kenigsberg: Looks good to me, but someone else must approve Idan Shaby: Verified
automation@ovirt.org has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 11:
* Update tracker::#1048696::OK * Set MODIFIED::bug 1048696::::#1048696::::IGNORE, not oVirt prod but Red Hat Enterprise Virtualization Manager
oVirt Jenkins CI Server has posted comments on this change.
Change subject: lvm: Raise an exception if missing physical volume ......................................................................
Patch Set 11:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el6-x86_64_merged/760/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-fc21-x86_64_me... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/4741/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc21-x86_64_merged/733/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-el6-x86_64_mer... : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el7-x86_64_merged/763/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-el7-x86_64_mer... : FAILURE
http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-fc20-x86_64_me... : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc20-x86_64_merged/767/ : FAILURE
vdsm-patches@lists.fedorahosted.org