9:31 a.m.
Idan Shaby has uploaded a new change for review.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
In change I9932b044c (lvm: Raise an exception if missing physical
volume), we changed lvm.getPV() to raise an InaccessiblePhysDev
exception when the device is no longer in lvm's cache.
This patch updates hsm.__processVGInfos() to log a warning in such a
case since if we called lvm.getPV() with a device that is no longer in
lvm's cache, we still want to get the information for the other PVs.
That is, we don't want to stop hsm.__processVGInfos() operation.
Note that blovkSD.getMetaDataMapping() also calls lvm.getPV(), but in
contrast to hsm.__processVGInfos(), we don't want
blovkSD.getMetaDataMapping() to handle InaccessiblePhysDev because it's
a part of a scenario that creates a storage domain, and we want to fail
the whole operation in case that the PV is no longer in lvm's cache.
Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Bug-Url: https://bugzilla.redhat.com/1048696
Signed-off-by: Idan Shaby <ishaby(a)redhat.com>
---
M vdsm/storage/hsm.py
1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/21/38421/1
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index b92349c..bbc0f84 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -3000,7 +3000,11 @@
vgType != dev["devtype"]):
vgType = multipath.DEV_MIXED
- pvInfo = lvm.getPV(pv)
+ try:
+ pvInfo = lvm.getPV(pv)
+ except se.InaccessiblePhysDev:
+ self.log.error("PV %s no longer exists", pv)
+ continue
vgInfo['pvlist'].append(self.__fillPVDict(dev, pvInfo, vgType))
if vgType == multipath.DEV_FCP:
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
9:31 a.m.
automation(a)ovirt.org has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1:
* 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'])
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No
9:34 a.m.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16300/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/15500/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16470/ : SUCCESS
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
9:34 a.m.
Idan Shaby has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1: Verified+1
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
5:34 p.m.
Nir Soffer has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
4:24 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
10:18 a.m.
Tal Nisan has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1:
Guys, any reason this patch doesn't have +2? Quite simple and straight forward... Dan,
can you upgrade to +2?
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
4:29 a.m.
Federico Simoncelli has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/38421/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:
Line 3003: try:
Line 3004: pvInfo = lvm.getPV(pv)
Line 3005: except se.InaccessiblePhysDev:
Line 3006: self.log.error("PV %s no longer exists", pv)
Line 3007: continue
Does this mean that now getVGInfo and getVGList may silently return incomplete
information? (e.g. a missing pv).
For example when a single lun (of a VG) is unreachable we don't have information about
it in the cache but still that PV is part of the VG.
Previously we were returning None (which is wrong from the xml-rpc point of view) but at
least there was a feedback on the unreachable PV.
Line 3008: vgInfo['pvlist'].append(self.__fillPVDict(dev, pvInfo,
vgType))
Line 3009:
Line 3010: if vgType == multipath.DEV_FCP:
Line 3011: vgType = sd.FCP_DOMAIN
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
6:51 a.m.
Idan Shaby has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/38421/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:
Line 3003: try:
Line 3004: pvInfo = lvm.getPV(pv)
Line 3005: except se.InaccessiblePhysDev:
Line 3006: self.log.error("PV %s no longer exists", pv)
Line 3007: continue
Does this mean that now getVGInfo and getVGList may silently return
incompl
Previously we got None in pvInfo and when we tried to access its attributes
we got an AttributeError. Now all we do is skip this pv and log the error. I think
it's a better feedback than an exception, isn't it?
Line 3008: vgInfo['pvlist'].append(self.__fillPVDict(dev, pvInfo,
vgType))
Line 3009:
Line 3010: if vgType == multipath.DEV_FCP:
Line 3011: vgType = sd.FCP_DOMAIN
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
6:08 p.m.
Nir Soffer has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/38421/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:
Line 3003: try:
Line 3004: pvInfo = lvm.getPV(pv)
Line 3005: except se.InaccessiblePhysDev:
Line 3006: self.log.error("PV %s no longer exists", pv)
Line 3007: continue
Does this mean that now getVGInfo and getVGList may silently return
incompl
Previously the entire request was failing with AttributeError, now the
request will return the available pvs.
It would be nice if we can return info about missing pvs, but it is not the scope of this
patch.
lvm.getPV() could return a missing pv in this case, or we can add MissingPV namedduple for
feeding __fillPVDict()
Line 3008: vgInfo['pvlist'].append(self.__fillPVDict(dev, pvInfo,
vgType))
Line 3009:
Line 3010: if vgType == multipath.DEV_FCP:
Line 3011: vgType = sd.FCP_DOMAIN
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
6:23 a.m.
Federico Simoncelli has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/38421/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:
Line 3003: try:
Line 3004: pvInfo = lvm.getPV(pv)
Line 3005: except se.InaccessiblePhysDev:
Line 3006: self.log.error("PV %s no longer exists", pv)
Line 3007: continue
Previously the entire request was failing with AttributeError, now
the requ
The problem here is not about swallowing the ugly exception, it's that
we don't want to feed partial/incomplete information to engine. So, in a sense failing
hard was better than letting believe the engine that the VG was fine with just those
luns.
Nir, can we easily add a patch on top of this that gives feedback to the engine about the
messing pv?
Line 3008: vgInfo['pvlist'].append(self.__fillPVDict(dev, pvInfo,
vgType))
Line 3009:
Line 3010: if vgType == multipath.DEV_FCP:
Line 3011: vgType = sd.FCP_DOMAIN
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
6:40 a.m.
Nir Soffer has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/38421/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:
Line 3003: try:
Line 3004: pvInfo = lvm.getPV(pv)
Line 3005: except se.InaccessiblePhysDev:
Line 3006: self.log.error("PV %s no longer exists", pv)
Line 3007: continue
The problem here is not about swallowing the ugly exception, it's
that we d
I'll check this.
Line 3008: vgInfo['pvlist'].append(self.__fillPVDict(dev, pvInfo,
vgType))
Line 3009:
Line 3010: if vgType == multipath.DEV_FCP:
Line 3011: vgType = sd.FCP_DOMAIN
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
11:06 a.m.
Idan Shaby has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1: Verified-1
(1 comment)
https://gerrit.ovirt.org/#/c/38421/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:
Line 3003: try:
Line 3004: pvInfo = lvm.getPV(pv)
Line 3005: except se.InaccessiblePhysDev:
Line 3006: self.log.error("PV %s no longer exists", pv)
Line 3007: continue
I'll check this.
After talking about this with Nir, we
realized two things:
1. We can't send the engine a "fake PV" (which is an implementation of your
idea), i.e a PV with a GUID and with no other info, since we don't have any other info
for the lost PV. The reason is that we can't prove there is no flow in the engine that
relies on the existence of these fields. That is, we can break the engine.
2. We can't send the engine a partial info about the PVs (which is what this patch
aims to do), i.e send the engine one or more PVs less than the real number of PVs. The
reason is that the engine will store the partial info in the DB, and we don't know
which flow will break because of that.
Fede, if you don't have any other idea, I think that I will abandon this patch.
What do you say?
Line 3008: vgInfo['pvlist'].append(self.__fillPVDict(dev, pvInfo,
vgType))
Line 3009:
Line 3010: if vgType == multipath.DEV_FCP:
Line 3011: vgType = sd.FCP_DOMAIN
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
5:27 p.m.
Nir Soffer has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1: Code-Review-1
I agree with Idan that this change or the change suggested by Federico will be too risky
and may break older engine versions.
For 4.0 we will be able to return info about missing pvs instead of failing the entire
request.
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
9:32 a.m.
Federico Simoncelli has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/38421/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:
Line 3003: try:
Line 3004: pvInfo = lvm.getPV(pv)
Line 3005: except se.InaccessiblePhysDev:
Line 3006: self.log.error("PV %s no longer exists", pv)
Line 3007: continue
After talking about this with Nir, we realized two things:
What
if you catch InaccessiblePhysDev and you take the entire vg out of the list? (so you
don't report that specific vg at all because you don't have the visibility of one
of its pv)
So you don't feed wrong information, you don't have the entire visibility of that
VG so you don't report it at all.
Line 3008: vgInfo['pvlist'].append(self.__fillPVDict(dev, pvInfo,
vgType))
Line 3009:
Line 3010: if vgType == multipath.DEV_FCP:
Line 3011: vgType = sd.FCP_DOMAIN
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
7:54 a.m.
Tal Nisan has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1:
ping?
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No
9:29 a.m.
Idan Shaby has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/38421/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:
Line 3003: try:
Line 3004: pvInfo = lvm.getPV(pv)
Line 3005: except se.InaccessiblePhysDev:
Line 3006: self.log.error("PV %s no longer exists", pv)
Line 3007: continue
What if you catch InaccessiblePhysDev and you take the entire vg out
of the
I ran a few tests. Here are the results when trying to add an iscsi domain:
1. A test that makes getPV in lvm.py always fail (raise an InaccessiblePhysDev), which is
the scenario that can most probably happen today - fails with the error "Error while
executing action New SAN Storage Domain: Multipath cannot access physical device".
There was no db pollution.
2. A test that makes getPV in lvm.py fail only for a specific pv - same error is shown,
and no db pollution.
3. A test that removes the vg out of the vg list (what Fede suggested) - the storage
domain is created, a new record is added to the storage_domain_static table but the lun
(the only one) is not added to the luns table.
That means that we saved a partial information in the db (db pollution).
If you create another storage domain, you will see the luns that you used earlier as free
luns (not greyed out), and if you try to edit the storage domain that we created, you will
get a bunch of java.lang.NullPointerException-s.
I am abandoning the patch.
Line 3008: vgInfo['pvlist'].append(self.__fillPVDict(dev, pvInfo,
vgType))
Line 3009:
Line 3010: if vgType == multipath.DEV_FCP:
Line 3011: vgType = sd.FCP_DOMAIN
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
9:32 a.m.
Idan Shaby has abandoned this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Abandoned
This patch will cause problems in the engine side.
For further explanations, please see the discussion in hsm.py.
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
9:32 a.m.
automation(a)ovirt.org has posted comments on this change.
Change subject: lvm: Update hsm.__processVGInfos() with lvm.getPV() new API
......................................................................
Patch Set 1:
* Update tracker::#1048696::OK
--
To view, visit https://gerrit.ovirt.org/38421
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I583c0493093d2c9c8bca8713df8ee123c415de7f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No
3234
days inactive
3393
days old
vdsm-patches@lists.fedorahosted.org
18 comments
7 participants
participants (7)
-
automation＠ovirt.org -
Dan Kenigsberg -
Federico Simoncelli -
ishaby＠redhat.com -
Jenkins CI RO -
Nir Soffer -
tnisan＠redhat.com