[PATCH 0 of 3] xencommons init script improvements

[PATCH 0 of 3] xencommons init script improvements

[George Dunlap]

This patch series introduces some improvements to the xencommons init
script:
* Kill xenstored when running xencommons stop
* Be more verbose about what's going on (like normal init scripts) 
* Handle a race with xenstored startup

[PATCH 1 of 3] xencommons: Kill xenstored when stopping xencommons

[George Dunlap]

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r bf64e1081333 -r e71645625934 tools/hotplug/Linux/init.d/xencommons
--- a/tools/hotplug/Linux/init.d/xencommons	Fri Jun 25 16:05:20 2010 +0100
+++ b/tools/hotplug/Linux/init.d/xencommons	Mon Jun 28 13:24:26 2010 +0100
@@ -22,6 +22,7 @@
 test -f /etc/sysconfig/xencommons && . /etc/sysconfig/xencommons
 
 XENCONSOLED_PIDFILE=/var/run/xenconsoled.pid
+XENSTORED_PIDFILE=/var/run/xenstored.pid
 shopt -s extglob
 
 if test "x$1" = xstart && \
@@ -42,7 +43,7 @@
 		test -z "$XENSTORED_ROOTDIR" || XENSTORED_ROOTDIR="/var/lib/xenstored"
 		rm -f "$XENSTORED_ROOTDIR"/tdb* &>/dev/null
 		test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
-		xenstored --pid-file=/var/run/xenstore.pid $XENSTORED_ARGS
+		xenstored --pid-file=$XENSTORED_PIDFILE $XENSTORED_ARGS
 		xenstore-write "/local/domain/0/name" "Domain-0"
 	fi
 
@@ -57,6 +58,12 @@
 		while kill -9 $pid >/dev/null 2>&1; do sleep 0.1; done
 		rm -f $XENCONSOLED_PIDFILE
 	fi
+
+	if read 2>/dev/null <$XENSTORED_PIDFILE pid; then
+		kill $pid
+		while kill -9 $pid >/dev/null 2>&1; do sleep 0.1; done
+		rm -f $XENSTORED_PIDFILE
+	fi
 }
 
 case "$1" in

Re: [PATCH 1 of 3] xencommons: Kill xenstored when stopping xencommons

[Stefano Stabellini]

xenstored doesn't stop correctly.
There is no point in stopping xenstored if we are going to break the
host.

On Mon, 28 Jun 2010, George Dunlap wrote:
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> diff -r bf64e1081333 -r e71645625934 tools/hotplug/Linux/init.d/xencommons
> --- a/tools/hotplug/Linux/init.d/xencommons	Fri Jun 25 16:05:20 2010 +0100
> +++ b/tools/hotplug/Linux/init.d/xencommons	Mon Jun 28 13:24:26 2010 +0100
> @@ -22,6 +22,7 @@
>  test -f /etc/sysconfig/xencommons && . /etc/sysconfig/xencommons
>  
>  XENCONSOLED_PIDFILE=/var/run/xenconsoled.pid
> +XENSTORED_PIDFILE=/var/run/xenstored.pid
>  shopt -s extglob
>  
>  if test "x$1" = xstart && \
> @@ -42,7 +43,7 @@
>  		test -z "$XENSTORED_ROOTDIR" || XENSTORED_ROOTDIR="/var/lib/xenstored"
>  		rm -f "$XENSTORED_ROOTDIR"/tdb* &>/dev/null
>  		test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
> -		xenstored --pid-file=/var/run/xenstore.pid $XENSTORED_ARGS
> +		xenstored --pid-file=$XENSTORED_PIDFILE $XENSTORED_ARGS
>  		xenstore-write "/local/domain/0/name" "Domain-0"
>  	fi
>  
> @@ -57,6 +58,12 @@
>  		while kill -9 $pid >/dev/null 2>&1; do sleep 0.1; done
>  		rm -f $XENCONSOLED_PIDFILE
>  	fi
> +
> +	if read 2>/dev/null <$XENSTORED_PIDFILE pid; then
> +		kill $pid
> +		while kill -9 $pid >/dev/null 2>&1; do sleep 0.1; done
> +		rm -f $XENSTORED_PIDFILE
> +	fi
>  }
>  
>  case "$1" in
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

Re: [PATCH 1 of 3] xencommons: Kill xenstored when stopping xencommons

[George Dunlap]

Seems like a comment to that effect in the script would be useful, then. :-)

What doesn't stop correctly?  I've manually killed it and started it 
again, and it seemed to work.  Granted, there wasn't a whole lot stored 
in it at the time...

  -George

On 28/06/10 14:08, Stefano Stabellini wrote:
> xenstored doesn't stop correctly.
> There is no point in stopping xenstored if we are going to break the
> host.
>
> On Mon, 28 Jun 2010, George Dunlap wrote:
>> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
>>
>> diff -r bf64e1081333 -r e71645625934 tools/hotplug/Linux/init.d/xencommons
>> --- a/tools/hotplug/Linux/init.d/xencommons	Fri Jun 25 16:05:20 2010 +0100
>> +++ b/tools/hotplug/Linux/init.d/xencommons	Mon Jun 28 13:24:26 2010 +0100
>> @@ -22,6 +22,7 @@
>>   test -f /etc/sysconfig/xencommons&&  . /etc/sysconfig/xencommons
>>
>>   XENCONSOLED_PIDFILE=/var/run/xenconsoled.pid
>> +XENSTORED_PIDFILE=/var/run/xenstored.pid
>>   shopt -s extglob
>>
>>   if test "x$1" = xstart&&  \
>> @@ -42,7 +43,7 @@
>>   		test -z "$XENSTORED_ROOTDIR" || XENSTORED_ROOTDIR="/var/lib/xenstored"
>>   		rm -f "$XENSTORED_ROOTDIR"/tdb*&>/dev/null
>>   		test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
>> -		xenstored --pid-file=/var/run/xenstore.pid $XENSTORED_ARGS
>> +		xenstored --pid-file=$XENSTORED_PIDFILE $XENSTORED_ARGS
>>   		xenstore-write "/local/domain/0/name" "Domain-0"
>>   	fi
>>
>> @@ -57,6 +58,12 @@
>>   		while kill -9 $pid>/dev/null 2>&1; do sleep 0.1; done
>>   		rm -f $XENCONSOLED_PIDFILE
>>   	fi
>> +
>> +	if read 2>/dev/null<$XENSTORED_PIDFILE pid; then
>> +		kill $pid
>> +		while kill -9 $pid>/dev/null 2>&1; do sleep 0.1; done
>> +		rm -f $XENSTORED_PIDFILE
>> +	fi
>>   }
>>
>>   case "$1" in
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>>

Re: [PATCH 1 of 3] xencommons: Kill xenstored when stopping xencommons

[Ian Campbell]

On Mon, 2010-06-28 at 14:10 +0100, George Dunlap wrote:
> Seems like a comment to that effect in the script would be useful, then. :-)

Yes, this isn't the first attempt to kill xenstored from this script.

It might be worth having a really-stop-i-know-what-im-doing target

> What doesn't stop correctly?  I've manually killed it and started it 
> again, and it seemed to work.  Granted, there wasn't a whole lot stored 
> in it at the time...

Watches aren't preserved, so for example netback's watch
on /local/domain/0/backends/ gets lost and you can't start any network
backends even after you start xenstored again, similarly for other
device types.

Same thing for any domU's you might have running, they will loose the
ability to attach and detach devices, the watch on the control node
which controls reboot etc goes away etc.

Ian.


> 
>   -George
> 
> On 28/06/10 14:08, Stefano Stabellini wrote:
> > xenstored doesn't stop correctly.
> > There is no point in stopping xenstored if we are going to break the
> > host.
> >
> > On Mon, 28 Jun 2010, George Dunlap wrote:
> >> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
> >>
> >> diff -r bf64e1081333 -r e71645625934 tools/hotplug/Linux/init.d/xencommons
> >> --- a/tools/hotplug/Linux/init.d/xencommons	Fri Jun 25 16:05:20 2010 +0100
> >> +++ b/tools/hotplug/Linux/init.d/xencommons	Mon Jun 28 13:24:26 2010 +0100
> >> @@ -22,6 +22,7 @@
> >>   test -f /etc/sysconfig/xencommons&&  . /etc/sysconfig/xencommons
> >>
> >>   XENCONSOLED_PIDFILE=/var/run/xenconsoled.pid
> >> +XENSTORED_PIDFILE=/var/run/xenstored.pid
> >>   shopt -s extglob
> >>
> >>   if test "x$1" = xstart&&  \
> >> @@ -42,7 +43,7 @@
> >>   		test -z "$XENSTORED_ROOTDIR" || XENSTORED_ROOTDIR="/var/lib/xenstored"
> >>   		rm -f "$XENSTORED_ROOTDIR"/tdb*&>/dev/null
> >>   		test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
> >> -		xenstored --pid-file=/var/run/xenstore.pid $XENSTORED_ARGS
> >> +		xenstored --pid-file=$XENSTORED_PIDFILE $XENSTORED_ARGS
> >>   		xenstore-write "/local/domain/0/name" "Domain-0"
> >>   	fi
> >>
> >> @@ -57,6 +58,12 @@
> >>   		while kill -9 $pid>/dev/null 2>&1; do sleep 0.1; done
> >>   		rm -f $XENCONSOLED_PIDFILE
> >>   	fi
> >> +
> >> +	if read 2>/dev/null<$XENSTORED_PIDFILE pid; then
> >> +		kill $pid
> >> +		while kill -9 $pid>/dev/null 2>&1; do sleep 0.1; done
> >> +		rm -f $XENSTORED_PIDFILE
> >> +	fi
> >>   }
> >>
> >>   case "$1" in
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xensource.com
> >> http://lists.xensource.com/xen-devel
> >>
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 1 of 3] xencommons: Kill xenstored when stopping xencommons

[Ian Campbell]

xenstored cannot be restarted once stopped so I think we want to be
careful about ever stopping it in a command which a user might
"normally" call.

On Mon, 2010-06-28 at 13:58 +0100, George Dunlap wrote:
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> diff -r bf64e1081333 -r e71645625934 tools/hotplug/Linux/init.d/xencommons
> --- a/tools/hotplug/Linux/init.d/xencommons	Fri Jun 25 16:05:20 2010 +0100
> +++ b/tools/hotplug/Linux/init.d/xencommons	Mon Jun 28 13:24:26 2010 +0100
> @@ -22,6 +22,7 @@
>  test -f /etc/sysconfig/xencommons && . /etc/sysconfig/xencommons
>  
>  XENCONSOLED_PIDFILE=/var/run/xenconsoled.pid
> +XENSTORED_PIDFILE=/var/run/xenstored.pid
>  shopt -s extglob
>  
>  if test "x$1" = xstart && \
> @@ -42,7 +43,7 @@
>  		test -z "$XENSTORED_ROOTDIR" || XENSTORED_ROOTDIR="/var/lib/xenstored"
>  		rm -f "$XENSTORED_ROOTDIR"/tdb* &>/dev/null
>  		test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
> -		xenstored --pid-file=/var/run/xenstore.pid $XENSTORED_ARGS
> +		xenstored --pid-file=$XENSTORED_PIDFILE $XENSTORED_ARGS
>  		xenstore-write "/local/domain/0/name" "Domain-0"
>  	fi
>  
> @@ -57,6 +58,12 @@
>  		while kill -9 $pid >/dev/null 2>&1; do sleep 0.1; done
>  		rm -f $XENCONSOLED_PIDFILE
>  	fi
> +
> +	if read 2>/dev/null <$XENSTORED_PIDFILE pid; then
> +		kill $pid
> +		while kill -9 $pid >/dev/null 2>&1; do sleep 0.1; done
> +		rm -f $XENSTORED_PIDFILE
> +	fi
>  }
>  
>  case "$1" in
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 1 of 3] xencommons: Kill xenstored when stopping xencommons

[Vincent Hanquez]

On 28/06/10 14:12, Ian Campbell wrote:
> xenstored cannot be restarted once stopped so I think we want to be
> careful about ever stopping it in a command which a user might
> "normally" call.
C xenstored cannot be stopped, Ocaml xenstored **can* be stopped.
shouldn't everyone be running ocaml xenstored nowadays ?

-- 
Vincent

Re: [PATCH 1 of 3] xencommons: Kill xenstored when stopping xencommons

[Ian Campbell]

On Mon, 2010-06-28 at 14:51 +0100, Vincent Hanquez wrote:
> On 28/06/10 14:12, Ian Campbell wrote:
> > xenstored cannot be restarted once stopped so I think we want to be
> > careful about ever stopping it in a command which a user might
> > "normally" call.
> C xenstored cannot be stopped, Ocaml xenstored **can* be stopped.
> shouldn't everyone be running ocaml xenstored nowadays ?

It's not so much the stopping per se but more the starting it again and
expecting the system to work. Can oxenstored cope with that? Where does
it persist its watches to?

Ian.

Re: [PATCH 1 of 3] xencommons: Kill xenstored when stopping xencommons

[Vincent Hanquez]

>
> It's not so much the stopping per se but more the starting it again and
> expecting the system to work. Can oxenstored cope with that? Where does
> it persist its watches to?
>
>    
yes, it does keep the watches (along with anything else required) in its 
database which it's supposed to dump when beeing kill nicely and using 
--restart so that it read the database at startup.

-- 
Vincent

Re: [PATCH 1 of 3] xencommons: Kill xenstored when stopping xencommons

[Ian Campbell]

On Mon, 2010-06-28 at 14:56 +0100, Vincent Hanquez wrote:
> >
> > It's not so much the stopping per se but more the starting it again and
> > expecting the system to work. Can oxenstored cope with that? Where does
> > it persist its watches to?
> >
> >    
> yes, it does keep the watches (along with anything else required) in its 
> database which it's supposed to dump when beeing kill nicely and using 
> --restart so that it read the database at startup.

Wow, it's like living in the future!

Was the intention that users only ever use a "restart" target (assuming
it passes --restart) in the initscript rather than the stop->start pair
some people are used to? 

Even if stop does a clean shutdown unless start uses --restart we have
the same issue with forgetting watches etc. Do we need a separate
start-like target which reads the db (for use after you've done a stop)
or is it safe to use --restart by default? Can an initscript distinguish
a start at start of day from a start run by the user (following an
earlier stop) and hence add the --restart automagically?

Ian.

Re: [PATCH 1 of 3] xencommons: Kill xenstored when stopping xencommons

[Vincent Hanquez]

>
> Wow, it's like living in the future!
>
> Was the intention that users only ever use a "restart" target (assuming
> it passes --restart) in the initscript rather than the stop->start pair
> some people are used to?
>
>    
no, i think the intention was not using a database from the old boot 
when the system restarted. with the correct logic in the script, there's 
a way to make it default i think.

> Even if stop does a clean shutdown unless start uses --restart we have
> the same issue with forgetting watches etc. Do we need a separate
> start-like target which reads the db (for use after you've done a stop)
> or is it safe to use --restart by default? Can an initscript distinguish
> a start at start of day from a start run by the user (following an
> earlier stop) and hence add the --restart automagically?
>    
You pinpointed the problem of why it's not on by default.
I think i investigated using the absolute boot time (which i can't 
remember where it is in proc) and compare the database mtime, but I 
can't remember if it came to any conclusion.

-- 
Vincent

Re: [PATCH 1 of 3] xencommons: Kill xenstored when stopping xencommons

[Keir Fraser]

On 28/06/2010 14:51, "Vincent Hanquez" <vincent.hanquez@eu.citrix.com>
wrote:

> On 28/06/10 14:12, Ian Campbell wrote:
>> xenstored cannot be restarted once stopped so I think we want to be
>> careful about ever stopping it in a command which a user might
>> "normally" call.
> C xenstored cannot be stopped, Ocaml xenstored **can* be stopped.
> shouldn't everyone be running ocaml xenstored nowadays ?

Then the init script should reference oxenstored, or have the option to do
so, shouldn't it.

 -- Keir

Re: [PATCH 1 of 3] xencommons: Kill xenstored when stopping xencommons

[Vincent Hanquez]

>
> Then the init script should reference oxenstored, or have the option to do
> so, shouldn't it.
>
>    
fair enough. I assumed that because oxenstored predated the script in 
the tree that it would use it by default. I'll see if I can make a patch.

-- 
Vincent

[PATCH 2 of 3] xencommons: Make init script more verbose

[George Dunlap]

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r e71645625934 -r 9781a01f6ccb tools/hotplug/Linux/init.d/xencommons
--- a/tools/hotplug/Linux/init.d/xencommons	Mon Jun 28 13:24:26 2010 +0100
+++ b/tools/hotplug/Linux/init.d/xencommons	Mon Jun 28 13:24:26 2010 +0100
@@ -43,10 +43,14 @@
 		test -z "$XENSTORED_ROOTDIR" || XENSTORED_ROOTDIR="/var/lib/xenstored"
 		rm -f "$XENSTORED_ROOTDIR"/tdb* &>/dev/null
 		test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
+
+		echo Starting xenstored...
 		xenstored --pid-file=$XENSTORED_PIDFILE $XENSTORED_ARGS
+		echo Setting domain 0 name...
 		xenstore-write "/local/domain/0/name" "Domain-0"
 	fi
 
+	echo Starting xenconsoled...
 	test -z "$XENCONSOLED_TRACE" || XENCONSOLED_ARGS=" -T /var/log/xen/xenstored-trace.log"
 	xenconsoled --pid-file=$XENCONSOLED_PIDFILE $XENCONSOLED_ARGS
 	test -z "$XENBACKENDD_DEBUG" || XENBACKENDD_ARGS="-d"
@@ -54,12 +58,14 @@
 }
 do_stop () {
 	if read 2>/dev/null <$XENCONSOLED_PIDFILE pid; then
+	        echo Stopping xenconsoled
 		kill $pid
 		while kill -9 $pid >/dev/null 2>&1; do sleep 0.1; done
 		rm -f $XENCONSOLED_PIDFILE
 	fi
 
 	if read 2>/dev/null <$XENSTORED_PIDFILE pid; then
+	        echo Stopping xenstored
 		kill $pid
 		while kill -9 $pid >/dev/null 2>&1; do sleep 0.1; done
 		rm -f $XENSTORED_PIDFILE

Re: [PATCH 2 of 3] xencommons: Make init script more verbose

[Ian Jackson]

George Dunlap writes ("[Xen-devel] [PATCH 2 of 3] xencommons: Make init script more verbose"):
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

This would be great except that it doesn't apply to xen-unstable
because it depends on your previous patch ...

Thanks,
Ian.

Re: [PATCH 2 of 3] xencommons: Make init script more verbose

[George Dunlap]

On 28/06/10 17:09, Ian Jackson wrote:
> This would be great except that it doesn't apply to xen-unstable
> because it depends on your previous patch ...

That wasn't the bit of the patch series I expected be controversial. :-) 
  I'll submit a new patch when I put the timeout in the wait loop.

  -George

[PATCH 3 of 3] xencommons: Wait for xenstored to start before setting dom0 name

[George Dunlap]

On one of my boxes, the xenstore-write setting dom0's name starts
before xenstored is actually ready to handle the connection properly,
resulting in the name set failing.  Wait for xenstored to be up and
responding to reads before continuing.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 9781a01f6ccb -r 60688cd86c83 tools/hotplug/Linux/init.d/xencommons
--- a/tools/hotplug/Linux/init.d/xencommons	Mon Jun 28 13:24:26 2010 +0100
+++ b/tools/hotplug/Linux/init.d/xencommons	Mon Jun 28 13:24:26 2010 +0100
@@ -44,8 +44,15 @@
 		rm -f "$XENSTORED_ROOTDIR"/tdb* &>/dev/null
 		test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
 
-		echo Starting xenstored...
+		echo -n Starting xenstored...
 		xenstored --pid-file=$XENSTORED_PIDFILE $XENSTORED_ARGS
+
+                while ! `xenstore-read -s / >/dev/null 2>&1` ; do
+                        echo -n .
+                        sleep 1
+                done
+		echo
+
 		echo Setting domain 0 name...
 		xenstore-write "/local/domain/0/name" "Domain-0"
 	fi

Re: [PATCH 3 of 3] xencommons: Wait for xenstored to start before setting dom0 name

[Ian Jackson]

George Dunlap writes ("[Xen-devel] [PATCH 3 of 3] xencommons: Wait for xenstored to start before setting dom0 name"):
> On one of my boxes, the xenstore-write setting dom0's name starts
> before xenstored is actually ready to handle the connection properly,
> resulting in the name set failing.  Wait for xenstored to be up and
> responding to reads before continuing.

This is a good plan but it should really have a timeout of some kind.
Otherwise if xenstored fails to start for any reason, the boot will
hang at this point (which will seriously irritate the admin).

Ian.

Re: [PATCH 3 of 3] xencommons: Wait for xenstored to start before setting dom0 name

[George Dunlap]

On Mon, Jun 28, 2010 at 5:01 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> This is a good plan but it should really have a timeout of some kind.
> Otherwise if xenstored fails to start for any reason, the boot will
> hang at this point (which will seriously irritate the admin).

Hmm, good point.  I'll see what I can do.

 -George