Merge lp:~benji/landscape-client/bug-1508110-resync-users-on-upgrade into lp:~landscape/landscape-client/trunk

[Benji York]

Add a complete refresh of user data after client install (remediation
for bug 1508110).

This branch is a remediation for bug 1508110 which caused incomplete
user data to be stored in LS.

Upon client install, the postinst script creates a flag file in the
client data directory which signals a complete refresh of user data.
Then when the usermonitor is triggered, it checks for the flag file
and if present sends a total refresh of user data (deleting the flag
file on completion).

Testing instructions:

- build a new set of packages (see "make package")
- stop the client on the test machine
- install -common and -client packages
- if you don't want to wait an hour for the full refresh to be
  triggered, edit the run_interval class attribute in usermonitor.py
- verify that /var/lib/landscape/client/user-update-flag exists
- set "log_level = debug" in /etc/landscape/client.conf
- start the client
- wait run_interval seconds and verify that a complete refresh took
  place via /var/log/landscape/monitor.log
- verify that /var/lib/landscape/client/user-update-flag has been
  removed
- if you were lucky enough to be testing with an account with
  incomplete user data, verify that LS now lists complete user data
  for the computer in question

[Chad Smith (chad.smith)]

Ok got through this, thank you.

Note 1:
Chatted a bit in IRC about the round trip between client sending this initial duplicate user data to OPL and whether we care about trimming that initial data to a single duplicated pair of user rows because we know it will likely all be thrown out anyway by OPL and re-requested.

Answer: We don't care about trimming the initial failed client user sync. Let the resync roundtrip from OPL handle things since this is a one time operation.

The OPL traceback and user scoped resynch request is here for reference.
https://pastebin.canonical.com/149414/

Marked needs fixing only to see the addressed postinst logic to handle upgrade path.

[Chad Smith (chad.smith)]

review: Needs Fixing

[lp:~benji/landscape-client/bug-1508110-resync-users-on-upgrade]

830. By Benji York on 2016-02-10

add code to postinst to respect the user's data_dir and a big comment explaining this insanity

[Benji York (benji)]

Chad, I've addressed your comments. Thanks very much for the good review.

[lp:~benji/landscape-client/bug-1508110-resync-users-on-upgrade]

831. By Benji York on 2016-02-10

remove uneeded variable

[Simon Poirier (simpoir)]

+1

review: Approve

[Chad Smith (chad.smith)]

Excellent, we can now resync the masses.

review: Approve

[Chad Smith (chad.smith)]

Upgrade path from a broken client as well greenfield registration tested with these new bits.

review: Approve

[lp:~benji/landscape-client/bug-1508110-resync-users-on-upgrade]

832. By Benji York on 2016-02-11

merge from trunk

=== modified file 'debian/landscape-client.postinst'
--- debian/landscape-client.postinst	2013-05-21 13:34:24 +0000
+++ debian/landscape-client.postinst	2016-02-11 14:22:25 +0000
@@ -119,6 +119,13 @@
             fi
          fi
 
+        # In response to bug 1508110 we need to trigger a complete update of
+        # user information.  The flag file will be removed by the client when
+        # the update completes.
+        DATA_PATH="`grep ^data_path /etc/landscape/client.conf | cut -d= -f2 | tr -d '[[:space:]]'`"
+        USER_UPDATE_FLAG_FILE="$DATA_PATH/user-update-flag"
+        echo "This file indicates that the Landscape client needs to send updated user information to the server." >> $USER_UPDATE_FLAG_FILE
+        chown landscape $USER_UPDATE_FLAG_FILE
     ;;
 
     abort-upgrade|abort-remove|abort-deconfigure)
 
=== modified file 'landscape/monitor/tests/test_usermonitor.py'
--- landscape/monitor/tests/test_usermonitor.py	2016-01-15 16:13:04 +0000
+++ landscape/monitor/tests/test_usermonitor.py	2016-02-11 14:22:25 +0000
@@ -1,3 +1,6 @@
+import os
+import tempfile
+
 from twisted.internet.defer import fail
 
 from landscape.amp import ComponentPublisher
@@ -7,6 +10,7 @@
 from landscape.user.tests.helpers import FakeUserProvider
 from landscape.tests.helpers import LandscapeTest, MonitorHelper
 from landscape.tests.mocker import ANY
+import landscape.monitor.usermonitor
 
 
 class UserMonitorNoManagerTest(LandscapeTest):
@@ -59,10 +63,16 @@
         self.publisher.start()
         self.provider = FakeUserProvider()
         self.plugin = UserMonitor(self.provider)
+        # Part of bug 1048576 remediation:
+        self._original_USER_UPDATE_FLAG_FILE = (
+            landscape.monitor.usermonitor.USER_UPDATE_FLAG_FILE)
 
     def tearDown(self):
         self.publisher.stop()
         self.plugin.stop()
+        # Part of bug 1048576 remediation:
+        landscape.monitor.usermonitor.USER_UPDATE_FLAG_FILE = (
+            self._original_USER_UPDATE_FLAG_FILE)
         return super(UserMonitorTest, self).tearDown()
 
     def test_constants(self):
@@ -327,6 +337,65 @@
         result.addCallback(lambda x: connector.disconnect())
         return result
 
+    def test_detect_changes_clears_user_provider_if_flag_file_exists(self):
+        """
+        Temporary bug 1508110 remediation: If a special flag file exists,
+        cached user data is dumped and a complete refresh of all user data is
+        transmitted.
+        """
+        self.monitor.add(self.plugin)
+
+        class FauxUserChanges(object):
+            cleared = False
+
+            def __init__(self, *args):
+                pass
+
+            def create_diff(self):
+                return None
+
+            def clear(self):
+                self.__class__.cleared = True
+
+        # Create the (temporary, test) user update flag file.
+        landscape.monitor.usermonitor.USER_UPDATE_FLAG_FILE = \
+            update_flag_file = tempfile.mkstemp()[1]
+        self.addCleanup(lambda: os.remove(update_flag_file))
+
+        # Trigger a detect changes.
+        self.plugin._persist = None
+        self.plugin._detect_changes([], UserChanges=FauxUserChanges)
+
+        # The clear() method was called.
+        self.assertTrue(FauxUserChanges.cleared)
+
+    def test_detect_changes_deletes_flag_file(self):
+        """
+        Temporary bug 1508110 remediation: The total user data refresh flag
+        file is deleted once the data has been sent.
+        """
+
+        def got_result(result):
+            # The flag file has been deleted.
+            self.assertFalse(os.path.exists(update_flag_file))
+
+        # Create the (temporary, test) user update flag file.
+        landscape.monitor.usermonitor.USER_UPDATE_FLAG_FILE = \
+            update_flag_file = tempfile.mkstemp()[1]
+
+        self.broker_service.message_store.set_accepted_types(["users"])
+        self.provider.users = [("jdoe", "x", 1000, 1000, "JD,,,,",
+                                "/home/jdoe", "/bin/sh")]
+        self.provider.groups = []
+
+        self.monitor.add(self.plugin)
+        connector = RemoteUserMonitorConnector(self.reactor, self.config)
+        result = connector.connect()
+        result.addCallback(lambda remote: remote.detect_changes())
+        result.addCallback(got_result)
+        result.addCallback(lambda x: connector.disconnect())
+        return result
+
     def test_no_message_if_not_accepted(self):
         """
         Don't add any messages at all if the broker isn't currently
 
=== modified file 'landscape/monitor/usermonitor.py'
--- landscape/monitor/usermonitor.py	2016-01-13 13:54:28 +0000
+++ landscape/monitor/usermonitor.py	2016-02-11 14:22:25 +0000
@@ -1,3 +1,7 @@
+import logging
+import os
+import os.path
+
 from twisted.internet.defer import maybeDeferred
 
 from landscape.lib.log import log_failure
@@ -8,6 +12,10 @@
 from landscape.user.provider import UserProvider
 
 
+# Part of bug 1048576 remediation:
+USER_UPDATE_FLAG_FILE = "user-update-flag"
+
+
 class UserMonitor(MonitorPlugin):
     """
     A plugin which monitors the system user databases.
@@ -87,7 +95,8 @@
             result.addErrback(lambda f: self._detect_changes([], operation_id))
         return result
 
-    def _detect_changes(self, locked_users, operation_id=None):
+    def _detect_changes(self, locked_users, operation_id=None,
+            UserChanges=UserChanges):
 
         def update_snapshot(result):
             changes.snapshot()
@@ -99,6 +108,22 @@
 
         self._provider.locked_users = locked_users
         changes = UserChanges(self._persist, self._provider)
+
+        # Part of bug 1048576 remediation: If the flag file exists, we need to
+        # do a full update of user data.
+        full_refresh = os.path.exists(self.user_update_flag_file_path)
+        if full_refresh:
+            # Clear the record of what changes have been sent to the server in
+            # order to force sending of all user data which will do one of two
+            # things server side:  either the server has no user data at all,
+            # in which case it will now have a complete copy, otherwise it
+            # will have at least some user data which this message will
+            # duplicate, provoking the server to note the inconsistency and
+            # request a full resync of the user data.  Either way, the result
+            # is the same: the client and server will be in sync with regard
+            # to users.
+            changes.clear()
+
         message = changes.create_diff()
 
         if message:
@@ -108,9 +133,35 @@
             result = self.registry.broker.send_message(
                 message, self._session_id, urgent=True)
             result.addCallback(update_snapshot)
+
+            # Part of bug 1048576 remediation:
+            if full_refresh:
+                # If we are doing a full refresh, we want to remove the flag
+                # file that triggered the refresh if it completes successfully.
+                result.addCallback(lambda _: self._remove_update_flag_file())
+
             result.addErrback(log_error)
             return result
 
+    def _remove_update_flag_file(self):
+        """Remove the full update flag file, logging any errors.
+
+        This is part of the bug 1048576 remediation.
+        """
+        try:
+            os.remove(self.user_update_flag_file_path)
+        except OSError:
+            logging.exception("Error removing user update flag file.")
+
+    @property
+    def user_update_flag_file_path(self):
+        """Location of the user update flag file.
+
+        This is part of the bug 1048576 remediation.
+        """
+        return os.path.join(
+            self.registry.config.data_path, USER_UPDATE_FLAG_FILE)
+
 
 class RemoteUserMonitorConnector(ComponentConnector):
 
 
=== modified file 'landscape/user/tests/helpers.py'
--- landscape/user/tests/helpers.py	2008-06-16 15:42:13 +0000
+++ landscape/user/tests/helpers.py	2016-02-11 14:22:25 +0000
@@ -42,7 +42,7 @@
             gecos_string = "%s,%s,%s,%s" % (name, location or "",
                                             work_phone or "", home_phone or "")
             userdata = (username, "x", uid, primary_gid, gecos_string,
-                        "/bin/sh" , "/home/user")
+                        "/bin/sh", "/home/user")
             self.provider.users.append(userdata)
         except KeyError:
             raise UserManagementError("add_user failed")
@@ -61,7 +61,8 @@
         data = self._users.get(username, None)
         if data:
             data["enabled"] = True
-            # This will generate a shadow file with only the unlocked user in it.
+            # This will generate a shadow file with only the unlocked user in
+            # it.
             self._make_fake_shadow_file([], [username])
             return "unlock_user succeeded"
         raise UserManagementError("unlock_user failed")
@@ -94,7 +95,7 @@
         userdata = (username, "x", data["uid"], data["primary-gid"],
                     "%s,%s,%s,%s," % (name, location, work_number,
                                       home_number),
-                    "/bin/sh" , "/home/user")
+                    "/bin/sh", "/home/user")
         self.provider.users = [userdata]
         return "set_user_details succeeded"
 
@@ -150,7 +151,7 @@
 
 class FakeUserProvider(UserProviderBase):
 
-    def __init__(self, users=None, groups=None, popen=None, shadow_file=None, 
+    def __init__(self, users=None, groups=None, popen=None, shadow_file=None,
                  locked_users=None):
         self.users = users
         self.groups = groups
@@ -169,6 +170,7 @@
             self.groups = []
         return self.groups
 
+
 class FakeUserInfo(object):
     """Implements enough functionality to work for Changes tests."""