Nir Soffer has posted comments on this change. Change subject: lvm: Prevent auto-actviation of logical volumes ......................................................................
Patch Set 1: (6 comments) .................................................... Commit Message 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. It is required to add the --ignoreactivationskip only when needed. How does it give less functionality? Line 21: Line 22: Note: upgrade of existing volumes is not implemented yet. Line 23: Line 24: Change-Id: Ie001c5a4c888bb1ca44bedaeeae6fb75e7cbacc0 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. Line 21: Line 22: Note: upgrade of existing volumes is not implemented yet. I also think so - the only problem is only the spm can invoke this upgrade. How do you suggest to trigger this tool from the spm? Line 23: Line 24: Change-Id: Ie001c5a4c888bb1ca44bedaeeae6fb75e7cbacc0 Line 25: Bug-Url: https://bugzilla.redhat.com/1009812 .................................................... File vdsm/storage/blockSD.py Line 990: operation and is resilient to open LVs, etc. Line 991: """ Line 992: prefix = blockVolume.TAG_PREFIX_IMAGE Line 993: try: Line 994: lvm.changelv(sdUUID, volUUIDs, ("--available", "y"), This is indeed not required, as the underlying call does support both short and long options, but it is more readable to use only long options in this case. How this change remove any functionality? Line 995: ("--deltag", prefix + imgUUID), Line 996: ("--addtag", prefix + opTag + imgUUID)) Line 997: except se.StorageException as e: Line 998: log.error("Can't activate or change LV tags in SD %s. " .................................................... File vdsm/storage/lvm.py Line 755: Line 756: vg: VG name Line 757: lvs: a single LV name or iterable of LV names. Line 758: attrs: lvchange argument tupples. Use None value for unary options. Line 759: e.g. ('--attr', 'value'), ('--unary', None) There are options that we do use, like --refresh. Adding this option allow fixing code calling lvchange directly to use changelv, removing duplicate code. I agree that calling with None is not pretty, but this was the smallest change needed to easily detect a call that does activation, and include the required activation flag. Line 760: Line 761: Note: Line 762: You may activate an activated LV without error Line 763: but lvchange returns an error (RC=5) when activating rw if already rw Line 772: for attr, value in attrs: Line 773: cmd.append(attr) Line 774: if value is not None: Line 775: cmd.append(value) Line 776: if attr in ("-a", "--available", "--activate") and value == "y": Why do you want to add unneeded command line option when it does not have any function? Line 777: cmd.append('--ignoreactivationskip') Line 778: cmd.extend(lvnames) Line 779: rc, out, err = _lvminfo.cmd(tuple(cmd), _lvminfo._getVGDevs((vg, ))) Line 780: _lvminfo._invalidatelvs(vg, lvs) Line 1022: cont = {True: "y", False: "n"}[contiguous] Line 1023: cmd = ["lvcreate"] Line 1024: cmd.extend(LVM_NOBACKUP) Line 1025: cmd.extend(("--contiguous", cont, "--size", "%sm" % size, Line 1026: "--setactivationskip", "y")) I'll add --ignoreactivationskip. Line 1027: if initialTag is not None: Line 1028: cmd.extend(("--addtag", initialTag)) Line 1029: cmd.extend(("--name", lvName, vgName)) Line 1030: 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Eduardo <ewars...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
