Wenyi Gao has uploaded a new change for review.
Change subject: [WIP] Move vdsmd.init to vdsm-tool ......................................................................
[WIP] Move vdsmd.init to vdsm-tool
The vdsmd.init script is a bit complex, so recode some functions with python and move it to vdsm-tool.
Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Signed-off-by: Wenyi Gao wenyi@linux.vnet.ibm.com --- M vdsm-tool/Makefile.am A vdsm-tool/vdsmd.py M vdsm.spec.in M vdsm/vdsmd.init.in 4 files changed, 700 insertions(+), 558 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/38/4738/1 -- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com
Adam Litke has posted comments on this change.
Change subject: [WIP] Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(17 inline comments)
Need to convert more shell commands to python equivalents.
.................................................... File vdsm-tool/vdsmd.py Line 49: EX_AWK = constants.EXT_AWK In python, do not call out to awk, sed, grep, chmod, chown, tail, etc. These functions all have easy python equivalents that are more efficient and which integrate better into the program. I will identify the ones I see in this file and suggest some alternatives...
Line 138: _exec_command([EX_SED, '-i', '--copy', '/%s/d' % BY_VDSM, file]) In this one you would want something like:
newfile = [] with open(file) as f: for line in f.readlines(): if line.endswith(BY_VDSM): continue newfile.append(line) with open(file, "w") as f: f.writelines(newfile)
Line 142: rc = _exec_command([EX_GREP, '-q', '^\s*%s\s*=' % key, file])[2] Here you would read the file with:
with open(file) as f: str = f.read()
Then search for your config entry:
if re.search('^\s*%s\s*=' % key, str) is None: ... do the f.write below
Line 149: if _exec_command([EX_GREP, '-q', '%s' % BY_VDSM, file])[2] == 0: Use the read and re example above here too.
Line 156: out = _exec_command([GET_CONF_ITEM, file, section, item, default])[0] The above is a simple python script. Now that your program is python, you can just copy the code from that script into this function.
Line 187: p1 = subprocess.Popen(['/bin/netstat', '-ntl'], stdout=subprocess.PIPE) Calling netstat is fine, but you should capture its output into a string and use re to parse that string instead of calling grep.
Line 202: _exec_command([EX_CHOWN, 'vdsm:kvm', path]) See python's os.chown()
http://docs.python.org/library/os.html
Line 232: All of these mk_*_path functions are pretty similar. Please collapse them into a single function:
def _mk_vdsm_path(path, mode, user, group, restorecon=False): ...
See os.chmod, os.chown http://docs.python.org/library/grp.html http://docs.python.org/library/pwd.html
Is is ok to call restorecon
Line 241: stdin=p2.stdout, stdout=subprocess.PIPE) read the file. use re.search to get a match object get the last match by using index -1 into the match object You can use a grouped regular expression to easily extract the configuration value.
See http://docs.python.org/library/re.html#match-objects
Line 306: stdout=subprocess.PIPE, env=environ) You know how to eliminate grep now.
Line 324: stdout=subprocess.PIPE) Awk just splits a string by whitespace. In python do this instead:
# read the output of df into str
# Get the last line
lastline = str.splitlines()[-1]
# Then get the 4th field
lastline.split()[3]
That's it!
Line 402: '"1:libvirt 3:event 3:json 1:util 1:qemu"') We can make this more efficient that the original shell script by setting all of the conf items at the same time. Do do this, you would pass a dictionary of config params to set into _set_vdsm_conf() and it should add all of them in one shot.
Line 412: ssl == 'ture'): ssl == 'true'
Line 425: pass You are silently ignoring lots of potential errors. You probably mean to ignore only the error if the file does not exist.
Line 430: llogr])[0] Read the file into an array like one of my previous examples. Then only keep the lines that are not between '#vdsm and # end vdsm'. Finally, write out the file again.
Line 477: os.rename('/etc/sysctl.conf.vdsm', sysconf) You should be able to replicate this file editing...
Line 513: p2.stdout.close() No grep or tail please.
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Wenyi Gao has posted comments on this change.
Change subject: [WIP] Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 1:
Thank you Adam very much for your helpful suggestion and some useful example code. I will follow your suggestions and submit it again.
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com
ShaoHe Feng has posted comments on this change.
Change subject: [WIP] Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 2: (1 inline comment)
.................................................... File vdsm-tool/vdsmd.py Line 196: continue I wonder where can vdsm-tool be used? can we use the vdsm-tool manually in the terminal? if yes, then we can not modify this key already in the configure file.
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com
Wenyi Gao has posted comments on this change.
Change subject: [WIP] Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 2: (1 inline comment)
.................................................... File vdsm-tool/vdsmd.py Line 196: continue The vdsm-tool is management tool for vdsm, it can be used anywhere you like. Yes, you can use vdsm-tool manually on the term. Currently, vdsmd.init shell script only add key and key value if the key is not in the configuration file. But you can update the key value by 'reconfigure'
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 4: (5 inline comments)
Partial review
.................................................... File vdsm-tool/vdsmd.py Line 306: This fucntion test if the "/sbin/initctl" can be executed. s/fucntion/function
Line 475: ssl == 'true'): I quite like to use all in these cases, and move the safest comparisons to the front.
if (ssl == 'true' and all(map(os.path.exists, (cacertpath, vdsmcertpath, vdsmkeypath)))):
or
if (ssl == 'true' and all(os.path.exists(p) for p in (cacertpath, vdsmcertpath, vdsmkeypath))):
Line 562: Stop libvirt SysV service if we intend to configure upstart Bit inconsistent indenting here.
Line 565: if(_upstart_libvirt()): No need for the enclosing parenthesis.
Line 749: _reconfigure(False) I was always taught to write this as:
_reconfigure(args == 'force')
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com
Adam Litke has posted comments on this change.
Change subject: Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 4: I would prefer that you didn't submit this
(17 inline comments)
Thanks for taking my advice and reducing the number of external commands you are executing. It is looking much better but there is still some work left to do.
.................................................... File vdsm/constants.py.in Line 95 Did you mean to remove this? Seems fishy to me.
.................................................... File vdsm-tool/vdsmd.py Line 71: VDSM_VER = '4.9.6' This needs to come from dsaversion.py. Otherwise we have to remember to update this file each time the vdsm version changes.
Line 113: return (out, err, rc) Hmm, this looks wrong to me. If there is an exception in line 107 you will still return rc = 0 and no output. If the program fails to run I think you want the exception to propagate up to the caller of this.
Line 128: print >> sys.stderr, "Execution failed:", e Why are you using print here and logging above? If there is a good reason, please add a comment here to explain why.
Line 130: return rc rc will be undefined if an exception was raised. This will cause a new exception. You need to rework your exception handling logic. You can move the return statement inside the try block. If you do this, then you need to figure out how you want to handle the exception. Perhaps you log the error and return -1?
Line 157: skip = False Line 157 is unreachable. Please remove it.
Line 182: lines.append(line) This looks a lot like the function above. Please try to combine them into a single function. You can pass the startPattern and endPattern as parameters.
Line 219: if os.path.exists('/etc/rhev-hypervisor-release'): Does vdsm define a constant for this file name yet?
Line 221: elif not len(glob.glob('/etc/ovirt-node-*-release')) == 0: How about this one?
Line 291: services = ['libvirt-guests'] I think this should be a constant at the top of the file.
Line 306: This fucntion test if the "/sbin/initctl" can be executed. also s/test/tests/
Line 313: services = ['iscsid', 'multipathd', 'ntpd', 'wdmd', 'sanlock'] This should be another constant.
Line 343: p = subprocess.Popen([EX_DF, '-P', path], stdout=subprocess.PIPE) It would be helpful if you add a docstring to show some example output from commands so that we can more easily understand the parsing you are doing.
Line 358: def _is_bond_dev_available(bonddev): Example command output please.
Line 475: ssl == 'true'): Hmm, I like it. You learn something new every day! :)
Line 547: f.write('%s\n' % VDSM_SYSCTL_VER) Please put these variables into a dict so that we can easily add/remove/update the settings.
.................................................... File vdsm/vdsmd.init.in Line 1 Nice!!
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 4: (6 inline comments)
.................................................... File vdsm/constants.py.in Line 142: EXT_TC = '@TC_PATH@' @Adam: He moved it here.
.................................................... File vdsm-tool/vdsmd.py Line 211: except: Maybe we could catch a specific exception here. KeyError or something?
Line 215: def _is_ovirt(): This looks like a copy of isOvirt in vdsm_reg/deployUtil.py. Maybe we could place this in some common place?
Line 232: return False I'd prefer:
return _get_config_item(...) == 'true'
Line 366: return False Again, great fan of a single return:
return bonddev in bond
Line 427: return 6 I think this could be a constant as well.
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com
Wenyi Gao has posted comments on this change.
Change subject: Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 4: (1 inline comment)
Thank you Adam and Ewoud for your very helpful suggestions.
.................................................... File vdsm/constants.py.in Line 142: EXT_TC = '@TC_PATH@' Yes, just sort the order.
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com
Wenyi Gao has posted comments on this change.
Change subject: Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 4: (17 inline comments)
Response for partial comments.
.................................................... File vdsm-tool/vdsmd.py Line 1: #!/usr/bin/python Yes, it should be removed.
Line 71: VDSM_VER = '4.9.6' Adam, here the version number only indicates if the configuration is changed. I am not sure if the version needs to keep consistent with the vdsm version. Also, the vdsmd.py will be installed in the directory "/usr/lib/python2.7/site-packets/vdsm/tool". It seems that it can't import dsaversion.py that is in the directory "/usr/share/vdsm"
Line 113: return (out, err, rc) Yes, you are right. There is something wrong with the code here.
Line 128: print >> sys.stderr, "Execution failed:", e Hmm, I will modify it to logger error.
Line 130: return rc Yes, rc is something wrong here. Your suggestion make much sense.
Line 157: skip = False Sorry for the obvious error.
Line 182: lines.append(line) Yes, I will combine them into a single one.
Line 211: except: Good point.
Line 215: def _is_ovirt(): Yes, I copy it form vdsm_reg/deployUtil.py. I will consider to move it to a common place.
Line 219: if os.path.exists('/etc/rhev-hypervisor-release'): Not defined in constants.py yet.
Line 221: elif not len(glob.glob('/etc/ovirt-node-*-release')) == 0: Also not defined.
Line 232: return False Yes. Looks simple.
Line 291: services = ['libvirt-guests'] Yes, good suggestion.
Line 306: This fucntion test if the "/sbin/initctl" can be executed. Yes, thanks.
Line 306: This fucntion test if the "/sbin/initctl" can be executed. Yes, thanks too.
Line 313: services = ['iscsid', 'multipathd', 'ntpd', 'wdmd', 'sanlock'] Yes, constant is much better.
Line 343: p = subprocess.Popen([EX_DF, '-P', path], stdout=subprocess.PIPE) Yes, I will add a docstring.
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com
Dan Kenigsberg has posted comments on this change.
Change subject: Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 4: I would prefer that you didn't submit this
(2 inline comments)
.................................................... File vdsm-tool/vdsmd.py Line 133: def _print_success_msg(msg): I do not see the great importance of keeping the [SUCCESS] output of `success`. If you think you need to report success, use `print`. Note that currently systemd swallows that and puts it in /var/log/messages.
Line 596: @vdsm_tool_expose('vdsm-start') Please expose smaller, atomic, functions.
In particular, I know that many people find 'reconfigure' as useful.
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com
Xu He Jie has posted comments on this change.
Change subject: Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 4: I would prefer that you didn't submit this
(1 inline comment)
minor comment
.................................................... File vdsm-tool/vdsmd.py Line 1: #!/usr/bin/python this file won't execute directly
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com
Dan Kenigsberg has posted comments on this change.
Change subject: Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 4:
In case I wasn't clear - I appreciate your undertaking, but I would very much prefer that you start up with small self-contained patches, such as moving test_conflicting_conf and configure_libvirt to vdsm-tool.
Each such patch should contain a unit test (and if impossible, a functional test).
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com
Wenyi Gao has posted comments on this change.
Change subject: Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 4: (9 inline comments)
Dan, thanks for your suggestion. I will divide it into some small patches.
.................................................... File vdsm-tool/vdsmd.py Line 133: def _print_success_msg(msg): Dan, the function don't print the output of 'success'. It just behaves like other initial boot script and pints '[ OK ]' as following:
configure ....... [ OK ]
Line 366: return False Yes, good suggestion.
Line 427: return 6 Yes, a constant is better.
Line 475: ssl == 'true'): Yes, good.
Line 547: f.write('%s\n' % VDSM_SYSCTL_VER) Yes, it makes sense.
Line 562: Stop libvirt SysV service if we intend to configure upstart Yes.
Line 565: if(_upstart_libvirt()): Yes.
Line 596: @vdsm_tool_expose('vdsm-start') Yes, good point, I will make some smaller functions exposed.
Line 749: _reconfigure(False) Yes, looks very simple.
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com
Douglas Schilling Landgraf has posted comments on this change.
Change subject: Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 4: I would prefer that you didn't submit this
Thanks for your patch Wenyi. I do agree with the previous comments, looking forward for your new split version.
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 4: No score
Build Started http://jenkins.ovirt.info/job/vdsm_unit_tests_by_patch/119/
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 4:
Build Successful
http://jenkins.ovirt.info/job/vdsm_unit_tests_by_patch/119/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Wenyi Gao has abandoned this change.
Change subject: Move vdsmd.init to vdsm-tool ......................................................................
Patch Set 4: Abandoned
split some small patches
-- To view, visit http://gerrit.ovirt.org/4738 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
vdsm-patches@lists.fedorahosted.org