[NEW PATCH] BZ#716964 - Add "devcapacity" to PV info (via gerrit-bot)

New patch submitted by Juan Hernandez (juan.hernandez(a)redhat.com)

You can review this change at: http://gerrit.usersys.redhat.com/734

commit f35033484a7e8d32b32077c3cd8f2bdaa14c363c
Author: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Date:   Fri Jul 22 20:56:33 2011 +0200

    BZ#716964 - Add "devcapacity" to PV info
    
    Add a new "devcapacity" field to the PV info provided by VDSM.
    This is obtained from the "dev_size" from "pvs". This and change
    Ic0bd3c94a6e4999cb91f96c798a0108890321fbd in RHEV-M are required
    to fix the bug.
    
    Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd

diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index ef472af..f0294e7 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -1398,6 +1398,7 @@ class HSM:
         info["fwrev"] = "0000"
         info["devtype"] = devtype
         info["capacity"] = str(pv.size)
+        info["devcapacity"] = str(pv.dev_size)
         info["vgUUID"] = str(pv.vg_uuid)
         info["pvUUID"] = str(pv.uuid)
         info["GUID"] = str(pv.guid)
diff --git a/vdsm/storage/lvm.py b/vdsm/storage/lvm.py
index ef60c02..076b776 100644
--- a/vdsm/storage/lvm.py
+++ b/vdsm/storage/lvm.py
@@ -131,7 +131,7 @@ log = logging.getLogger("Storage.LVM")
 
 LVM_DEFAULT_TTL = 100
 
-PV_FIELDS =
"uuid,name,size,vg_name,vg_uuid,pe_start,pe_count,pe_alloc_count,mda_count"
+PV_FIELDS =
"uuid,name,size,vg_name,vg_uuid,pe_start,pe_count,pe_alloc_count,mda_count,dev_size"
 VG_FIELDS =
"uuid,name,attr,size,free,extent_size,extent_count,free_count,tags"
 LV_FIELDS = "uuid,name,vg_name,attr,size,seg_start_pe,devices,tags"

Change in vdsm[master]: BZ#716964 - Add "devcapacity" to PV info

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#716964 - Add "devcapacity" to PV info
......................................................................


Patch Set 1: I would prefer that you didn't submit this

thanks, Juan. Would you remove the leading BZ# from the commit message? The bug specified
there is not a vdsm bug, and this may confuse our automatic scripts. Move it somewhere
into the commit message, please.

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

Change in vdsm[master]: Add "devcapacity" to PV info

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Add "devcapacity" to PV info
......................................................................


Patch Set 2: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;

Change in vdsm[master]: Add "devcapacity" to PV info

Saggi Mizrahi has posted comments on this change.

Change subject: Add "devcapacity" to PV info
......................................................................


Patch Set 2: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: Add "devcapacity" to PV info

Eduardo Warszawski has posted comments on this change.

Change subject: Add "devcapacity" to PV info
......................................................................


Patch Set 2: I would prefer that you didn't submit this

In spite that this patch is simply enough, I wonder if is the optimal way to fix a
management bug.
No matter what is the "device" capacity the system can use only the PV size.

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: Add "devcapacity" to PV info

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Add "devcapacity" to PV info
......................................................................


Patch Set 2: I would prefer that you didn't submit this

I believe that Eduardo has a good point: getVGInfo supplies lvm-related info. The device
capacity is accessible by getDeviceInfo, and the the correlation between the two should be
done using the GUID of the LUN, not using its size. We have a noble tendency to make life
easier on rhevm, but here we are making our API ever so slightly uglier.

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: Add "devcapacity" to PV info

Juan Hernandez has posted comments on this change.

Change subject: Add "devcapacity" to PV info
......................................................................


Patch Set 2:

I agree that it is not the perfect solution. A better solution would be a
"getDeviceInfo" operation where we can provide as parameters the UUIDS of the
devices, otherwise we have to get the list of all devices (and those could be hundreds in
a large environment) just to extract the info of a few of them. But this is to a large
change for 3.0 and in addition it will make the API imcompatible with 2.X.

On the other hand I think that adding this "devcapacity" field is not a bad
thing, as it is an inherent property of a PV, already reported by tools like
"pvs".

Also note that RHEVM is not using internally the size to correlate things: it uses the LUN
UUID and the PV and VG UUIDs. What we want to do is to report to the end user the size of
the LUN (in addtition to the LUN UUID, which we already report) so that he/she, in his/her
mind, can do that correlation better.

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: Add "devcapacity" to PV info

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Add "devcapacity" to PV info
......................................................................


Patch Set 2:

Juan, getDeviceInfo(self, guid) exists in the API (unfortunately, it allows only a single
guid to be specified). Would it be helpful?

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: Add "devcapacity" to PV info

Juan Hernandez has posted comments on this change.

Change subject: Add "devcapacity" to PV info
......................................................................


Patch Set 2:

I already proposed using "getDeviceInfo" for each device, but there we find
another potential performance problem: it needs to be invoked once per LUN. It will be a
problem when a lot of LUNs are being added to the storage domain.

Take a look at (abandoned) change I42187b9e922a5780d87554b03e0581238ec4921c in RHEVM.

How complicated will be adding a bulk "getDeviceInfo" operation? I mean, an
operation that receives a list of LUN UUIDS instead of just one.

Not probably for 3.0, but maybe it is good to start a discussion about this kind of bulk
operations. They help reducing network round trips and improving performance.

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: Add "devcapacity" to PV info

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Add "devcapacity" to PV info
......................................................................


Patch Set 2:

getDevicesInfo() that accepts a list of guids sounds pretty easy to implement.

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#725720 getDeviceList: Limit output to specified GUIDs

Saggi Mizrahi has posted comments on this change.

Change subject: BZ#725720 getDeviceList: Limit output to specified GUIDs
......................................................................


Patch Set 4: Looks good to me, but someone else must approve

Looks, good, don't expect that much of a speed boost though. The major time killer is
the multipath scan and it has to scan all devices.

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#725720 getDeviceList: Limit output to specified GUIDs

Juan Hernandez has posted comments on this change.

Change subject: BZ#725720 getDeviceList: Limit output to specified GUIDs
......................................................................


Patch Set 4:

Are the devices being scanned every time the operation is invoked? Aren't we caching
that information somewhere?

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#725720 getDeviceList: Limit output to specified GUIDs

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#725720 getDeviceList: Limit output to specified GUIDs
......................................................................


Patch Set 4:

Yes, getDeviceList is usually called after storage connectivity has changed, and includes
a fresh multipath scan. Please test if that's bearable for your use case, and if not -
the -1 button is right above...

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#725720 getDeviceList: Limit output to specified GUIDs

Juan Hernandez has posted comments on this change.

Change subject: BZ#725720 getDeviceList: Limit output to specified GUIDs
......................................................................


Patch Set 4: I would prefer that you didn't submit this

After thinking about the performance implications and the possible incompatibilities
between RHEV-M 3 and VDSM 2.3 I think that this is too risky for 3.0.

I still think that is the right solution, but not now. Can we revert to patch set 2 ?

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#725720 getDeviceList: Limit output to specified GUIDs

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#725720 getDeviceList: Limit output to specified GUIDs
......................................................................


Patch Set 4:

I is not unthinkable to rethink patch set 2. But would you elaborate on the risks that you
see in patch set 4?

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#725720 Add "devcapacity" to PV info

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#725720 Add "devcapacity" to PV info
......................................................................


Patch Set 5: Verified; Looks good to me, approved

Sorry, Eduardo, I am caving in in order to make life easier on RHEV-M.

--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#725720 Add "devcapacity" to PV info

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: BZ#725720 Add "devcapacity" to PV info
......................................................................


BZ#725720 Add "devcapacity" to PV info

Add a new "devcapacity" field to the PV info provided by VDSM.
This is obtained from the "dev_size" from "pvs". This and change
Ic0bd3c94a6e4999cb91f96c798a0108890321fbd in RHEV-M are
required to fix bug #716964 in RHEV-M.

Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
---
M vdsm/storage/hsm.py
M vdsm/storage/lvm.py
2 files changed, 2 insertions(+), 1 deletion(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved


--
To view, visit http://gerrit.usersys.redhat.com/734
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic0bd3c94a6e4999cb91f96c798a0108890321fbd
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo Warszawski <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Juan Hernandez <juan.hernandez(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;