Merge lp:~bjornt/landscape-client/add-remove-dpkg-holds into lp:~landscape/landscape-client/trunk

[Björn Tillenius]

Add a message for adding and removing package holds, handling the
message in the package changer.

[Alberto Donato (ack)]

Looks good! +1

#1:
+ """Log that a package holds result is sent and sent the response."""

typo, "and send"

review: Approve

[Geoff Teale (tealeg)]

+1 Looks good. Please fix:

landscape/package/tests/test_changer.py:1067:1: E303 too many blank lines (3)

review: Approve

[lp:~bjornt/landscape-client/add-remove-dpkg-holds]

448. By Björn Tillenius on 2012-01-20

Typo.

449. By Björn Tillenius on 2012-01-20

Lint.

[Björn Tillenius (bjornt)]

On Thu, Jan 19, 2012 at 04:06:23PM -0000, Alberto Donato wrote:
> Review: Approve
>
> Looks good! +1
>
> #1:
> + """Log that a package holds result is sent and sent the response."""
>
> typo, "and send"

Thanks, fixed.

--
Björn Tillenius | https://launchpad.net/~bjornt

[Björn Tillenius (bjornt)]

On Fri, Jan 20, 2012 at 10:43:24AM -0000, Geoff Teale wrote:
> Review: Approve
>
> +1 Looks good. Please fix:
>
> landscape/package/tests/test_changer.py:1067:1: E303 too many blank lines (3)

Oops, missed that. Fixed.

--
Björn Tillenius | https://launchpad.net/~bjornt

=== modified file 'landscape/message_schemas.py'
--- landscape/message_schemas.py	2012-01-11 13:37:17 +0000
+++ landscape/message_schemas.py	2012-01-20 12:58:25 +0000
@@ -294,6 +294,12 @@
      "deleted": package_locks},
     optional=["created", "deleted"])
 
+CHANGE_PACKAGE_HOLDS = Message(
+    "change-package-holds",
+    {"created": List(utf8),
+     "deleted": List(utf8)},
+    optional=["created", "deleted"])
+
 CHANGE_PACKAGES_RESULT = Message(
     "change-packages-result",
     {"operation-id": Int(),

=== modified file 'landscape/package/changer.py'
--- landscape/package/changer.py	2012-01-13 14:01:43 +0000
+++ landscape/package/changer.py	2012-01-20 12:58:25 +0000
@@ -18,7 +18,7 @@
 from landscape.package.taskhandler import (
     PackageTaskHandler, PackageTaskHandlerConfiguration, PackageTaskError,
     run_task_handler)
-from landscape.manager.manager import SUCCEEDED
+from landscape.manager.manager import FAILED, SUCCEEDED
 
 
 class UnknownPackageData(Exception):
@@ -103,6 +103,8 @@
             return result.addErrback(self.unknown_package_data_error, task)
         if message["type"] == "change-package-locks":
             return self.handle_change_package_locks(message)
+        if message["type"] == "change-package-holds":
+            return self.handle_change_package_holds(message)
 
     def unknown_package_data_error(self, failure, task):
         """Handle L{UnknownPackageData} data errors.
@@ -296,6 +298,58 @@
                      "exchange urgently.")
         return self._broker.send_message(response, True)
 
+    def _send_change_package_holds_response(self, response):
+        """Log that a package holds result is sent and send the response."""
+        logging.info("Queuing message with change package holds results to "
+                     "exchange urgently.")
+        return self._broker.send_message(response, True)
+
+    def handle_change_package_holds(self, message):
+        """Handle a C{change-package-holds} message.
+
+        Create and delete package holds as requested by the given C{message}.
+        """
+        if not self._facade.supports_package_holds:
+            response = {
+                "type": "operation-result",
+                "operation-id": message.get("operation-id"),
+                "status": FAILED,
+                "result-text": "This client doesn't support package holds.",
+                "result-code": 1}
+            return self._send_change_package_holds_response(response)
+
+        not_installed = set()
+        holds_to_create = message.get("create", [])
+        for name in holds_to_create:
+            versions = self._facade.get_packages_by_name(name)
+            if not versions or not versions[0].package.installed:
+                not_installed.add(name)
+        if not_installed:
+            response = {
+                "type": "operation-result",
+                "operation-id": message.get("operation-id"),
+                "status": FAILED,
+                "result-text": "Package holds not added, since the following" +
+                               " packages are not installed: %s" % (
+                                   ", ".join(sorted(not_installed))),
+                "result-code": 1}
+            return self._send_change_package_holds_response(response)
+
+        for hold in holds_to_create:
+            self._facade.set_package_hold(hold)
+        self._facade.reload_channels()
+        for hold in message.get("delete", []):
+            self._facade.remove_package_hold(hold)
+        self._facade.reload_channels()
+
+        response = {"type": "operation-result",
+                    "operation-id": message.get("operation-id"),
+                    "status": SUCCEEDED,
+                    "result-text": "Package holds successfully changed.",
+                    "result-code": 0}
+
+        return self._send_change_package_holds_response(response)
+
     @staticmethod
     def find_command():
         return find_changer_command()

=== modified file 'landscape/package/facade.py'
--- landscape/package/facade.py	2012-01-16 15:08:15 +0000
+++ landscape/package/facade.py	2012-01-20 12:58:25 +0000
@@ -1,5 +1,6 @@
 import hashlib
 import os
+import subprocess
 import tempfile
 from cStringIO import StringIO
 
@@ -79,10 +80,14 @@
         database.
     """
 
+    supports_package_holds = True
+
     def __init__(self, root=None):
         self._root = root
+        self._dpkg_args = []
         if self._root is not None:
             self._ensure_dir_structure()
+            self._dpkg_args.extend(["--root", self._root])
         # don't use memonly=True here because of a python-apt bug on Natty when
         # sources.list contains invalid lines (LP: #886208)
         self._cache = apt.cache.Cache(rootdir=root)
@@ -100,6 +105,10 @@
         self._ensure_sub_dir("var/cache/apt/archives/partial")
         self._ensure_sub_dir("var/lib/apt/lists/partial")
         dpkg_dir = self._ensure_sub_dir("var/lib/dpkg")
+        self._ensure_sub_dir("var/lib/dpkg/info")
+        self._ensure_sub_dir("var/lib/dpkg/updates")
+        self._ensure_sub_dir("var/lib/dpkg/triggers")
+        create_file(os.path.join(dpkg_dir, "available"), "")
         self._dpkg_status = os.path.join(dpkg_dir, "status")
         if not os.path.exists(self._dpkg_status):
             create_file(self._dpkg_status, "")
@@ -157,6 +166,37 @@
         """
         return []
 
+    def get_package_holds(self):
+        """Return the name of all the packages that are on hold."""
+        return [version.package.name for version in self.get_locked_packages()]
+
+    def _set_dpkg_selections(self, selection):
+        """Set the dpkg selection.
+
+        It basically does "echo $selection | dpkg --set-selections".
+        """
+        process = subprocess.Popen(
+            ["dpkg", "--set-selections"] + self._dpkg_args,
+            stdin=subprocess.PIPE)
+        process.communicate(selection)
+
+    def set_package_hold(self, name):
+        """Add a dpkg hold for a package.
+
+        @param name: The name of the package to hold.
+        """
+        self._set_dpkg_selections(name + " hold")
+
+    def remove_package_hold(self, name):
+        """Removes a dpkg hold for a package.
+
+        @param name: The name of the package to unhold.
+        """
+        versions = self.get_packages_by_name(name)
+        if not versions or not self._is_package_held(versions[0].package):
+            return
+        self._set_dpkg_selections(name + " install")
+
     def reload_channels(self):
         """Reload the channels and update the cache."""
         self._cache.open(None)
@@ -469,6 +509,7 @@
     """
 
     _deb_package_type = None
+    supports_package_holds = False
 
     def __init__(self, smart_init_kwargs={}, sysconf_args=None):
         self._smart_init_kwargs = smart_init_kwargs.copy()

=== modified file 'landscape/package/tests/helpers.py'
--- landscape/package/tests/helpers.py	2011-12-14 10:41:04 +0000
+++ landscape/package/tests/helpers.py	2012-01-20 12:58:25 +0000
@@ -40,7 +40,6 @@
                 Architecture: %(architecture)s
                 Source: source
                 Version: %(version)s
-                Config-Version: 1.0
                 Description: description
                 """ % {"name": name, "version": version,
                        "architecture": architecture})

=== modified file 'landscape/package/tests/test_changer.py'
--- landscape/package/tests/test_changer.py	2011-12-13 10:29:04 +0000
+++ landscape/package/tests/test_changer.py	2012-01-20 12:58:25 +0000
@@ -24,7 +24,7 @@
 from landscape.package.tests.helpers import (
     SmartFacadeHelper, HASH1, HASH2, HASH3, PKGDEB1, PKGDEB2, PKGNAME2,
     AptFacadeHelper, SimpleRepositoryHelper)
-from landscape.manager.manager import SUCCEEDED
+from landscape.manager.manager import FAILED, SUCCEEDED
 
 
 class PackageChangerTestMixin(object):
@@ -1037,6 +1037,31 @@
             self.assertIn("(2)", text)
         return result.addCallback(got_result)
 
+    def test_change_package_holds(self):
+        """
+        If C{SmartFacade} is used, the L{PackageChanger.handle_tasks}
+        method fails the activity, since it can't add or remove dpkg holds.
+        """
+        self.facade.reload_channels()
+        self.store.add_task("changer", {"type": "change-package-holds",
+                                        "create": ["name1"],
+                                        "delete": ["name2"],
+                                        "operation-id": 123})
+
+        def assert_result(result):
+            self.assertIn("Queuing message with change package holds results "
+                          "to exchange urgently.", self.logfile.getvalue())
+            self.assertMessages(
+                self.get_pending_messages(),
+                [{"type": "operation-result",
+                  "operation-id": 123,
+                  "status": FAILED,
+                  "result-text": "This client doesn't support package holds.",
+                  "result-code": 1}])
+
+        result = self.changer.handle_tasks()
+        return result.addCallback(assert_result)
+
 
 class AptPackageChangerTest(LandscapeTest, PackageChangerTestMixin):
 
@@ -1129,3 +1154,100 @@
     def get_package_name(self, version):
         """Return the name of the package."""
         return version.package.name
+
+    def test_change_package_holds(self):
+        """
+        The L{PackageChanger.handle_tasks} method appropriately creates and
+        deletes package holds as requested by the C{change-package-holds}
+        message.
+        """
+        self._add_system_package("foo")
+        self._add_system_package("bar")
+        self.facade.set_package_hold("bar")
+        self.facade.reload_channels()
+        self.store.add_task("changer", {"type": "change-package-holds",
+                                        "create": ["foo"],
+                                        "delete": ["bar"],
+                                        "operation-id": 123})
+
+        def assert_result(result):
+            self.assertEqual(["foo"], self.facade.get_package_holds())
+            self.assertIn("Queuing message with change package holds results "
+                          "to exchange urgently.", self.logfile.getvalue())
+            self.assertMessages(
+                self.get_pending_messages(),
+                [{"type": "operation-result",
+                  "operation-id": 123,
+                  "status": SUCCEEDED,
+                  "result-text": "Package holds successfully changed.",
+                  "result-code": 0}])
+
+        result = self.changer.handle_tasks()
+        return result.addCallback(assert_result)
+
+    def test_change_package_holds_create_not_installed(self):
+        """
+        If the C{change-package-holds} message requests to add holds for
+        packages that aren't installed, the whole activity is failed. If
+        multiple holds are specified, those won't be added. There's no
+        difference between a package that is available in some
+        repository and a package that the facade doesn't know about at
+        all.
+        """
+        self._add_system_package("foo")
+        self._add_package_to_deb_dir(self.repository_dir, "bar")
+        self.facade.reload_channels()
+        self.store.add_task("changer", {"type": "change-package-holds",
+                                        "create": ["foo", "bar", "baz"],
+                                        "operation-id": 123})
+
+        def assert_result(result):
+            self.facade.reload_channels()
+            self.assertEqual([], self.facade.get_package_holds())
+            self.assertIn("Queuing message with change package holds results "
+                          "to exchange urgently.", self.logfile.getvalue())
+            self.assertMessages(
+                self.get_pending_messages(),
+                [{"type": "operation-result",
+                  "operation-id": 123,
+                  "status": FAILED,
+                  "result-text": "Package holds not added, since the "
+                                  "following packages are not installed: "
+                                  "bar, baz",
+                  "result-code": 1}])
+
+        result = self.changer.handle_tasks()
+        return result.addCallback(assert_result)
+
+    def test_change_package_holds_delete_not_held(self):
+        """
+        If the C{change-package-holds} message requests to remove holds
+        for packages that aren't held, the activity still succeeds. If
+        other valid holds are specified, those will be removed.
+        There's no difference between a package that is installed
+        and a package that isn't installed. In either case, the
+        result is that the package isn't held.
+        """
+        self._add_system_package("foo")
+        self._add_system_package("bar")
+        self.facade.set_package_hold("bar")
+        self.facade.reload_channels()
+        self.store.add_task("changer", {"type": "change-package-holds",
+                                        "delete": ["foo", "bar", "baz"],
+                                        "operation-id": 123})
+
+        def assert_result(result):
+            self.facade.reload_channels()
+            self.assertEqual([], self.facade.get_package_holds())
+            self.assertIn("Queuing message with change package holds results "
+                          "to exchange urgently.", self.logfile.getvalue())
+            self.assertMessages(
+                self.get_pending_messages(),
+                [{"type": "operation-result",
+                  "operation-id": 123,
+                  "status": SUCCEEDED,
+                  "result-text": "Package holds successfully changed.",
+                  "result-code": 0}])
+
+        result = self.changer.handle_tasks()
+        return result.addCallback(assert_result)

=== modified file 'landscape/package/tests/test_facade.py'
--- landscape/package/tests/test_facade.py	2012-01-16 15:08:15 +0000
+++ landscape/package/tests/test_facade.py	2012-01-20 12:58:25 +0000
@@ -1385,6 +1385,95 @@
             sorted(error.packages, key=self.version_sortkey),
             sorted([bar, baz], key=self.version_sortkey))
 
+    def test_get_package_holds_with_no_hold(self):
+        """
+        If no package holds are set, C{get_package_holds} returns
+        an empty C{list}.
+        """
+        self._add_system_package("foo")
+        self.facade.reload_channels()
+        self.assertEqual([], self.facade.get_package_holds())
+
+    def test_get_package_holds_with_holds(self):
+        """
+        If package holds are set, C{get_package_holds} returns
+        the name of the packages that are held.
+        """
+        self._add_system_package(
+            "foo", control_fields={"Status": "hold ok installed"})
+        self._add_system_package("bar")
+        self._add_system_package(
+            "baz", control_fields={"Status": "hold ok installed"})
+        self.facade.reload_channels()
+
+        self.assertEqual(
+            ["baz", "foo"], sorted(self.facade.get_package_holds()))
+
+    def test_set_package_hold(self):
+        """
+        C{set_package_hold} marks a package to be on hold.
+        """
+        self._add_system_package("foo")
+        self.facade.reload_channels()
+        self.facade.set_package_hold("foo")
+        self.facade.reload_channels()
+
+        self.assertEqual(["foo"], self.facade.get_package_holds())
+
+    def test_set_package_hold_existing_hold(self):
+        """
+        If a package is already hel, C{set_package_hold} doesn't return
+        an error.
+        """
+        self._add_system_package(
+            "foo", control_fields={"Status": "hold ok installed"})
+        self.facade.reload_channels()
+        self.facade.set_package_hold("foo")
+        self.facade.reload_channels()
+
+        self.assertEqual(["foo"], self.facade.get_package_holds())
+
+    def test_remove_package_hold(self):
+        """
+        C{remove_package_hold} marks a package not to be on hold.
+        """
+        self._add_system_package(
+            "foo", control_fields={"Status": "hold ok installed"})
+        self.facade.reload_channels()
+        self.facade.remove_package_hold("foo")
+        self.facade.reload_channels()
+
+        self.assertEqual([], self.facade.get_package_holds())
+
+    def test_remove_package_hold_no_package(self):
+        """
+        If a package doesn't exist, C{remove_package_hold} doesn't
+        return an error. It's up to the caller to make sure that the
+        package exist, if it's important.
+        """
+        self._add_system_package("foo")
+        self.facade.reload_channels()
+        self.facade.remove_package_hold("bar")
+        self.facade.reload_channels()
+
+        self.assertEqual([], self.facade.get_package_holds())
+
+    def test_remove_package_hold_no_hold(self):
+        """
+        If a package isn't held, the existing selection is retained when
+        C{remove_package_hold} is called.
+        """
+        self._add_system_package(
+            "foo", control_fields={"Status": "deinstall ok installed"})
+        self.facade.reload_channels()
+        self.facade.remove_package_hold("foo")
+        self.facade.reload_channels()
+
+        self.assertEqual([], self.facade.get_package_holds())
+        [foo] = self.facade.get_packages_by_name("foo")
+        self.assertEqual(
+            apt_pkg.SELSTATE_DEINSTALL, foo.package._pkg.selected_state)
+
     if not hasattr(Package, "shortname"):
         # The 'shortname' attribute was added when multi-arch support
         # was added to python-apt. So if it's not there, it means that