Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes ......................................................................
Patch Set 4: (20 comments) .................................................... Commit Message Line 13: corruption sooner or later, when a vm is trying to write to logical Line 14: volume with stale metadata. Line 15: Line 16: This patch adds new vdsm-tool lvm-deactivate-lvs command, deactivating Line 17: vdsm active logical volumes. The command is invoked during boot as part Not finished yet. Line 18: of vdsmd pre-start. If vdsmd was disabled during boot, or an older vdsm Line 19: version was started during boot, the command is invoked in the first Line 20: time vdsmd is started. Line 21: Line 23: running vms, the lvm-deactivate-lvs command does not deactivate open Line 24: devices. Line 25: Line 26: We chose this solution because it is compatible with all upstream and Line 27: downstream supported versions, easy to implement, and does not require Done Line 28: changes in vdsm delicate runtime. Line 29: Line 30: We considered several other solutions: Line 31: - Using volume_list lvm configuration: By adding system vgs to this Line 38: - Modifying system init scripts: We rejected this because it is not Line 39: their responsibility to activate only certain vgs, and touching them is Line 40: last resort. Line 41: Line 42: This patch should be easy to backport to RHEV 3.2, invoking the new I''l replace with ovirt-3.2. Line 43: vdsm-tool command from the legacy vdsmd.init script instead of pre-start Line 44: available only in ovirt 3.3. Line 45: Line 46: Change-Id: I8f72a68ad09566ba222aa45448c78d1577c40d21 .................................................... File lib/vdsm/tool/lvm_deactivate_lvs.py Line 18: # Line 19: Line 20: """ Line 21: When using FC storage, physical volumes are connected during boot, and vdsm Line 22: logical volumes are auto-activated during boot. These volumes do not pick Commit message does not replace module documentation. To get the commit message you must search through history, reading multiple commit messages. The reader needs this info here. Line 23: changes done by the SPM on the storage, which may lead to data corruption Line 24: sooner or later, when a vm is trying to write to logical volume with stale Line 25: metadata. Line 26: Line 33: import sys Line 34: Line 35: from vdsm import tool Line 36: from vdsm import utils Line 37: from vdsm import constants I'll use absolute import. Line 38: Line 39: NAME = 'lvm-deactivate-lvs' Line 40: RUN_FILE = os.path.join(constants.P_VDSM_RUN, NAME) Line 41: Line 38: Line 39: NAME = 'lvm-deactivate-lvs' Line 40: RUN_FILE = os.path.join(constants.P_VDSM_RUN, NAME) Line 41: Line 42: LVM_SEPARATOR = "|" Module constant, see vdsm.storage.lvm. Line 43: Line 44: # Tags used to detect vdsm FC vgs Line 45: VG_SD_TAG = "RHAT_storage_domain" Line 46: VG_FCP_TAG = "MDT_TYPE=FCP" Line 48: Line 49: class Error(Exception): Line 50: """ Line 51: Raised for expected errors in this module. Line 52: """ pass is redundant when you have a docstring. This is *lvm_deactivate_lvs.Error* - meaning error in this module which should be treated differently then other errors. Used here to log the error and return a non-zero return code on expected failures. RuntimeError is a library error and should not be used in your code unless you want to make it harder for the caller to tell a real RuntimeError and your module sepcific errors. See http://docs.python.org/2.6/library/exceptions.html#exceptions.RuntimeError Line 53: Line 54: Line 55: @tool.expose(NAME) Line 56: def main(*args, **kwargs): Line 51: Raised for expected errors in this module. Line 52: """ Line 53: Line 54: Line 55: @tool.expose(NAME) Why not a constant? And why not lvm-deactivate-lvs, like libvirt-configure? Line 56: def main(*args, **kwargs): Line 57: """ Line 58: Deactivates unused vdsm volume lvs. Line 59: """ Line 52: """ Line 53: Line 54: Line 55: @tool.expose(NAME) Line 56: def main(*args, **kwargs): I took nwfilter as example - isn't the expose is enough to give info the the tool? Line 57: """ Line 58: Deactivates unused vdsm volume lvs. Line 59: """ Line 60: try: Line 56: def main(*args, **kwargs): Line 57: """ Line 58: Deactivates unused vdsm volume lvs. Line 59: """ Line 60: try: Entire function is covered so I can throw Error everywhere in this module when an expected failure is detected. This does not effect other exceptions anyway. For example, was_run does not throw an Error today, but if we start throwing it in the future, we don't have to change the main logic. Line 61: if was_run(): Line 62: log("already run") Line 63: return 0 Line 64: set_was_run() Line 71: log("no vgs found") Line 72: return 0 Line 73: except Error as e: Line 74: log(e) Line 75: return 1 I took sanlock.py as example and it does return either 0 or 1 - are you sure about it? Line 76: Line 77: Line 78: def log(msg): Line 79: sys.stderr.write("%s: %s\n" % (NAME, msg)) Line 74: log(e) Line 75: return 1 Line 76: Line 77: Line 78: def log(msg): Do you suggest to duplicate this logging code everywhere? Most of the log calls here are for debugging - we probably need a way to disable them in production. Maybe use logging module? Line 79: sys.stderr.write("%s: %s\n" % (NAME, msg)) Line 80: Line 81: Line 82: def was_run(): Line 79: sys.stderr.write("%s: %s\n" % (NAME, msg)) Line 80: Line 81: Line 82: def was_run(): Line 83: return os.path.exists(RUN_FILE) I prefer self explaining code to comments. Aren't these functions clear? Line 84: Line 85: Line 86: def set_was_run(): Line 87: with open(RUN_FILE, 'w'): Line 83: return os.path.exists(RUN_FILE) Line 84: Line 85: Line 86: def set_was_run(): Line 87: with open(RUN_FILE, 'w'): This creates RUN_FILE - how os.acess is related? Line 88: pass Line 89: Line 90: Line 91: def find_vdsm_vgs(): Line 87: with open(RUN_FILE, 'w'): Line 88: pass Line 89: Line 90: Line 91: def find_vdsm_vgs(): Everything here is private except main - do you want to prefix everthing with _? Line 92: rc, out, err = lvm("vgs", "--noheading", "--separator", LVM_SEPARATOR, Line 93: "-o", "vg_name,tags") Line 94: if rc != 0: Line 95: raise Error("Error finding vgs: %s" % err) Line 94: if rc != 0: Line 95: raise Error("Error finding vgs: %s" % err) Line 96: for line in out.splitlines(): Line 97: vg, tags = line.split(LVM_SEPARATOR, 1) Line 98: if is_vdsm_vg(tags): Same reason is_active_lv and is_open_lv are used. Line 99: yield vg Line 100: Line 101: Line 102: def find_lvs(vg): Line 95: raise Error("Error finding vgs: %s" % err) Line 96: for line in out.splitlines(): Line 97: vg, tags = line.split(LVM_SEPARATOR, 1) Line 98: if is_vdsm_vg(tags): Line 99: yield vg Yielding it cleaner here and allow both iterative use for the caller, and if needed, the caller can collect the value using list(find_vgs) as needed. More options, less code. Line 100: Line 101: Line 102: def find_lvs(vg): Line 103: rc, out, err = lvm("lvs", "--noheading", "--separator", LVM_SEPARATOR, Line 110: continue Line 111: if is_open_lv(attr): Line 112: log("Warning: skipping %s/%s: device is open" % (vg, lv)) Line 113: continue Line 114: yield lv Same reason Line 115: Line 116: Line 117: def is_vdsm_vg(vg_tags): Line 118: tags = set(vg_tags.split(",")) Line 120: Line 121: Line 122: def is_active_lv(lv_attr): Line 123: # -wi-ao--- Line 124: return lv_attr[4] == "a" What do you think is more readable: if not is_active_lv(attr): continue if attr[4] == "-": # is not active (-wi------) continue The extra function call cost nothing, the readability improved. I can replace this with is_inactive, as this is what we look for: if is_inactive(attr): continue Line 125: Line 126: Line 127: def is_open_lv(lv_attr): Line 128: # -wi-ao--- Line 136: if rc != 0: Line 137: raise Error("Error deactivating %s: %s" % (lv_path, err)) Line 138: Line 139: Line 140: def lvm(*args): I'll call it run_lvm as this is lvm specific. Line 141: cmd = [constants.EXT_LVM] Line 142: cmd.extend(args) -- To view, visit http://gerrit.ovirt.org/20720 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8f72a68ad09566ba222aa45448c78d1577c40d21 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@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
