Yaniv Bronhaim has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
......................................................................
Patch Set 4:
(21 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
the invocation is not part of that patch.. why not?
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
s/ "all upstream and downstreadm supported versions"/ "all vdsm supported
versions"
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
should be ovirt 3.2 ? or shouldn't mentioned at all
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 is enough
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
use: from .. import tool, utils, constants
Line 38:
Line 39: NAME = 'lvm-deactivate-lvs'
Line 40: RUN_FILE = os.path.join(constants.P_VDSM_RUN, NAME)
Line 41:
Line 36: from vdsm import utils
Line 37: from vdsm import constants
Line 38:
Line 39: NAME = 'lvm-deactivate-lvs'
Line 40: RUN_FILE = os.path.join(constants.P_VDSM_RUN, NAME)
no need for NAME:
RUN_FILE = os.path.join(constants.P_VDSM_RUN, 'lvm-deactivate-lvs')
Line 41:
Line 42: LVM_SEPARATOR = "|"
Line 43:
Line 44: # Tags used to detect vdsm FC vgs
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 = "|"
private?
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 ?
can you give it more meaningful name than Error? why not using 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)
please use a string explicitly as in all pys. should me - "deactivate-lvs"
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):
why main? call it "def deactivate_lvs"
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:
cover try only where needed.
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
should raise an error on failures
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):
we don't do it anywhere else.. why do you add it here? just call to sys.stderr
whenever you need please.
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)
you can explain the use of RUN_FILE and avoid those one line functions. if it
is explained, remove those functions and use os.path.exists and os.access
directly
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'):
you can use os.access
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():
all helper functions should be private (use _ prefix)
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):
why not:
if VG_SD_TAG and VG_FCP_TAG in set(tags.aplit(",")):
...
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
why not returning all list for iteration instead of yield? more understandable
imo (get_vdsm_vgs_list than find_vdsm_vg)
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 as above ^
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"
is_activate_lv = True if lv_attr[4] = "a" else False -> can do the work without
additional function
same for is_open_lv
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):
call it run_cmd as we did in fuser
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
