xencommons init script improvements

PATCH New George Dunlap 7 participants 19 messages Jun 28, 2010

Share & tools

George Dunlap

author

2010-06-28 12:58 UTC #0 lore raw

PATCH 1/1 xencommons init script improvements

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

George Dunlap

author

2010-06-28 12:58 UTC #1 lore raw

PATCH 1/1 xencommons: Kill xenstored when stopping xencommons

Signed-off-by George Dunlap <[email protected]>

tools/hotplug/Linux/init.d/xencommons +8 -1

diff -r bf64e1081333 -r e71645625934 tools/hotplug/Linux/init.d/xencommons
@@ -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

George Dunlap

author

2010-06-28 12:58 UTC #13 lore raw

PATCH 1/1 xencommons: Make init script more verbose

In reply to George Dunlap

Signed-off-by George Dunlap <[email protected]>

tools/hotplug/Linux/init.d/xencommons +6 -0

diff -r e71645625934 -r 9781a01f6ccb tools/hotplug/Linux/init.d/xencommons
@@ -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

George Dunlap

author

2010-06-28 12:58 UTC #16 lore raw

PATCH 1/1 xencommons: Wait for xenstored to start before setting dom0 name

In reply to 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 <[email protected]>

tools/hotplug/Linux/init.d/xencommons +8 -1

diff -r 9781a01f6ccb -r 60688cd86c83 tools/hotplug/Linux/init.d/xencommons
@@ -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

Stefano Stabellini

2010-06-28 13:08 UTC #2 lore raw

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

In reply to George Dunlap

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

Quoted 41 lines · George Dunlap

Signed-off-by George Dunlap <[email protected]>

diff -r bf64e1081333 -r e71645625934 tools/hotplug/Linux/init.d/xencommons

tools/hotplug/Linux/init.d/xencommons +8 -1

@@ -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
[email protected]
http://lists.xensource.com/xen-devel

George Dunlap

author

2010-06-28 13:10 UTC #3 lore raw

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

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

Quoted 46 lines · Stefano Stabellini

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

Quoted 41 lines · George Dunlap

Signed-off-by George Dunlap<[email protected]>

diff -r bf64e1081333 -r e71645625934 tools/hotplug/Linux/init.d/xencommons

tools/hotplug/Linux/init.d/xencommons +8 -1

@@ -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
[email protected]
http://lists.xensource.com/xen-devel

Ian Campbell

2010-06-28 13:12 UTC #5 lore raw

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

In reply to George Dunlap

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.

Quoted 40 lines · George Dunlap

Signed-off-by George Dunlap <[email protected]>

diff -r bf64e1081333 -r e71645625934 tools/hotplug/Linux/init.d/xencommons

tools/hotplug/Linux/init.d/xencommons +8 -1

@@ -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
[email protected]
http://lists.xensource.com/xen-devel

Ian Campbell

2010-06-28 13:23 UTC #4 lore raw

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

In reply to George Dunlap

Quoted 1 line · George Dunlap

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

Quoted 3 lines

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.

Quoted 56 lines

-George
Quoted 39 lines · Stefano Stabellini
xenstored doesn't stop correctly.
There is no point in stopping xenstored if we are going to break the host.
Quoted 34 lines · George Dunlap
Signed-off-by George Dunlap<[email protected]>
diff -r bf64e1081333 -r e71645625934 tools/hotplug/Linux/init.d/xencommons
tools/hotplug/Linux/init.d/xencommons +8 -1
@@ -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
  }
 
Quoted 7 lines
Quoted 7 lines
tools/hotplug/Linux/init.d/xencommons +0 -0
  case "$1" in
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Ian Campbell

2010-06-28 13:48 UTC #7 lore raw

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

In reply to Vincent Hanquez

Quoted 6 lines · Vincent Hanquez

Quoted 3 lines · 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.
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.

Keir Fraser

2010-06-28 13:50 UTC #11 lore raw

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

In reply to Vincent Hanquez

Quoted 6 lines · Vincent Hanquez

Quoted 3 lines · 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.
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

Vincent Hanquez

2010-06-28 13:51 UTC #6 lore raw

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

In reply to Ian Campbell

Quoted 3 lines · 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.

C xenstored cannot be stopped, Ocaml xenstored **can* be stopped. shouldn't everyone be running ocaml xenstored nowadays ?

Signature · 1 line

Vincent

Vincent Hanquez

2010-06-28 13:56 UTC #8 lore raw

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

In reply to Ian Campbell

Quoted 6 lines

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.

Signature · 1 line

Vincent

Ian Campbell

2010-06-28 14:01 UTC #9 lore raw

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

Quoted 9 lines · Vincent Hanquez

Quoted 6 lines
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.

Vincent Hanquez

2010-06-28 14:16 UTC #12 lore raw

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

In reply to Keir Fraser

Quoted 5 lines

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.

Signature · 1 line

Vincent

Vincent Hanquez

2010-06-28 14:23 UTC #10 lore raw

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

In reply to Ian Campbell

Quoted 8 lines

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.

Quoted 7 lines

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.

Signature · 1 line

Vincent

Ian Jackson

2010-06-28 16:01 UTC #17 lore raw

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

In reply to George Dunlap

George Dunlap writes ("[Xen-devel] [PATCH 3 of 3] xencommons: Wait for xenstored to start before setting dom0 name"):

Quoted 4 lines

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.

Ian Jackson

2010-06-28 16:09 UTC #14 lore raw

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

In reply to George Dunlap

George Dunlap writes ("[Xen-devel] [PATCH 2 of 3] xencommons: Make init script more verbose"):

Quoted 1 line

Signed-off-by George Dunlap <[email protected]>

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

Thanks,
Ian.

George Dunlap

author

2010-06-28 16:18 UTC #18 lore raw

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

In reply to Ian Jackson

Quoted 3 lines · Ian Jackson

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

George Dunlap

author

2010-06-28 16:30 UTC #15 lore raw

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

In reply to Ian Jackson

Quoted 2 lines · Ian Jackson

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