Merge lp:~therve/landscape-client/config-apt-update-interval into lp:~landscape/landscape-client/trunk

[Thomas Herve]

The branch adds 2 new parameters: one to change the package monitor interval, the other to reduce the apt update interval. I also change the default apt update interval to be 6h instead of 1, which seems more than enough for me.

[Jerry Seutter (jseutter)]

+1 looks good.

[1]
74 + parser.add_option("--package-monitor-interval", default=30 * 60,
75 + type="int",
76 + help="The interval between apt update runs "
77 + "(default 1800).")
78 + parser.add_option("--apt-update-interval", default=6 * 60 * 60,
79 + type="int",
80 + help="The interval between apt update runs "
81 + "(default 21600).")

The help text on package-monitor-interval is incorrect.

review: Approve

[Alberto Donato (ack)]

Looks good, +1!

#1:
you can use '%default' in the options help text to avoid duplicating the default value.

review: Approve

=== modified file 'example.conf'
--- example.conf	2012-09-27 18:47:34 +0000
+++ example.conf	2012-12-13 13:43:55 +0000
@@ -11,17 +11,17 @@
 # GENERAL OPTIONS
 
 # The directory in which landscape-client will store data files in.
-data_path = /var/lib/landscape/client/ 
+data_path = /var/lib/landscape/client/
 
 # If set to True, do not log to standard output.
 quiet = False
 
 # The directory in which to store log files.
-log_dir = /var/log/landscape/ 
+log_dir = /var/log/landscape/
 
 # The log level at which to log events.
 # Values can be one of: "debug", "info", "warning", "error", "critical"
-log_level = info 
+log_level = info
 
 # The main URL for the landscape server to connect this client to. If you
 # purchased a Landscape Dedicated Server (LDS), change this to point to your
@@ -32,7 +32,7 @@
 #
 # Example:
 #   url = https://landscape.example.com/message-system
-url = https://landscape.canonical.com/message-system 
+url = https://landscape.canonical.com/message-system
 
 # The ping url you want this client to report to.
 #
@@ -122,6 +122,12 @@
 # The number of seconds between pings.
 ping_interval = 30
 
+# The number of seconds between apt update calls.
+apt_update_interval = 21600
+
+# The number of seconds between package monitor runs.
+package_monitor_interval = 1800
+
 # The URL of the http proxy to use, if any.
 # This value is optional.
 #
@@ -138,7 +144,7 @@
 # registration.
 #
 # This has no default.
-otp
+#otp = ABCD1234
 
 # A comma-separated list of tags to attach to this computer.
 #

=== modified file 'landscape/deployment.py'
--- landscape/deployment.py	2012-10-15 08:17:35 +0000
+++ landscape/deployment.py	2012-12-13 13:43:55 +0000
@@ -217,8 +217,7 @@
         all_options.update(self._command_line_options)
         all_options.update(self._set_options)
         for name, value in all_options.items():
-            if (name != "config" and
-                name not in self.unsaved_options):
+            if name != "config" and name not in self.unsaved_options:
                 if value == self._command_line_defaults.get(name):
                     config_parser.remove_option(self.config_section, name)
                 else:
@@ -330,6 +329,14 @@
         parser.add_option("--ignore-sigusr1", action="store_true",
                           default=False, help="Ignore SIGUSR1 signal to "
                                               "rotate logs.")
+        parser.add_option("--package-monitor-interval", default=30 * 60,
+                          type="int",
+                          help="The interval between apt update runs "
+                               "(default 1800).")
+        parser.add_option("--apt-update-interval", default=6 * 60 * 60,
+                          type="int",
+                          help="The interval between apt update runs "
+                               "(default 21600).")
 
         # Hidden options, used for load-testing to run in-process clones
         parser.add_option("--clones", default=0, type=int, help=SUPPRESS_HELP)

=== modified file 'landscape/monitor/packagemonitor.py'
--- landscape/monitor/packagemonitor.py	2012-05-08 01:13:59 +0000
+++ landscape/monitor/packagemonitor.py	2012-12-13 13:43:55 +0000
@@ -22,6 +22,7 @@
 
     def register(self, registry):
         self.config = registry.config
+        self.run_interval = self.config.package_monitor_interval
         if self.config.clones and self.config.is_clone:
             # Run clones a bit more frequently in order to catch up
             self.run_interval = 60  # 300

=== modified file 'landscape/monitor/tests/test_packagemonitor.py'
--- landscape/monitor/tests/test_packagemonitor.py	2011-07-05 05:09:11 +0000
+++ landscape/monitor/tests/test_packagemonitor.py	2012-12-13 13:43:55 +0000
@@ -43,6 +43,15 @@
         self.monitor.dispatch_message(message)
         self.assertTrue(os.path.isfile(filename))
 
+    def test_run_interval(self):
+        """
+        The C{run_interval} of L{PackageMonitor} can be customized via the
+        C{package_monitor_interval} configuration parameter.
+        """
+        self.monitor.config.package_monitor_interval = 1234
+        self.package_monitor.register(self.monitor)
+        self.assertEqual(1234, self.package_monitor.run_interval)
+
     def test_dont_spawn_reporter_if_message_not_accepted(self):
         self.monitor.add(self.package_monitor)
 

=== modified file 'landscape/package/reporter.py'
--- landscape/package/reporter.py	2012-05-16 08:35:27 +0000
+++ landscape/package/reporter.py	2012-12-13 13:43:55 +0000
@@ -40,14 +40,11 @@
     """Report information about the system packages.
 
     @cvar queue_name: Name of the task queue to pick tasks from.
-    @cvar apt_update_interval: Don't update the APT index more often
-        than the given interval in minutes.
     """
     config_factory = PackageReporterConfiguration
 
     queue_name = "reporter"
 
-    apt_update_interval = 60
     apt_update_filename = "/usr/lib/landscape/apt-update"
     sources_list_filename = "/etc/apt/sources.list"
     sources_list_directory = "/etc/apt/sources.list.d"
@@ -184,7 +181,7 @@
         # check stamp file mtime
         last_update = os.stat(stamp)[8]
         now = int(time.time())
-        return (last_update + interval * 60) < now
+        return (last_update + interval) < now
 
     def run_apt_update(self):
         """Run apt-update and log a warning in case of non-zero exit code.
@@ -192,7 +189,8 @@
         @return: a deferred returning (out, err, code)
         """
         if (self._config.force_apt_update or self._apt_sources_have_changed()
-            or self._apt_update_timeout_expired(self.apt_update_interval)):
+            or self._apt_update_timeout_expired(
+                self._config.apt_update_interval)):
 
             result = spawn_process(self.apt_update_filename)
 
@@ -204,7 +202,7 @@
 
                 if code != 0:
                     logging.warning("'%s' exited with status %d (%s)" % (
-                            self.apt_update_filename, code, err))
+                        self.apt_update_filename, code, err))
                 elif not self._facade.get_channels():
                     code = 1
                     err = ("There are no APT sources configured in %s or %s." %

=== modified file 'landscape/package/tests/test_reporter.py'
--- landscape/package/tests/test_reporter.py	2012-05-10 19:01:46 +0000
+++ landscape/package/tests/test_reporter.py	2012-12-13 13:43:55 +0000
@@ -145,7 +145,8 @@
 
             self.assertTrue(message_store.is_pending(message_id))
 
-            self.assertMessages(message_store.get_pending_messages(),
+            self.assertMessages(
+                message_store.get_pending_messages(),
                 [{"packages": [{"description": u"Description1",
                                 "installed-size": 28672,
                                 "name": u"name1",
@@ -164,7 +165,7 @@
                                 "summary": u"Summary1",
                                 "type": 65537,
                                 "version": u"version1-release1"}],
-                                "request-id": request2.id,
+                  "request-id": request2.id,
                   "type": "add-packages"}])
 
         deferred = self.reporter.handle_tasks()
@@ -185,7 +186,8 @@
         def got_result(result):
             message = message_store.get_pending_messages()[0]
             request2 = self.store.get_hash_id_request(message["request-id"])
-            self.assertMessages(message_store.get_pending_messages(),
+            self.assertMessages(
+                message_store.get_pending_messages(),
                 [{"packages": [{"description": u"Description1",
                                 "installed-size": None,
                                 "name": u"name1",
@@ -195,7 +197,7 @@
                                 "summary": u"Summary1",
                                 "type": 65537,
                                 "version": u"version1-release1"}],
-                                "request-id": request2.id,
+                  "request-id": request2.id,
                   "type": "add-packages"}])
 
         class FakePackage(object):
@@ -239,9 +241,9 @@
 
         def got_result(result):
             self.assertMessages(message_store.get_pending_messages(), [])
-            self.assertEqual([request.id for request in
-                               self.store.iter_hash_id_requests()],
-                              [request_id])
+            self.assertEqual(
+                [request.id for request in self.store.iter_hash_id_requests()],
+                [request_id])
 
         result = self.reporter.handle_tasks()
         self.assertFailure(result, Boom)
@@ -391,7 +393,7 @@
 
         # The failure should be properly logged
         logging_mock = self.mocker.replace("logging.warning")
-        logging_mock("Couldn't determine which hash=>id database to use: "\
+        logging_mock("Couldn't determine which hash=>id database to use: "
                      "unknown dpkg architecture")
         self.mocker.result(None)
 
@@ -1218,7 +1220,8 @@
             result = self.reporter.run_apt_update()
 
             def callback(ignore):
-                self.assertMessages(message_store.get_pending_messages(),
+                self.assertMessages(
+                    message_store.get_pending_messages(),
                     [{"type": "package-reporter-result",
                       "code": 2, "err": u"error"}])
             result.addCallback(callback)
@@ -1247,7 +1250,8 @@
                 error = "There are no APT sources configured in %s or %s." % (
                     self.reporter.sources_list_filename,
                     self.reporter.sources_list_directory)
-                self.assertMessages(message_store.get_pending_messages(),
+                self.assertMessages(
+                    message_store.get_pending_messages(),
                     [{"type": "package-reporter-result",
                       "code": 1, "err": error}])
             result.addCallback(callback)
@@ -1271,7 +1275,8 @@
             result = self.reporter.run_apt_update()
 
             def callback(ignore):
-                self.assertMessages(message_store.get_pending_messages(),
+                self.assertMessages(
+                    message_store.get_pending_messages(),
                     [{"type": "package-reporter-result",
                       "code": 2, "err": u"error"}])
             result.addCallback(callback)
@@ -1294,7 +1299,8 @@
             result = self.reporter.run_apt_update()
 
             def callback(ignore):
-                self.assertMessages(message_store.get_pending_messages(),
+                self.assertMessages(
+                    message_store.get_pending_messages(),
                     [{"type": "package-reporter-result",
                       "code": 0, "err": u"message"}])
             result.addCallback(callback)
@@ -1353,6 +1359,37 @@
         reactor.callWhenRunning(do_test)
         return deferred
 
+    def test_config_apt_update_interval(self):
+        """
+        L{PackageReporter} uses the C{apt_update_interval} configuration
+        parameter to check the age of the update stamp file.
+        """
+        self.config.apt_update_interval = 1234
+        message_store = self.broker_service.message_store
+        message_store.set_accepted_types(["package-reporter-result"])
+        intervals = []
+
+        def apt_update_timeout_expired(interval):
+            intervals.append(interval)
+            return False
+
+        deferred = Deferred()
+
+        self.reporter._apt_sources_have_changed = lambda: False
+        self.reporter._apt_update_timeout_expired = apt_update_timeout_expired
+
+        def do_test():
+            result = self.reporter.run_apt_update()
+
+            def callback(ignore):
+                self.assertMessages(message_store.get_pending_messages(), [])
+                self.assertEqual([1234], intervals)
+            result.addCallback(callback)
+            result.chainDeferred(deferred)
+
+        reactor.callWhenRunning(do_test)
+        return deferred
+
 
 class GlobalPackageReporterAptTest(LandscapeTest):
 

=== modified file 'man/landscape-config.1'
--- man/landscape-config.1	2012-09-27 18:52:37 +0000
+++ man/landscape-config.1	2012-12-13 13:43:55 +0000
@@ -1,5 +1,5 @@
 .\"Text automatically generated by txt2man
-.TH landscape-config 1 "27 September 2012" "" ""
+.TH landscape-config 1 "13 December 2012" "" ""
 .SH NAME
 \fBlandscape-config \fP- configure the Landscape management client
 \fB
@@ -109,6 +109,10 @@
 \fB--ping-url\fP=PING_URL
 The URL to perform lightweight exchange initiation
 with (default: 'http://landscape.canonical.com/ping').
+\fB--package-monitor-interval\fP=PACKAGE_MONITOR_INTERVAL
+The interval between apt update runs (default 1800).
+\fB--apt-update-interval\fP=APT_UPDATE_INTERVAL
+The interval between apt update runs (default 21600).
 .TP
 .B
 \fB--http-proxy\fP=URL

=== modified file 'man/landscape-config.txt'
--- man/landscape-config.txt	2012-09-27 18:52:37 +0000
+++ man/landscape-config.txt	2012-12-13 13:43:55 +0000
@@ -48,6 +48,10 @@
   --ping-interval=INTERVAL  The number of seconds between pings (default: 30).
   --ping-url=PING_URL      The URL to perform lightweight exchange initiation
                            with (default: 'http://landscape.canonical.com/ping').
+  --package-monitor-interval=PACKAGE_MONITOR_INTERVAL
+                           The interval between apt update runs (default 1800).
+  --apt-update-interval=APT_UPDATE_INTERVAL
+                           The interval between apt update runs (default 21600).
   --http-proxy=URL         The URL of the HTTP proxy, if one is needed.
   --https-proxy=URL        The URL of the HTTPS proxy, if one is needed.
   --cloud                  Set this if this computer is a cloud instance in