Change in vdsm[master]: lvm: ignore skipped clustered vgs

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

lvm: ignore skipped clustered vgs

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=820982
Change-Id: Idcabb5c1c19106f0db1385787251d951e12ab0cf
Signed-off-by: Federico Simoncelli 
---
M vdsm.spec.in
M vdsm/storage/lvm.py
2 files changed, 3 insertions(+), 3 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/48/20248/1

diff --git a/vdsm.spec.in b/vdsm.spec.in
index 0aec5fa..49e652d 100644
--- a/vdsm.spec.in
+++ b/vdsm.spec.in
@@ -159,7 +159,7 @@
 Requires: policycoreutils-python >= 2.0.83-19.30
 Requires: python-pthreading
 Requires: selinux-policy-targeted >= 3.7.19-195.el6.13
-Requires: lvm2 >= 2.02.95-10.el6_3.2
+Requires: lvm2 >= 2.02.100-5
 Requires: logrotate < 3.8.0
 %endif
 %else
@@ -187,7 +187,7 @@
 Requires: policycoreutils-python
 Requires: sed >= 4.2.1-10
 Requires: ed
-Requires: lvm2 >= 2.02.95
+Requires: lvm2 >= 2.02.103

%if 0%{?fedora} >= 18
 %if 0%{?fedora} >= 19
diff --git a/vdsm/storage/lvm.py b/vdsm/storage/lvm.py
index 0c2964e..39a0b3f 100644
--- a/vdsm/storage/lvm.py
+++ b/vdsm/storage/lvm.py
@@ -81,7 +81,7 @@
 SEPARATOR = "|"
 LVM_NOBACKUP = ("--autobackup", "n")
 LVM_FLAGS = ("--noheadings", "--units", "b", "--nosuffix", "--separator",
-             SEPARATOR)
+             SEPARATOR, "--ignoreskippedcluster")

PV_PREFIX = "/dev/mapper"
 # Assuming there are no spaces in the PV name

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 1:

(1 comment)

....................................................
File vdsm.spec.in
Line 186: Requires: sanlock >= 2.4-2, sanlock-python
Line 187: Requires: policycoreutils-python
Line 188: Requires: sed >= 4.2.1-10
Line 189: Requires: ed
Line 190: Requires: lvm2 >= 2.02.103
Let's wait for the upstream release.
Line 191: 
Line 192: %if 0%{?fedora} >= 18
Line 193: %if 0%{?fedora} >= 19
Line 194: Requires: selinux-policy-targeted >= 3.12.1-71

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 1: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5018/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4131/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4941/ : SUCCESS

Ayal Baron has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 1:

(1 comment)

....................................................
File vdsm/storage/lvm.py
Line 80: 
Line 81: SEPARATOR = "|"
Line 82: LVM_NOBACKUP = ("--autobackup", "n")
Line 83: LVM_FLAGS = ("--noheadings", "--units", "b", "--nosuffix", "--separator",
Line 84:              SEPARATOR, "--ignoreskippedcluster")
1. would this mean that we would no longer be able to even list such PVs? (e.g. in getDeviceList)

2. could we make the option resolve to None if lvm version is lower than required instead of requiring the minimal lvm version (which is not available yet afaiu?)
Line 85: 
Line 86: PV_PREFIX = "/dev/mapper"
Line 87: # Assuming there are no spaces in the PV name
Line 88: re_pvName = re.compile(PV_PREFIX + '[^\s"]+', re.MULTILINE)

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 1: Code-Review-1

(4 comments)

This patch may be great but it does not explain why it is needed, not in the commit message or code. And it will possibly break the installation again, adding dependency on newer versions of lvm.

....................................................
Commit Message
Line 3: AuthorDate: 2013-10-16 11:38:25 -0400
Line 4: Commit:     Federico Simoncelli 
Line 5: CommitDate: 2013-10-16 11:39:55 -0400
Line 6: 
Line 7: lvm: ignore skipped clustered vgs
Why? What is the context of this change?
Line 8: 
Line 9: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=820982
Line 10: Change-Id: Idcabb5c1c19106f0db1385787251d951e12ab0cf

....................................................
File vdsm.spec.in
Line 158: Requires: policycoreutils >= 2.0.83-19.30
Line 159: Requires: policycoreutils-python >= 2.0.83-19.30
Line 160: Requires: python-pthreading
Line 161: Requires: selinux-policy-targeted >= 3.7.19-195.el6.13
Line 162: Requires: lvm2 >= 2.02.100-5
Is it available now?
Line 163: Requires: logrotate < 3.8.0
Line 164: %endif
Line 165: %else
Line 166: %if 0%{?fedora} >= 19

Line 186: Requires: sanlock >= 2.4-2, sanlock-python
Line 187: Requires: policycoreutils-python
Line 188: Requires: sed >= 4.2.1-10
Line 189: Requires: ed
Line 190: Requires: lvm2 >= 2.02.103
Not sure what do you mean by this comment. Is this version available now?
Line 191: 
Line 192: %if 0%{?fedora} >= 18
Line 193: %if 0%{?fedora} >= 19
Line 194: Requires: selinux-policy-targeted >= 3.12.1-71

....................................................
File vdsm/storage/lvm.py
Line 80: 
Line 81: SEPARATOR = "|"
Line 82: LVM_NOBACKUP = ("--autobackup", "n")
Line 83: LVM_FLAGS = ("--noheadings", "--units", "b", "--nosuffix", "--separator",
Line 84:              SEPARATOR, "--ignoreskippedcluster")
Using this option only if available should be better, unless this code will never run with older lvm.
Line 85: 
Line 86: PV_PREFIX = "/dev/mapper"
Line 87: # Assuming there are no spaces in the PV name
Line 88: re_pvName = re.compile(PV_PREFIX + '[^\s"]+', re.MULTILINE)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5114/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4310/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5188/ : SUCCESS

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 1:

(5 comments)

....................................................
Commit Message
Line 3: AuthorDate: 2013-10-16 11:38:25 -0400
Line 4: Commit:     Federico Simoncelli 
Line 5: CommitDate: 2013-10-16 11:39:55 -0400
Line 6: 
Line 7: lvm: ignore skipped clustered vgs
Done
Line 8: 
Line 9: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=820982
Line 10: Change-Id: Idcabb5c1c19106f0db1385787251d951e12ab0cf

....................................................
File vdsm.spec.in
Line 158: Requires: policycoreutils >= 2.0.83-19.30
Line 159: Requires: policycoreutils-python >= 2.0.83-19.30
Line 160: Requires: python-pthreading
Line 161: Requires: selinux-policy-targeted >= 3.7.19-195.el6.13
Line 162: Requires: lvm2 >= 2.02.100-5
Yes it was.
Line 163: Requires: logrotate < 3.8.0
Line 164: %endif
Line 165: %else
Line 166: %if 0%{?fedora} >= 19

Line 186: Requires: sanlock >= 2.4-2, sanlock-python
Line 187: Requires: policycoreutils-python
Line 188: Requires: sed >= 4.2.1-10
Line 189: Requires: ed
Line 190: Requires: lvm2 >= 2.02.103
No it wasn't (that was the point of the comment) but now it is.
Line 191: 
Line 192: %if 0%{?fedora} >= 18
Line 193: %if 0%{?fedora} >= 19
Line 194: Requires: selinux-policy-targeted >= 3.12.1-71

....................................................
File vdsm/storage/lvm.py
Line 80: 
Line 81: SEPARATOR = "|"
Line 82: LVM_NOBACKUP = ("--autobackup", "n")
Line 83: LVM_FLAGS = ("--noheadings", "--units", "b", "--nosuffix", "--separator",
Line 84:              SEPARATOR, "--ignoreskippedcluster")
1. this affects vgs (not pvs), and we were never able to see them to begin with

2. that needs infrastructure (long discussed in the past)
Line 85: 
Line 86: PV_PREFIX = "/dev/mapper"
Line 87: # Assuming there are no spaces in the PV name
Line 88: re_pvName = re.compile(PV_PREFIX + '[^\s"]+', re.MULTILINE)

Line 80: 
Line 81: SEPARATOR = "|"
Line 82: LVM_NOBACKUP = ("--autobackup", "n")
Line 83: LVM_FLAGS = ("--noheadings", "--units", "b", "--nosuffix", "--separator",
Line 84:              SEPARATOR, "--ignoreskippedcluster")
As previous 2, we don't have the correct infrastructure at the moment. I'd rather tackle this properly (correct infra), and anyway in this case the fix is something that requires an hard dependency (as it's not just an improvement).
Line 85: 
Line 86: PV_PREFIX = "/dev/mapper"
Line 87: # Assuming there are no spaces in the PV name
Line 88: re_pvName = re.compile(PV_PREFIX + '[^\s"]+', re.MULTILINE)

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 2: Code-Review-1

(2 comments)

....................................................
File vdsm.spec.in
Line 159: Requires: policycoreutils >= 2.0.83-19.30
Line 160: Requires: policycoreutils-python >= 2.0.83-19.30
Line 161: Requires: python-pthreading
Line 162: Requires: selinux-policy-targeted >= 3.7.19-195.el6.13
Line 163: Requires: lvm2 >= 2.02.100-5
that's not yet in the open. We'd have to wait with this patch until el6.5 is out.
Line 164: Requires: logrotate < 3.8.0
Line 165: %endif
Line 166: %else
Line 167: %if 0%{?fedora} >= 19

Line 187: Requires: sanlock >= 2.4-2, sanlock-python
Line 188: Requires: policycoreutils-python
Line 189: Requires: sed >= 4.2.1-10
Line 190: Requires: ed
Line 191: Requires: lvm2 >= 2.02.103
I'd like to keep support for f19 (which lacks this). Can this feature be backported to f19's lvm2?

I hate hate feature negotiation, but in this case, sending the extra flag only to lvm2 that support it may be a good way forward.
Line 192: 
Line 193: %if 0%{?fedora} >= 18
Line 194: %if 0%{?fedora} >= 19
Line 195: Requires: selinux-policy-targeted >= 3.12.1-71

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 2:

(2 comments)

....................................................
File vdsm.spec.in
Line 159: Requires: policycoreutils >= 2.0.83-19.30
Line 160: Requires: policycoreutils-python >= 2.0.83-19.30
Line 161: Requires: python-pthreading
Line 162: Requires: selinux-policy-targeted >= 3.7.19-195.el6.13
Line 163: Requires: lvm2 >= 2.02.100-5
this patch is for the master (unstable) branch which means that it might be using not fully released builds.
Line 164: Requires: logrotate < 3.8.0
Line 165: %endif
Line 166: %else
Line 167: %if 0%{?fedora} >= 19

Line 187: Requires: sanlock >= 2.4-2, sanlock-python
Line 188: Requires: policycoreutils-python
Line 189: Requires: sed >= 4.2.1-10
Line 190: Requires: ed
Line 191: Requires: lvm2 >= 2.02.103
again this is the master branch, f20 (which this branch is targeting) already has it
Line 192: 
Line 193: %if 0%{?fedora} >= 18
Line 194: %if 0%{?fedora} >= 19
Line 195: Requires: selinux-policy-targeted >= 3.12.1-71

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 2:

(2 comments)

....................................................
File vdsm.spec.in
Line 159: Requires: policycoreutils >= 2.0.83-19.30
Line 160: Requires: policycoreutils-python >= 2.0.83-19.30
Line 161: Requires: python-pthreading
Line 162: Requires: selinux-policy-targeted >= 3.7.19-195.el6.13
Line 163: Requires: lvm2 >= 2.02.100-5
Still, this makes "master" unusable, and we've got plenty of users complaining about such breakage. I'd like to minimize them.
Line 164: Requires: logrotate < 3.8.0
Line 165: %endif
Line 166: %else
Line 167: %if 0%{?fedora} >= 19

Line 187: Requires: sanlock >= 2.4-2, sanlock-python
Line 188: Requires: policycoreutils-python
Line 189: Requires: sed >= 4.2.1-10
Line 190: Requires: ed
Line 191: Requires: lvm2 >= 2.02.103
I try to support two consecutive Fedora versions; we have no f20 testing slave yet. It's too early for forsaking F19.
Line 192: 
Line 193: %if 0%{?fedora} >= 18
Line 194: %if 0%{?fedora} >= 19
Line 195: Requires: selinux-policy-targeted >= 3.12.1-71

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 2: Code-Review+1

Looks good, not sure how we can deal with the Fedora 19 dependency.

Itamar Heim has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 2:

The code must handle running on distro with and without this version of lvm. once we know a distro has this lvm version, would be good to require it.
eduardo has the same issue in another patch

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 2:

Blocked on https://bugzilla.redhat.com/show_bug.cgi?id=1065105

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 3: Code-Review+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7333/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6431/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7215/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/347/ : SUCCESS

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 3: Verified+1

Verified checking the spec requires for el6, f19 and newer. When nothing else matches we require 2.02.103 (upstream official version).

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 3: Code-Review+2

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

lvm: ignore skipped clustered vgs

In VDSM we use lvm locking_type 1 (local flocks on files in locking_dir)
because the clustered vg access is handled with the SPM-HSM logic.
When locking_type is configured to 1 the clustered vgs are skipped for
all the operations (as they need locking_type 2) and by default the exit
code of the lvm command is 5 triggering errors in our module.
Adding the --ignoreskippedcluster flag to lvm commands we avoid the exit
code error in the event of any clustered vg being present in some of the
attached LUNs.

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=820982
Change-Id: Idcabb5c1c19106f0db1385787251d951e12ab0cf
Signed-off-by: Federico Simoncelli 
Reviewed-on: http://gerrit.ovirt.org/20248
Reviewed-by: Nir Soffer 
Reviewed-by: Dan Kenigsberg 
---
M vdsm.spec.in
M vdsm/storage/lvm.py
2 files changed, 7 insertions(+), 3 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, but someone else must approve
  Federico Simoncelli: Verified
  Dan Kenigsberg: Looks good to me, approved

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: lvm: ignore skipped clustered vgs
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/349/ : SUCCESS