Change in vdsm[master]: lvm: Enhance changelv function to fix option repeated error
tjeyasin at redhat.com
tjeyasin at redhat.com
Thu Mar 12 10:38:21 UTC 2015
Timothy Asir has posted comments on this change.
Change subject: lvm: Enhance changelv function to fix option repeated error
......................................................................
Patch Set 6:
(5 comments)
https://gerrit.ovirt.org/#/c/38195/6//COMMIT_MSG
Commit Message:
Line 3: AuthorDate: 2015-02-26 17:13:42 +0530
Line 4: Commit: Timothy Asir Jeyasingh <tjeyasin at redhat.com>
Line 5: CommitDate: 2015-03-11 06:54:33 +0530
Line 6:
Line 7: lvm: Enhance changelv function to fix option repeated error
> I think what you mean is:
yes. I will update the sentence
Line 8:
Line 9: Currently autobackup option sets to 'false' inside the function in
Line 10: most of the lvm functions like lvcreate, lvremove, etc., in storage.lvm.
Line 11: This will not allow any call to automatically back up metadata after a change.
Line 12:
Line 13: Also currently most of the functions in lvm module accepts list
Line 14: of params in a tuple format and have some mandatory options.
Line 15: This will allow to break the lvm call if any thing called with
Line 16: different values for such mandatory options.
> I don't understand the last sentence. Do you mean: This may cause lvm call
yes. I will update the sentence
Line 17:
Line 18: ex:- lvchange(vg, lvs, ('-autobackup', 'y'))
Line 19: This will returns "Option -A/--autobackup may not be repeated" error.
Line 20:
Line 15: This will allow to break the lvm call if any thing called with
Line 16: different values for such mandatory options.
Line 17:
Line 18: ex:- lvchange(vg, lvs, ('-autobackup', 'y'))
Line 19: This will returns "Option -A/--autobackup may not be repeated" error.
> Lets make it more clear:
Done
Line 20:
Line 21: This patch fix this issue for changelv function.
Line 22:
Line 23: Change-Id: I8190878fe071fad46800e666795b94779d745abf
Line 17:
Line 18: ex:- lvchange(vg, lvs, ('-autobackup', 'y'))
Line 19: This will returns "Option -A/--autobackup may not be repeated" error.
Line 20:
Line 21: This patch fix this issue for changelv function.
> Why do changelv must support -autobackup y, and other functions do not?
Yes this change required in every functions. I will update the patch based on your suggestion.
in our gluster brick creation task, we will be using this without specifying this autobackup option like follows:
lvconvert --chunksize <dataalignment_val> --thinpool <poolname> --poolmetadata <vg/metadata_name> --poolmetadataspar n -y
Line 22:
Line 23: Change-Id: I8190878fe071fad46800e666795b94779d745abf
https://gerrit.ovirt.org/#/c/38195/6/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:
Line 802: # so we invalidate cache to reload these volumes on first occasion
Line 803: lvnames = tuple("%s/%s" % (vg, lv) for lv in lvs)
Line 804: cmd = ["lvchange"]
Line 805: if not (('--autobackup', 'n') in attrs or ('--autobackup', 'y') in attrs
Line 806: or ('-A', 'n') in attrs or ('-A', 'y') in attrs):
> This will not work as attrs may be a single tuple or list of tuples. It wil
Sure, I agree your suggestion. Later we will enhance to accept kwargs for lvm options.
Line 807: cmd.extend(LVM_NOBACKUP)
Line 808: if isinstance(attrs[0], str):
Line 809: # ("--attribute", "value")
Line 810: cmd.extend(attrs)
--
To view, visit https://gerrit.ovirt.org/38195
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8190878fe071fad46800e666795b94779d745abf
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir <tjeyasin at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at gmail.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Sandro Bonazzola <sbonazzo at redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list