Change in vdsm[master]: [WIP]change startup process for vdsm and supervdsm

Royce Lv has uploaded a new change for review.

Change subject: [WIP]change startup process for vdsm and supervdsm
......................................................................

[WIP]change startup process for vdsm and supervdsm

Using vdsmService to take care of start up and respawn process of
vdsm and supervdsm. Remove the function of vdsm to start previlege
supervdsm process.

Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Signed-off-by: Royce Lv 
---
M vdsm.spec.in
M vdsm/Makefile.am
M vdsm/storage/misc.py
M vdsm/supervdsm.py
M vdsm/supervdsmServer.py
M vdsm/vdsm
A vdsm/vdsmService
M vdsm/vdsmd.init.in
M vdsm_reg/Makefile.am
9 files changed, 208 insertions(+), 63 deletions(-)

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: [WIP]change startup process for vdsm and supervdsm
......................................................................

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

(4 inline comments)

thanks for tackling this annoying issue! I wish vdsm startup is normalized.

however, replacing the general-porpose 'respawn' by a vdsm-specific non-trivial script such as vdsmService is not quite the cleanup I was hoping for.

What is the motivation for your patch? Has it been accomplished in your opinion?

I would appreciate if you could keep 'respawn' (for EL6), and make it start superVdsm. superVdsm can then fork, relinquish root, and exec vdsm. This is not trivial to implement, since we would like superVdsm to die if vdsm crashes. But with this, the 'respawn' logic can be replace by systemd for Fedora or upstart in Debian.

In any case, I have not figured it all up; we may end up needing something as complex as the current suggestion.

....................................................
File vdsm/storage/misc.py
Line 724: def retry(func, argList=[], expectedException=Exception, tries=None,
better use a tupple here.

....................................................
File vdsm/supervdsm.py
Line 51:             raise
that's just like not handling the exception, but slower...

....................................................
File vdsm/vdsmService
Line 120: def respawn(cmd, sudo=True):
it took me some time to stabilize that bash code. what is the urgency to rewrite it in Python? Note that modern distributions (aka Fedora) has systemd, which kinda makes 'respawn' redundant, and older ones can make with the current bash.

Line 150:     uid, gid = getpwnam('vdsm')[2:4:]
@VDSMUSER@

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Xu He Jie

Royce Lv has posted comments on this change.

Change subject: [WIP]change startup process for vdsm and supervdsm
......................................................................

Patch Set 1:

Thanks a lot,Dan, for reviewing this patch, this is just a prototype,but I really appreciate your opinion about in which way this should be done so post it with bugs and dirty codes:)

The motivation of this patch is we expect vdsm and supervdsm can crash independently,let something like respawn/vdsmService to take care their authorised communication, We indeed considered to start supervdsm and then let it to fork vdsm, but we don't want svdsm server be mixed up with startup process, more over,we expect super vdsm crash will not result in restarting vdsm.It has been accomplished at this point.

Reason for rewriting respawn in python is we'd like start up and restart process convenient to debug, But I agree with you, let systemd or upstart to take care of it is better.

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Xu He Jie

Royce Lv has posted comments on this change.

Change subject: [WIP]change startup process for vdsm and supervdsm
......................................................................

Patch Set 1:

Thanks a lot,Dan, for reviewing this patch, this is just a prototype, 
but I really appreciate your opinion about 
in which way this should be done so post it with bugs and dirty codes:)

The motivation of this patch is we expect vdsm and supervdsm
 can crash independently,let something like respawn/vdsmService 
to take care their authorised communication, 
We indeed considered to start supervdsm and then let it to fork vdsm, 
but we don't want svdsm server be mixed up with startup process, 
more over,we expect super vdsm crash will not result in restarting vdsm.
It has been accomplished at this point.

Reason for rewriting respawn in python is 
we'd like start up and restart process convenient to debug, 
But I agree with you, let systemd or upstart to take care of it is better.

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Xu He Jie

Shu Ming has posted comments on this change.

Change subject: [WIP]change startup process for vdsm and supervdsm
......................................................................

Patch Set 1: (3 inline comments)

....................................................
File vdsm/vdsmService
Line 43: def __findServicePath(name):
Is here any specific reason to use double underscore to prefix the function name?

Line 67: def parse_args():
These global variables can be removed.  And make the function return the parsed value with dict object.

Line 106:         sys.exit(1)
Adding the abnormal  exit message here will be helpful.

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Xu He Jie

ShaoHe Feng has posted comments on this change.

Change subject: [WIP]change startup process for vdsm and supervdsm
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/vdsm
Line 72:  
whitespace.  you can set git color to find whitespace easily.

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Xu He Jie

ShaoHe Feng has posted comments on this change.

Change subject: [WIP]change startup process for vdsm and supervdsm
......................................................................

Patch Set 1: (2 inline comments)

....................................................
File vdsm/storage/misc.py
Line 751:             return func(*argList)
yes,  * is used for tupple.

....................................................
File vdsm/vdsmService
Line 43: def __findServicePath(name):
it is similar to __supervdsmServerPath() in vdsm/supervdsm.py.
I think this function is private, so it will not be called by other module.

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Xu He Jie

[Xu He Jie]

Xu He Jie has posted comments on this change.

Change subject: [WIP]change startup process for vdsm and supervdsm
......................................................................

Patch Set 2: (4 inline comments)

....................................................
File vdsm/supervdsmServer.py
Line 313:         chown(ADDRESS, METADATA_USER, METADATA_GROUP)
This user should be same with the user which used to startup vdsm.py

....................................................
File vdsm/vdsm
Line 28:     print "     -h  - Display this help message"
I guess you forgot change this usage message

Line 104:     os.setuid(uid)
Hmm, move this to supervdsmServer? let supervdsmServer to decide which user will be used. vdsm needn't know which user it want to use.

....................................................
File vdsm/vdsmd.init.in
Line 492:     LC_ALL=C NICELEVEL=$vdsm_nice daemon @VDSMDIR@/respawn --minlifetime 10 --daemon --masterpid $RESPAWNPIDFILE $SVDSM_BIN
for fedora, respawn can be done by systemd, did you have plan for this?

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Xu He Jie

Wenyi Gao has posted comments on this change.

Change subject: [WIP]change startup process for vdsm and supervdsm
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/supervdsmServer.py
Line 63: VDSM_BIN = "/usr/share/vdsm/vdsm"
VDSM_PIDFILE path should be '@VDSMRUNDIR@/vdsmd.pid'  in supervdsmServer.py.in?
VDSM_BIN path should be  '@VDSMDIR@/vdsm' ?

Same problem for other path such as SVDSM_PIDFILE ?

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie

Change in vdsm[master]: [WIP]change startup process for vdsm and supervdsm star...

Saggi Mizrahi has posted comments on this change.

Change subject: [WIP]change startup process for vdsm and supervdsm      start vdsm process with supervdsmServer.py, use vdsm.init to respwan supervdsmServer. supervdsm will terminate when vdsm crashes. If supervdsm crashes, vdsm can only do tasks which doesn't need conne
......................................................................

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

Having a physical socket is redundant when you fork from the same root process. You should use a pipe.

Also, having supervdsm as the main file is not recommended as supervdsm is a tool and not the main file.
As I see it.

vdsm.py:
createPipes()
forkAndStartsvdsm(pipes)
setCreds()
startRegualVdsm()

In each of the processes:
   If the pipe breaks on either side exit

Sorry if being too vague

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie

Adam Litke has posted comments on this change.

Change subject: [WIP]change startup process for vdsm and supervdsm
......................................................................

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

(15 inline comments)

I performed the following tests with your patch:

* Normal service start, stop, restart: OK

* Kill vdsm, what happens? FAIL
vdsm process shuts down but remains in defunct state.  Nothing is respawned.  The pokeParent function is unable to identify a problem with the vdsm process since 'kill -0' still returns 0.  See the end of this comment for an idea to fix this.

* Kill vdsm, then restart service: OK.

* Kill supervdsm, what happens? FAIL.

Traceback (most recent call last):
  File "/usr/share/vdsm/vdsm", line 98, in startVdsm
    serve_clients(log)
  File "/usr/share/vdsm/vdsm", line 46, in serve_clients
    cif = clientIF.clientIF(log)
  File "/usr/share/vdsm/clientIF.py", line 96, in __init__
    self._prepareBindings()
  File "/usr/share/vdsm/clientIF.py", line 134, in _prepareBindings
    self._loadBindingXMLRPC()
  File "/usr/share/vdsm/clientIF.py", line 120, in _loadBindingXMLRPC
    default_bridge)
  File "/usr/share/vdsm/BindingXMLRPC.py", line 48, in __init__
    self.server = self._createXMLRPCServer()
  File "/usr/share/vdsm/BindingXMLRPC.py", line 137, in _createXMLRPCServer
    requestHandler=LoggingHandler, logRequests=True)
  File "/usr/lib64/python2.7/SimpleXMLRPCServer.py", line 590, in __init__
    SocketServer.TCPServer.__init__(self, addr, requestHandler, bind_and_activate)
  File "/usr/lib64/python2.7/SocketServer.py", line 408, in __init__
    self.server_bind()
  File "/usr/lib64/python2.7/SocketServer.py", line 419, in server_bind
    self.socket.bind(self.server_address)
  File "/usr/lib64/python2.7/socket.py", line 224, in meth
    return getattr(self._sock,name)(*args)
error: [Errno 98] Address already in use

I believe this happens because the vdsm child was still running and when supervdsm was respawned it tried to start a new child.

* Kill supervdsm, then restart service: OK.

Would this be a better startup sequence...
1) As root, create a new process for supervdsmServer
2) Drop privileges in the parent
3) Start vdsm in the parent

Then, if vdsm detects a problem with supervdsm, it can just exit.  This will cause respawn to restart everything again.  So if either process (vdsm, or supervdsm) dies, the whole thing will be respawned.  Maybe there are problems with my idea too.  What do you think?

....................................................
File vdsm/supervdsm.py
Line 52:         except:
This is a bad condition.  I think there should be some logging of the exception that is causing vdsm to shut down.  Also, are you sure you want to do this for all types of exceptions?  I think it's ok but we should double-check.

Line 53:             os.unlink(ADDRESS)
I don't understand this.  Don't we want supervdsmServer to clean up it's own socket file when it shuts itself down in response to this failure.

Am I missing something?

Line 54:             os.kill(os.getpid(), signal.SIGTERM)
Is this all we need to do to properly shut down vdsm (a question for smarter reviewers than I)?  vdsm does have an API called prepareForShutdown() that performs a lot more work to gracefully shut down the daemon.  Maybe we need to look at using it instead.

Line 68:             os.unlink(ADDRESS)
Again, I am dubious about removing the server's socket file from here.

Line 69:             os.kill(os.getpid(), signal.SIGTERM)
prepareForShutdown again?

....................................................
File vdsm/supervdsmServer.py
Line 267
This function no longer operates on a parent.  Let's rename it to 'pokeVdsm' and change the arg to vdsmPid to make it more clear.

Line 300:         #kill old supervdsm
Add a space between # and kill.

....................................................
File vdsm/vdsm
Line 80:             return
Instead of quietly returning here I would prefer that you raise an exception.  If the user doesn't start vdsm as root we want to catch it right away and provide a clear error message.

I know I contributed this example code :)

Please consider adding a functional test to cover this case.

Line 81:         vdsm_uid,vdsm_gid = getpwnam(VDSM_USER)[2:4:]
space between ',' and vdsm_gid

Line 99: 
This blank line is not necessary.

Line 110:     supervdsmServer.restartSvdsm(manager, vdsmPid)
This is just a one-liner.  Why not inline the code directly in the function below?

Line 116:     # None key make children inherit it from parent
May I suggest rewording this comment for clarity?

# When key is None, connections are restricted to child processes.

Line 117:     manager = _SuperVdsmManager(ADDRESS)
Since you reference the authKey parameter in a comment, best to supply it as an explicit parameter to the function:

manager = _SuperVdsmManager(ADDRESS, None)

Line 127:         os.kill(vdsmPid, signal.SIGTERM)
What happens if vdsmPid isn't defined yet because an exception was thrown by vdsmProc.start() ?  You should either move vdsmProc.start() out of the try block or test that the value of vdsmPid makes sense.

Line 129:     vdsmProc.join()                                                    
Trailing whitespace.

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie

Royce Lv has posted comments on this change.

Change subject: [WIP]change startup process for vdsm and supervdsm
......................................................................

Patch Set 4: (4 inline comments)

....................................................
File vdsm/supervdsm.py
Line 52:         except:
For exception raised inside svdsm function I think we should not kill vdsm, will change here

Line 53:             os.unlink(ADDRESS)
You're right, even if svdsm has not clean up when exit, it will cleaned when restart, it's redundent

Line 54:             os.kill(os.getpid(), signal.SIGTERM)
The vdsm has signal handler:
def sigtermHandler(signum, frame):	
    if cif:	
        cif.prepareForShutdown()

if cif and cif.irs:
    cif.irs.spmStop(cif.irs.getConnectedStoragePoolsList()['poollist'][0])
signal.signal(signal.SIGTERM, sigtermHandler)

That is why I choose to just send a signal to vdsm:)

....................................................
File vdsm/vdsm
Line 80:             return
I will add test cases for all of start,restart and so on:)I thought non-root would fail later, but here is better

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie

Royce Lv has posted comments on this change.

Change subject: [WIP]change startup process for vdsm and supervdsm
......................................................................

Patch Set 4: (1 inline comment)

I think the two failure you tested is because I should kill then start vdsm instead just start vdsm.

....................................................
File vdsm/supervdsm.py
Line 53:             os.unlink(ADDRESS)
After a second thought, I think if svdsm is killed and not clean this ADDRESS, but in the startup period, vdsm is started prior to svdsm, and if it doesn't clean ADDR, it will raise "address already in use" error, so it is necessary.

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie

Adam Litke has posted comments on this change.

Change subject: [WIP]change startup process for vdsm and supervdsm
......................................................................

Patch Set 5:

Hi Royce.  The code looks good.  I think you are getting closer but it is still not working quite right for me.  I noticed that when I use kill -9 for the vdsm process it restarts as expected but if I use any other signal it will not restart.  Also, when I kill the supervdsm process, vdsm notices and shuts down but the process stays alive and so it is not respawned.

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie

Change in vdsm[master]: change startup process for vdsm and supervdsm

Royce Lv has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 6: Verified

verified:vdsm reboot after super vdsm killed, vdsm reboot after vdsm killed, vdsm can react on xmlrpc, vdsm will not loose connection to libvirt after 10min.

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie

Change in vdsm[master]: change startup process for vdsm and supervdsm

Saggi Mizrahi has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 6: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie

Change in vdsm[master]: change startup process for vdsm and supervdsm

Royce Lv has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 6: Fails; I would prefer that you didn't submit this

Rebased code failed storage functional test, will submit new version after resolve it.

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie

Change in vdsm[master]: change startup process for vdsm and supervdsm

Royce Lv has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 7:

verified:
(1)start & restart service
(2)kill vdsm, service automatically restart
(3)kill supervdsm, service automatically restart
(4)getVdsCaps functions after every restart
(5)libvirt connection never lost
(6)localfs:create domain/pool/volume/vm pass

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie

Change in vdsm[master]: change startup process for vdsm and supervdsm

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 8:

Build Started http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/404/

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 8:

Build Failed

http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/404/ : ABORTED

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 9:

Build Started http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/405/

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 9:

Build Failed

http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/405/ : ABORTED

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

Royce Lv has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 9:

verfiied:vdsm process kill,supervdsm process kill and service restart, storage test for NFS,ISCSI,vm creation test

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

[Xu He Jie]

Xu He Jie has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 9: (1 inline comment)

....................................................
File vdsm/supervdsmServer.py
Line 354:         log.debug("Creating remote object manager")
Line 355:         manager.register('instance', callable=_SuperVdsm)
Line 356: 
Line 357:         server = manager.get_server()
Line 358:         servThread = threading.Thread(target=server.serve_forever)
Why run server as another thread?
Line 359:         servThread.setDaemon(True)
Line 360:         servThread.start()
Line 361: 
Line 362:         chown(ADDRESS, VDSM_USER, VDSM_GROUP)

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 10:

Build Failed

http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/476/ : ABORTED

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

Adam Litke has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 10: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

[Xu He Jie]

Xu He Jie has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 10: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

Saggi Mizrahi has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

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

There are a lot of places where you os.kil(os.getpid(), ...)

You should use misc panic for that. And I also don't really understand why that can't be funneled to a single spot where you panic.

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

Royce Lv has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 10:

Saggi,
Thanks a lot for review.About tunnel all os.kill to a single point:

(1)vdsm check if supervdsm died and killed itself.
(2)vdsm check call supervdsm failed and kill self(the hsm or clientIF will except the error)
(3)supervdsm check if vdsm alive or killed itself.

I thought they could not be tunneled to a single point. Please feel free to correct me.In fact I wait on IRC for few days wanting to ask about this:)

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

Royce Lv has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 11:

Use prepareforShutdown instead of misc.panic(kill-9) to make spm stats consistent and not being rude when error happens.

keep using os.kill in supervdsm proxy because add clientif.prepareforShutdown will make it unclean, also vdsm file is not a python module, and not proper for supervdsm proxy to reference.

supervdsmServer's os.kill keep it was,made nothing changed.

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

Adam Litke has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 11: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

Adam Litke has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 11:

Hi Royce,

I was testing out your patch and I found that when I kill the supervdsm process vdsm does not restart but if I kill the main vdsmd one then it does restart everything.  Is this expected behavior?

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

Royce Lv has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 11:

Thanks a lot for your review, Adam
 Kill supervdsm really takes more time for the whole to restart (5s estimated), but it will restart as we expect at last.
The reason is prepareForshutdown may be called several times,and here always take a while.

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

Royce Lv has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 11:

Dear maintainers, as this patch is really long due and rebased for a lot of time. Would you please give some feedback on this one? Thanks!

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

Yaniv Bronhaim has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

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

(7 inline comments)

This patch is too big and changes not only svdsm implementation but also vdsm startup process and the way it starts and stops svdsm.

You didn't include unit tests for this patch so it's hard to know if it works in different cases.

I prefer that you will split this patch and rebase it over upstream code before accepting it, but if you want to keep it that way (what I think is wrong..), try to rebase it and add uts for that and I'll try to verify if it works properly.

thanks.

....................................................
File vdsm/supervdsm.py
Line 53:         try:
Line 54:             return callMethod()
Line 55:         except(IOError, socket.error, AuthenticationError):
Line 56:             os.kill(os.getpid(), signal.SIGTERM)
Line 57:             raise
we don't need to kill svdsm if we get IOError or socket.error, it could be raised from svdsm internal implementation, like when delNetwork raised IOError.. it doesn't mean that something is wrong with svdsm.. we should just raise the error.
Although, AuthenticationError must lead to svdsm restart.
Line 58: 
Line 59: 
Line 60: class SuperVdsmProxy(object):
Line 61:     """

Line 67:         try:
Line 68:             misc.retry(self._connect, Exception, timeout=60)
Line 69:         except Exception:
Line 70:             self._log.debug("error raised when connecting to super vdsm")
Line 71:             os.unlink(ADDRESS)
unlink can cause exception and the next kill line will never run.. use utils.rmFile
Line 72:             os.kill(os.getpid(), signal.SIGTERM)
Line 73:         self._log.debug("Connected to Super Vdsm")
Line 74: 
Line 75:     def open(self, *args, **kwargs):

....................................................
File vdsm/supervdsmServer.py
Line 100:         return _getLsBlk(*args, **kwargs)
Line 101: 
Line 102:     @logDecorator
Line 103:     def stopSuperVdsm(self):
Line 104:         os.unlink(ADDRESS)
you should use utils.rmFile here. but why do we need the option to stop svdsm from outside?..
Line 105:         os.kill(os.getpid(), signal.SIGTERM)
Line 106: 
Line 107:     @logDecorator
Line 108:     def readMultipathConf(self):

Line 335: 
Line 336:     log = logging.getLogger("SuperVdsm.Server")
Line 337:     try:
Line 338:         # kill old supervdsm
Line 339:         _killSupervdsm(log)
supservdsm.py should control the initialization, termination and calling for requests to svdsm, supervdsmServer supposed to handle all svdsm api and startup. (and if svdsm should die when vdsm goes done, the suicide process).

This implementation mix this concepts and turn supervdsmServer.py to be the manager. so why do you keep supervdsm.py?
Line 340: 
Line 341:         log.debug("Creating PID file")
Line 342: 
Line 343:         with open(PIDFILE, "w") as f:

....................................................
File vdsm/vdsm
Line 81:     log = logging.getLogger('vds')
Line 82:     try:
Line 83:         logging.root.handlers.append(logging.StreamHandler())
Line 84:         log.handlers.append(logging.StreamHandler())
Line 85:         chown(LOGFILE, VDSM_USER, VDSM_GROUP)
We don't want to perform this chown every startup.. permission to log are not supposed to change.
This's why we start vdsm with user vdsm and not as root, we don't want to run vdsm as root because we don't know what other programmers added to vdsm startup. 
It is safer to change the user from root before invoking your process.
Line 86:     except:
Line 87:         log.error("Exception raised", exc_info=True)
Line 88:     return log
Line 89:

Line 142: 
Line 143:     # When key is None, connections are restricted to child processes
Line 144:     manager = _SuperVdsmManager(ADDRESS, None)
Line 145:     vdsmPid = os.getpid()
Line 146:     svdsmProc = Process(target=restartSvdsm, args=(manager, vdsmPid, ))
why did you change to Process instead of multiprocessing manager?
Line 147:     try:
Line 148:         svdsmProc.start()
Line 149:         waitThread = threading.Thread(target=__waitSvdsm,
Line 150:                                       args=[svdsmProc])

Line 145:     vdsmPid = os.getpid()
Line 146:     svdsmProc = Process(target=restartSvdsm, args=(manager, vdsmPid, ))
Line 147:     try:
Line 148:         svdsmProc.start()
Line 149:         waitThread = threading.Thread(target=__waitSvdsm,
I think it's important and we can add it to supervdsm.py (not here.. just to separate responsibilities, this py should only initialize  vdsm), but I think we shouldn't stop vdsm if svdsm stops.. but i need another opinion about it ..
Line 150:                                       args=[svdsmProc])
Line 151:         waitThread.setDaemon(True)
Line 152:         waitThread.start()
Line 153:         startVdsm(logger)

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

Royce Lv has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 11: (4 inline comments)

....................................................
File vdsm/supervdsm.py
Line 53:         try:
Line 54:             return callMethod()
Line 55:         except(IOError, socket.error, AuthenticationError):
Line 56:             os.kill(os.getpid(), signal.SIGTERM)
Line 57:             raise
I'll rebase on yours
Line 58: 
Line 59: 
Line 60: class SuperVdsmProxy(object):
Line 61:     """

Line 67:         try:
Line 68:             misc.retry(self._connect, Exception, timeout=60)
Line 69:         except Exception:
Line 70:             self._log.debug("error raised when connecting to super vdsm")
Line 71:             os.unlink(ADDRESS)
Done
Line 72:             os.kill(os.getpid(), signal.SIGTERM)
Line 73:         self._log.debug("Connected to Super Vdsm")
Line 74: 
Line 75:     def open(self, *args, **kwargs):

....................................................
File vdsm/vdsm
Line 142: 
Line 143:     # When key is None, connections are restricted to child processes
Line 144:     manager = _SuperVdsmManager(ADDRESS, None)
Line 145:     vdsmPid = os.getpid()
Line 146:     svdsmProc = Process(target=restartSvdsm, args=(manager, vdsmPid, ))
<ref:python doc>:Manager() controls a server process which holds Python objects and
 allows other processes to manipulate them using proxies.

According to me, Our supervdsm process is more than just callable objects:
 supervdsm process = main thread(for thread join and zombie join)+heart beat for 
vdsm+manager.

Correct me if I misunderstand you.:)
Line 147:     try:
Line 148:         svdsmProc.start()
Line 149:         waitThread = threading.Thread(target=__waitSvdsm,
Line 150:                                       args=[svdsmProc])

Line 145:     vdsmPid = os.getpid()
Line 146:     svdsmProc = Process(target=restartSvdsm, args=(manager, vdsmPid, ))
Line 147:     try:
Line 148:         svdsmProc.start()
Line 149:         waitThread = threading.Thread(target=__waitSvdsm,
1. if svdsm stops what should vdsm do?
maybe I can try to let vdsm run until it discovered svdsm died when call svdsm proxy.
since vdsm does not has privilege to restart svdsm, it just leave us one choice to restart vdsm and svdsm all over. Or else we will need a daemon dedicate for 'vdsm' and 'svdsm' respectively stop and start , this logic is more complex, that is what I do in *patch set 1*, see Danken's opinion about that.
Line 150:                                       args=[svdsmProc])
Line 151:         waitThread.setDaemon(True)
Line 152:         waitThread.start()
Line 153:         startVdsm(logger)

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

Yaniv Bronhaim has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 11:

http://gerrit.ovirt.org/#/c/11910 - is based on your changes here and adds the use in rpyc

--
To view, visit http://gerrit.ovirt.org/4145
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I69aae6b0b9529c80291d90c6ad14ff82b21aea53
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Wenyi Gao 
Gerrit-Reviewer: Xu He Jie 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: change startup process for vdsm and supervdsm

[Dan Kenigsberg]

Dan Kenigsberg has abandoned this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Abandoned

supervdsm startup has been revamped base on this work, but now the patch can be abandoned.

Change in vdsm[master]: change startup process for vdsm and supervdsm

Yaniv Bronhaim has posted comments on this change.

Change subject: change startup process for vdsm and supervdsm
......................................................................

Patch Set 11:

all with topic "change-vdsm-startup" - http://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic:cha...

can be abandoned.
thanks.