Change in vdsm[master]: Move uninstall preun section to vdsm-unregister

Lei Li has uploaded a new change for review.

Change subject: Move uninstall preun section to vdsm-unregister
......................................................................

Move uninstall preun section to vdsm-unregister

The specfile vdsm.spec.in has very long %post and %preun
sections, move the install/uninstall hooks into vdsm-tool.

This patch move unregister to a script which will be
encapsulated into a vdsm-tool function stop-vdsm-service.

Change-Id: I4e7b5dc969dfb51e6880b9bb209a363609f5e123
Signed-off-by: Lei Li 
---
M vdsm.spec.in
M vdsm/Makefile.am
A vdsm/vdsm-unregister.in
3 files changed, 36 insertions(+), 42 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/26/4526/1
--
To view, visit http://gerrit.ovirt.org/4526
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4e7b5dc969dfb51e6880b9bb209a363609f5e123
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li

Adam Litke has posted comments on this change.

Change subject: Move uninstall preun section to vdsm-unregister
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/vdsm-unregister.in
Line 23: /usr/sbin/saslpasswd2 -p -a libvirt -d vdsm@rhevh
Hmm, we have a vdsm-tool command to set this password.  Should we have one to remove it as well?

--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Ryan Harper

Adam Litke has posted comments on this change.

Change subject: Move uninstall preun section to vdsm-unregister
......................................................................

Patch Set 1: I would prefer that you didn't submit this

Again, you have moved some code to vdsm-tool but you are not using it yet.  Have you tested these patches on a clean system?  I think the install might now succeed.

--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Ryan Harper

Lei Li has posted comments on this change.

Change subject: Move uninstall preun section to vdsm-unregister
......................................................................

Patch Set 1:

Hi Adam,

I think your concern from the same question, and I have replied in the link here:

http://gerrit.ovirt.org/#patch,sidebyside,4528,3,vdsm.spec.in

--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper

Adam Litke has posted comments on this change.

Change subject: Move uninstall preun section to vdsm-unregister
......................................................................

Patch Set 1: No score

Replied in the other change... Removing -1 to invite further review.

--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Move uninstall preun section to vdsm-unregister
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(2 inline comments)

Thanks for attending to the task of spec cleanup.

However, %preun should have content, most probably.
It should be several calls to vdsm-tool functions. EL6 should probably have different content than Fedora, as is the case today.

Plain moving of code out of the spec gives us nothing, certainly if it forces Fedora and RHEL to behave the same.

....................................................
File vdsm.spec.in
Line 372: %preun
nothing at all should be done when the rpm is removed? I doubt that.

....................................................
File vdsm/vdsm-unregister.in
Line 1: !/bin/bash
#!/bin/bash

please test your code.

--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Lei Li has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 1: (3 inline comments)

....................................................
File vdsm.spec.in
Line 372: %preun
Yes, you are right. After the discussion on vdsm-devel mailing list, will call vdsm-tool command here.

....................................................
File vdsm/vdsm-unregister.in
Line 1: !/bin/bash
Sorry for my negligence... Just tested on fedora, I will test it on el6 too.

Line 23: /usr/sbin/saslpasswd2 -p -a libvirt -d vdsm@rhevh
Good idea, I will add it in a separate patch after passwd module accepted.

--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File vdsm/vdsm-unregister.in
Line 1: #!/bin/bash
again: please break this chunk of code into several vdsm-tool verbs.

* one to stop sysv service.
* another to remove "by vdsm" config
* yet another to clear selinux policy changes.

Line 6: /bin/sed -i '/# by vdsm$/d' \
Federico, why don't we clean after ourselves in Fedora? Was this intentional?

--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 4: I would prefer that you didn't submit this

(6 inline comments)

much better, but there's still a long way to go.

....................................................
File vdsm/constants.py.in
Line 71: # Configuration file definitions
constants.py is an unloved dumpster. do not put here stuff that is needed only by one marginal module.

....................................................
File vdsm.spec.in
Line 400: # Stop vdsm service
please keep the indentation.

Line 412: # Restore libvirt service
since you have good function names, there's no need in comments.

comments are good only when they add information.

....................................................
File vdsm-tool/unregister.py
Line 26: BY_VDSM = '# by vdsm'
there used to be version-specific code here. please maintain feature parity.

which reminds me - functional tests are required for this patches.

Line 51: def __remove_from_file(fileName, begin=None, end=None):
the semantics of the function is not very clear.

it is impolite that your default args leads the function to skip lines ending with None.

unittest most certainly needed here.

Line 72:         f.writelines(newfile)
this is quite bad, as it resets the old file before writing the new one. on crash/poweroff, you end up with an inconsistent file.

try to use the atomic move trick (write to another name, then move) but remember to ovirt-node case, where this is more complicated (ask mburns).

--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Lei Li has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 4: (6 inline comments)

....................................................
File vdsm/constants.py.in
Line 71: # Configuration file definitions
These constants were defined because I found that they will also be used by the patch re-coding vdsmd.in from WenYi Gao. If you still think it's useless, I will git rid of them in next set.

....................................................
File vdsm.spec.in
Line 400: # Stop vdsm service
Oh.. my mistake here..!

Line 412: # Restore libvirt service
OK

....................................................
File vdsm-tool/unregister.py
Line 26: BY_VDSM = '# by vdsm'
'feature parity'?  Sorry I don't quiet understand here, since I just recode it based on the spec file. Do you mean check by different version?

Sure, I will add test cases for this patch later.

Line 51: def __remove_from_file(fileName, begin=None, end=None):
Do you mean the name of this function should be change?

Sure, I will add unit test later.

Line 72:         f.writelines(newfile)
Yes, you are right. I will modify it follow the 'atomic move trick' way.
BTW: what's the 'ovirt-node' case? Can I find mburns on IRC?

--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Lei Li has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 4: (1 inline comment)

....................................................
File vdsm-tool/unregister.py
Line 26: BY_VDSM = '# by vdsm'
Hi Dan,

I just look at the file vdsmd.init.in, seems these key words for config  are no long just '# by vdsm', changed to:

by_vdsm="by vdsm"
   by_vdsm_vers="4.9.6"
   start_conf_section="## beginning of configuration section ${by_vdsm}"
   end_conf_section="## end of configuration section ${by_vdsm}"

As for my reply to your comment on constants I added, how about define all of these field to constants?  Then:

1) The patch about move vdsmd.init.in into vdsm-tool can also reuse it.
2) Whenever change the flag field for vdsm config, just modify the constants we defined.

So what's your opinion?

--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Lei Li has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 5:

Hi Dan,

1) For the unit tests, I will add it later.
2) For the constants, I will git rid of them if you still think we should after looking at my reply.

--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Lei Li has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 6:

And thanks to Xu He Jie and Zhou ZhengSheng for their help with the hacking of importing vdsm_tool.

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Wenyi Gao has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 6: (1 inline comment)

....................................................
File vdsm_tool/passwd.py
Line 56:                     constants.SASL_USERNAME])
Maybe better to add a path check for 'saslpasswd2' in configure.ac.

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: Wenyi Gao

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

ShaoHe Feng 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

(2 inline comments)

....................................................
File vdsm_tool/passwd.py
Line 55:     subprocess.call(['/usr/sbin/saslpasswd2', '-p', '-a', 'libvirt', '-d',
yes. shell command should be checked by autotools

....................................................
File vdsm_tool/unregister.py
Line 123:     subprocess.call(['/sbin/chkconfig', 'libvirtd', 'on'])
#which chkconfig
/usr/sbin/chkconfig
surly there is also a command "chkconfig" 
why not use autotool to check the  shell command path?
I wonder what will happen if chkconfig in other path, not in /sbin?

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

ShaoHe Feng has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 6: Fails

$ sudo ls /sbin/initctl
ls: cannot access /sbin/initctl: No such file or directory

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Lei Li has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 6: (1 inline comment)

Shaohe:
Please give more details about which vdsm-tool command failed..and the which os you used, thanks.

....................................................
File vdsm_tool/passwd.py
Line 56:                     constants.SASL_USERNAME])
Seems vdsm spec file already has the check for this shell command:

"Requires(post): /usr/sbin/saslpasswd2"

Can you let me know the reason for this action as your comment?

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

ShaoHe Feng has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 6: No score

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

ShaoHe Feng has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 6: No score

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Wenyi Gao has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 6: (1 inline comment)

....................................................
File vdsm_tool/passwd.py
Line 56:                     constants.SASL_USERNAME])
Yes, I missed the check in the spec file. Ok, since the shell command has been 
checked, here '/usr/sbin/saslpasswd2' should be okey.

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 6: No score

Build Started http://jenkins.ovirt.info/job/vdsm_unit_tests_by_patch/182/

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 6:

Build Successful

http://jenkins.ovirt.info/job/vdsm_unit_tests_by_patch/182/ : SUCCESS

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

[Ryan Harper]

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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 7:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/10/

--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 7: Fails

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/10/ : FAILURE

--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 8:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/11/

--
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 8: Fails

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/11/ : FAILURE

--
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Lei Li has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 8:

I am petty sure that I have tested the code and pass the build...

Take a look at the build error from Jenkin:

Traceback (most recent call last):
  File "../tests/testrunner.py", line 274, in <module>
    hackVdsmModule()
  File "../tests/testrunner.py", line 252, in hackVdsmModule
    sub = __import__(name, globals(), locals(), [], -1)
  File "/home/jenkins/workspace/vdsm_unit_tests_manual_gerrit/vdsm/SecureXMLRPCServer.py", line 39, in <module>
    from M2Crypto import SSL, X509
ImportError: No module named M2Crypto

Looks like it's caused by the commit: 5ea9b646f4fcd56ca48ed673810e86e26d44b523?

--
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Lei Li 
Gerrit-Reviewer: Ryan Harper 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Itamar Heim has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 8:

still relevant or should be abandoned?

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Yaniv Bronhaim has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 8:

Didn't notice that patch. reviewing it

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Yaniv Bronhaim has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 8:

This patch contains very usable parts. I prefer to split it a bit and get it merged. Lei Li, if you can contact me please before I take over, it will be great.

Thanks

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Yaniv Bronhaim has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 8: Code-Review-1

I created http://gerrit.ovirt.org/#/c/21772/ to replace this work. It is too old to rebase

This can be abandoned imo. Thanks.

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Lei Li has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 8:

I am glad that this still could be useful, it'd be great if
you could help on the rebase the pushing!

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Itamar Heim has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 8:

ping

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Yaniv Bronhaim has posted comments on this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Patch Set 8:

I created http://gerrit.ovirt.org/#/c/21772/ to replace this work. It is too old to rebase and can be abandoned imo. Thanks.

Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

Itamar Heim has abandoned this change.

Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................

Abandoned

ok, abandoning per last comment.