Change in vdsm[master]: Introducing configurator package in vdsm-tool
ybronhei at redhat.com
ybronhei at redhat.com
Tue Oct 15 15:56:38 UTC 2013
Yaniv Bronhaim has posted comments on this change.
Change subject: Introducing configurator package in vdsm-tool
......................................................................
Patch Set 9:
(7 comments)
....................................................
File lib/vdsm/tool/configurator.py
Line 95:
Line 96: try:
Line 97: with open(sanlock_pid_file, "r") as f:
Line 98: sanlock_pid = f.readline().strip()
Line 99: with open(proc_status_path % sanlock_pid, "r") as sanlock_status:
" The return value is the concatenation of path1, and optionally path2, etc., .."
this adds the sanlock_pid after /proc/.. , join doesn't help much
Line 100: for status_line in sanlock_status:
Line 101: if status_line.startswith(proc_status_group_prefix):
Line 102: groups = [int(x) for x in
Line 103: status_line[len(proc_status_group_prefix):].
Line 110: if e.errno == os.errno.ENOENT:
Line 111: raise RuntimeError("sanlock service is not running")
Line 112: raise
Line 113:
Line 114: diskimage_gid = grp.getgrnam(DISKIMAGE_GROUP)[2]
why not?
if grp.getgrnam(DISKIMAGE_GROUP)[2] not in groups --> is too bahh in my opinion. don't you think? what is it in [2]? the variable name saves us for commenting here
Line 115: if diskimage_gid not in groups:
Line 116: raise RuntimeError("sanlock service requires restart")
Line 117: else:
Line 118: sys.stdout.write("sanlock service is already configured\n")
Line 123: @vdsm.tool.expose("configure-all")
Line 124: def configure_and_restart_all(*args):
Line 125: """
Line 126: Configure related services for vdsm. --force flag is set for
Line 127: forcing restart of related services after configure
too many manual operation to do ... don't you prefer to make the user's life a bit more easy.. configure will do a bit more then just call configure... otherwise this verb is redundant.
And, sanlock_check_service doesn't do much, there is not really a configuration process here.. it just tells us if restart is required. so actually what you ask can be part of the verb itself, like configure_libvirt will fail if the service is up. --force will stop it before (as I suggest here and forgot to do :) will add it)
Line 128: """
Line 129: libvirt_related_services = ["supervdsmd", "libvirtd"]
Line 130: service_should_start = []
Line 131:
Line 130: service_should_start = []
Line 131:
Line 132: force = False
Line 133: if '--force' in args:
Line 134: force = True
right. for one parameter "force = '--force' in args" is enough
and maybe later we'll refactor this verb anyway.
Line 135:
Line 136: # libvirt configuration
Line 137: if force:
Line 138: for srv in libvirt_related_services:
Line 134: force = True
Line 135:
Line 136: # libvirt configuration
Line 137: if force:
Line 138: for srv in libvirt_related_services:
but its different with sanlock .. the "configure" for sanlock tells us if it needs to be restarted.
Line 139: if service.service_status(srv):
Line 140: service_should_start.append(srv)
Line 141:
Line 142: configure_libvirt(*args)
Line 136: # libvirt configuration
Line 137: if force:
Line 138: for srv in libvirt_related_services:
Line 139: if service.service_status(srv):
Line 140: service_should_start.append(srv)
sorry, missed that
Line 141:
Line 142: configure_libvirt(*args)
Line 143: for srv in reversed(service_should_start):
Line 144: service.service_start(srv)
Line 144: service.service_start(srv)
Line 145:
Line 146: # sanlock configuration
Line 147: if sanlock_check_service(*args) and force:
Line 148: service.service_restart("sanlock")
sanlock configure should be separate part in my opinion .
Line 149:
--
To view, visit http://gerrit.ovirt.org/20100
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16bf5894e7e55a84b4c2a0caacde383ae7c19242
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list