Change in vdsm[master]: StorageDomain.getInfo - report lvm metadata device for block sd

Liron Aravot has uploaded a new change for review.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

StorageDomain.getInfo - report lvm metadata device for block sd

When a block storage domain is created, we disable the lvm metadata
on all the domain pvs except to the first one.

As we intend to add support for operations that modify the vg
stracture (like reducing devices) - we want to prevent those
operations from being made on the device containing the vg metadata.

This patch adds the reporting of that info, so that the engine can
leverage the info to block such operations.

Change-Id: I4a7763d2ab7d796be633ecd69f661cba96e29dde
Signed-off-by: Liron Aravot 
---
M lib/api/vdsm-api.yml
M lib/vdsm/storage/exception.py
M vdsm/storage/blockSD.py
M vdsm/storage/lvm.py
4 files changed, 48 insertions(+), 1 deletion(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/33/64433/1

diff --git a/lib/api/vdsm-api.yml b/lib/api/vdsm-api.yml
index 8168fbb..02e1ede 100644
--- a/lib/api/vdsm-api.yml
+++ b/lib/api/vdsm-api.yml
@@ -5653,6 +5653,11 @@
             type: *UUID

-   defaultvalue: null
+            description: The GUID of the device containing the domain lvm metadata (optional)
+            name: lvmmetadatadevice
+            type: string
+
+        -   defaultvalue: null
             description: The GUID of the first device containing the domain metadata lv (optional)
             name: metadatadevice
             type: string
diff --git a/lib/vdsm/storage/exception.py b/lib/vdsm/storage/exception.py
index 23e2628..743c5d9 100644
--- a/lib/vdsm/storage/exception.py
+++ b/lib/vdsm/storage/exception.py
@@ -1556,6 +1556,11 @@
     code = 615
     message = "Could not resize PV"

+class CannotGetVGMetadataPV(StorageException):
+    def __init__(self, vgname):
+        self.value = "vgname=%s" % (vgname)
+    code = 616
+    message = "Cannot get Volume Group metadata PV"

#################################################
 #  SPM/HSM Exceptions
diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py
index b164551..ec060c1 100644
--- a/vdsm/storage/blockSD.py
+++ b/vdsm/storage/blockSD.py
@@ -449,6 +449,9 @@
         dev, _ = lvm.getFirstExt(self.sdUUID, sd.METADATA)
         return os.path.basename(dev)

+    def getVgMetadataDevice(self):
+        return os.path.basename(lvm.getVgMetadataPv(self.sdUUID))
+
     @classmethod
     def getMetaDataMapping(cls, vgName, oldMapping={}):
         firstDev, firstExtent = lvm.getFirstExt(vgName, sd.METADATA)
@@ -1108,6 +1111,7 @@
         info['vguuid'] = vg.uuid
         info['state'] = vg.partial
         info['metadatadevice'] = self._manifest.getMetadataLVFirstDevice()
+        info['lvmmetadatadevice'] = self._manifest.getVgMetadataDevice()
         return info

def getStats(self):
diff --git a/vdsm/storage/lvm.py b/vdsm/storage/lvm.py
index 3d3c31a..c0a5c1c 100644
--- a/vdsm/storage/lvm.py
+++ b/vdsm/storage/lvm.py
@@ -51,7 +51,7 @@
 LVM_DEFAULT_TTL = 100

PV_FIELDS = ("uuid,name,size,vg_name,vg_uuid,pe_start,pe_count,"
-             "pe_alloc_count,mda_count,dev_size")
+             "pe_alloc_count,mda_count,dev_size,mda_used_count")
 VG_FIELDS = ("uuid,name,attr,size,free,extent_size,extent_count,free_count,"
              "tags,vg_mda_size,vg_mda_free,lv_count,pv_count,pv_name")
 LV_FIELDS = "uuid,name,vg_name,attr,size,seg_start_pe,devices,tags"
@@ -555,6 +555,25 @@
                 reloaded = self._reloadpvs(stalepvs)
                 pvs.update(reloaded)
         return pvs.values()
+
+    def _isMetadataPv(self, pv):
+        return int(pv.pv_mda_used_count) > 0
+
+    def getVgMetadataPv(self, vgName):
+        stalepvs = []
+        for pvName in self.listPVNames(vgName):
+            pv = self._pvs.get(pvName)
+            if not pv or isinstance(pv, Stub):
+                stalepvs.append(pvName)
+            if self._isMetadataPv(pv):
+                return pv
+
+        pvs = self._reloadpvs(pvName=stalepvs)
+        for pv in pvs:
+            if not isinstance(pv, Stub) and self._isMetadataPv(pv):
+                return pv
+
+        raise se.CouldNotResizePhysicalVolume(vgName)

def getVg(self, vgName):
         # Get specific VG
@@ -1319,6 +1338,20 @@
     return getLV(vg, lv).devices.strip(" )").split("(")

+def getVgMetadataPv(vgName):
+    pvs = _lvminfo.getVgPvs(vgName)
+    for pv in pvs:
+        try:
+            if _isMetadataPv(pv):
+                return pv
+        except AttributeError:
+            pass
+
+    raise se.CannotGetVGMetadataPV(vgName)
+
+def _isMetadataPv(pv):
+    return pv.mda_used_count == '2'
+
 def listPVNames(vgName):
     try:
         pvNames = _lvminfo._vgs[vgName].pv_name

gerrit-hooks has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 2:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

gerrit-hooks has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 3:

* update_tracker: OK
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

Liron Aravot has abandoned this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Abandoned

gerrit-hooks has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 2:

* update_tracker: OK

Liron Aravot has restored this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Restored

gerrit-hooks has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 3:

* update_tracker: OK
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

gerrit-hooks has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 3:

* update_tracker: OK
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

gerrit-hooks has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 3:

* update_tracker: OK
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 2:

(2 comments)

https://gerrit.ovirt.org/#/c/64433/2/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 448:     def getMetadataLVFirstDevice(self):
Line 449:         dev, _ = lvm.getFirstExt(self.sdUUID, sd.METADATA)
Line 450:         return os.path.basename(dev)
Line 451: 
Line 452:     def getVgMetadataDevice(self):
Sync with getMetadataLVFirstDevice after renaming.
Line 453:         return os.path.basename(lvm.getVgMetadataPv(self.sdUUID))
Line 454: 
Line 455:     @classmethod
Line 456:     def getMetaDataMapping(cls, vgName, oldMapping={}):

Line 1110:         vg = lvm.getVG(self.sdUUID)  # vg.name = self.sdUUID
Line 1111:         info['vguuid'] = vg.uuid
Line 1112:         info['state'] = vg.partial
Line 1113:         info['metadatadevice'] = self._manifest.getMetadataLVFirstDevice()
Line 1114:         info['lvmmetadatadevice'] = self._manifest.getVgMetadataDevice()
vgmetadatadevice

Consider separate words with _
Line 1115:         return info
Line 1116: 
Line 1117:     def getStats(self):
Line 1118:         """

gerrit-hooks has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 4:

* update_tracker: OK
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 4:

(6 comments)

https://gerrit.ovirt.org/#/c/64433/4/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 556:                 pvs.update(reloaded)
Line 557:         return pvs.values()
Line 558: 
Line 559:     def getVgPvNames(self, vgName):
Line 560:         return self.getVg(vgName).pv_name
I don't think we need this helper, getting a vg and accessing pv.name is perfect.
Line 561: 
Line 562:     def getVgPvs(self, vgName):
Line 563:         stalepvs = []
Line 564:         vgpvs = []

Line 563:         stalepvs = []
Line 564:         vgpvs = []
Line 565:         for pvName in self.getVgPvNames(vgName):
Line 566:             pv = self._pvs.get(pvName)
Line 567:             if pv is None or isinstance(pv, Stub):
Need to check what is the difference between None and Stub - do we need to reload in both case?
Line 568:                 stalepvs.append(pvName)
Line 569:             else:
Line 570:                 vgpvs.append(pv)
Line 571:

Line 568:                 stalepvs.append(pvName)
Line 569:             else:
Line 570:                 vgpvs.append(pv)
Line 571: 
Line 572:         vgpvs.extend(self._reloadpvs(pvName=stalepvs).values())
This line is doing too much, this style is a recipe for bugs, lets write simpler code like:

reloadedpvs =  self._reloadpvs(pvName=stalepvs)
    vgcps.extend(reloadedpv.values())
Line 573:         return vgpvs
Line 574: 
Line 575:     def getVg(self, vgName):
Line 576:         # Get specific VG

Line 1340:     for pv in pvs:
Line 1341:         try:
Line 1342:             if _isMetadataPv(pv):
Line 1343:                 return pv
Line 1344:         except AttributeError:
This code is trying to ignore unreadable pv, assuming that unreadable pv does not have the attribute we check in _isMetadataPv. This is very bad for two reasons:

- fragile, will break when someone adds this attribute to the unreadable class
- does not reveal your intent, reader has to read the underlying code to understand what are you trying to do.

The code should do something like:

if type(pv) is Unreadable:
        # handle it...

If PV was a real class, we should do a nicer check:

if not pv.is_readable():
        ...

But PV is a namedtuple, a stupid readonly struct without any behavior.
Line 1345:             pass
Line 1346: 
Line 1347:     raise se.CannotGetVGMetadataPV(vgName)
Line 1348:

Line 1341:         try:
Line 1342:             if _isMetadataPv(pv):
Line 1343:                 return pv
Line 1344:         except AttributeError:
Line 1345:             pass
This should work when the vg is ok, but if some other code or lvm itself will enable the metadata area on another pv, this code will silently hide this issue.  I prefer that abnormal configuration will fail loudly in this case.

So we can do something like:

mdpavs = [pv for pv in pvs if isMetadataPv(vg)]

And then we can fail:

if len(mdpvs) != 1):
        raise InvalidVG...

return mdpvs[0]

Now we should also think what is the best solution when one of the pvs is unreadable. If the unreadable pv is was the metadata pv, we will fail with no metadata pv. If some other pv is not readable, we can return the metadata pv, assuming that we should have only one, or fail since we cannot read all pvs.

If we fail when one pv is unreadable, we are safer, so I tend to prefer to fail fast with (probably existing) UnreachablePhysicalVolume.

I don't think we want to do touch a vg with missing pv, the user has to fix this issue.
Line 1346: 
Line 1347:     raise se.CannotGetVGMetadataPV(vgName)
Line 1348: 
Line 1349:

Line 1346: 
Line 1347:     raise se.CannotGetVGMetadataPV(vgName)
Line 1348: 
Line 1349: 
Line 1350: def _isMetadataPv(pv):
Please add docstring explaining this and referring to the place configuring pvs in such way.

This is also the place to fail with UnreachablePhysicalVolume if this pv is unreadable.
Line 1351:     return pv.mda_used_count == '2'
Line 1352: 
Line 1353: 
Line 1354: def listPVNames(vgName):

Liron Aravot has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 4:

(6 comments)

https://gerrit.ovirt.org/#/c/64433/4/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 556:                 pvs.update(reloaded)
Line 557:         return pvs.values()
Line 558: 
Line 559:     def getVgPvNames(self, vgName):
Line 560:         return self.getVg(vgName).pv_name
> I don't think we need this helper, getting a vg and accessing pv.name is pe

I don't agree as this code is already repeated, but Done.
Line 561: 
Line 562:     def getVgPvs(self, vgName):
Line 563:         stalepvs = []
Line 564:         vgpvs = []

Line 563:         stalepvs = []
Line 564:         vgpvs = []
Line 565:         for pvName in self.getVgPvNames(vgName):
Line 566:             pv = self._pvs.get(pvName)
Line 567:             if pv is None or isinstance(pv, Stub):
> Need to check what is the difference between None and Stub - do we need to

That's the standard way we check the pvs in this file.
Let's use it here as well - if we find out its unnecessary let's change all the occurrences at once.
Line 568:                 stalepvs.append(pvName)
Line 569:             else:
Line 570:                 vgpvs.append(pv)
Line 571:

Line 568:                 stalepvs.append(pvName)
Line 569:             else:
Line 570:                 vgpvs.append(pv)
Line 571: 
Line 572:         vgpvs.extend(self._reloadpvs(pvName=stalepvs).values())
> This line is doing too much, this style is a recipe for bugs, lets write si

Done
Line 573:         return vgpvs
Line 574: 
Line 575:     def getVg(self, vgName):
Line 576:         # Get specific VG

Line 1340:     for pv in pvs:
Line 1341:         try:
Line 1342:             if _isMetadataPv(pv):
Line 1343:                 return pv
Line 1344:         except AttributeError:
> This code is trying to ignore unreadable pv, assuming that unreadable pv do

Checking for the specific case is obviously better, I've checked for AttributeError as that's the standard way we handle that scenario all across that file.
Line 1345:             pass
Line 1346: 
Line 1347:     raise se.CannotGetVGMetadataPV(vgName)
Line 1348:

Line 1341:         try:
Line 1342:             if _isMetadataPv(pv):
Line 1343:                 return pv
Line 1344:         except AttributeError:
Line 1345:             pass
> This should work when the vg is ok, but if some other code or lvm itself wi

1. Done, i see your point.

2. I'm not sure about fail fast. few reasons:
a. we extend the failure cases for this method opposed for today - I'm not sure it's desired (StorageDomain.getInfo() will fail when one pv is unreachable) and maintains BC.
b. currently all the methods here doesn't fail fast (for example, when failing to load vg/pv/lv we don't raise but use the Unreadable object) - so it makes sense that this method will be written using the same approach.
Line 1346: 
Line 1347:     raise se.CannotGetVGMetadataPV(vgName)
Line 1348: 
Line 1349:

Line 1346: 
Line 1347:     raise se.CannotGetVGMetadataPV(vgName)
Line 1348: 
Line 1349: 
Line 1350: def _isMetadataPv(pv):
> Please add docstring explaining this and referring to the place configuring

1. I'll add the docstring.

2. The approach used in this file is to try to access the pv object, if it is an instance of Unreadable/Stub it will raise AttributeError.
I'd say that if we want to raise UnreachablePhysicalVolume we should modify the Unreadable class (because that's why it was added, for unified behavior) and not to add special treatment to each method.
Line 1351:     return pv.mda_used_count == '2'
Line 1352: 
Line 1353: 
Line 1354: def listPVNames(vgName):

Liron Aravot has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/64433/4/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 1341:         try:
Line 1342:             if _isMetadataPv(pv):
Line 1343:                 return pv
Line 1344:         except AttributeError:
Line 1345:             pass
> Done, i see your point.

elaboration to b:
What i'm trying to say is that we do check the pvs here, but just to get "information" related to the vg (we are trying to understand what pv stored its metadata).
Therefore if we manage to get that info, it seems better to not raise.
Line 1346: 
Line 1347:     raise se.CannotGetVGMetadataPV(vgName)
Line 1348: 
Line 1349:

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/64433/4/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 556:                 pvs.update(reloaded)
Line 557:         return pvs.values()
Line 558: 
Line 559:     def getVgPvNames(self, vgName):
Line 560:         return self.getVg(vgName).pv_name
> I don't agree as this code is already repeated, but Done.

We can work on refactoring this class later if you think such helper is needed, lets focus on adding the new feature.
Line 561: 
Line 562:     def getVgPvs(self, vgName):
Line 563:         stalepvs = []
Line 564:         vgpvs = []

Line 563:         stalepvs = []
Line 564:         vgpvs = []
Line 565:         for pvName in self.getVgPvNames(vgName):
Line 566:             pv = self._pvs.get(pvName)
Line 567:             if pv is None or isinstance(pv, Stub):
> That's the standard way we check the pvs in this file.

OK
Line 568:                 stalepvs.append(pvName)
Line 569:             else:
Line 570:                 vgpvs.append(pv)
Line 571:

Liron Aravot has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/64433/4/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 1343:                 return pv
Line 1344:         except AttributeError:
Line 1345:             pass
Line 1346: 
Line 1347:     raise se.CannotGetVGMetadataPV(vgName)
Do we want to have a general exception or to use that specific one?
Line 1348: 
Line 1349: 
Line 1350: def _isMetadataPv(pv):
Line 1351:     return pv.mda_used_count == '2'

gerrit-hooks has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 5:

* update_tracker: OK
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

Liron Aravot has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/64433/4/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 556:                 pvs.update(reloaded)
Line 557:         return pvs.values()
Line 558: 
Line 559:     def getVgPvNames(self, vgName):
Line 560:         return self.getVg(vgName).pv_name
> We can work on refactoring this class later if you think such helper is nee

Well, i use this method in the following patch - so i assume i'll return it (see line 913 in https://gerrit.ovirt.org/#/c/63270/8/vdsm/storage/lvm.py)
Line 561: 
Line 562:     def getVgPvs(self, vgName):
Line 563:         stalepvs = []
Line 564:         vgpvs = []

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 5:

(8 comments)

https://gerrit.ovirt.org/#/c/64433/5/lib/api/vdsm-api.yml
File lib/api/vdsm-api.yml:

Line 5659:             type: string
Line 5660: 
Line 5661:         -   defaultvalue: null
Line 5662:             description: The GUID of the device containing the domain lvm metadata (optional)
Line 5663:             name: lvmmetadatadevice
Update names
Line 5664:             type: string
Line 5665:         type: object
Line 5666: 
Line 5667:     StorageDomainStatus: &StorageDomainStatus

https://gerrit.ovirt.org/#/c/64433/5/lib/vdsm/storage/exception.py
File lib/vdsm/storage/exception.py:

Line 1558: 
Line 1559: 
Line 1560: class CannotGetVGMetadataPV(StorageException):
Line 1561:     def __init__(self, vgname):
Line 1562:         self.value = "vgname=%s" % (vgname)
Expected one pv with pv metadata, vgname=%s, pvs=%s
Line 1563:     code = 616
Line 1564:     message = "Cannot get Volume Group metadata PV"
Line 1565: 
Line 1566:

https://gerrit.ovirt.org/#/c/64433/5/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 1109
Line 1110
Line 1111
Line 1112
Line 1113
Take this

Line 1116: <<<<<<< HEAD
Line 1117:         info['metadatadevice'] = self._manifest.getMetadataLVDevice()
Line 1118: =======
Line 1119:         info['metadatadevice'] = self._manifest.getMetadataLVFirstDevice()
Line 1120:         info['lvmmetadatadevice'] = self._manifest.getVgMetadataDevice()
And this, but
- use vgmetadatadevice
- use separate words metadata_device, vg_metadata_device
Line 1121: >>>>>>> 7da51c9... StorageDomain.getInfo - report lvm metadata device for block sd
Line 1122:         return info
Line 1123: 
Line 1124:     def getStats(self):

https://gerrit.ovirt.org/#/c/64433/5/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 585
Line 586
Line 587
Line 588
Line 589
Let put the new method here, more specific.

Line 502:     def _invalidatevgs(self, vgNames):
Line 503:         vgNames = _normalizeargs(vgNames)
Line 504:         with self._lock:
Line 505:             for vgName in vgNames:
Line 506:                 self._vgs[vgName] = aStub(vgName, True)
Not related.
Line 507: 
Line 508:     def _invalidateAllVgs(self):
Line 509:         with self._lock:
Line 510:             self._stalevg = True

Line 555:                 reloaded = self._reloadpvs(stalepvs)
Line 556:                 pvs.update(reloaded)
Line 557:         return pvs.values()
Line 558: 
Line 559:     def getVgPvs(self, vgName):
Add docsting explaining this new function compared with getPv.
Line 560:         stalepvs = []
Line 561:         vgpvs = []
Line 562:         vg = self.getVg(vgName)
Line 563:         for pvName in vg.pv_name:

Line 1337: def getVgMetadataPv(vgName):
Line 1338:     pvs = _lvminfo.getVgPvs(vgName)
Line 1339:     mdpvs = [pv for pv in pvs if not isinstance(pv, Stub) and _isMetadataPv(pv)]
Line 1340:     if len(mdpvs) != 1:
Line 1341:         se.CannotGetVGMetadataPV(vgName)
"Expected one metadata pv in vg: %s, found: %s" % (vg.name, pvs)

Can we use an error name about the issue an not about the operation?

e,g UnexpectedVolumeGroupMetadata?
Line 1342:     return mdpvs[0]
Line 1343: 
Line 1344: def _isMetadataPv(pv):
Line 1345:     """

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 5:

(2 comments)

https://gerrit.ovirt.org/#/c/64433/5/lib/api/vdsm-api.yml
File lib/api/vdsm-api.yml:

Line 5659:             type: string
Line 5660: 
Line 5661:         -   defaultvalue: null
Line 5662:             description: The GUID of the device containing the domain lvm metadata (optional)
Line 5663:             name: lvmmetadatadevice
> Update names

vgMetadataDevice
Line 5664:             type: string
Line 5665:         type: object
Line 5666: 
Line 5667:     StorageDomainStatus: &StorageDomainStatus

https://gerrit.ovirt.org/#/c/64433/5/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 1116: <<<<<<< HEAD
Line 1117:         info['metadatadevice'] = self._manifest.getMetadataLVDevice()
Line 1118: =======
Line 1119:         info['metadatadevice'] = self._manifest.getMetadataLVFirstDevice()
Line 1120:         info['lvmmetadatadevice'] = self._manifest.getVgMetadataDevice()
> And this, but

Use mixed  case name, already used in this verb response.
Line 1121: >>>>>>> 7da51c9... StorageDomain.getInfo - report lvm metadata device for block sd
Line 1122:         return info
Line 1123: 
Line 1124:     def getStats(self):

Liron Aravot has posted comments on this change.

Change subject: StorageDomain.getInfo - report lvm metadata device for block sd
......................................................................

Patch Set 5:

(8 comments)

https://gerrit.ovirt.org/#/c/64433/5/lib/api/vdsm-api.yml
File lib/api/vdsm-api.yml:

Line 5659:             type: string
Line 5660: 
Line 5661:         -   defaultvalue: null
Line 5662:             description: The GUID of the device containing the domain lvm metadata (optional)
Line 5663:             name: lvmmetadatadevice
> vgMetadataDevice

Done
Line 5664:             type: string
Line 5665:         type: object
Line 5666: 
Line 5667:     StorageDomainStatus: &StorageDomainStatus

https://gerrit.ovirt.org/#/c/64433/5/lib/vdsm/storage/exception.py
File lib/vdsm/storage/exception.py:

Line 1558: 
Line 1559: 
Line 1560: class CannotGetVGMetadataPV(StorageException):
Line 1561:     def __init__(self, vgname):
Line 1562:         self.value = "vgname=%s" % (vgname)
> Expected one pv with pv metadata, vgname=%s, pvs=%s

As this error is intended to be "general" - the code that raises will pass the exact error.
Line 1563:     code = 616
Line 1564:     message = "Cannot get Volume Group metadata PV"
Line 1565: 
Line 1566:

https://gerrit.ovirt.org/#/c/64433/5/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 1109
Line 1110
Line 1111
Line 1112
Line 1113
> Take this

Done

Line 1116: <<<<<<< HEAD
Line 1117:         info['metadatadevice'] = self._manifest.getMetadataLVDevice()
Line 1118: =======
Line 1119:         info['metadatadevice'] = self._manifest.getMetadataLVFirstDevice()
Line 1120:         info['lvmmetadatadevice'] = self._manifest.getVgMetadataDevice()
> Use mixed  case name, already used in this verb response.

Done
Line 1121: >>>>>>> 7da51c9... StorageDomain.getInfo - report lvm metadata device for block sd
Line 1122:         return info
Line 1123: 
Line 1124:     def getStats(self):

https://gerrit.ovirt.org/#/c/64433/5/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 585
Line 586
Line 587
Line 588
Line 589
> Let put the new method here, more specific.

Done

Line 502:     def _invalidatevgs(self, vgNames):
Line 503:         vgNames = _normalizeargs(vgNames)
Line 504:         with self._lock:
Line 505:             for vgName in vgNames:
Line 506:                 self._vgs[vgName] = aStub(vgName, True)
> Not related.

Done
Line 507: 
Line 508:     def _invalidateAllVgs(self):
Line 509:         with self._lock:
Line 510:             self._stalevg = True

Line 555:                 reloaded = self._reloadpvs(stalepvs)
Line 556:                 pvs.update(reloaded)
Line 557:         return pvs.values()
Line 558: 
Line 559:     def getVgPvs(self, vgName):
> Add docsting explaining this new function compared with getPv.

Done
Line 560:         stalepvs = []
Line 561:         vgpvs = []
Line 562:         vg = self.getVg(vgName)
Line 563:         for pvName in vg.pv_name:

Line 1337: def getVgMetadataPv(vgName):
Line 1338:     pvs = _lvminfo.getVgPvs(vgName)
Line 1339:     mdpvs = [pv for pv in pvs if not isinstance(pv, Stub) and _isMetadataPv(pv)]
Line 1340:     if len(mdpvs) != 1:
Line 1341:         se.CannotGetVGMetadataPV(vgName)
> "Expected one metadata pv in vg: %s, found: %s" % (vg.name, pvs)

Done
Line 1342:     return mdpvs[0]
Line 1343: 
Line 1344: def _isMetadataPv(pv):
Line 1345:     """

Change in vdsm[master]: StorageDomain.getInfo - report vg metadata device for block sd

gerrit-hooks has posted comments on this change.

Change subject: StorageDomain.getInfo - report vg metadata device for block sd
......................................................................

Patch Set 6:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

Change in vdsm[master]: StorageDomain.getInfo - report vg metadata device for block sd

gerrit-hooks has posted comments on this change.

Change subject: StorageDomain.getInfo - report vg metadata device for block sd
......................................................................

Patch Set 7:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

Change in vdsm[master]: StorageDomain.getInfo - report vg metadata device for block sd

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomain.getInfo - report vg metadata device for block sd
......................................................................

Patch Set 7:

(5 comments)

https://gerrit.ovirt.org/#/c/64433/7/lib/vdsm/storage/exception.py
File lib/vdsm/storage/exception.py:

Line 1558: 
Line 1559: 
Line 1560: class UnexpectedVolumeGroupMetadata(StorageException):
Line 1561:     def __init__(self, err):
Line 1562:         self.value = "err=%s" % err
reason=?
Line 1563:     code = 616
Line 1564:     message = "Volume Group metadata isn't as expected"
Line 1565: 
Line 1566:

https://gerrit.ovirt.org/#/c/64433/7/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 451:         """
Line 452:         dev, _ = lvm.getFirstExt(self.sdUUID, sd.METADATA)
Line 453:         return os.path.basename(dev)
Line 454: 
Line 455:     def getVgMetadataDevice(self):
Add docstring similar to getMetadataLVDevice (see my comment in the previous patch).
Line 456:         return os.path.basename(lvm.getVgMetadataPv(self.sdUUID))
Line 457: 
Line 458:     @classmethod
Line 459:     def getMetaDataMapping(cls, vgName, oldMapping={}):

https://gerrit.ovirt.org/#/c/64433/7/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 588:         return vgs.values()
Line 589: 
Line 590:     def getVgPvs(self, vgName):
Line 591:         """
Line 592:         Returns the pvs of the given vg
... reloading pvs only once if some of the pvs are missing or stale.

Hopefully we can use this elsewhere instead of calling multiple getPv calls.

I wonder if this should be be renamed to getPvs and moved next to getPv and getAllPvs - The common behavior is method returning pv objects.
Line 593:         """
Line 594:         stalepvs = []
Line 595:         vgpvs = []
Line 596:         vg = self.getVg(vgName)

Line 594:         stalepvs = []
Line 595:         vgpvs = []
Line 596:         vg = self.getVg(vgName)
Line 597:         for pvName in vg.pv_name:
Line 598:             pv = self._pvs.get(pvName)
One issue here, you are accessing a dict which may be modified by other theads on both spm/hsm. The methods in the cache accessing this dict typically take the lock (see reload pvs).

Not all code is thread safe here, but when adding new code we better be thread safe.

Can be easily fixed by locking the part getting stale pvs:

with self._lock:
        # find vgpvs and stalepvs here

# reload outside of the lock (otherwise it will deadlock)
Line 599:             if pv is None or isinstance(pv, Stub):
Line 600:                 stalepvs.append(pvName)
Line 601:             else:
Line 602:                 vgpvs.append(pv)

Line 1341:     pvs = _lvminfo.getVgPvs(vgName)
Line 1342:     mdpvs = [pv for pv in pvs if not isinstance(pv, Stub) and _isMetadataPv(pv)]
Line 1343:     if len(mdpvs) != 1:
Line 1344:         se.UnexpectedVolumeGroupMetadata("Expected one metadata pv in vg: %s, "
Line 1345:                                          "vg pvs: %s" % (vgName, pvs))
Errors looks fine, raising it would be even better :-)
Line 1346:     return mdpvs[0]
Line 1347: 
Line 1348: def _isMetadataPv(pv):
Line 1349:     """

Change in vdsm[master]: StorageDomain.getInfo - report vg metadata device for block sd

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomain.getInfo - report vg metadata device for block sd
......................................................................

Patch Set 7:

Please fix pep8 errors:

19:19:38 ./vdsm/storage/lvm.py:1342:80: E501 line too long (80 > 79 characters)
19:19:38 ./vdsm/storage/lvm.py:1348:1: E302 expected 2 blank lines, found 1
19:19:38 ./vdsm/storage/lvm.py:1351:80: E501 line too long (84 > 79 characters)
19:19:38 ./vdsm/storage/lvm.py:1352:80: E501 line too long (80 > 79 characters)

Change in vdsm[master]: StorageDomain.getInfo - report vg metadata device for block sd

Liron Aravot has posted comments on this change.

Change subject: StorageDomain.getInfo - report vg metadata device for block sd
......................................................................

Patch Set 7:

(4 comments)

https://gerrit.ovirt.org/#/c/64433/7/lib/vdsm/storage/exception.py
File lib/vdsm/storage/exception.py:

Line 1558: 
Line 1559: 
Line 1560: class UnexpectedVolumeGroupMetadata(StorageException):
Line 1561:     def __init__(self, err):
Line 1562:         self.value = "err=%s" % err
> reason=?

Done
Line 1563:     code = 616
Line 1564:     message = "Volume Group metadata isn't as expected"
Line 1565: 
Line 1566:

https://gerrit.ovirt.org/#/c/64433/7/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 451:         """
Line 452:         dev, _ = lvm.getFirstExt(self.sdUUID, sd.METADATA)
Line 453:         return os.path.basename(dev)
Line 454: 
Line 455:     def getVgMetadataDevice(self):
> Add docstring similar to getMetadataLVDevice (see my comment in the previou

Done
Line 456:         return os.path.basename(lvm.getVgMetadataPv(self.sdUUID))
Line 457: 
Line 458:     @classmethod
Line 459:     def getMetaDataMapping(cls, vgName, oldMapping={}):

https://gerrit.ovirt.org/#/c/64433/7/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 588:         return vgs.values()
Line 589: 
Line 590:     def getVgPvs(self, vgName):
Line 591:         """
Line 592:         Returns the pvs of the given vg
> ... reloading pvs only once if some of the pvs are missing or stale.

1. Done
2. moved and renamed to getPvs
Line 593:         """
Line 594:         stalepvs = []
Line 595:         vgpvs = []
Line 596:         vg = self.getVg(vgName)

Line 594:         stalepvs = []
Line 595:         vgpvs = []
Line 596:         vg = self.getVg(vgName)
Line 597:         for pvName in vg.pv_name:
Line 598:             pv = self._pvs.get(pvName)
> One issue here, you are accessing a dict which may be modified by other the

I'm not sure that's needed.
Currently the methods that take the lock are the one's the modify the dicts and the objects in them - so that look is essentially a write lock only (except GetVGDevs).
In this method we don't modify the dict, only _reloadpvs does and it takes the lock when it does.
acquiring the lock here means that we'll here on the different operations - do we really need it? I can see that you actually changed the lock to be a write lock in change I86153c34c3ddc4055bb0679ce46aa48757d9cae1 :)

I'd start by adding the method as is, keeping the same semantics - if we'll see its needed we can add it (though i believe it doesn't).
Line 599:             if pv is None or isinstance(pv, Stub):
Line 600:                 stalepvs.append(pvName)
Line 601:             else:
Line 602:                 vgpvs.append(pv)

Change in vdsm[master]: StorageDomain.getInfo - report vg metadata device for block sd

Liron Aravot has posted comments on this change.

Change subject: StorageDomain.getInfo - report vg metadata device for block sd
......................................................................

Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/64433/7/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 1341:     pvs = _lvminfo.getVgPvs(vgName)
Line 1342:     mdpvs = [pv for pv in pvs if not isinstance(pv, Stub) and _isMetadataPv(pv)]
Line 1343:     if len(mdpvs) != 1:
Line 1344:         se.UnexpectedVolumeGroupMetadata("Expected one metadata pv in vg: %s, "
Line 1345:                                          "vg pvs: %s" % (vgName, pvs))
> Errors looks fine, raising it would be even better :-)

Yes..you've got a point :)
Line 1346:     return mdpvs[0]
Line 1347: 
Line 1348: def _isMetadataPv(pv):
Line 1349:     """

Change in vdsm[master]: StorageDomain.getInfo - report vg metadata device for block sd

gerrit-hooks has posted comments on this change.

Change subject: StorageDomain.getInfo - report vg metadata device for block sd
......................................................................

Patch Set 8:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])