Change in vdsm[master]: lvm: Prevent auto-actviation of logical volumes

ewarszaw at redhat.com ewarszaw at redhat.com
Mon Nov 4 07:09:04 UTC 2013


Eduardo has posted comments on this change.

Change subject: lvm: Prevent auto-actviation of logical volumes
......................................................................


Patch Set 3: Code-Review-1

(5 comments)

....................................................
Commit Message
Line 8: 
Line 9: When using FC storage, physical volumes are connected during boot, and
Line 10: vdsm logical volumes are auto-activated by /etc/rc.sysinit and/or
Line 11: /etc/init.d/netfs.  This is abnormal situation that vdsm cannot handle,
Line 12: and leads to data corruption.
""" This is abnormal situation that vdsm cannot handle,	11
			and leads to data corruption."""
May lead. Please remove this sentence.
Line 13: 
Line 14: This patch prevents auto-activation of vdsm volumes using new
Line 15: --setactivationskip and --ignoreactivationskip options, introcuded in
Line 16: lvm 2.02.100.


Line 16: lvm 2.02.100.
Line 17: 
Line 18: To make this change easy, lvm.changelv() was modified to accept variable
Line 19: argument tuples, instead of multiple non-related types. This also
Line 20: simplify the only caller that use multiple arguments.
Please remove.
Line 21: 
Line 22: Change-Id: Ie001c5a4c888bb1ca44bedaeeae6fb75e7cbacc0
Line 23: Bug-Url: https://bugzilla.redhat.com/1009812


....................................................
File vdsm/storage/lvm.py
Line 748:             raise se.VolumeGroupActionError(
Line 749:                 "vgchange on vg(s) %s failed. %d %s %s" % (vgs, rc, out, err))
Line 750: 
Line 751: 
Line 752: def changelv(vg, lvs, *attrs):
There is no reason for changing the interface of this function.
Line 753:     """
Line 754:     Change multiple attributes on multiple LVs.
Line 755: 
Line 756:     vg: VG name


Line 769:     cmd = ["lvchange"]
Line 770:     cmd.extend(LVM_NOBACKUP)
Line 771:     for attr, value in attrs:
Line 772:         cmd.extend((attr, value))
Line 773:         if attr in ("-a", "--available", "--activate") and value == "y":
The if is redundant as said before.
There is no need for more logic when it is not necessary. Let lvm do is best.
 And for log's sake will be better (TM) if --ignoreactivationskip is added in the invariant part of the command.
Line 774:             cmd.append("--ignoreactivationskip")
Line 775:     cmd.extend(lvnames)
Line 776:     rc, out, err = _lvminfo.cmd(tuple(cmd), _lvminfo._getVGDevs((vg, )))
Line 777:     _lvminfo._invalidatelvs(vg, lvs)


Line 1019:     cont = {True: "y", False: "n"}[contiguous]
Line 1020:     cmd = ["lvcreate"]
Line 1021:     cmd.extend(LVM_NOBACKUP)
Line 1022:     cmd.extend(("--contiguous", cont, "--size", "%sm" % size,
Line 1023:                 "--setactivationskip", "y", "--ignoreactivationskip"))
In the invariant part, as before.
Line 1024:     if initialTag is not None:
Line 1025:         cmd.extend(("--addtag", initialTag))
Line 1026:     cmd.extend(("--name", lvName, vgName))
Line 1027:     rc, out, err = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vgName, )))


-- 
To view, visit http://gerrit.ovirt.org/20832
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie001c5a4c888bb1ca44bedaeeae6fb75e7cbacc0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgotliv at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list