Merge lp:~jseutter/landscape-client/record-playback into lp:~landscape/landscape-client/trunk

[Jerry Seutter]

With this change, landscape-client can now record the information that was sent from the client broker to landscape server. It adds --record and --record-dir command line options to the client broker.

I created a PayloadRecorder class that persists information to disk. I could not figure out how mocker could stub out the os module for unit testing, so some of the methods are not tested. Suggestions on how to improve the unit tests would be appreciated.

There are several lintian-triggered changes in the diff as well.

[lp:~jseutter/landscape-client/record-playback]

343. By Jerry Seutter on 2011-06-24

Cleanups.

[Alberto Donato (ack)]

+0. The branch is ok, but I'd like to see a few changes:

[1]

+ parser.add_option("--record", action="store_true",
+ help="Record data sent to the server on disk (see "
+ "--record-dir).")

Rather than "on disk" I would use something like "on filesystem".

[2]

+ if self.last_payload_time == payload_time:
+ payload_time = payload_time + .001

Can you please use 0.001 here?

[3]

+ self._time = time.time
+ self._time_offset = self._time()

and

+ payload_time = self._time() - self._time_offset

AFAICT saving the reference to the time function is only needed for tests.
self._time should be dropped.

You can use the functionalities provided by Mocker to override the time() function.

Something like this should do:

    def test_get_payload_filename(self):
        """
        L{PayloadRecorder.get_payload_filename} should return a filename that
        is equal to the number of seconds since it was created.
        """
        mock = self.mocker.replace("time.time")
        mock()
        self.mocker.result(0.0)
        mock()
        self.mocker.result(12.345)
        self.mocker.count(1)
        self.mocker.replay()

        recorder = PayloadRecorder(True, None)

        payload_name = recorder.get_payload_filename()
        self.assertEquals("12.345", payload_name)

Similarly for the other related test.

Thanks!

review: Needs Fixing

[lp:~jseutter/landscape-client/record-playback]

344. By Jerry Seutter on 2011-06-27

- Implemented fixes suggested by ack.
- Improved unit test coverage.

[Jerry Seutter (jseutter)]

Thanks ack. 1, 2 and 3 are fixed. Thank you for the pointers on mocker. The unit tests are much more thorough now.

[Free Ekanayaka (free.ekanayaka)]

Hi Jerry, looks good, however please use mocking only for time.time.

[1]

+ parser.add_option("--record-dir", metavar="record_dir",
+ help="Directory used to store server exchanges ("
+ "use with --record).")

Is there some use case where we want to set this to a particular directory? If not, I'd drop the option and go for something like:

class BrokerConfiguration(Configuration):

    @property
    def record_directory(self):
        return os.path.join(self.data_path, "exchanges")

which will return /var/lib/landscape/client/exchanges by default.

[2]

+ if self._payload_recorder:
+ self._payload_recorder.save(spayload)

Please check against None explicitly:

        if self._payload_recorder is not None:

[3]

+ file(self.destination_dir + '/' + payload_name, 'w').write(payload)

You can use landscape.lib.create_file instead, which closes the file description explicitly, also please use os.path.join:

+ create_file(os.path.join(self.destination_dir, payload_name), payload)

[3]

+ if self.recording and self.destination_dir != None:

and

+ if self.recording:

Instead of duplicating the check for self.recording, I think you could drop self.recording entirely and delegate the decision upper in the stack in landscape.broker.service.BrokerService.__init__:

[4]

+ self.persist = Persist(filename=self.persist_filename)

This change doesn't seem to be needed (in landscape/broker/tests/test_store.py)

[5]

- self.assertEquals(r.request.received_headers["x-computer-id"], "34")
+ self.assertEquals(r.request.received_headers["x-computer-id"],
+ "34")

Instead of breaking the line, you could make lint happy by s/assertEquals/assertEqual/, as assertEquals is also deprecated. (there are couple of this)

[6]

+class PayloadRecorderTest(MockerTestCase):
+ def test_get_payload_filename(self):

Please put a blank line between the class and the test.

[7]

+ def __init__(self, url, pubkey=None, payload_recorder=None):

Please add a @param entry of the new parameter in the class docstring.

[8]

+ def __init__(self, recording, destination_dir):

Please add a @param for these two parameters in the class docstring.

[9]

+ self.destination_dir = destination_dir
+ self.last_payload_time = -1
+ self.recording = recording

It seems none of these attributes needs to be exposed by the PayloadRecorder interface, I'd suggest making them private.

[10]

+ for file in os.listdir(self.destination_dir):
+ if os.path.isfile(self.destination_dir + '/' + file):
+ os.unlink(self.destination_dir + '/' + file)

Please use os.path.join, and a temp variable to avoid duplication. Also I'd recommend s/file/filename/ as 'file' overrides Python's builtin type :

+ for filename in os.listdir(self.destination_dir):
+ path = os.path.join(self.destination_dir, filename)
+ if os.path.isfile(path):
+ os.unlink(path)

[11]

+ recorder = PayloadRecorder(True, "./tmp")

You can use self.makeDir() instead, it...

review: Needs Fixing

[Alberto Donato (ack)]

Thanks for taking care of comments.

review: Approve

[lp:~jseutter/landscape-client/record-playback]

345. By Jerry Seutter on 2011-06-28

Changes based on review feedback.

[Jerry Seutter (jseutter)]

Hi Free,

1, 2, 3, 3(2), 4 are fixed.

> [5]
>
> - self.assertEquals(r.request.received_headers["x-computer-id"],
> "34")
> + self.assertEquals(r.request.received_headers["x-computer-id"],
> + "34")
>
> Instead of breaking the line, you could make lint happy by
> s/assertEquals/assertEqual/, as assertEquals is also deprecated. (there are
> couple of this)

assertEquals is used in several places. I will create a separate bug for this.

6, 7 are fixed.

> [8]
>
> + def __init__(self, recording, destination_dir):
>
> Please add a @param for these two parameters in the class docstring.

Are you sure? These are documented in the docstring for the method.

I will create a separate bug to fix this in other classes as well as this one.

9, 10, 11, 12, 13 are fixed.

[lp:~jseutter/landscape-client/record-playback]

346. By Jerry Seutter on 2011-06-28

Making docstring match code.

347. By Jerry Seutter on 2011-06-28

Fixing unit tests.

[Free Ekanayaka (free.ekanayaka)]

Nice work, +1!

[14]

+ def record_directory(self):

Please make it a @property for consistency with other similar methods in the same class, like exchange_store_path and message_store_path. Maybe also rename it to record_path, also for consistency.

[15]

+ if config.record is not None:
+ self.payload_recorder = PayloadRecorder(config.record_dir)
+ else:
+ self.payload_recorder = None

Well done, moving this conditional up the stack looks better :)

[16]

+ self.assertEquals("payload data", file(file_path).read())

You can use landscape.lib.fs.read_file, which will also close the file descriptor.

[17]

+ @param payload_recorder: PayloadRecorder used for recording exchanges
+ with the server.

I'd append a "If C{None}, exchanges won't be recorded at all."

[18]

+ if self._payload_recorder is not None:
+ self._payload_recorder.save(spayload)

This isn't unit-tested, afaics. It'd be good add a test to landscape.broker.tests.test_transport.HTTPTransportTest, passing a non-None PayloadRecorder to the HTTPTransport constructor, running an exchange and asserting that the payload gets saved.

review: Approve

[Jerry Seutter (jseutter)]

> Nice work, +1!
>
> [14]
>
> + def record_directory(self):
>
> Please make it a @property for consistency with other similar methods in the
> same class, like exchange_store_path and message_store_path. Maybe also rename
> it to record_path, also for consistency.

Oops. Fixed.

> [16]
>
> + self.assertEquals("payload data", file(file_path).read())
>
> You can use landscape.lib.fs.read_file, which will also close the file
> descriptor.

Done.

> [17]
>
> + @param payload_recorder: PayloadRecorder used for recording exchanges
> + with the server.
>
> I'd append a "If C{None}, exchanges won't be recorded at all."

Good point. Added.

> [18]
>
> + if self._payload_recorder is not None:
> + self._payload_recorder.save(spayload)
>
> This isn't unit-tested, afaics. It'd be good add a test to
> landscape.broker.tests.test_transport.HTTPTransportTest, passing a non-None
> PayloadRecorder to the HTTPTransport constructor, running an exchange and
> asserting that the payload gets saved.

I added a test when payload_recorder is defined and a second test without payloa
d_recorder defined.

[lp:~jseutter/landscape-client/record-playback]

348. By Jerry Seutter on 2011-06-29

Addressing review comments.

=== modified file 'landscape/broker/config.py'
--- landscape/broker/config.py	2011-01-12 16:30:14 +0000
+++ landscape/broker/config.py	2011-06-29 17:53:31 +0000
@@ -22,6 +22,10 @@
     def exchange_store_path(self):
         return os.path.join(self.data_path, "exchange.database")
 
+    @property
+    def record_directory(self):
+        return os.path.join(self.data_path, "exchanges")
+
     def make_parser(self):
         """Parser factory for broker-specific options.
 
@@ -39,6 +43,7 @@
               - C{https_proxy}
               - C{cloud}
               - C{otp}
+              - C{record}
         """
         parser = super(BrokerConfiguration, self).make_parser()
 
@@ -76,6 +81,8 @@
         parser.add_option("--tags",
                           help="Comma separated list of tag names to be sent "
                                "to the server.")
+        parser.add_option("--record", action="store_true",
+                          help="Record data sent to the server on filesystem ")
         return parser
 
     @property

=== modified file 'landscape/broker/service.py'
--- landscape/broker/service.py	2011-03-25 08:42:48 +0000
+++ landscape/broker/service.py	2011-06-29 17:53:31 +0000
@@ -5,7 +5,7 @@
 from landscape.service import LandscapeService, run_landscape_service
 from landscape.broker.registration import RegistrationHandler, Identity
 from landscape.broker.config import BrokerConfiguration
-from landscape.broker.transport import HTTPTransport
+from landscape.broker.transport import HTTPTransport, PayloadRecorder
 from landscape.broker.exchange import MessageExchange
 from landscape.broker.exchangestore import ExchangeStore
 from landscape.broker.ping import Pinger
@@ -48,8 +48,13 @@
         self.persist_filename = os.path.join(
             config.data_path, "%s.bpickle" % (self.service_name,))
         super(BrokerService, self).__init__(config)
-        self.transport = self.transport_factory(config.url,
-                                                config.ssl_public_key)
+
+        if config.record is not None:
+            self.payload_recorder = PayloadRecorder(config.record_directory)
+        else:
+            self.payload_recorder = None
+        self.transport = self.transport_factory(
+            config.url, config.ssl_public_key, self.payload_recorder)
         self.message_store = get_default_message_store(
             self.persist, config.message_store_path)
         self.identity = Identity(self.config, self.persist)

=== modified file 'landscape/broker/tests/test_store.py'
--- landscape/broker/tests/test_store.py	2011-06-16 20:05:54 +0000
+++ landscape/broker/tests/test_store.py	2011-06-29 17:53:31 +0000
@@ -4,7 +4,8 @@
 
 from landscape.lib.persist import Persist
 from landscape.broker.store import MessageStore
-from landscape.schema import InvalidError, Message, Int, String, UnicodeOrString
+from landscape.schema import (
+    InvalidError, Message, Int, String, UnicodeOrString)
 
 from landscape.tests.helpers import LandscapeTest
 from landscape.tests.mocker import ANY
@@ -22,7 +23,8 @@
 
     def create_store(self):
         persist = Persist(filename=self.persist_filename)
-        store = MessageStore(persist, self.temp_dir, 20, get_time=self.get_time)
+        store = MessageStore(persist, self.temp_dir, 20,
+                             get_time=self.get_time)
         store.set_accepted_types(["empty", "data"])
         store.add_schema(Message("empty", {}))
         store.add_schema(Message("empty2", {}))
@@ -156,20 +158,23 @@
 
     def test_unaccepted(self):
         for i in range(10):
-            self.store.add(dict(type=["data", "unaccepted"][i%2], data=str(i)))
+            self.store.add(dict(type=["data", "unaccepted"][i % 2],
+                                data=str(i)))
         il = [m["data"] for m in self.store.get_pending_messages(20)]
         self.assertEquals(il, map(str, [0, 2, 4, 6, 8]))
 
     def test_unaccepted_with_offset(self):
         for i in range(10):
-            self.store.add(dict(type=["data", "unaccepted"][i%2], data=str(i)))
+            self.store.add(dict(type=["data", "unaccepted"][i % 2],
+                                data=str(i)))
         self.store.set_pending_offset(2)
         il = [m["data"] for m in self.store.get_pending_messages(20)]
         self.assertEquals(il, map(str, [4, 6, 8]))
 
     def test_unaccepted_reaccepted(self):
         for i in range(10):
-            self.store.add(dict(type=["data", "unaccepted"][i%2], data=str(i)))
+            self.store.add(dict(type=["data", "unaccepted"][i % 2],
+                                data=str(i)))
         self.store.set_pending_offset(2)
         il = [m["data"] for m in self.store.get_pending_messages(2)]
         self.store.set_accepted_types(["data", "unaccepted"])
@@ -178,7 +183,8 @@
 
     def test_accepted_unaccepted(self):
         for i in range(10):
-            self.store.add(dict(type=["data", "unaccepted"][i%2], data=str(i)))
+            self.store.add(dict(type=["data", "unaccepted"][i % 2],
+                                data=str(i)))
         # Setting pending offset here means that the first two
         # messages, even though becoming unaccepted now, were already
         # accepted before, so they shouldn't be marked for hold.
@@ -192,7 +198,8 @@
 
     def test_accepted_unaccepted_old(self):
         for i in range(10):
-            self.store.add(dict(type=["data", "unaccepted"][i%2], data=str(i)))
+            self.store.add(dict(type=["data", "unaccepted"][i % 2],
+                                data=str(i)))
         self.store.set_pending_offset(2)
         self.store.set_accepted_types(["unaccepted"])
         il = [m["data"] for m in self.store.get_pending_messages(20)]
@@ -323,7 +330,6 @@
         self.assertRaises(InvalidError,
                           self.store.add, {"type": "data", "data": 3})
 
-
     def test_coercion_ignores_custom_api(self):
         """
         If a custom 'api' key is specified in the message, it should

=== modified file 'landscape/broker/tests/test_transport.py'
--- landscape/broker/tests/test_transport.py	2009-03-16 22:10:37 +0000
+++ landscape/broker/tests/test_transport.py	2011-06-29 17:53:31 +0000
@@ -1,11 +1,13 @@
 import os
 
 from landscape import VERSION
-from landscape.broker.transport import HTTPTransport
+from landscape.broker.transport import HTTPTransport, PayloadRecorder
 from landscape.lib.fetch import PyCurlError
+from landscape.lib.fs import create_file, read_file
 from landscape.lib import bpickle
 
-from landscape.tests.helpers import LandscapeTest, LogKeeperHelper
+from landscape.tests.helpers import (
+    LandscapeTest, LogKeeperHelper, MockerTestCase)
 
 from twisted.web import server, resource
 from twisted.internet import reactor
@@ -25,6 +27,7 @@
 
 class DataCollectingResource(resource.Resource):
     request = content = None
+
     def getChild(self, request, name):
         return self
 
@@ -67,13 +70,14 @@
         r = DataCollectingResource()
         port = reactor.listenTCP(0, server.Site(r), interface="127.0.0.1")
         self.ports.append(port)
-        transport = HTTPTransport("http://localhost:%d/"
-                                  % (port.getHost().port,))
+        transport = HTTPTransport(
+            "http://localhost:%d/" % (port.getHost().port,))
         result = deferToThread(transport.exchange, "HI", computer_id="34",
                                message_api="X.Y")
 
         def got_result(ignored):
-            self.assertEquals(r.request.received_headers["x-computer-id"], "34")
+            self.assertEquals(r.request.received_headers["x-computer-id"],
+                              "34")
             self.assertEquals(r.request.received_headers["user-agent"],
                               "landscape-client/%s" % (VERSION,))
             self.assertEquals(r.request.received_headers["x-message-api"],
@@ -89,19 +93,18 @@
         public key specified.
         """
         r = DataCollectingResource()
-        context_factory = DefaultOpenSSLContextFactory(PRIVKEY,
-                                                       PUBKEY)
+        context_factory = DefaultOpenSSLContextFactory(PRIVKEY, PUBKEY)
         port = reactor.listenSSL(0, server.Site(r), context_factory,
                                  interface="127.0.0.1")
         self.ports.append(port)
-        transport = HTTPTransport("https://localhost:%d/"
-                                  % (port.getHost().port,),
-                                  pubkey=PUBKEY)
+        transport = HTTPTransport(
+            "https://localhost:%d/" % (port.getHost().port,), PUBKEY)
         result = deferToThread(transport.exchange, "HI", computer_id="34",
                                message_api="X.Y")
 
         def got_result(ignored):
-            self.assertEquals(r.request.received_headers["x-computer-id"], "34")
+            self.assertEquals(r.request.received_headers["x-computer-id"],
+                              "34")
             self.assertEquals(r.request.received_headers["user-agent"],
                               "landscape-client/%s" % (VERSION,))
             self.assertEquals(r.request.received_headers["x-message-api"],
@@ -110,7 +113,6 @@
         result.addCallback(got_result)
         return result
 
-
     def test_ssl_verification_negative(self):
         """
         If the SSL server provides a key which is not verified by the
@@ -130,6 +132,7 @@
 
         result = deferToThread(transport.exchange, "HI", computer_id="34",
                                message_api="X.Y")
+
         def got_result(ignored):
             self.assertEquals(r.request, None)
             self.assertEquals(r.content, None)
@@ -137,3 +140,127 @@
                             in self.logfile.getvalue())
         result.addCallback(got_result)
         return result
+
+    def test_payload_recording_works(self):
+        """
+        When C{HTTPTransport} is configured with a payload recorder, exchanges
+        with the server should be saved to the filesystem.
+        """
+        path = self.makeDir()
+        recorder = PayloadRecorder(path)
+
+        def static_filename():
+            return "filename"
+        recorder.get_payload_filename = static_filename
+
+        transport = HTTPTransport("http://localhost",
+                                  payload_recorder=recorder)
+
+        def fake_curl(param1, param2, param3):
+            """Stub out the curl network call."""
+            class Curly(object):
+                def getinfo(self, param1):
+                    return 200
+            return (Curly(), bpickle.dumps("pay load response"))
+        transport._curl = fake_curl
+
+        transport.exchange("pay load")
+
+        file_path = os.path.join(path, static_filename())
+        self.assertEquals("pay load", bpickle.loads(read_file(file_path)))
+
+    def test_exchange_works_without_payload_recording(self):
+        """
+        When C{HTTPTransport} is configured without a payload recorder,
+        exchanges with the server should still complete.
+        """
+        transport = HTTPTransport("http://localhost")
+        self.called = False
+                
+        def fake_curl(param1, param2, param3):
+            """Stub out the curl network call."""
+            self.called = True
+            class Curly(object):
+                def getinfo(self, param1):
+                    return 200
+            return (Curly(), bpickle.dumps("pay load response"))
+        transport._curl = fake_curl
+
+        transport.exchange("pay load")
+
+        self.assertTrue(self.called)
+
+class PayloadRecorderTest(MockerTestCase):
+
+    def test_get_payload_filename(self):
+        """
+        L{PayloadRecorder.get_payload_filename} should return a filename that
+        is equal to the number of seconds since it was created.
+        """
+        mock = self.mocker.replace("time.time")
+        mock()
+        self.mocker.result(0.0)
+        mock()
+        self.mocker.result(12.3456)
+        self.mocker.replay()
+        recorder = PayloadRecorder(None)
+
+        payload_name = recorder.get_payload_filename()
+
+        self.assertEquals("12.346", payload_name)
+
+    def test_get_payload_filename_no_duplicates(self):
+        """
+        L{PayloadRecorder.get_payload_filename} should not generate duplicate
+        payload names.
+        """
+        mock = self.mocker.replace("time.time")
+        mock()
+        self.mocker.result(0.0)
+        mock()
+        self.mocker.result(12.345)
+        mock()
+        self.mocker.result(12.345)
+        self.mocker.replay()
+
+        recorder = PayloadRecorder(None)
+
+        payload_name_1 = recorder.get_payload_filename()
+        payload_name_2 = recorder.get_payload_filename()
+
+        self.assertEquals("12.345", payload_name_1)
+        self.assertEquals("12.346", payload_name_2)
+
+    def test_save(self):
+        """L{PayloadRecorder.save} should save the payload to the filesystem.
+        """
+        path = self.makeDir()
+        recorder = PayloadRecorder(path)
+
+        def static_filename():
+            return "filename"
+        recorder.get_payload_filename = static_filename
+        recorder.save("payload data")
+        file_path = os.path.join(path, static_filename())
+        self.assertEquals("payload data", read_file(file_path))
+
+    def test_create_destination_dir(self):
+        """
+        L{PayloadRecorder} should create the destination directory if it does
+        not exist.
+        """
+        path = self.makeDir()
+        os.rmdir(path)
+        PayloadRecorder(path)
+        self.assertTrue(os.path.isdir(path))
+
+    def test_delete_old_payloads(self):
+        """
+        L{PayloadRecorder} should remove all files from the destination
+        directory before writing new files.
+        """
+        path = self.makeDir()
+        create_file(os.path.join(path, "one"), "one")
+        create_file(os.path.join(path, "two"), "two")
+        PayloadRecorder(path)
+        self.assertEquals([], os.listdir(path))

=== modified file 'landscape/broker/transport.py'
--- landscape/broker/transport.py	2010-09-09 16:10:07 +0000
+++ landscape/broker/transport.py	2011-06-29 17:53:31 +0000
@@ -1,4 +1,5 @@
 """Low-level server communication."""
+import os
 import time
 import logging
 import pprint
@@ -6,6 +7,7 @@
 import pycurl
 
 from landscape.lib.fetch import fetch
+from landscape.lib.fs import create_file
 from landscape.lib import bpickle
 from landscape.log import format_delta
 from landscape import SERVER_API, VERSION
@@ -14,13 +16,16 @@
 class HTTPTransport(object):
     """Transport makes a request to exchange message data over HTTP."""
 
-    def __init__(self, url, pubkey=None):
+    def __init__(self, url, pubkey=None, payload_recorder=None):
         """
         @param url: URL of the remote Landscape server message system.
         @param pubkey: SSH public key used for secure communication.
+        @param payload_recorder: PayloadRecorder used for recording exchanges
+            with the server.  If C{None}, exchanges will not be recorded.
         """
         self._url = url
         self._pubkey = pubkey
+        self._payload_recorder = payload_recorder
 
     def get_url(self):
         """Get the URL of the remote message system."""
@@ -31,9 +36,9 @@
         self._url = url
 
     def _curl(self, payload, computer_id, message_api):
-        headers= {"X-Message-API": message_api,
-                  "User-Agent": "landscape-client/%s" % VERSION,
-                  "Content-Type": "application/octet-stream"}
+        headers = {"X-Message-API": message_api,
+                   "User-Agent": "landscape-client/%s" % VERSION,
+                   "Content-Type": "application/octet-stream"}
         if computer_id:
             headers["X-Computer-ID"] = computer_id
         curl = pycurl.Curl()
@@ -55,6 +60,8 @@
 
         """
         spayload = bpickle.dumps(payload)
+        if self._payload_recorder is not None:
+            self._payload_recorder.save(spayload)
         try:
             start_time = time.time()
             if logging.getLogger().getEffectiveLevel() <= logging.DEBUG:
@@ -85,11 +92,70 @@
         return response
 
 
+class PayloadRecorder(object):
+    """
+    L{PayloadRecorder} records client exchanges with the server to disk for
+    later playback.
+
+    Exchange payloads will be stored one per file, where the file name is
+    the elapsed time since the client was started.
+    """
+
+    def __init__(self, destination_dir):
+        """
+        @param destination_dir - The directory to record exchanges in.
+        """
+        self._time_offset = time.time()
+        self._destination_dir = destination_dir
+        self._last_payload_time = -1
+        if self._destination_dir is not None:
+            self._create_destination_dir(self._destination_dir)
+            self._delete_old_payloads()
+
+    def _create_destination_dir(self, destination_dir):
+        """Create the destination directory if it does not exist.
+
+        @param destination_dir: The directory to be created.
+        """
+        if not os.path.exists(destination_dir):
+            os.mkdir(destination_dir)
+
+    def _delete_old_payloads(self):
+        """Delete payloads lying around from a previous session."""
+        for filename in os.listdir(self._destination_dir):
+            file_path = os.path.join(self._destination_dir, filename)
+            if os.path.isfile(file_path):
+                os.unlink(file_path)
+
+    def save(self, payload):
+        """Persist the given payload to disk.
+
+        @param payload: The payload to be persisted.
+        """
+        payload_name = self.get_payload_filename()
+        create_file(os.path.join(self._destination_dir, payload_name),
+                    payload)
+
+    def get_payload_filename(self):
+        """
+        Generate a payload filename.  This method ensures that payloads
+        will have a unique name.
+        """
+        payload_time = time.time() - self._time_offset
+        last_payload_time = '%.3f' % self._last_payload_time
+        this_payload_time = '%.3f' % payload_time
+        if last_payload_time == this_payload_time:
+            payload_time = payload_time + 0.001
+        self._last_payload_time = payload_time
+        return '%.3f' % payload_time
+
+
 class FakeTransport(object):
     """Fake transport for testing purposes."""
 
-    def __init__(self, url=None, pubkey=None):
+    def __init__(self, url=None, pubkey=None, payload_recorder=None):
         self._pubkey = pubkey
+        self._payload_recorder = payload_recorder
         self.payloads = []
         self.responses = []
         self._current_response = 0
@@ -101,7 +167,7 @@
 
     def get_url(self):
         return self._url
-    
+
     def set_url(self, url):
         self._url = url
 

=== modified file 'landscape/hal.py'
--- landscape/hal.py	2010-06-11 13:23:00 +0000
+++ landscape/hal.py	2011-06-29 17:53:31 +0000
@@ -21,7 +21,7 @@
         """Returns a list of HAL devices.
 
         @note: If it wasn't possible to connect to HAL over DBus, then an
-            emtpy list will be returned. This can happen if the HAL or DBus
+            empty list will be returned. This can happen if the HAL or DBus
             services are not running.
         """
         if not self._manager:

=== modified file 'landscape/lib/persist.py'
--- landscape/lib/persist.py	2009-04-02 10:25:04 +0000
+++ landscape/lib/persist.py	2011-06-29 17:53:31 +0000
@@ -18,7 +18,8 @@
 # along with this Python module; if not, write to the Free Software
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 #
-import sys, os
+import os
+import sys
 import copy
 import re
 
@@ -43,7 +44,7 @@
 
     """Persist a hierarchical database of key=>value pairs.
 
-    There are three different kinds of opition maps, regarding the
+    There are three different kinds of option maps, regarding the
     persistence and priority that maps are queried.
 
       - hard - Options are persistent.
@@ -113,7 +114,7 @@
         try:
             self._hardmap = self._backend.load(filepath)
         except:
-            filepathold = filepath+".old"
+            filepathold = filepath + ".old"
             if (os.path.isfile(filepathold) and
                 os.path.getsize(filepathold) > 0):
                 # warning("Broken configuration file at %s" % filepath)
@@ -139,7 +140,7 @@
             filepath = self.filename
         filepath = os.path.expanduser(filepath)
         if os.path.isfile(filepath):
-            os.rename(filepath, filepath+".old")
+            os.rename(filepath, filepath + ".old")
         dirname = os.path.dirname(filepath)
         if dirname and not os.path.isdir(dirname):
             os.makedirs(dirname)
@@ -263,7 +264,7 @@
             current = self._traverse(map, path)
             if type(current) is list and value in current:
                 return
-        path = path+(sys.maxint,)
+        path = path + (sys.maxint,)
         self._traverse(map, path, setvalue=value)
 
     def remove(self, path, value=NOTHING, soft=False, weak=False):
@@ -360,49 +361,50 @@
     def has(self, path, value=NOTHING, soft=False, hard=False, weak=False):
         if type(path) is str:
             path = path_string_to_tuple(path)
-        return self.parent.has(self.root+path, value, soft, hard, weak)
+        return self.parent.has(self.root + path, value, soft, hard, weak)
 
     def keys(self, path, soft=False, hard=False, weak=False):
         if type(path) is str:
             path = path_string_to_tuple(path)
-        return self.parent.keys(self.root+path, soft, hard, weak)
+        return self.parent.keys(self.root + path, soft, hard, weak)
 
     def get(self, path, default=None, soft=False, hard=False, weak=False):
         if type(path) is str:
             path = path_string_to_tuple(path)
-        return self.parent.get(self.root+path, default, soft, hard, weak)
+        return self.parent.get(self.root + path, default, soft, hard, weak)
 
     def set(self, path, value, soft=False, weak=False):
         if type(path) is str:
             path = path_string_to_tuple(path)
-        return self.parent.set(self.root+path, value, soft, weak)
+        return self.parent.set(self.root + path, value, soft, weak)
 
     def add(self, path, value, unique=False, soft=False, weak=False):
         if type(path) is str:
             path = path_string_to_tuple(path)
-        return self.parent.add(self.root+path, value, unique, soft, weak)
+        return self.parent.add(self.root + path, value, unique, soft, weak)
 
     def remove(self, path, value=NOTHING, soft=False, weak=False):
         if type(path) is str:
             path = path_string_to_tuple(path)
-        return self.parent.remove(self.root+path, value, soft, weak)
+        return self.parent.remove(self.root + path, value, soft, weak)
 
     def move(self, oldpath, newpath, soft=False, weak=False):
         if type(oldpath) is str:
             oldpath = path_string_to_tuple(oldpath)
         if type(newpath) is str:
             newpath = path_string_to_tuple(newpath)
-        return self.parent.move(self.root+oldpath, self.root+newpath,
+        return self.parent.move(self.root + oldpath, self.root + newpath,
                                 soft, weak)
 
     def root_at(self, path):
         if type(path) is str:
             path = path_string_to_tuple(path)
-        return self.parent.root_at(self.root+path)
+        return self.parent.root_at(self.root + path)
 
 
 _splitpath = re.compile(r"(\[-?\d+\])|(?<!\\)\.").split
 
+
 def path_string_to_tuple(path):
     if "." not in path and "[" not in path:
         return (path,)
@@ -610,4 +612,3 @@
         return Backend.copy(self, value)
 
 # vim:ts=4:sw=4:et
-

=== modified file 'landscape/package/taskhandler.py'
--- landscape/package/taskhandler.py	2010-04-20 08:59:44 +0000
+++ landscape/package/taskhandler.py	2011-06-29 17:53:31 +0000
@@ -132,7 +132,7 @@
         failure.trap(PackageTaskError)
 
     def handle_task(self, task):
-        """Handle a sigle task.
+        """Handle a single task.
 
         Sub-classes must override this method in order to trigger task-specific
         actions.

=== modified file 'scripts/landscape-dbus-proxy'
--- scripts/landscape-dbus-proxy	2010-07-09 14:51:01 +0000
+++ scripts/landscape-dbus-proxy	2011-06-29 17:53:31 +0000
@@ -33,7 +33,7 @@
 
 
 class BrokerDBusObject(Object):
-    """A DBus-published object proxing L{RemoteBroker.send_message}.
+    """A DBus-published object proxying L{RemoteBroker.send_message}.
 
     It is used when upgrading from a DBus-based version of the Landscape client
     to the newer AMP-based one, for letting the old package-changer process