Merge lp:~therve/landscape-client/detect-task-changes into lp:~landscape/landscape-client/trunk

[Thomas Herve]

OK, here's the change. I'm not sure it fixes everything that Andreas saw, but I believe it's a first step: what I observers is that the reporter was translation hash to ids, but then didn't declare those installed/available. Looking at the changes we made recently, it's clear it's related to the check we're making on the local package state. I added an additional state when we handle a task, it's bit clunky but it fixes the problem.

[Chris Glass (tribaal)]

Looks good! +1

[1]
+ touch_file(self.check_stamp_file)

I think this and the related assertTrue don't really test that the code detected changes, maybe something like the following would?

touch_file(self.check_stamp_file)
time_touched = os.stat(self.check_stamp_file).st_mtime

...

time_modified = os.stat(self.check_stamp_file).st_mtime
self.assertTrue(time_modified > time_touched)

review: Approve

[Christopher Armstrong (radix)]

Looks good.

review: Approve

=== modified file 'landscape/package/reporter.py'
--- landscape/package/reporter.py	2013-02-28 10:09:09 +0000
+++ landscape/package/reporter.py	2013-02-28 15:23:27 +0000
@@ -50,8 +50,11 @@
     apt_update_filename = "/usr/lib/landscape/apt-update"
     sources_list_filename = "/etc/apt/sources.list"
     sources_list_directory = "/etc/apt/sources.list.d"
+    _got_task = False
 
     def run(self):
+        self._got_task = False
+
         result = Deferred()
 
         result.addCallback(lambda x: self.run_apt_update())
@@ -251,8 +254,10 @@
     def handle_task(self, task):
         message = task.data
         if message["type"] == "package-ids":
+            self._got_task = True
             return self._handle_package_ids(message)
         if message["type"] == "resynchronize":
+            self._got_task = True
             return self._handle_resynchronize()
 
     def _handle_package_ids(self, message):
@@ -447,7 +452,7 @@
         Check if any information regarding packages have changed, and if so
         compute the changes and send a signal.
         """
-        if self._package_state_has_changed():
+        if self._got_task or self._package_state_has_changed():
             return self._compute_packages_changes()
         else:
             return succeed(None)
@@ -462,12 +467,12 @@
 
         @return True if the status changed, False otherwise.
         """
+        stamp_file = self._config.detect_package_changes_stamp
+        if not os.path.exists(stamp_file):
+            return True
+
         status_file = apt_pkg.config.find_file("dir::state::status")
         lists_dir = apt_pkg.config.find_dir("dir::state::lists")
-        stamp_file = self._config.detect_package_changes_stamp
-        if not os.path.exists(stamp_file):
-            return True
-
         files = [status_file, lists_dir]
         files.extend(glob.glob("%s/*Packages" % lists_dir))
 

=== modified file 'landscape/package/tests/test_reporter.py'
--- landscape/package/tests/test_reporter.py	2013-01-04 10:50:53 +0000
+++ landscape/package/tests/test_reporter.py	2013-02-28 15:23:27 +0000
@@ -4,7 +4,7 @@
 import time
 import apt_pkg
 
-from twisted.internet.defer import Deferred, succeed
+from twisted.internet.defer import Deferred, succeed, inlineCallbacks
 from twisted.internet import reactor
 
 
@@ -943,6 +943,31 @@
         result = self.reporter.detect_packages_changes()
         return result.addCallback(got_result)
 
+    @inlineCallbacks
+    def test_detect_packages_after_tasks(self):
+        """
+        When the L{PackageReporter} got a task to handle, it forces itself to
+        detect package changes, not checking the local state of package.
+        """
+        message_store = self.broker_service.message_store
+        message_store.set_accepted_types(["packages"])
+
+        self.store.set_hash_ids({HASH1: 1, HASH2: 2, HASH3: 3})
+
+        touch_file(self.check_stamp_file)
+
+        self.store.add_task("reporter",
+                            {"type": "package-ids", "ids": [123, 456],
+                             "request-id": 123})
+
+        yield self.reporter.handle_tasks()
+
+        yield self.reporter.detect_packages_changes()
+
+        self.assertMessages(message_store.get_pending_messages(),
+                            [{"type": "packages", "available": [(1, 3)]}])
+        self.assertTrue(os.path.exists(self.check_stamp_file))
+
     def test_detect_packages_changes_with_not_locked_and_ranges(self):
         """
         Ranges are used when reporting changes to 3 or more not locked packages