Ryan Harper has posted comments on this change.
Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(9 inline comments)
....................................................
File vdsm/constants.py.in
Line 72: #
Line 73: P_SYSTEMCTL_CONF = '/etc/sysctl.conf'
Line 74: P_VDSM_LCONF = '/etc/libvirt/libvirtd.conf'
Line 75: P_VDSM_LDCONF = '/etc/sysconfig/libvirtd'
Line 76: P_VDSM_QCONF = '/etc/libvirt/qemu.conf'
We probably need to use the sysconf path instead of /etc.
Line 77:
Line 78: #
Line 79: # External programs (sorted, please keep in order).
Line 80: #
Line 84: EXT_BLOCKDEV = '@BLOCKDEV_PATH@'
Line 85: EXT_BRCTL = '@BRCTL_PATH@'
Line 86:
Line 87: EXT_CAT = '@CAT_PATH@'
Line 88: EXT_CHKCONFIG = '@CHKCONFIG_PATH@'
You add chkconfig path here, but don't use it in the vdsm-tool program, same
with the service command.
Line 89: EXT_CHOWN = '@CHOWN_PATH@'
Line 90: EXT_CP = '@CP_PATH@'
Line 91:
Line 92: EXT_DD = '@DD_PATH@'
....................................................
File vdsm_tool/unregister.py
Line 35: Stop vdsm service on el6
Line 36: """
Line 37: subprocess.call([constants.EXT_SERVICE, 'vdsmd', 'stop'])
Line 38:
Line 39: subprocess.call([constants.EXT_CHKCONFIG, '--del', 'vdsmd'])
These should be separate methods.
We may want to stop the service(service stop) but not disable the service
(chkconfig).
Line 40:
Line 41:
Line 42: @expose("stop-vdsm-service-fedora")
Line 43: def stop_vdsm_service_fedora():
Line 45: Stop vdsm service on fedora
Line 46: """
Line 47: subprocess.call([constants.EXT_SYSTEMCTL,
Line 48: '--no-reload', 'disable', 'vdsmd.service'])
Line 49: subprocess.call([constants.EXT_SYSTEMCTL, 'stop', 'vdsmd.service'])
Same comment here, seperate stop from disable.
Line 50:
Line 51:
Line 52: def _remove_config_lines(fileName, beginField=None, endField=None):
Line 53: newfile = []
Line 50:
Line 51:
Line 52: def _remove_config_lines(fileName, beginField=None, endField=None):
Line 53: newfile = []
Line 54: tempfile = '/tmp/tempfile'
you need to use something like http://docs.python.org/library/tempfile.html
because multiple calls to _remove_config_lines will clobber a common tempfile.
Line 55:
Line 56: if not beginField and not endField:
Line 57: return
Line 58: else:
Line 75: continue
Line 76: newfile.append(line)
Line 77: with open(tempfile, 'w') as f:
Line 78: f.writelines(newfile)
Line 79: shutil.move(tempfile, fileName)
How about using string.find() Something like:
% cat remove.py
#!/usr/bin/python2.7
import tempfile
beginSection = '## beginning of configuration section by vdsm-4.9.10\n'
endSection = '## end of configuration section by vdsm-4.9.10\n'
f = tempfile.NamedTemporaryFile(delete=False)
print f.name
lines = open('/tmp/libvirtd.conf', 'r').read()
start = lines.find(beginSection)
# add len of substring to get the index after the substring
end = lines.rfind(endSection)+len(endSection)
newlines = lines[:start]+lines[end:]
print newlines
f.write(newlines)
f.close()
(platechiller) tmp % diff -u libvirtd.conf /tmp/tmptFOXPR
--- libvirtd.conf 2012-09-14 11:06:55.610859821 -0500
+++ /tmp/tmptFOXPR 2012-09-14 11:07:38.342837950 -0500
@@ -2,16 +2,4 @@
# support keepalive protocol. Defaults to 0.
#
#keepalive_required = 1
-## beginning of configuration section by vdsm-4.9.10
-listen_addr="0.0.0.0"
-unix_sock_group="kvm"
-unix_sock_rw_perms="0770"
-auth_unix_rw="sasl"
-host_uuid="78aa4a50-dbdd-480d-9473-cdf3eef4b736"
-log_outputs="1:file:/var/log/libvirtd.log"
-log_filters="1:libvirt 3:event 3:json 1:util 1:qemu"
-ca_file="/etc/pki/vdsm/certs/cacert.pem"
-cert_file="/etc/pki/vdsm/certs/vdsmcert.pem"
-key_file="/etc/pki/vdsm/keys/vdsmkey.pem"
-## end of configuration section by vdsm-4.9.10
Make sure you check the result of the find() ops and skip rewriting/moving the
file if the section has already been moved.
Line 80:
Line 81:
Line 82: @expose("remove-vdsm-configs")
Line 83: def remove_vdsm_configs():
Line 108: def clear_selinux_policy():
Line 109: """
Line 110: Clear the changes of selinux policy
Line 111: """
Line 112: selinux.security_set_boolean('virt_use_nfs', 0)
I'd like to either see each specific selinux boolean have it's own method, so
we can toggle this programatically, or a general method for toggling the
boolean along with one that enumerates the booleans vdsm uses.
Line 113: selinux.security_commit_booleans()
Line 114:
Line 115:
Line 116: @expose("restore-libvirt-service")
Line 118: """
Line 119: Restore libvirt service
Line 120: """
Line 121: subprocess.call(['/sbin/initctl', 'stop', 'libvirtd'])
Line 122: os.remove('/etc/init/libvirtd.conf')
This can't be right... we can't just remove libvirtd.conf
Also, if we are removing a file, we need to use try() and catch the error.
Line 123: subprocess.call(['/sbin/chkconfig', 'libvirtd', 'on'])
Line 119: Restore libvirt service
Line 120: """
Line 121: subprocess.call(['/sbin/initctl', 'stop', 'libvirtd'])
Line 122: os.remove('/etc/init/libvirtd.conf')
Line 123: subprocess.call(['/sbin/chkconfig', 'libvirtd', 'on'])
Yeah, maybe update the constants.py.in with the paths of the binaries
discovered by autoconf.
--
To view, visit http://gerrit.ovirt.org/4526
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e7b5dc969dfb51e6880b9bb209a363609f5e123
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Lei Li <[email protected]>
Gerrit-Reviewer: Ryan Harper <[email protected]>
Gerrit-Reviewer: ShaoHe Feng <[email protected]>
Gerrit-Reviewer: Wenyi Gao <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
