Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 2: Code-Review-1 Verified-1 Build Failed http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4511/ : FAILURE http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5311/ : UNSTABLE http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5389/ : FAILURE -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 2: Early draft missing build, packaging, pre-start, and tests. I put this here mainly for backup, review if you like. -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 3: Spelling and whitespace. -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 3: Verified-1 Build Failed http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4512/ : FAILURE http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5312/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5390/ : FAILURE -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 4: Remove unused imports, typos. -- 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 Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 4: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4513/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5313/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5391/ : SUCCESS -- 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 Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 5: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4516/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5316/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5394/ : SUCCESS -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 5: First try the fast path, deactivating the whole vg. It it fails, try to deactivate active lvs. -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
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.w
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Yaniv Bronhaim has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 5: Code-Review-1 see my comments on ps4. thanks -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Yaniv Bronhaim has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 5:
(1 comment)
File lib/vdsm/tool/lvm_deactivate_lvs.py
Line 135: return lv_attr[5] == "o"
Line 136:
Line 137:
Line 138: def deactivate_vg(vg):
Line 139: log("deactivating %s" % vg)
stderr?!
Line 140: rc, out, err = lvm("vgchange", "--available", "n", vg)
Line 141: if rc != 0:
Line 142: raise Error("Error deactivating %s: %s" % (vg, err))
Line 143:
--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Alon Bar-Lev
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 6: Address Yaniv comments: - Commit message cleanup - Use new style imports - Rename Error to clarify its role - Rename find_ function to _iter to clarify their usage - Rename lvm to run_lvm conforming with other commands -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 5: Fine - will add the pre-start invocation in this patch. -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 6: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4525/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5325/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5403/ : SUCCESS -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Alon Bar-Lev has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 5: > Fine - will add the pre-start invocation in this patch. I am almost sure it will be simpler to write this logic entirely in the pre-start using shell. -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Alon Bar-Lev has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 5: > What do you mean by "this logic"? Whatever in: lib/vdsm/tool/lvm_deactivate_lvs.py -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Alon Bar-Lev has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 5: > Forgot to mention - this patch should not include the invocation from > pre-start, to allow easy backport to 3.2, where there is no pre-start. > Invocation will be a separate patch. when you write patch to master you should provide a complete solution, the backport for 3.2 is unrelated to design/implementation considerations. -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 5: What do you mean by "this logic"? -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 5: Forgot to mention - this patch should not include the invocation from pre-start, to allow easy backport to 3.2, where there is no pre-start. Invocation will be a separate patch. -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
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 a
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 5:
(1 comment)
File lib/vdsm/tool/lvm_deactivate_lvs.py
Line 135: return lv_attr[5] == "o"
Line 136:
Line 137:
Line 138: def deactivate_vg(vg):
Line 139: log("deactivating %s" % vg)
Logging go to stderr - this command as meaningful output.
Does it create any issue for the tool or for pre-start?
Line 140: rc, out, err = lvm("vgchange", "--available", "n", vg)
Line 141: if rc != 0:
Line 142: raise Error("Error deactivating %s: %s" % (vg, err))
Line 143:
--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Alon Bar-Lev
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 7: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4526/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5326/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5404/ : SUCCESS -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 7: - Remove the incorrect use of for-else and debug messages when no vgs or lvs are found. - Use more specific function name to make the logic more clear. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Alon Bar-Lev has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 7: Code-Review-1 OK, my note in more clear way. In master the implementation should be future maintainable not backward portable as suggested. This means that if the entire logic can be closed in shell at pre-start in a simple manner, add this to pre-start and not vdsm-tool. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 7: > In master the implementation should be future maintainable not backward > portable as suggested. I think that backward portability is not less important than future maintainability. > This means that if the entire logic can be closed in shell at pre-start in a > simple manner, add this to pre-start and not vdsm-tool. I can re-implement this in shell and put this in vdsmd_init_common.sh, and then create another patch using same or similar shell script for downstream, but this will be more work and will be harder to test and maintain. I would like to have one file with the deactivation logic, that I can test separately from the context where it is used, and use the same file in both upstream and downstream. How about installing this Python script where vdsm_init_common.sh is, and invoking it from pre-start task function in upstream, and from vdsmd.init in downstream? -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Alon Bar-Lev has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 7: > I can re-implement this in shell and put this in vdsmd_init_common.sh, and > then create another patch using same or similar shell script for downstream, > but this will be more work and will be harder to test and maintain. It won't be harder to maintain if I looking into the future. You ask people to maintain something that is not natural only because you want to save you work of backporting a patch one time? I do not understand. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 7: >> I can re-implement this in shell and put this in vdsmd_init_common.sh, and >> then create another patch using same or similar shell script for downstream, >> but this will be more work and will be harder to test and maintain. > It won't be harder to maintain if I looking into the future. You ask people > to maintain something that is not natural only because you want to save you > work of backporting a patch one time? I do not understand. Looking into the future, we may find a bug in the deactivation code, and then we would have to fix both upstream copy and downstream copy. So not only we have more work now, we will have more work in the future. And of course adding this amount of logic to vdsmd_init_common.sh does not makes sense. This script already calls either vsdm-tool verbs or run other Python scritps like vdsm-restore-net-config. I think that same solution used for vdsm-restore-net-config is what we need here. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Alon Bar-Lev has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 7: > Looking into the future, we may find a bug in the deactivation code, and then > we would have to fix both upstream copy and downstream copy. So not only we > have more work now, we will have more work in the future. nobody cares about downstream, this is upstream project. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 7: We are not making any progress - lets talk only about upstream: Don't you think that separate easy to test script is better then dumping lot of hard to test code into vdsmd_init_common.sh? -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Yaniv Bronhaim has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 7: Please add the pre-start \ backwards compatibility part as next patch that depends on this one. That way Alon and I will be able to continue reviewing the usage part and we'll might get into conclusion that the vdsm-tool verb is redundant , and we'll might not. lets see the usage "logic" first Thanks. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Alon Bar-Lev has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 7: > Don't you think that separate easy to test script is better then dumping lot > of hard to test code into vdsmd_init_common.sh? you can argue that for any piece of code, right? -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 8: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4542/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5342/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5421/ : SUCCESS -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 8: Changes: - Remove module documentation - Remove debug logs - Prefix private names with _ - Raise RuntimeError on error and return 0 on success - Merge deactivate_vg and deactivate_lv, since the difference was confusing, and it is uninteresting implementation detail - When deactivating multiple lvs, call lvchange with all lvs instead of multiple calls. - Remove some pointless constants -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 9: Use same import for expose as used in other tool commands. -- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 9: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4543/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5343/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5422/ : SUCCESS -- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 10: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4550/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5350/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5429/ : SUCCESS -- 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: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 10: Rebased over new mock library, required for testing this tool. -- 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: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 11: Start tests and fix first real bug. -- 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: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 11: Verified-1 Build Failed http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4581/ : FAILURE http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5381/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5460/ : FAILURE -- 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: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 12: Test should run now on jenkins. -- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 12: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4582/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5382/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5461/ : SUCCESS -- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Allon Mureinik has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 12:
(1 comment)
File lib/vdsm/tool/lvm_deactivate_lvs.py
Line 36: """
Line 37: if _was_run():
Line 38: _log("already run")
Line 39: return 0
Line 40: _set_was_run()
what if this crashes here, without deactivating the lvs?
Line 41: for vg in _iter_vdsm_vgs():
Line 42: _deactivate_lvs(vg)
Line 43: _log("deactivated lvs")
Line 44: return 0
--
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Alon Bar-Lev
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 12:
(1 comment)
File lib/vdsm/tool/lvm_deactivate_lvs.py
Line 36: """
Line 37: if _was_run():
Line 38: _log("already run")
Line 39: return 0
Line 40: _set_was_run()
If we set this only on successful run, then it will run on every start of vdsm
until it succeeds. Do we want that?
Line 41: for vg in _iter_vdsm_vgs():
Line 42: _deactivate_lvs(vg)
Line 43: _log("deactivated lvs")
Line 44: return 0
--
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Alon Bar-Lev
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Allon Mureinik has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 12:
(1 comment)
File lib/vdsm/tool/lvm_deactivate_lvs.py
Line 36: """
Line 37: if _was_run():
Line 38: _log("already run")
Line 39: return 0
Line 40: _set_was_run()
Sounds good to me.
(note that we will succeed even if not all the lvs are deactivated).
I don't see a downside here. What am I missing?
Line 41: for vg in _iter_vdsm_vgs():
Line 42: _deactivate_lvs(vg)
Line 43: _log("deactivated lvs")
Line 44: return 0
--
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Alon Bar-Lev
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Yaniv Bronhaim has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 12: I need to turn back on myself, as we say in hebrew.. when getting more deeply into it I see that nobody except once in the pre-tasks will use that verb in vdsm-tool. am I right? if no, and an admin might use this verb one day, it should be there. but looks like it doesn't. do you have the following usage patch of that new verb? -- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 13: Code-Review-1 Verified-1 Build Failed http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4618/ : FAILURE http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5418/ : UNSTABLE http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5497/ : FAILURE -- 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: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 14: Complete tests - please check the behavior after various errors and confirm that this is what we want. -- 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: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 14: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4619/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5419/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5498/ : SUCCESS -- 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: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 14: > I need to turn back on myself, as we say in hebrew.. when getting more deeply > into it I see that nobody except once in the pre-tasks will use that verb in > vdsm-tool. am I right? if no, and an admin might use this verb one day, it > should be there. but looks like it doesn't. I don't see why an admin would like to use this. Were do like to install this instead? > do you have the following usage patch of that new verb? Not yet, but it is basically one line in task function "vdsm-tool lvm-deactivate-lvs". -- 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: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 14: pep8 indent fixes. -- 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: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Alon Bar-Lev has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 14: > Not yet, but it is basically one line in task function "vdsm-tool > lvm-deactivate-lvs". gerrit is not a drive for you to save stuff before ready for review. please avoid any more pushes until work is more or less done. thank you. -- 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: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 15: Hopefully complete now. -- 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: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Alon Bar-Lev has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 15: (1 comment) File init/vdsmd_init_common.sh.in Line 222 Line 223 Line 224 Line 225 Line 226 and you actually tested this to be working? how can it work without adding the task to this list? -- 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: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim 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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 15: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4621/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5421/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5500/ : SUCCESS -- 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: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 16: Added the task to the list - sorry for the noise. -- 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: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 16: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4623/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5423/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5502/ : SUCCESS -- 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: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Yaniv Bronhaim has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 16:
(1 comment)
File init/vdsmd_init_common.sh.in
Line 180: }
Line 181:
Line 182: task_lvm_deactivate_lvs(){
Line 183: "$VDSM_TOOL" lvm-deactivate-lvs
Line 184: }
FINALLY ! :)
now I can say that you can just put here:
task_lvm_deactivate_lvs(){
local vgs_info=$(/sbin/lvm vgs --noheadings -o vg_name,tags)
if [ $? -ne 0 ]; then
log_failure_msg "$prog: error checking vgs"
return 1
fi
$(echo $vgs_info | /bin/awk '/MDT_TYPE=FCP/ {print $1}') || return 1
[ ! lvm vgchange -a n $vdsm_vgs ] || return 1
return 0
}
its quite more simple .. and anyhow, you plan to use it only here. no?
Line 185: pre-start tasks end
Line 186:
Line 187:
Line 188: post-stop tasks
--
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: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Alon Bar-Lev
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 16:
(1 comment)
File init/vdsmd_init_common.sh.in
Line 180: }
Line 181:
Line 182: task_lvm_deactivate_lvs(){
Line 183: "$VDSM_TOOL" lvm-deactivate-lvs
Line 184: }
It is simpler because it is does not have the same functionality, not testable
and less modular. Otherwise it is fine :-)
Line 185: pre-start tasks end
Line 186:
Line 187:
Line 188: post-stop tasks
--
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: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Alon Bar-Lev
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Alon Bar-Lev has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 16:
(1 comment)
File init/vdsmd_init_common.sh.in
Line 180: }
Line 181:
Line 182: task_lvm_deactivate_lvs(){
Line 183: "$VDSM_TOOL" lvm-deactivate-lvs
Line 184: }
Please do, and drop the complex unreadable python code.
Or at least workout the python code so it will be sane.
But for these kind of tasks there is no need for that.
task_lvm_deactivate_lvs() {
local guard="@VDSMRUNDIR@/vdsm-lvm"
[ -f "${guard}" ] || return 0
touch "${guard}"
local line
lvm vgs --noheading --separator '|' -o name,tags | while IFS='|' read vg
tags; do
vdsmFCVG() {
local tags="$1"
echo "${tags}" | grep -q "RHAT_storage_domain" &&
echo "${tags}" | grep -q "MDT_TYPE=FCP"
}
vg=$(echo ${vg}) # strip
if vdsmFCVG "${tags}"; then
if ! lvm vgchange --available n "${vg}"; then
lvm lvs --noheading --separator '|' -o name,attr "${vg}" |
while IFS='|' read lv attr; do
lv=$(echo ${lv}) # strip
case "${attr}" in
ao*|a-*)
echo "Skiping ${vg}/${lv} active or open"
;;
*)
lvm lvchange --available n "${vg}/${lv}" || exit 1
;;
esac
done || exit 1
fi
fi
done
if [ $? != 0 ]; then
echo "Failed to diactivate lvs"
return 1
fi
return 0
}
Line 185: pre-start tasks end
Line 186:
Line 187:
Line 188: post-stop tasks
--
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: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Alon Bar-Lev
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Yaniv Bronhaim has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 16:
(2 comments)
File init/vdsmd_init_common.sh.in
Line 180: }
Line 181:
Line 182: task_lvm_deactivate_lvs(){
Line 183: "$VDSM_TOOL" lvm-deactivate-lvs
Line 184: }
you can put it in separate sh file and add tests in it that runs during make if
you prefer. or add this logic to lvm.py with same tests and call it. I agree
that tests are required here and seems like you add alot of thoughts into
them...
but as we see, my first advice of putting the code in vdsm-tool was wrong.
Line 185: pre-start tasks end
Line 186:
Line 187:
Line 188: post-stop tasks
File tests/toolLvmDeactivateLvsTests.py
Line 18: #
Line 19: # Refer to the README and COPYING files for full details of the license
Line 20: #
Line 21:
Line 22: from vdsm.tool import lvm_deactivate_lvs as module
this is confusing .. leave it lvm_deactivate_lvs
Line 23: from vdsm import constants
Line 24: from vdsm import utils
Line 25:
Line 26: from testrunner import VdsmTestCase as TestCaseBase
--
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: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Alon Bar-Lev
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Alon Bar-Lev has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 16:
(1 comment)
File init/vdsmd_init_common.sh.in
Line 180: }
Line 181:
Line 182: task_lvm_deactivate_lvs(){
Line 183: "$VDSM_TOOL" lvm-deactivate-lvs
Line 184: }
We do not require to perform unit test to lvm program... this logic is not to
be changed unless lvm is changed.
Sorry... had misunderstanding of the python logic... a fix:
task_lvm_deactivate_lvs() {
local guard="@VDSMRUNDIR@/vdsm-lvm"
[ -f "${guard}" ] || return 0
touch "${guard}"
local line
lvm vgs --noheading --separator '|' -o name,tags | while IFS='|' read vg
tags; do
vdsmFCVG() {
local tags="$1"
echo "${tags}" | grep -q "RHAT_storage_domain" &&
echo "${tags}" | grep -q "MDT_TYPE=FCP"
}
vg=$(echo ${vg}) # strip
if vdsmFCVG "${tags}"; then
if ! lvm vgchange --available n "${vg}"; then
lvm lvs --noheading --separator '|' -o name,attr "${vg}" |
while IFS='|' read lv attr; do
lv=$(echo ${lv}) # strip
case "${attr}" in
-*);;
ao*)
echo "Skiping ${vg}/${lv} active or open"
;;
*)
lvm lvchange --available n "${vg}/${lv}" || exit 1
;;
esac
done || exit 1
fi
fi
done
if [ $? != 0 ]; then
echo "Failed to diactivate lvs"
return 1
fi
return 0
}
Line 185: pre-start tasks end
Line 186:
Line 187:
Line 188: post-stop tasks
--
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: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Alon Bar-Lev
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Yaniv Bronhaim has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 16:
(1 comment)
File init/vdsmd_init_common.sh.in
Line 180: }
Line 181:
Line 182: task_lvm_deactivate_lvs(){
Line 183: "$VDSM_TOOL" lvm-deactivate-lvs
Line 184: }
The tests check the calls more than the lvm itself, like the parameters we send
to the lvm command and the guard logic.. it may be nice to have those and it
will fail if someone will decide to change it. although, if someone decides to
change the cmd parameters because lvm was changed, now he'll just need to
modify it in two locations.
it doesn't test more except the parameter you except to get and the output you
except to get if specific parameters were sent.. or am i missing something
there?
Line 185: pre-start tasks end
Line 186:
Line 187:
Line 188: post-stop tasks
--
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: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Alon Bar-Lev
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 17: Use original module name in the tests. -- 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: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
oVirt Jenkins CI Server has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 17: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4632/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5432/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5511/ : SUCCESS -- 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: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Alon Bar-Lev has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 17: I am out of this review. Have fun. -- 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: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 16:
(2 comments)
File init/vdsmd_init_common.sh.in
Line 180: }
Line 181:
Line 182: task_lvm_deactivate_lvs(){
Line 183: "$VDSM_TOOL" lvm-deactivate-lvs
Line 184: }
This is very smart shell script, but I'm not sure that it is equivalent to the
python code or correct, and it is not less complex. Since we don't know yet how
to clone you, it will be easier to maintain the Python version.
Can you point to the complex or unclear code in the Python version so I can fix
them?
Line 185: pre-start tasks end
Line 186:
Line 187:
Line 188: post-stop tasks
Line 180: }
Line 181:
Line 182: task_lvm_deactivate_lvs(){
Line 183: "$VDSM_TOOL" lvm-deactivate-lvs
Line 184: }
The tests check that lvm is called with the correct parameters, and lvm output
is handled correctly.
The only thing that we don't have to test there is parameter order;
[--noheadings, --separator, "|"] is the same as [--separator, "|",
--noheadings]. To support that we need a much smarter mock or stub which knows
lvm semantics. I don't think it worth the effort to go in this direction, as
parameter order is unlikely to change.
Line 185: pre-start tasks end
Line 186:
Line 187:
Line 188: post-stop tasks
--
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: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Allon Mureinik has posted comments on this change. Change subject: fcp: Deactivate vdsm logical volumes .. Patch Set 17: Code-Review+1 -- 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: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 17:
(2 comments)
File lib/vdsm/tool/lvm_deactivate_lvs.py
Line 32: @expose(_NAME)
Line 33: def lvm_deactivate_lvs(*args, **kwargs):
Line 34: """
Line 35: Deactivates unused vdsm lvs.
Line 36: """
Add comment:
Deactivating can be very slow if a vg has many lvs. So don't want to do this on
each start of the service, even if it may fix accidental activation of some
lvs. We need another solution for this case.
Line 37: if _was_run():
Line 38: _log("already run")
Line 39: return 0
Line 40: _set_was_run()
Line 58:
Line 59:
Line 60: def _iter_vdsm_vgs():
Line 61: rc, out, err = _run_lvm(["vgs", "--noheading", "--separator",
Line 62: _SEPARATOR, "-o", "name,tags"])
We should use here filter as used in lvm module. Otherwise it may be much
slower, or we may fail accessing unrelated storage. We have this code in
vdsm.storage.lvm.
Maybe move this to lvm.bootstrap()?
Line 63: if rc != 0:
Line 64: raise RuntimeError("Error finding vgs: %s" % err)
Line 65: for line in out.splitlines():
Line 66: line = line.strip()
--
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: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Federico Simoncelli has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 17:
(4 comments)
Nice. Few minor questions.
File lib/vdsm/tool/lvm_deactivate_lvs.py
Line 36: """
Line 37: if _was_run():
Line 38: _log("already run")
Line 39: return 0
Line 40: _set_was_run()
What was the conclusion about this? It shouldn't be moved after deactivation?
Line 41: for vg in _iter_vdsm_vgs():
Line 42: _deactivate_lvs(vg)
Line 43: _log("deactivated lvs")
Line 44: return 0
Line 56: def _log(msg):
Line 57: sys.stderr.write("%s\n" % (msg,))
Line 58:
Line 59:
Line 60: def _iter_vdsm_vgs():
I have the feeling that you can get rid of this entirely, check comment at line
~74.
Line 61: rc, out, err = _run_lvm(["vgs", "--noheading", "--separator",
Line 62: _SEPARATOR, "-o", "name,tags"])
Line 63: if rc != 0:
Line 64: raise RuntimeError("Error finding vgs: %s" % err)
Line 70:
Line 71:
Line 72: def _iter_active_lvs(vg):
Line 73: rc, out, err = _run_lvm(["lvs", "--noheading", "--separator",
Line 74: _SEPARATOR, "-o", "name,attr", vg])
I think that using:
lvs @RHAT_storage_domain
(plus the other required options) would do. I'm not sure if you can specify
somehow also the MDT_TYPE=FCP. But we probably don't care so much about that
one.
Line 75: if rc != 0:
Line 76: raise RuntimeError("Error finding lvs: %s" % err)
Line 77: for line in out.splitlines():
Line 78: line = line.strip()
Line 103: def _deactivate_lvs(vg):
Line 104: cmd = ["vgchange", "--available", "n", vg]
Line 105: rc, out, err = _run_lvm(cmd)
Line 106: if rc != 0:
Line 107: # Probbaly there are open lvs. lvm does not gives us any way
to detect
Typo
Line 108: # error type.
Line 109: cmd = ["lvchange", "--available", "n"]
Line 110: cmd.extend("%s/%s" % (vg, lv) for lv in _iter_active_lvs(vg))
Line 111: rc, out, err = _run_lvm(cmd)
--
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: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 17:
(4 comments)
File lib/vdsm/tool/lvm_deactivate_lvs.py
Line 36: """
Line 37: if _was_run():
Line 38: _log("already run")
Line 39: return 0
Line 40: _set_was_run()
We want to avoid a situation where deactivation fails, we do net set the was
run flag, then deactivation fails again on the next run... and so on.
Line 41: for vg in _iter_vdsm_vgs():
Line 42: _deactivate_lvs(vg)
Line 43: _log("deactivated lvs")
Line 44: return 0
Line 56: def _log(msg):
Line 57: sys.stderr.write("%s\n" % (msg,))
Line 58:
Line 59:
Line 60: def _iter_vdsm_vgs():
This will work only if run before sanlock opens the ids lv. When running
normally, ids lv is always open and vgchange will always fail. Do you know when
sanlock is opening the ids lv?
Line 61: rc, out, err = _run_lvm(["vgs", "--noheading", "--separator",
Line 62: _SEPARATOR, "-o", "name,tags"])
Line 63: if rc != 0:
Line 64: raise RuntimeError("Error finding vgs: %s" % err)
Line 70:
Line 71:
Line 72: def _iter_active_lvs(vg):
Line 73: rc, out, err = _run_lvm(["lvs", "--noheading", "--separator",
Line 74: _SEPARATOR, "-o", "name,attr", vg])
Ayal thinks that we should remove the FCP check, and handle any lv active when
it should not. I added check because we have a problem with FC lvs, and I did
not want to change stuff which is not broken.
Line 75: if rc != 0:
Line 76: raise RuntimeError("Error finding lvs: %s" % err)
Line 77: for line in out.splitlines():
Line 78: line = line.strip()
Line 103: def _deactivate_lvs(vg):
Line 104: cmd = ["vgchange", "--available", "n", vg]
Line 105: rc, out, err = _run_lvm(cmd)
Line 106: if rc != 0:
Line 107: # Probbaly there are open lvs. lvm does not gives us any way
to detect
Done
Line 108: # error type.
Line 109: cmd = ["lvchange", "--available", "n"]
Line 110: cmd.extend("%s/%s" % (vg, lv) for lv in _iter_active_lvs(vg))
Line 111: rc, out, err = _run_lvm(cmd)
--
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: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes
..
Patch Set 17:
(2 comments)
File lib/vdsm/tool/lvm_deactivate_lvs.py
Line 56: def _log(msg):
Line 57: sys.stderr.write("%s\n" % (msg,))
Line 58:
Line 59:
Line 60: def _iter_vdsm_vgs():
Sorry - I was talking about vgchange bellow.
Line 61: rc, out, err = _run_lvm(["vgs", "--noheading", "--separator",
Line 62: _SEPARATOR, "-o", "name,tags"])
Line 63: if rc != 0:
Line 64: raise RuntimeError("Error finding vgs: %s" % err)
Line 70:
Line 71:
Line 72: def _iter_active_lvs(vg):
Line 73: rc, out, err = _run_lvm(["lvs", "--noheading", "--separator",
Line 74: _SEPARATOR, "-o", "name,attr", vg])
lvs @tag indeed works.
Line 75: if rc != 0:
Line 76: raise RuntimeError("Error finding lvs: %s" % err)
Line 77: for line in out.splitlines():
Line 78: line = line.strip()
--
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: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Maor Lipchuk
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
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
Change in vdsm[master]: fcp: Deactivate vdsm logical volumes
Nir Soffer has abandoned this change. Change subject: fcp: Deactivate vdsm logical volumes .. Abandoned Replaced by http://gerrit.ovirt.org/#/c/21291/ -- To view, visit http://gerrit.ovirt.org/20720 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I8f72a68ad09566ba222aa45448c78d1577c40d21 Gerrit-PatchSet: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Sergey Gotliv Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
