Nir Soffer has uploaded a new change for review.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
fcp: Deactivate vdsm volume groups during boot
When using FC storage, physical volumes are connected during boot, and vdsm volume groups are auto-activated by /etc/rc.sysinit and/or /etc/init.d/netfs. This is abnormal situation that vdsm cannot handle, and leads to data corruption.
This patch adds a new init script that deactivate FC volume groups during boot.
This script must also be used during installation or ugprade. It is safe to invoke it multiple times; it will modify volume groups only on the first run.
On RHEL 6.5 we can use new activation skipping option instead of this script. I'll address this in a separate patch.
Change-Id: I8f72a68ad09566ba222aa45448c78d1577c40d21 Bug-Url: https://bugzilla.redhat.com/1009812 Signed-off-by: Nir Soffer nsoffer@redhat.com --- A init/sysvinit/vdsm-deactivate-vgs.init 1 file changed, 88 insertions(+), 0 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/20/20720/1
diff --git a/init/sysvinit/vdsm-deactivate-vgs.init b/init/sysvinit/vdsm-deactivate-vgs.init new file mode 100644 index 0000000..0b56c43 --- /dev/null +++ b/init/sysvinit/vdsm-deactivate-vgs.init @@ -0,0 +1,88 @@ +#! /bin/sh +# +# Copyright 2013 Red Hat, Inc. and/or its affiliates. +# +# Licensed to you under the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. See the files README and +# LICENSE_GPL_v2 which accompany this distribution. +# + +# chkconfig: 2345 98 00 +# +### BEGIN INIT INFO +# Provides: vdsm-deacivate-vgs +# Required-Start: $syslog $network +# Should-Start: $time +# Required-Stop: +# Default-Start: 2 3 4 5 +# Default-Stop: 0 1 6 +# Description: dactivate VDS management server logical volumes +# Short-Description: dactivate VDS management server logical volumes +### END INIT INFO + +. /etc/init.d/functions + +run_file="var/run/vdsm/lvm/deactivate-vgs" +prog="vdsm-deactivate-vgs" +retval=0 + +log_failure_msg() +{ + printf "$@"; failure "$@"; echo; +} + +log_success_msg() +{ + printf "$@"; success "$@"; echo; +} + +is_first_run() +{ + test ! -f $run_file +} + +set_was_run() +{ + touch $run_file +} + +deactivate_vdsm_vgs() +{ + 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 + + local vdsm_vgs=$(echo $vgs_info | /bin/awk '/MDT_TYPE=FCP/ {print $1}') + if [ $? -ne 0 ]; then + log_failure_msg "$prog: error filtering vgs" + return 1 + fi + + if ! lvm vgchange -a n $vdsm_vgs; then + log_failure_msg "$prog: error deactivating vdsm vgs" + return 1 + fi + + log_success_msg "$prog: deactivated vdsm vgs" + return 0 +} + +case "$1" in + start) + if is_first_run; then + set_was_run + deactivate_vdsm_vgs + retval=$? + fi + ;; + stop) + ;; + *) + echo "Usage: $0 {start|stop}" + retval=2 +esac + +exit $retval
oVirt Jenkins CI Server has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1: Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5219/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4340/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5144/ : SUCCESS
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
I will handle packaging in the next commit.
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
As usual, jenkins build is broken :-)
Alon Bar-Lev has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
I am unsure why this is needed.
If vdsm does not support this state, why not just fix vdsm?
Why not add this command to pre-start of vdsm?
I am sure there is some method to configure system to not start if that what is actually require without this script.
This script breaks systemd, upstrart support, and should not be added.
Alon Bar-Lev has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
Also... no added to build system nor packaging...
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
If vdsm does not support this state, why not just fix vdsm?
There is nothing to fix in vdsm - it is the system configuration that is broken here. vdsm must have complete control over its volumes - they should not be activated behind vdsm back.
There may be another option to solve this entirely within vdsm. When connecting to FC storage domain, vdsm could deactivate all vgs. However, vdsm does not get an event when connecting to FC storage domain, since it was considered always connected. This options requires adding a new singal to engine, and handling all the possible cases (new vdsm, old engine, old vds, new engine, etc.) Since we need the fix for current customers using released versions (see related bug), I don't think it is the best approach.
Why not add this command to pre-start of vdsm?
I considered it, but this solution seems much simpler. Why do you think it is better?
I am sure there is some method to configure system to not start if that what is actually require without this script.
Not sure what do you mean.
This script breaks systemd, upstrart support, and should not be added.
Proper scripts and configuration files for systemd and upstart will be added later.
Please review what we have here now.
Alon Bar-Lev has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
1. if connection of new volume group can happen post boot, how come adding anything to init script solves the issue?
2. consider using udev rule, maybe it will solve the issue post boot as well.
3. consider that on startup vdsm will perform whatever action you do here via svdsm, doing so in startup is similar to doing this on boot.
4. if init.d operation is required use pre-start task of the vdsmd service.
Thanks!
Ayal Baron has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
"1. if connection of new volume group can happen post boot, how come adding anything to init script solves the issue?"
As explained in the commit message, auto activation is done by: /etc/rc.sysinit and/or /etc/init.d/netfs so post boot the added VGs will not be auto activated.
"2. consider using udev rule, maybe it will solve the issue post boot as well." This would be a big no no as it would be fighting with vdsm when we actually do need to activate
"3. consider that on startup vdsm will perform whatever action you do here via svdsm, doing so in startup is similar to doing this on boot."
vdsm can restart while there are running VMs, doing this on startup would cut the storage from under their feet. This should be done only once, only on startup, right after the system erroneously activates these VGs (or preferably, just prevent them from being activated in the first place, but we couldn't find a reasonable way of doing that)
4. if init.d operation is required use pre-start task of the vdsmd service.
"Again, iiuc, this would mean every time vdsm service starts which would lead to VMs stopping"
Alon Bar-Lev has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
Not that I understand all the answers.... but I leave the storage stuff to you.
- if init.d operation is required use pre-start task of the vdsmd service.
"Again, iiuc, this would mean every time vdsm service starts which would lead to VMs stopping"
there is no difference in dual run prevention (/var/run/xxx) within a service or within the pre-start-task.
we worked very hard to re-org services so these are portable and generic, please use the offering.
Dan Kenigsberg has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1: Code-Review-1
(3 comments)
Ayal, the current implementation would not cut the storage under a running VM, as it makes sure deactivate_vdsm_vgs runs only once since boot.
Moving the code to a pre-task would save you the need to duplicate this service for upstart and systemd.
Nir, please add the new file to the Makefile, spec and deb definitions.
.................................................... Commit Message Line 17: This script must also be used during installation or ugprade. It is safe Line 18: to invoke it multiple times; it will modify volume groups only on the Line 19: first run. Line 20: Line 21: On RHEL 6.5 we can use new activation skipping option instead of this So this is an el6.4.z only patch? Do we need something else for Fedora?
p.s. try to avoid the RHEL trademark, use "el" instead. Line 22: script. I'll address this in a separate patch. Line 23: Line 24: Change-Id: I8f72a68ad09566ba222aa45448c78d1577c40d21 Line 25: Bug-Url: https://bugzilla.redhat.com/1009812
.................................................... File init/sysvinit/vdsm-deactivate-vgs.init Line 22: ### END INIT INFO Line 23: Line 24: . /etc/init.d/functions Line 25: Line 26: run_file="var/run/vdsm/lvm/deactivate-vgs" why no leading slash? Line 27: prog="vdsm-deactivate-vgs" Line 28: retval=0 Line 29: Line 30: log_failure_msg()
Line 38: } Line 39: Line 40: is_first_run() Line 41: { Line 42: test ! -f $run_file unquoted variables make me weep. Line 43: } Line 44: Line 45: set_was_run() Line 46: {
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
(3 comments)
Ayal, the current implementation would not cut the storage under a running VM, as it makes sure deactivate_vdsm_vgs runs only once since boot.
Moving the code to a pre-task would save you the need to duplicate this service for upstart and systemd.
Using pre-start will work for install or when upgrading from fixed version to newer one, but is not correct when upgrading unfixed version, because there may be running vms using activated lvs. For this kind of upgrade we may either require a reboot, or detect which lvs are used and deactivate the rest.
Nir, please add the new file to the Makefile, spec and deb definitions.
I'm working an alternative fix. I'll continue in this direction if the alternative fix is rejected.
.................................................... Commit Message Line 17: This script must also be used during installation or ugprade. It is safe Line 18: to invoke it multiple times; it will modify volume groups only on the Line 19: first run. Line 20: Line 21: On RHEL 6.5 we can use new activation skipping option instead of this This will work for Fedora, with the proper systemd configuration file.
Will fix RHEL trademark issue. Line 22: script. I'll address this in a separate patch. Line 23: Line 24: Change-Id: I8f72a68ad09566ba222aa45448c78d1577c40d21 Line 25: Bug-Url: https://bugzilla.redhat.com/1009812
.................................................... File init/sysvinit/vdsm-deactivate-vgs.init Line 22: ### END INIT INFO Line 23: Line 24: . /etc/init.d/functions Line 25: Line 26: run_file="var/run/vdsm/lvm/deactivate-vgs" Will fix. Line 27: prog="vdsm-deactivate-vgs" Line 28: retval=0 Line 29: Line 30: log_failure_msg()
Line 38: } Line 39: Line 40: is_first_run() Line 41: { Line 42: test ! -f $run_file Will fix. Line 43: } Line 44: Line 45: set_was_run() Line 46: {
Alon Bar-Lev has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
For this kind of upgrade we may either require a reboot, or detect which lvs are used and deactivate the rest.
and because of this the entire solution of doing that in the packaging seems to be wrong. you require application logic.
I do not understand why vdsm cannot start, determine if this fixup is required via looking at system, communicating with libvirt, and if the fixup is required call svdsm to perform this on its behalf.
Dan Kenigsberg has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
Nir, right, i did not think about upgrade logic. Though you can touch var/run in the spec file to avoid this problem.
Alon, I much prefer to keep this (temporary) hack out of the main vdsm logic.
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
For this kind of upgrade we may either require a reboot, or detect which lvs are used and deactivate the rest.
and because of this the entire solution of doing that in the packaging seems to be wrong. you require application logic.
I do not understand why vdsm cannot start, determine if this fixup is required via looking at system, communicating with libvirt, and if the fixup is required call svdsm to perform this on its behalf.
We can do that, but it is much more complicated, and really needed only when upgrading from unfixed version when using FCP.
Alon Bar-Lev has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
you can touch var/run in the spec file to avoid this problem.
of course that everything that is not long living can be done at pre-start...
Alon, I much prefer to keep this (temporary) hack out of the main vdsm logic.
why is it temporary?
anyway, from what I see /now/ within the pre-tasks... a cleaner design would have used this logic within vdsm->vdsmd at startup... I think that it is better to create that logic and start a move... which much more control over the process.
Yaniv Bronhaim has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1: Code-Review-1
(2 comments)
using a file under /var/run to sign first boot is enough. also bare in mind that user can have vdsmd installed without running it at all, so why forcing him this in each restart? it should run only on first boot when vdsmd starts up. Upgrading or restarting won't harm when using that logic only on first boot. this logic must be under pre-start tasks, using another external script just make vdsm more complex to understand imo, and harder to maintain for other distributions
.................................................... File init/sysvinit/vdsm-deactivate-vgs.init Line 22: ### END INIT INFO Line 23: Line 24: . /etc/init.d/functions Line 25: Line 26: run_file="var/run/vdsm/lvm/deactivate-vgs" why not using constants? Line 27: prog="vdsm-deactivate-vgs" Line 28: retval=0 Line 29: Line 30: log_failure_msg()
Line 80: ;; Line 81: stop) Line 82: ;; Line 83: *) Line 84: echo "Usage: $0 {start|stop}" stop does nothing.. so why having it? Line 85: retval=2 Line 86: esac Line 87:
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
(2 comments)
also bare in mind that user can have vdsmd installed without running it at all, so why forcing him this in each restart?
Force the deactivation?
it should run only on first boot when vdsmd starts up.
vdsm volumes should never be activated, even if vdsm is not running.
Upgrading or restarting won't harm when using that logic only on first boot.
When upgrading from old version that did not run on first boot, we should not deactivate volumes without checking they are used.
this logic must be under pre-start tasks
To handle upgrade correctly, we depend on application logic and state. Or we can require reboot for this one time upgrade when using FCP.
.................................................... File init/sysvinit/vdsm-deactivate-vgs.init Line 22: ### END INIT INFO Line 23: Line 24: . /etc/init.d/functions Line 25: Line 26: run_file="var/run/vdsm/lvm/deactivate-vgs" Do you mean UPPERCASE? Line 27: prog="vdsm-deactivate-vgs" Line 28: retval=0 Line 29: Line 30: log_failure_msg()
Line 80: ;; Line 81: stop) Line 82: ;; Line 83: *) Line 84: echo "Usage: $0 {start|stop}" Script is being called with stop argument on shutdown, and doing nothing is currently the best implementation. Line 85: retval=2 Line 86: esac Line 87:
Yaniv Bronhaim has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
first: no "require reboot" ever
second: files under /var/run persist during upgrade, so I don't understand the problem
third: "volumes should never be activated" - that's how it works and you change it, anything you change should not be forced if you don't run, as i see it
Yaniv Bronhaim has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
(1 comment)
.................................................... File init/sysvinit/vdsm-deactivate-vgs.init Line 22: ### END INIT INFO Line 23: Line 24: . /etc/init.d/functions Line 25: Line 26: run_file="var/run/vdsm/lvm/deactivate-vgs" also.. and find your match in configure.ac, should be @VDSMRUNDIR@ Line 27: prog="vdsm-deactivate-vgs" Line 28: retval=0 Line 29: Line 30: log_failure_msg()
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
Current plan is to have this patch only for zstream, and use lvm activation skipping for el 6.5 (depends on lvm-2.02.100).
To support upgrade on FCP storage, we must detect volumes used by qemu and deactivate only those.
If we have enough infrastructure in zstream to write this as a pre-start script, I'll do this. This will handle both boot and upgrade use cases.
Alon Bar-Lev has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
Current plan is to have this patch only for zstream
so why do you submit it into master? strange...
Ayal Baron has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
Alon is correct, this patch should not be here.
Ayal Baron has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1: Code-Review-1
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
Until http://gerrit.ovirt.org/#/c/20832/ is merged, this is relevant also to upstream.
20382 depends on availability of lvm2-2.02.100 for Fedora and EL, probably through ovirt repositories.
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm volume groups during boot ......................................................................
Patch Set 1:
It seems that we cannot distribute lvm2, which blocks 20832 for few month.
Current plan:
- Safer deactivation, not touching lvs used by vdsm/qemu. That way we can use same deactivation for upgrading old versions. - Move implementation to vdsm-tool lvm-deactivate-vgs - For upstream: trigger deactivation from pre-start - For downstream: trigger deactivation from vdsm.init
Open issues:
- Handling used lvs when upgrading a machine
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
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.
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes ......................................................................
Patch Set 3:
Spelling and whitespace.
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
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes ......................................................................
Patch Set 4:
Remove unused imports, typos.
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
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
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.
Yaniv Bronhaim has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes ......................................................................
Patch Set 4:
(21 comments)
.................................................... Commit Message Line 13: corruption sooner or later, when a vm is trying to write to logical Line 14: volume with stale metadata. Line 15: Line 16: This patch adds new vdsm-tool lvm-deactivate-lvs command, deactivating Line 17: vdsm active logical volumes. The command is invoked during boot as part the invocation is not part of that patch.. why not? Line 18: of vdsmd pre-start. If vdsmd was disabled during boot, or an older vdsm Line 19: version was started during boot, the command is invoked in the first Line 20: time vdsmd is started. Line 21:
Line 23: running vms, the lvm-deactivate-lvs command does not deactivate open Line 24: devices. Line 25: Line 26: We chose this solution because it is compatible with all upstream and Line 27: downstream supported versions, easy to implement, and does not require s/ "all upstream and downstreadm supported versions"/ "all vdsm supported versions" Line 28: changes in vdsm delicate runtime. Line 29: Line 30: We considered several other solutions: Line 31: - Using volume_list lvm configuration: By adding system vgs to this
Line 38: - Modifying system init scripts: We rejected this because it is not Line 39: their responsibility to activate only certain vgs, and touching them is Line 40: last resort. Line 41: Line 42: This patch should be easy to backport to RHEV 3.2, invoking the new should be ovirt 3.2 ? or shouldn't mentioned at all Line 43: vdsm-tool command from the legacy vdsmd.init script instead of pre-start Line 44: available only in ovirt 3.3. Line 45: Line 46: Change-Id: I8f72a68ad09566ba222aa45448c78d1577c40d21
.................................................... File lib/vdsm/tool/lvm_deactivate_lvs.py Line 18: # Line 19: Line 20: """ Line 21: When using FC storage, physical volumes are connected during boot, and vdsm Line 22: logical volumes are auto-activated during boot. These volumes do not pick commit message is enough Line 23: changes done by the SPM on the storage, which may lead to data corruption Line 24: sooner or later, when a vm is trying to write to logical volume with stale Line 25: metadata. Line 26:
Line 33: import sys Line 34: Line 35: from vdsm import tool Line 36: from vdsm import utils Line 37: from vdsm import constants use: from .. import tool, utils, constants Line 38: Line 39: NAME = 'lvm-deactivate-lvs' Line 40: RUN_FILE = os.path.join(constants.P_VDSM_RUN, NAME) Line 41:
Line 36: from vdsm import utils Line 37: from vdsm import constants Line 38: Line 39: NAME = 'lvm-deactivate-lvs' Line 40: RUN_FILE = os.path.join(constants.P_VDSM_RUN, NAME) no need for NAME:
RUN_FILE = os.path.join(constants.P_VDSM_RUN, 'lvm-deactivate-lvs') Line 41: Line 42: LVM_SEPARATOR = "|" Line 43: Line 44: # Tags used to detect vdsm FC vgs
Line 38: Line 39: NAME = 'lvm-deactivate-lvs' Line 40: RUN_FILE = os.path.join(constants.P_VDSM_RUN, NAME) Line 41: Line 42: LVM_SEPARATOR = "|" private? Line 43: Line 44: # Tags used to detect vdsm FC vgs Line 45: VG_SD_TAG = "RHAT_storage_domain" Line 46: VG_FCP_TAG = "MDT_TYPE=FCP"
Line 48: Line 49: class Error(Exception): Line 50: """ Line 51: Raised for expected errors in this module. Line 52: """ pass ?
can you give it more meaningful name than Error? why not using RuntimeError? Line 53: Line 54: Line 55: @tool.expose(NAME) Line 56: def main(*args, **kwargs):
Line 51: Raised for expected errors in this module. Line 52: """ Line 53: Line 54: Line 55: @tool.expose(NAME) please use a string explicitly as in all pys. should me - "deactivate-lvs" Line 56: def main(*args, **kwargs): Line 57: """ Line 58: Deactivates unused vdsm volume lvs. Line 59: """
Line 52: """ Line 53: Line 54: Line 55: @tool.expose(NAME) Line 56: def main(*args, **kwargs): why main? call it "def deactivate_lvs" Line 57: """ Line 58: Deactivates unused vdsm volume lvs. Line 59: """ Line 60: try:
Line 56: def main(*args, **kwargs): Line 57: """ Line 58: Deactivates unused vdsm volume lvs. Line 59: """ Line 60: try: cover try only where needed. Line 61: if was_run(): Line 62: log("already run") Line 63: return 0 Line 64: set_was_run()
Line 71: log("no vgs found") Line 72: return 0 Line 73: except Error as e: Line 74: log(e) Line 75: return 1 should raise an error on failures Line 76: Line 77: Line 78: def log(msg): Line 79: sys.stderr.write("%s: %s\n" % (NAME, msg))
Line 74: log(e) Line 75: return 1 Line 76: Line 77: Line 78: def log(msg): we don't do it anywhere else.. why do you add it here? just call to sys.stderr whenever you need please. Line 79: sys.stderr.write("%s: %s\n" % (NAME, msg)) Line 80: Line 81: Line 82: def was_run():
Line 79: sys.stderr.write("%s: %s\n" % (NAME, msg)) Line 80: Line 81: Line 82: def was_run(): Line 83: return os.path.exists(RUN_FILE) you can explain the use of RUN_FILE and avoid those one line functions. if it is explained, remove those functions and use os.path.exists and os.access directly Line 84: Line 85: Line 86: def set_was_run(): Line 87: with open(RUN_FILE, 'w'):
Line 83: return os.path.exists(RUN_FILE) Line 84: Line 85: Line 86: def set_was_run(): Line 87: with open(RUN_FILE, 'w'): you can use os.access Line 88: pass Line 89: Line 90: Line 91: def find_vdsm_vgs():
Line 87: with open(RUN_FILE, 'w'): Line 88: pass Line 89: Line 90: Line 91: def find_vdsm_vgs(): all helper functions should be private (use _ prefix) Line 92: rc, out, err = lvm("vgs", "--noheading", "--separator", LVM_SEPARATOR, Line 93: "-o", "vg_name,tags") Line 94: if rc != 0: Line 95: raise Error("Error finding vgs: %s" % err)
Line 94: if rc != 0: Line 95: raise Error("Error finding vgs: %s" % err) Line 96: for line in out.splitlines(): Line 97: vg, tags = line.split(LVM_SEPARATOR, 1) Line 98: if is_vdsm_vg(tags): why not:
if VG_SD_TAG and VG_FCP_TAG in set(tags.aplit(",")): ... Line 99: yield vg Line 100: Line 101: Line 102: def find_lvs(vg):
Line 95: raise Error("Error finding vgs: %s" % err) Line 96: for line in out.splitlines(): Line 97: vg, tags = line.split(LVM_SEPARATOR, 1) Line 98: if is_vdsm_vg(tags): Line 99: yield vg why not returning all list for iteration instead of yield? more understandable imo (get_vdsm_vgs_list than find_vdsm_vg) Line 100: Line 101: Line 102: def find_lvs(vg): Line 103: rc, out, err = lvm("lvs", "--noheading", "--separator", LVM_SEPARATOR,
Line 110: continue Line 111: if is_open_lv(attr): Line 112: log("Warning: skipping %s/%s: device is open" % (vg, lv)) Line 113: continue Line 114: yield lv same as above ^ Line 115: Line 116: Line 117: def is_vdsm_vg(vg_tags): Line 118: tags = set(vg_tags.split(","))
Line 120: Line 121: Line 122: def is_active_lv(lv_attr): Line 123: # -wi-ao--- Line 124: return lv_attr[4] == "a" is_activate_lv = True if lv_attr[4] = "a" else False -> can do the work without additional function
same for is_open_lv Line 125: Line 126: Line 127: def is_open_lv(lv_attr): Line 128: # -wi-ao---
Line 136: if rc != 0: Line 137: raise Error("Error deactivating %s: %s" % (lv_path, err)) Line 138: Line 139: Line 140: def lvm(*args): call it run_cmd as we did in fuser Line 141: cmd = [constants.EXT_LVM] Line 142: cmd.extend(args)
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
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:
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
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"?
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
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
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.
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.
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.
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.
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:
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes ......................................................................
Patch Set 4:
(20 comments)
.................................................... Commit Message Line 13: corruption sooner or later, when a vm is trying to write to logical Line 14: volume with stale metadata. Line 15: Line 16: This patch adds new vdsm-tool lvm-deactivate-lvs command, deactivating Line 17: vdsm active logical volumes. The command is invoked during boot as part Not finished yet. Line 18: of vdsmd pre-start. If vdsmd was disabled during boot, or an older vdsm Line 19: version was started during boot, the command is invoked in the first Line 20: time vdsmd is started. Line 21:
Line 23: running vms, the lvm-deactivate-lvs command does not deactivate open Line 24: devices. Line 25: Line 26: We chose this solution because it is compatible with all upstream and Line 27: downstream supported versions, easy to implement, and does not require Done Line 28: changes in vdsm delicate runtime. Line 29: Line 30: We considered several other solutions: Line 31: - Using volume_list lvm configuration: By adding system vgs to this
Line 38: - Modifying system init scripts: We rejected this because it is not Line 39: their responsibility to activate only certain vgs, and touching them is Line 40: last resort. Line 41: Line 42: This patch should be easy to backport to RHEV 3.2, invoking the new I''l replace with ovirt-3.2. Line 43: vdsm-tool command from the legacy vdsmd.init script instead of pre-start Line 44: available only in ovirt 3.3. Line 45: Line 46: Change-Id: I8f72a68ad09566ba222aa45448c78d1577c40d21
.................................................... File lib/vdsm/tool/lvm_deactivate_lvs.py Line 18: # Line 19: Line 20: """ Line 21: When using FC storage, physical volumes are connected during boot, and vdsm Line 22: logical volumes are auto-activated during boot. These volumes do not pick Commit message does not replace module documentation. To get the commit message you must search through history, reading multiple commit messages. The reader needs this info here. Line 23: changes done by the SPM on the storage, which may lead to data corruption Line 24: sooner or later, when a vm is trying to write to logical volume with stale Line 25: metadata. Line 26:
Line 33: import sys Line 34: Line 35: from vdsm import tool Line 36: from vdsm import utils Line 37: from vdsm import constants I'll use absolute import. Line 38: Line 39: NAME = 'lvm-deactivate-lvs' Line 40: RUN_FILE = os.path.join(constants.P_VDSM_RUN, NAME) Line 41:
Line 38: Line 39: NAME = 'lvm-deactivate-lvs' Line 40: RUN_FILE = os.path.join(constants.P_VDSM_RUN, NAME) Line 41: Line 42: LVM_SEPARATOR = "|" Module constant, see vdsm.storage.lvm. Line 43: Line 44: # Tags used to detect vdsm FC vgs Line 45: VG_SD_TAG = "RHAT_storage_domain" Line 46: VG_FCP_TAG = "MDT_TYPE=FCP"
Line 48: Line 49: class Error(Exception): Line 50: """ Line 51: Raised for expected errors in this module. Line 52: """ pass is redundant when you have a docstring.
This is *lvm_deactivate_lvs.Error* - meaning error in this module which should be treated differently then other errors. Used here to log the error and return a non-zero return code on expected failures.
RuntimeError is a library error and should not be used in your code unless you want to make it harder for the caller to tell a real RuntimeError and your module sepcific errors.
See http://docs.python.org/2.6/library/exceptions.html#exceptions.RuntimeError Line 53: Line 54: Line 55: @tool.expose(NAME) Line 56: def main(*args, **kwargs):
Line 51: Raised for expected errors in this module. Line 52: """ Line 53: Line 54: Line 55: @tool.expose(NAME) Why not a constant? And why not lvm-deactivate-lvs, like libvirt-configure? Line 56: def main(*args, **kwargs): Line 57: """ Line 58: Deactivates unused vdsm volume lvs. Line 59: """
Line 52: """ Line 53: Line 54: Line 55: @tool.expose(NAME) Line 56: def main(*args, **kwargs): I took nwfilter as example - isn't the expose is enough to give info the the tool? Line 57: """ Line 58: Deactivates unused vdsm volume lvs. Line 59: """ Line 60: try:
Line 56: def main(*args, **kwargs): Line 57: """ Line 58: Deactivates unused vdsm volume lvs. Line 59: """ Line 60: try: Entire function is covered so I can throw Error everywhere in this module when an expected failure is detected. This does not effect other exceptions anyway.
For example, was_run does not throw an Error today, but if we start throwing it in the future, we don't have to change the main logic. Line 61: if was_run(): Line 62: log("already run") Line 63: return 0 Line 64: set_was_run()
Line 71: log("no vgs found") Line 72: return 0 Line 73: except Error as e: Line 74: log(e) Line 75: return 1 I took sanlock.py as example and it does return either 0 or 1 - are you sure about it? Line 76: Line 77: Line 78: def log(msg): Line 79: sys.stderr.write("%s: %s\n" % (NAME, msg))
Line 74: log(e) Line 75: return 1 Line 76: Line 77: Line 78: def log(msg): Do you suggest to duplicate this logging code everywhere?
Most of the log calls here are for debugging - we probably need a way to disable them in production. Maybe use logging module? Line 79: sys.stderr.write("%s: %s\n" % (NAME, msg)) Line 80: Line 81: Line 82: def was_run():
Line 79: sys.stderr.write("%s: %s\n" % (NAME, msg)) Line 80: Line 81: Line 82: def was_run(): Line 83: return os.path.exists(RUN_FILE) I prefer self explaining code to comments. Aren't these functions clear? Line 84: Line 85: Line 86: def set_was_run(): Line 87: with open(RUN_FILE, 'w'):
Line 83: return os.path.exists(RUN_FILE) Line 84: Line 85: Line 86: def set_was_run(): Line 87: with open(RUN_FILE, 'w'): This creates RUN_FILE - how os.acess is related? Line 88: pass Line 89: Line 90: Line 91: def find_vdsm_vgs():
Line 87: with open(RUN_FILE, 'w'): Line 88: pass Line 89: Line 90: Line 91: def find_vdsm_vgs(): Everything here is private except main - do you want to prefix everthing with _? Line 92: rc, out, err = lvm("vgs", "--noheading", "--separator", LVM_SEPARATOR, Line 93: "-o", "vg_name,tags") Line 94: if rc != 0: Line 95: raise Error("Error finding vgs: %s" % err)
Line 94: if rc != 0: Line 95: raise Error("Error finding vgs: %s" % err) Line 96: for line in out.splitlines(): Line 97: vg, tags = line.split(LVM_SEPARATOR, 1) Line 98: if is_vdsm_vg(tags): Same reason is_active_lv and is_open_lv are used. Line 99: yield vg Line 100: Line 101: Line 102: def find_lvs(vg):
Line 95: raise Error("Error finding vgs: %s" % err) Line 96: for line in out.splitlines(): Line 97: vg, tags = line.split(LVM_SEPARATOR, 1) Line 98: if is_vdsm_vg(tags): Line 99: yield vg Yielding it cleaner here and allow both iterative use for the caller, and if needed, the caller can collect the value using list(find_vgs) as needed. More options, less code. Line 100: Line 101: Line 102: def find_lvs(vg): Line 103: rc, out, err = lvm("lvs", "--noheading", "--separator", LVM_SEPARATOR,
Line 110: continue Line 111: if is_open_lv(attr): Line 112: log("Warning: skipping %s/%s: device is open" % (vg, lv)) Line 113: continue Line 114: yield lv Same reason Line 115: Line 116: Line 117: def is_vdsm_vg(vg_tags): Line 118: tags = set(vg_tags.split(","))
Line 120: Line 121: Line 122: def is_active_lv(lv_attr): Line 123: # -wi-ao--- Line 124: return lv_attr[4] == "a" What do you think is more readable:
if not is_active_lv(attr): continue
if attr[4] == "-": # is not active (-wi------) continue
The extra function call cost nothing, the readability improved.
I can replace this with is_inactive, as this is what we look for:
if is_inactive(attr): continue Line 125: Line 126: Line 127: def is_open_lv(lv_attr): Line 128: # -wi-ao---
Line 136: if rc != 0: Line 137: raise Error("Error deactivating %s: %s" % (lv_path, err)) Line 138: Line 139: Line 140: def lvm(*args): I'll call it run_lvm as this is lvm specific. Line 141: cmd = [constants.EXT_LVM] Line 142: cmd.extend(args)
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
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.
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.
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?
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.
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.
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.
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.
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?
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?
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
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
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.
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
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.
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
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.
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
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes ......................................................................
Patch Set 12:
Test should run now on jenkins.
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
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
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
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
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?
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
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.
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
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".
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes ......................................................................
Patch Set 14:
pep8 indent fixes.
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.
Nir Soffer has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes ......................................................................
Patch Set 15:
Hopefully complete now.
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
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?
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.
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
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 ####
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 ####
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 ####
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
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 ####
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 ####
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.
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
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.
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 ####
Allon Mureinik has posted comments on this change.
Change subject: fcp: Deactivate vdsm logical volumes ......................................................................
Patch Set 17: Code-Review+1
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()
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)
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)
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()
Nir Soffer has abandoned this change.
Change subject: fcp: Deactivate vdsm logical volumes ......................................................................
Abandoned
Replaced by http://gerrit.ovirt.org/#/c/21291/
vdsm-patches@lists.fedorahosted.org