Merge lp:~ack/landscape-client/package-reporter-update into lp:~landscape/landscape-client/trunk

[Alberto Donato]

This branch makes the package reporter use APT to update the package cache instead of smart, when USE_APT_FACADE is set.

At the moment, force_smart_update and smart_update_interval are used with the same effect also when updating via APT.

[Thomas Herve (therve)]

It looks good, but I suspect we need to get this reviewed by someone from the platform team. It shouldn't block the branch, but I suggest getting in touch with them sooner than later.

+1!

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Great! +1

[1]

+ else:
+ err = ("'%s' didn't run, update interval has not passed" %
+ self.apt_update_filename)
+ result = succeed(("", err, 1))

I think we should not reproduce the behavior of smart-update in this case, we should just return 0 as fake exit code, and this will simplify the callback.

[2]

Please add a copyright notice to apt-update.c, like in smart-update.c

review: Approve

[lp:~ack/landscape-client/package-reporter-update]

398. By Alberto Donato on 2011-11-08

Addressed Free's comments.

=== modified file '.bzrignore'
--- .bzrignore	2011-10-04 12:13:35 +0000
+++ .bzrignore	2011-11-08 11:04:00 +0000
@@ -8,8 +8,17 @@
 debian/landscape-client.postrm.debhelper
 debian/landscape-client.prerm.debhelper
 debian/landscape-client.substvars
+debian/landscape-client.debhelper.log
+debian/landscape-common/
+debian/landscape-common.debhelper.log
+debian/landscape-common.postinst.debhelper
+debian/landscape-common.postrm.debhelper
+debian/landscape-common.prerm.debhelper
+debian/landscape-common.substvars
 man/landscape-client.1
 man/landscape-config.1
 man/landscape-message.1
+smart-update/smart-update
+apt-update/apt-update
 docs
 tags

=== added directory 'apt-update'
=== added file 'apt-update/Makefile'
--- apt-update/Makefile	1970-01-01 00:00:00 +0000
+++ apt-update/Makefile	2011-11-08 11:04:00 +0000
@@ -0,0 +1,7 @@
+NAME = apt-update
+
+$(NAME): $(NAME).c
+	$(CC) $(CFLAGS) -Wall $< -o $@
+
+clean:
+	rm -f $(NAME)

=== added file 'apt-update/apt-update.c'
--- apt-update/apt-update.c	1970-01-01 00:00:00 +0000
+++ apt-update/apt-update.c	2011-11-08 11:04:00 +0000
@@ -0,0 +1,81 @@
+/*
+
+ Copyright (c) 2011 Canonical, Ltd.
+
+*/
+
+#define _GNU_SOURCE
+#include <sys/resource.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <grp.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <stdio.h>
+#include <pwd.h>
+
+int main(int argc, char *argv[], char *envp[])
+{
+  char *apt_argv[] = {"/usr/bin/apt-get", "-q", "update", NULL};
+  char *apt_envp[] = {"PATH=/bin:/usr/bin", NULL, NULL};
+
+  // Set the HOME environment variable
+  struct passwd *pwd = getpwuid(geteuid());
+  if (!pwd) {
+    fprintf(stderr, "error: Unable to find passwd entry for uid %d (%s)\n",
+            geteuid(), strerror(errno));
+    exit(1);
+  }
+  if (asprintf(&apt_envp[1], "HOME=%s", pwd->pw_dir) == -1) {
+    perror("error: Unable to create HOME environment variable");
+    exit(1);
+  }
+
+  // Drop any supplementary group
+  if (setgroups(0, NULL) == -1) {
+    perror("error: Unable to set supplementary groups IDs");
+    exit(1);
+  }
+
+  // Set real/effective gid and uid
+  if (setregid(pwd->pw_gid, pwd->pw_gid) == -1) {
+    fprintf(stderr, "error: Unable to set real and effective gid (%s)\n",
+            strerror(errno));
+    exit(1);
+  }
+  if (setreuid(pwd->pw_uid, pwd->pw_uid) == -1) {
+    perror("error: Unable to set real and effective uid");
+    exit(1);
+  }
+
+  // Close all file descriptors except the standard ones
+  struct rlimit rlp;
+  if (getrlimit(RLIMIT_NOFILE, &rlp) == -1) {
+    perror("error: Unable to determine file descriptor limits");
+    exit(1);
+  }
+  int file_max;
+  if (rlp.rlim_max == RLIM_INFINITY || rlp.rlim_max > 4096)
+    file_max = 4096;
+  else
+    file_max = rlp.rlim_max;
+  int file;
+  for (file = 3; file < file_max; file++) {
+    close(file);
+  }
+
+  // Set umask to 022
+  umask(S_IWGRP | S_IWOTH);
+
+  if (chdir("/") == -1) {
+    perror("error: Unable to change working directory");
+    exit(1);
+  }
+
+  // Run apt-get update
+  execve(apt_argv[0], apt_argv, apt_envp);
+  perror("error: Unable to execute apt-get");
+  return 1;
+}

=== modified file 'debian/landscape-client.postinst'
--- debian/landscape-client.postinst	2011-10-07 11:52:47 +0000
+++ debian/landscape-client.postinst	2011-11-08 11:04:00 +0000
@@ -102,12 +102,17 @@
             echo "Old monolithic client data file migrated to new format."
         fi
 
-        # Add the setuid flag to smart-update and it be executable by users in
-        # the landscape group (that normally means landscape itself)
+        # Add the setuid flag to smart-update and apt-update, and make them be
+        # executable by users in the landscape group (that normally means
+        # landscape itself)
         smart_update=/usr/lib/landscape/smart-update
         if ! dpkg-statoverride --list $smart_update >/dev/null 2>&1; then
             dpkg-statoverride --update --add root landscape 4754 $smart_update
         fi
+        apt_update=/usr/lib/landscape/apt-update
+        if ! dpkg-statoverride --list $apt_update >/dev/null 2>&1; then
+            dpkg-statoverride --update --add root landscape 4754 $apt_update
+        fi
 
         # Remove old cron jobs
         old_cron_job=/etc/cron.hourly/landscape-client

=== modified file 'debian/landscape-client.prerm'
--- debian/landscape-client.prerm	2009-06-08 13:29:20 +0000
+++ debian/landscape-client.prerm	2011-11-08 11:04:00 +0000
@@ -20,11 +20,15 @@
 case "$1" in
     remove|upgrade|deconfigure)
 
-        # Remove statoverride for smart-update
+        # Remove statoverride for smart-update and apt-update
         smart_update=/usr/lib/landscape/smart-update
         if dpkg-statoverride --list $smart_update >/dev/null 2>&1; then
             dpkg-statoverride --remove $smart_update
         fi
+        apt_update=/usr/lib/landscape/apt-update
+        if dpkg-statoverride --list $apt_update >/dev/null 2>&1; then
+            dpkg-statoverride --remove $apt_update
+        fi
 
     ;;
 

=== modified file 'debian/rules'
--- debian/rules	2011-04-26 09:47:16 +0000
+++ debian/rules	2011-11-08 11:04:00 +0000
@@ -30,6 +30,7 @@
 	sed -i -e "s/^DEBIAN_REVISION = \"\"/DEBIAN_REVISION = \"-$(revision)\"/g" landscape/__init__.py
 	python setup.py build
 	make -C smart-update
+	make -C apt-update
 	touch build-stamp
 
 clean:
@@ -38,6 +39,7 @@
 	rm -f build-stamp
 	rm -rf build
 	make -C smart-update clean
+	make -C apt-update clean
 	dh_clean
 	debconf-updatepo
 	sed -i -e "s/^DEBIAN_REVISION = .*/DEBIAN_REVISION = \"\"/g" landscape/__init__.py
@@ -57,6 +59,7 @@
 	install -D -o root -g root -m 755 debian/landscape-sysinfo.wrapper $(root_dir)/usr/share/landscape/landscape-sysinfo.wrapper
 	install -D -o root -g root -m 644 debian/cloud-default.conf $(root_dir)/usr/share/landscape/cloud-default.conf
 	install -D -o root -g root -m 755 smart-update/smart-update $(root_dir)/usr/lib/landscape/smart-update
+	install -D -o root -g root -m 755 apt-update/apt-update $(root_dir)/usr/lib/landscape/apt-update
 	install -D -o root -g root -m 644 dbus/landscape.conf $(root_dir)/etc/dbus-1/system.d/landscape.conf
 
 binary-indep:

=== modified file 'landscape/package/reporter.py'
--- landscape/package/reporter.py	2011-10-19 13:16:30 +0000
+++ landscape/package/reporter.py	2011-11-08 11:04:00 +0000
@@ -49,15 +49,20 @@
 
     smart_update_interval = 60
     smart_update_filename = "/usr/lib/landscape/smart-update"
+    apt_update_filename = "/usr/lib/landscape/apt-update"
     sources_list_filename = "/etc/apt/sources.list"
     sources_list_directory = "/etc/apt/sources.list.d"
 
     def run(self):
         result = Deferred()
 
-        # Run smart-update before anything else, to make sure that
-        # the SmartFacade will load freshly updated channels
-        result.addCallback(lambda x: self.run_smart_update())
+        if os.environ.get("USE_APT_FACADE"):
+            # Update APT cache if APT facade is enabled.
+            result.addCallback(lambda x: self.run_apt_update())
+        else:
+            # Run smart-update before anything else, to make sure that
+            # the SmartFacade will load freshly updated channels
+            result.addCallback(lambda x: self.run_smart_update())
 
         # If the appropriate hash=>id db is not there, fetch it
         result.addCallback(lambda x: self.fetch_hash_id_db())
@@ -215,6 +220,54 @@
         result.addCallback(callback)
         return result
 
+    def _apt_update_timeout_expired(self, interval):
+        """Check if the apt-update timeout has passed."""
+        stamp = self._config.apt_update_stamp_filename
+
+        if not os.path.exists(stamp):
+            return True
+        # check stamp file mtime
+        last_update = os.stat(stamp)[8]
+        now = int(time.time())
+        return (last_update + interval * 60) < now
+
+    def run_apt_update(self):
+        """Run apt-update and log a warning in case of non-zero exit code.
+
+        @return: a deferred returning (out, err, code)
+        """
+        if (self._config.force_smart_update or self._apt_sources_have_changed()
+            or self._apt_update_timeout_expired(self.smart_update_interval)):
+
+            result = spawn_process(self.apt_update_filename)
+
+            def callback((out, err, code)):
+                touch_file(self._config.apt_update_stamp_filename)
+                logging.debug(
+                    "'%s' exited with status %d (out='%s', err='%s')" % (
+                        self.apt_update_filename, code, out, err))
+
+                if code != 0:
+                    logging.warning("'%s' exited with status %d (%s)" % (
+                            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." %
+                           (self.sources_list_filename,
+                            self.sources_list_directory))
+
+                deferred = self._broker.call_if_accepted(
+                    "package-reporter-result", self.send_result, code, err)
+                deferred.addCallback(lambda ignore: (out, err, code))
+                return deferred
+
+            return result.addCallback(callback)
+
+        else:
+            logging.debug("'%s' didn't run, update interval has not passed" %
+                          self.apt_update_filename)
+            return succeed(("", "", 0))
+
     def send_result(self, code, err):
         """
         Report the package reporter result to the server in a message.

=== modified file 'landscape/package/taskhandler.py'
--- landscape/package/taskhandler.py	2011-10-26 13:08:13 +0000
+++ landscape/package/taskhandler.py	2011-11-08 11:04:00 +0000
@@ -40,6 +40,11 @@
         """Get the path to the smart-update stamp file."""
         return os.path.join(self.package_directory, "smart-update-stamp")
 
+    @property
+    def apt_update_stamp_filename(self):
+        """Get the path to the apt-update stamp file."""
+        return os.path.join(self.package_directory, "apt-update-stamp")
+
 
 class LazyRemoteBroker(object):
     """Wrapper class around L{RemoteBroker} providing lazy initialization.

=== modified file 'landscape/package/tests/test_reporter.py'
--- landscape/package/tests/test_reporter.py	2011-10-26 12:55:00 +0000
+++ landscape/package/tests/test_reporter.py	2011-11-08 11:04:00 +0000
@@ -1693,6 +1693,262 @@
         """Make it so that package "name1" is considered installed."""
         self._install_deb_file(os.path.join(self.repository_dir, PKGNAME1))
 
+    def _make_fake_apt_update(self, out="output", err="error", code=0):
+        """Create a fake apt-update executable"""
+        self.reporter.apt_update_filename = self.makeFile(
+            "#!/bin/sh\n"
+            "echo -n %s\n"
+            "echo -n %s >&2\n"
+            "exit %d" % (out, err, code))
+        os.chmod(self.reporter.apt_update_filename, 0755)
+
+    def test_run_apt_update(self):
+        """
+        The L{PackageReporter.run_apt_update} method should run apt-update.
+        """
+        self.reporter.sources_list_filename = "/I/Dont/Exist"
+        self._make_fake_apt_update()
+        debug_mock = self.mocker.replace("logging.debug")
+        debug_mock("'%s' exited with status 0 (out='output', err='error')" %
+                   self.reporter.apt_update_filename)
+        warning_mock = self.mocker.replace("logging.warning")
+        self.expect(warning_mock(ANY)).count(0)
+        self.mocker.replay()
+        deferred = Deferred()
+
+        def do_test():
+
+            result = self.reporter.run_apt_update()
+
+            def callback((out, err, code)):
+                self.assertEqual("output", out)
+                self.assertEqual("error", err)
+                self.assertEqual(0, code)
+            result.addCallback(callback)
+            result.chainDeferred(deferred)
+
+        reactor.callWhenRunning(do_test)
+        return deferred
+
+    def test_run_apt_update_with_force_smart_update(self):
+        """
+        L{PackageReporter.run_apt_update} forces an apt-update run if the
+        '--force-smart-update' command line option was passed.
+        """
+        self.makeFile("", path=self.config.apt_update_stamp_filename)
+        self.config.load(["--force-smart-update"])
+        self._make_fake_apt_update()
+
+        deferred = Deferred()
+
+        def do_test():
+            result = self.reporter.run_apt_update()
+
+            def callback((out, err, code)):
+                self.assertEqual("output", out)
+            result.addCallback(callback)
+            result.chainDeferred(deferred)
+
+        reactor.callWhenRunning(do_test)
+        return deferred
+
+    def test_run_apt_update_with_force_smart_update_if_sources_changed(self):
+        """
+        L{PackageReporter.run_apt_update} forces an apt-update run if the APT
+        sources.list file has changed.
+
+        """
+        self.assertEqual(self.reporter.sources_list_filename,
+                         "/etc/apt/sources.list")
+        self.reporter.sources_list_filename = self.makeFile("deb ftp://url ./")
+        self._make_fake_apt_update()
+
+        deferred = Deferred()
+
+        def do_test():
+            result = self.reporter.run_apt_update()
+
+            def callback((out, err, code)):
+                self.assertEqual("output", out)
+            result.addCallback(callback)
+            result.chainDeferred(deferred)
+
+        reactor.callWhenRunning(do_test)
+        return deferred
+
+    def test_run_apt_update_warns_about_failures(self):
+        """
+        The L{PackageReporter.run_apt_update} method should log a warning in
+        case apt-update terminates with a non-zero exit code.
+        """
+        self._make_fake_apt_update(code=2)
+        logging_mock = self.mocker.replace("logging.warning")
+        logging_mock("'%s' exited with status 2"
+                     " (error)" % self.reporter.apt_update_filename)
+        self.mocker.replay()
+        deferred = Deferred()
+
+        def do_test():
+            result = self.reporter.run_apt_update()
+
+            def callback((out, err, code)):
+                self.assertEqual("output", out)
+                self.assertEqual("error", err)
+                self.assertEqual(2, code)
+            result.addCallback(callback)
+            result.chainDeferred(deferred)
+
+        reactor.callWhenRunning(do_test)
+        return deferred
+
+    def test_run_apt_update_report_apt_failure(self):
+        """
+        If L{PackageReporter.run_apt_update} fails, a message is sent to the
+        server reporting the error, to be able to fix the problem centrally.
+        """
+        message_store = self.broker_service.message_store
+        message_store.set_accepted_types(["package-reporter-result"])
+        self._make_fake_apt_update(code=2)
+        deferred = Deferred()
+
+        def do_test():
+            result = self.reporter.run_apt_update()
+
+            def callback(ignore):
+                self.assertMessages(message_store.get_pending_messages(),
+                    [{"type": "package-reporter-result",
+                      "code": 2, "err": u"error"}])
+            result.addCallback(callback)
+            result.chainDeferred(deferred)
+
+        reactor.callWhenRunning(do_test)
+        return deferred
+
+    def test_run_apt_update_report_no_sources(self):
+        """
+        L{PackageReporter.run_apt_update} reports a failure if apt succeeds but
+        there are no APT sources defined. APT doesn't fail if there are no
+        sources, but we fake a failure in order to re-use the
+        PackageReporterAlert on the server.
+        """
+        self.facade.reset_channels()
+        message_store = self.broker_service.message_store
+        message_store.set_accepted_types(["package-reporter-result"])
+        self._make_fake_apt_update()
+        deferred = Deferred()
+
+        def do_test():
+            result = self.reporter.run_apt_update()
+
+            def callback(ignore):
+                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(),
+                    [{"type": "package-reporter-result",
+                      "code": 1, "err": error}])
+            result.addCallback(callback)
+            result.chainDeferred(deferred)
+
+        reactor.callWhenRunning(do_test)
+        return deferred
+
+    def test_run_apt_update_report_apt_failure_no_sources(self):
+        """
+        If L{PackageReporter.run_apt_update} fails and there are no
+        APT sources configured, the APT error takes precedence.
+        """
+        self.facade.reset_channels()
+        message_store = self.broker_service.message_store
+        message_store.set_accepted_types(["package-reporter-result"])
+        self._make_fake_apt_update(code=2)
+        deferred = Deferred()
+
+        def do_test():
+            result = self.reporter.run_apt_update()
+
+            def callback(ignore):
+                self.assertMessages(message_store.get_pending_messages(),
+                    [{"type": "package-reporter-result",
+                      "code": 2, "err": u"error"}])
+            result.addCallback(callback)
+            result.chainDeferred(deferred)
+
+        reactor.callWhenRunning(do_test)
+        return deferred
+
+    def test_run_apt_update_report_success(self):
+        """
+        L{PackageReporter.run_apt_update} also reports success to be able to
+        know the proper state of the client.
+        """
+        message_store = self.broker_service.message_store
+        message_store.set_accepted_types(["package-reporter-result"])
+        self._make_fake_apt_update(err="message")
+        deferred = Deferred()
+
+        def do_test():
+            result = self.reporter.run_apt_update()
+
+            def callback(ignore):
+                self.assertMessages(message_store.get_pending_messages(),
+                    [{"type": "package-reporter-result",
+                      "code": 0, "err": u"message"}])
+            result.addCallback(callback)
+            result.chainDeferred(deferred)
+
+        reactor.callWhenRunning(do_test)
+        return deferred
+
+    def test_run_apt_update_no_run_in_interval(self):
+        """
+        The L{PackageReporter.run_apt_update} logs a debug message if
+        apt-update doesn't run because interval has not passed.
+        """
+        self.makeFile("", path=self.config.apt_update_stamp_filename)
+
+        logging_mock = self.mocker.replace("logging.debug")
+        logging_mock("'%s' didn't run, update interval has not passed" %
+                     self.reporter.apt_update_filename)
+        self.mocker.replay()
+        deferred = Deferred()
+
+        def do_test():
+
+            result = self.reporter.run_apt_update()
+
+            def callback((out, err, code)):
+                self.assertEqual("", out)
+                self.assertEqual("", err)
+                self.assertEqual(0, code)
+            result.addCallback(callback)
+            result.chainDeferred(deferred)
+
+        reactor.callWhenRunning(do_test)
+        return deferred
+
+    def test_run_apt_update_touches_stamp_file(self):
+        """
+        The L{PackageReporter.run_apt_update} method touches a stamp file
+        after running the apt-update wrapper.
+        """
+        self.reporter.sources_list_filename = "/I/Dont/Exist"
+        self._make_fake_apt_update()
+        deferred = Deferred()
+
+        def do_test():
+
+            result = self.reporter.run_apt_update()
+
+            def callback(ignored):
+                self.assertTrue(
+                    os.path.exists(self.config.apt_update_stamp_filename))
+            result.addCallback(callback)
+            result.chainDeferred(deferred)
+
+        reactor.callWhenRunning(do_test)
+        return deferred
+
 
 class GlobalPackageReporterTestMixin(object):
 

=== modified file 'landscape/package/tests/test_taskhandler.py'
--- landscape/package/tests/test_taskhandler.py	2011-07-05 05:09:11 +0000
+++ landscape/package/tests/test_taskhandler.py	2011-11-08 11:04:00 +0000
@@ -35,6 +35,16 @@
             config.smart_update_stamp_filename,
             "/var/lib/landscape/client/package/smart-update-stamp")
 
+    def test_force_apt_update_option(self):
+        """
+        L{PackageReporterConfiguration.apt_update_stamp_filename} points
+        to the apt-update stamp file.
+        """
+        config = PackageTaskHandlerConfiguration()
+        self.assertEqual(
+            config.apt_update_stamp_filename,
+            "/var/lib/landscape/client/package/apt-update-stamp")
+
 
 class PackageTaskHandlerTest(LandscapeTest):