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