Merge lp:~free.ekanayaka/landscape-client/socket-locks-fix into lp:~landscape/landscape-client/trunk

[Free Ekanayaka]

This branch fixes the client shutdown sequence, given Twisted the chance of unlinking sockets files and symlinks.

[lp:~free.ekanayaka/landscape-client/socket-locks-fix]

284. By Free Ekanayaka on 2010-08-11

Cleanup sockets in the init-script in case it's needed

285. By Free Ekanayaka on 2010-08-11

Use tabs for consistency

286. By Free Ekanayaka on 2010-08-11

Typo

[Björn Tillenius (bjornt)]

< BjornT> free: can you expand a bit on what effect removing.
          self.port.stopListening() does? is that what fixes the issue,.
          or is it lines 93-94, and the rest are just cosmetic changes?
< free> BjornT: two things fix the issues, removing.
        self.port.stopListening() and replacing reactor.crash() with.
        reactor.stop()
< free> BjornT: this makes the shutdown sequence compliant with what.
        Twisted requires
< free> BjornT: and cleans up the sockets
< BjornT> free: ok, sounds good. what about the addition to the.
          changelog? shouldn't it list what got fixed?
< free> BjornT: I just added it to bump the version of the builds in the.
        PPA, we will finalize it when we do the actual release with.
        everything that was changed
< BjornT> free: ok, i trust you :)
< BjornT> +1

review: Approve

[Gavin Panella (allenap)]

Nice, a few comments. :)

[1]

+ if ls $SOCKETDIR/* > /dev/null 2>&1; then
+ if ! pidofproc -p $PIDFILE $DAEMON > /dev/null; then
+ rm -f $SOCKETDIR/*
+ fi
+ fi

OMGMYEYES... I just looked at the definition of pidofproc in
/lib/lsb/init-functions and found at least one bug just from reading
it.

Back to the snippet. If SOCKETDIR was ever not set this would expand
to:

    if ls /* > /dev/null 2>&1; then
        if ! pidofproc -p $PIDFILE $DAEMON > /dev/null; then
            rm -f /*
        fi
    fi

Which could have quite a detrimental effect on a system! It may be
that set -u is in force, but it's probably better to make sure it
breaks anyway:

    if ls ${SOCKETDIR:?}/* > /dev/null 2>&1; then
        if ! pidofproc -p $PIDFILE $DAEMON > /dev/null; then
            rm -f ${SOCKETDIR:?}/*
        fi
    fi

Even then, if SOCKETDIR had a trailing space (don't know why, but it
pays to be defensive), this could expand to:

    if ls /socket/dir /* > /dev/null 2>&1; then
        if ! pidofproc -p $PIDFILE $DAEMON > /dev/null; then
            rm -f /socket/dir /*
        fi
    fi

So I think it's best to find a way to avoid globbing altogether here,
and quote arguments:

    if ! pidofproc -p "$PIDFILE" "$DAEMON" > /dev/null
    then
        find "${SOCKETDIR:?}" -print0 | xargs -r0 rm -f
    fi

Also, when using rm -f, I'd feel more comfortable if the focus was
narrowed to only delete sockets, to minimize collateral damage if
things go wrong:

    if ! pidofproc -p "$PIDFILE" "$DAEMON" > /dev/null
    then
        find "${SOCKETDIR:?}" -type s -print0 | xargs -r0 rm -f
    fi

Sorry for tearing into this code. Scripting with bash and its ilk is
so difficult to make safe. Init scripts should be written in Python :)

[2]

     def get_reactor(self):
- return TwistedReactor()
+ reactor = TwistedReactor()
+ # It's not possible to stop the reactor in a Trial test, calling
+ # reactor.crash instead
+ reactor._reactor.stop = reactor._reactor.crash

The last line above modifies the global reactor. That doesn't seem
like a good thing to do.

I've also been told by Jono Lange that stopping and starting the
reactor is not supported. He said something along the lines of: it
might work for you, but it's known to be problematic, it's not
supported, and you get to keep the pieces when it goes wrong.

In short, reactor.stop() and reactor.crash() should not be used except
as part of process tear-down.

This means that the whole of TwistedReactorTest is problematic because
it calls these methods on the real reactor. It works though, so what
do I know? :)

review: Needs Fixing

[Free Ekanayaka (free.ekanayaka)]

[1]

Wow, that was a nice analysis. Thanks for all the hints it was quite interesting, changed as you suggest and also added:

   find "${SOCKETDIR:?}" -type l -print0 | xargs -r0 rm -f

because we need to delete the symbolic links that Twisted uses to keep track of the PID that created the socket.

[2]

Yeah I know, the whole TwistedReactorTest is somehow unfortunate (actually I've been wondering if we need a TwistedReactor class at all and if we could in principle go with the straight Twisted reactor). Those tests have been there for a long time and they seem to work (read "pass"), so I don't think we need to touch them for now, or at least not in this branch.

As for patching the global reactor, good catch :) I've added an addCleanup:

        saved_stop = reactor._reactor.stop
        reactor._reactor.stop = reactor._reactor.crash
        self.addCleanup(lambda: setattr(reactor._reactor, "stop", saved_stop))
        return reactor

which I believe fixes the issue.

[lp:~free.ekanayaka/landscape-client/socket-locks-fix]

287. By Free Ekanayaka on 2010-08-12

Address Gavin's comments

288. By Free Ekanayaka on 2010-08-12

OMG, I'm tired

[Gavin Panella (allenap)]

Cool, +1 :)

[2]

Thanks for the explanation, and I agree they should be left alone for now.

> self.addCleanup(lambda: setattr(reactor._reactor, "stop", saved_stop))

Not that it matters a lot, but I think this can be simplified to:

  self.addCleanup(setattr, reactor._reactor, "stop", saved_stop)

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2010-07-28 06:30:31 +0000
+++ debian/changelog	2010-08-12 15:59:39 +0000
@@ -1,3 +1,9 @@
+landscape-client (1.5.5-0ubuntu0.10.10.0) UNRELEASED; urgency=low
+
+  * New upstream version
+
+ -- Free Ekanayaka <[e-mail]>  Wed, 11 Aug 2010 12:41:16 +0200
+
 landscape-client (1.5.4-0ubuntu0.10.10.0) maverick; urgency=low
 
   * New upstream version (LP: #610744):

=== modified file 'debian/landscape-client.init'
--- debian/landscape-client.init	2010-05-11 08:02:29 +0000
+++ debian/landscape-client.init	2010-08-12 15:59:39 +0000
@@ -15,6 +15,7 @@
 NAME=landscape-client
 DAEMON=/usr/bin/$NAME
 PIDDIR=/var/run/landscape
+SOCKETDIR=/var/lib/landscape/client/sockets
 PIDFILE=$PIDDIR/$NAME.pid
 RUN=0 # overridden in /etc/default/landscape-client
 CLOUD=0 # overridden in /etc/default/landscape-client
@@ -58,6 +59,15 @@
 			mkdir $PIDDIR
 			chown landscape $PIDDIR
 		fi
+		# Cleanup leftover sockets if there's no other landscape process
+		# running. This shouldn't be usually necessary, but it can
+		# happen in case the client crashed badly and the socket points
+		# to a process with the same PID.
+		if ! pidofproc -p "$PIDFILE" "$DAEMON" > /dev/null; then
+			find "${SOCKETDIR:?}" -type s -print0 | xargs -r0 rm -f
+			find "${SOCKETDIR:?}" -type l -print0 | xargs -r0 rm -f
+		fi
+
 		FULL_COMMAND="start-stop-daemon --start --quiet --oknodo --startas $DAEMON --pidfile $PIDFILE -g $DAEMON_GROUP -- --daemon --pid-file $PIDFILE"
 		if [ x"$DAEMON_USER" != x ]; then
 			sudo -u $DAEMON_USER $FULL_COMMAND

=== modified file 'landscape/broker/tests/test_service.py'
--- landscape/broker/tests/test_service.py	2010-04-23 14:42:21 +0000
+++ landscape/broker/tests/test_service.py	2010-08-12 15:59:39 +0000
@@ -94,4 +94,5 @@
         connected.addCallback(lambda remote: remote.get_server_uuid())
         connected.addCallback(lambda x: connector.disconnect())
         connected.addCallback(lambda x: self.service.stopService())
+        connected.addCallback(lambda x: self.service.port.stopListening())
         return connected

=== modified file 'landscape/manager/tests/test_service.py'
--- landscape/manager/tests/test_service.py	2010-04-19 13:24:13 +0000
+++ landscape/manager/tests/test_service.py	2010-08-12 15:59:39 +0000
@@ -54,7 +54,9 @@
             [connector] = self.broker_service.broker.get_connectors()
             connector.disconnect()
             self.service.stopService()
+            self.service.port.stopListening()
             self.broker_service.stopService()
+            self.broker_service.port.stopListening()
 
         def assert_broker_connection(ignored):
             self.assertEquals(len(self.broker_service.broker.get_clients()), 1)

=== modified file 'landscape/monitor/tests/test_service.py'
--- landscape/monitor/tests/test_service.py	2010-04-19 13:24:13 +0000
+++ landscape/monitor/tests/test_service.py	2010-08-12 15:59:39 +0000
@@ -57,7 +57,9 @@
             [connector] = self.broker_service.broker.get_connectors()
             connector.disconnect()
             self.service.stopService()
+            self.service.port.stopListening()
             self.broker_service.stopService()
+            self.broker_service.port.stopListening()
 
         def assert_broker_connection(ignored):
             self.assertEquals(len(self.broker_service.broker.get_clients()), 1)
@@ -82,3 +84,4 @@
         self.service.port.stopListening()
         self.mocker.replay()
         self.service.stopService()
+        self.service.port.stopListening()

=== modified file 'landscape/reactor.py'
--- landscape/reactor.py	2010-04-23 14:42:21 +0000
+++ landscape/reactor.py	2010-08-12 15:59:39 +0000
@@ -379,7 +379,7 @@
     def stop(self):
         """Stop the reactor, a C{"stop"} event will be fired."""
 
-        self._reactor.crash()
+        self._reactor.stop()
         self._cleanup()
 
     def time(self):

=== modified file 'landscape/service.py'
--- landscape/service.py	2010-05-03 13:08:02 +0000
+++ landscape/service.py	2010-08-12 15:59:39 +0000
@@ -45,7 +45,8 @@
         self.port = self.reactor.listen_unix(self.socket, self.factory)
 
     def stopService(self):
-        self.port.stopListening()
+        # We don't need to call port.stopListening(), because the reactor
+        # shutdown sequence will do that for us.
         Service.stopService(self)
         logging.info("%s stopped with config %s" % (
             self.service_name.capitalize(), self.config.get_config_filename()))

=== modified file 'landscape/tests/helpers.py'
--- landscape/tests/helpers.py	2010-07-30 13:05:59 +0000
+++ landscape/tests/helpers.py	2010-08-12 15:59:39 +0000
@@ -26,13 +26,9 @@
 from landscape.monitor.monitor import Monitor
 from landscape.manager.manager import Manager
 
-# FIXME: We can drop the "_" suffix and replace the current classes once the
-# AMP migration is completed
-from landscape.broker.service import BrokerService as BrokerService_
-from landscape.broker.amp import (
-    FakeRemoteBroker as FakeRemoteBroker_, RemoteBrokerConnector)
-from landscape.manager.config import (
-    ManagerConfiguration as ManagerConfiguration_)
+from landscape.broker.service import BrokerService
+from landscape.broker.amp import FakeRemoteBroker, RemoteBrokerConnector
+from landscape.manager.config import ManagerConfiguration
 
 
 DEFAULT_ACCEPTED_TYPES = [
@@ -284,12 +280,20 @@
         config = BrokerConfiguration()
         config.load(["-c", test_case.config_filename])
 
-        class FakeBrokerService(BrokerService_):
+        class FakeBrokerService(BrokerService):
             reactor_factory = FakeReactor
             transport_factory = FakeTransport
 
+            def stopService(service):
+                # We need to explictely stop listening to the socket
+                # because the reactor would still have active selectables
+                # at the end of the test otherwise
+                if os.path.exists(service.port.port):
+                    service.port.connectionLost(None)
+                super(FakeBrokerService, service).stopService()
+
         test_case.broker_service = FakeBrokerService(config)
-        test_case.remote = FakeRemoteBroker_(
+        test_case.remote = FakeRemoteBroker(
             test_case.broker_service.exchanger,
             test_case.broker_service.message_store)
 
@@ -351,7 +355,7 @@
 
     def set_up(self, test_case):
         super(ManagerHelper, self).set_up(test_case)
-        test_case.config = ManagerConfiguration_()
+        test_case.config = ManagerConfiguration()
         test_case.config.load(["-c", test_case.config_filename])
         test_case.reactor = FakeReactor()
         test_case.manager = Manager(test_case.reactor, test_case.config)

=== modified file 'landscape/tests/test_reactor.py'
--- landscape/tests/test_reactor.py	2010-04-23 14:42:21 +0000
+++ landscape/tests/test_reactor.py	2010-08-12 15:59:39 +0000
@@ -395,7 +395,13 @@
 class TwistedReactorTest(LandscapeTest, ReactorTestMixin):
 
     def get_reactor(self):
-        return TwistedReactor()
+        reactor = TwistedReactor()
+        # It's not possible to stop the reactor in a Trial test, calling
+        # reactor.crash instead
+        saved_stop = reactor._reactor.stop
+        reactor._reactor.stop = reactor._reactor.crash
+        self.addCleanup(lambda: setattr(reactor._reactor, "stop", saved_stop))
+        return reactor
 
     def test_real_time(self):
         reactor = self.get_reactor()

=== modified file 'landscape/tests/test_service.py'
--- landscape/tests/test_service.py	2010-05-03 13:08:02 +0000
+++ landscape/tests/test_service.py	2010-08-12 15:59:39 +0000
@@ -114,6 +114,7 @@
             self.assertTrue(service.port.connected)
             connector.disconnect()
             service.stopService()
+            return service.port.stopListening()
 
         connected = connector.connect()
         return connected.addCallback(assert_port)