Merge lp:~benji/lpsetup/bug-1023519-store-version-information into lp:lpsetup

[Benji York]

adds creation of a version directory (/var/lib/lpsetup/VERSION_NUMBER) and backing up of system files to a new "originals" directory underneath it

This branch adds creation of a version directory (/var/lib/lpsetup/VERSION_NUMBER) and backing up of system files to a new "originals" directory underneath it.

This branch fixes bug 1023519 but we may want to create a new follow-up bug that contains the parts of 1023519 that were optional and this branch does not address.

[j.c.sackett (jcsackett)]

There doesn't actually appear to be a diff here, Benji. Expected, or a commit/MP error?

[lp:~benji/lpsetup/bug-1023519-store-version-information]

52. By Benji York on 2012-07-13

add a version directory and backing up of system files we change/delete

[Benji York (benji)]

> There doesn't actually appear to be a diff here, Benji. Expected, or a
> commit/MP error?

Heh, yeah PEBKAC error. It should be better now.

[lp:~benji/lpsetup/bug-1023519-store-version-information]

53. By Benji York on 2012-07-13

merge from tunk, fixing conflicts

[Brad Crittenden (bac)]

Benji,

* It is a wee optimization in make_version_dir but since mkdirs creates parents you could eliminate the call mkdirs(version_path()) and just do mkdirs(originals). Doing so will have impact on your tests, though, as your mock mkdirs has different behavior.

* make_version_dir should utilize backup_path.

* Why explicitly use os.path.sep instead of os.path.join?

* I would expand your comment in the docstring for initialize_lxc. I had to read it a few times before realizing what you meant. Perhaps adding "so that it is only called when in a container" would make it clearer.

* In make_backup I was initially confused until i saw that backup_path is not a path but a method. Perhaps a name that would make that more obvious could be used.

Does this strategy of using the version number for storing the original files put any restrictions on how we update the version? Or is the idea that when the version changes we'll first restore the files saved in the previous version, remove the saved copies, and then proceed, applying the new workarounds under the new version number?

review: Approve (code)

[Benji York (benji)]

> Benji,
>
> * It is a wee optimization in make_version_dir but since mkdirs creates
> parents you could eliminate the call mkdirs(version_path()) and just do
> mkdirs(originals). Doing so will have impact on your tests, though, as your
> mock mkdirs has different behavior.

Right, I think it's a slight win to keep the version and originals
directories explicitly created.

> * make_version_dir should utilize backup_path.

Good catch! Fixed.

> * Why explicitly use os.path.sep instead of os.path.join?

I guess that bit deserves a comment. Here is the one I added:

# We don't use os.path.join here because both of the input paths are
# absolute (i.e., they start with a slash) so we instead construct the
# path ourselves in order to "re-root" the otherwise absolute source path
# in the backup directory.

> * I would expand your comment in the docstring for initialize_lxc. I had to
> read it a few times before realizing what you meant. Perhaps adding "so that
> it is only called when in a container" would make it clearer.

Good idea. Done.

> * In make_backup I was initially confused until i saw that backup_path is not
> a path but a method. Perhaps a name that would make that more obvious could
> be used.

I vacillated a bit on that. I normally prefer verb function names too.
You mentioning it has pushed me back to the other side. I changed
backup_path and version_path to be get_backup_path and get_version_path.

> Does this strategy of using the version number for storing the original files
> put any restrictions on how we update the version? Or is the idea that when
> the version changes we'll first restore the files saved in the previous
> version, remove the saved copies, and then proceed, applying the new
> workarounds under the new version number?

We aren't exactly sure how the saved files will be used, if at all. At
a minimum they provide some protection against a bug in lpsetup
catastrophically modifying a person's system. They also provide some
hint as to what lpsetup has changed (after the fact).

[lp:~benji/lpsetup/bug-1023519-store-version-information]

54. By Benji York on 2012-07-13

respond to code review by renaming some methods and adding/enchancing some
comments

[Gary Poster (gary)]

> Or is the idea that when
> the version changes we'll first restore the files saved in the previous
> version, remove the saved copies, and then proceed, applying the new
> workarounds under the new version number?

Yeah, that's my idea, at least.

=== modified file 'lpsetup/subcommands/inithost.py'
--- lpsetup/subcommands/inithost.py	2012-07-12 22:12:09 +0000
+++ lpsetup/subcommands/inithost.py	2012-07-13 15:18:33 +0000
@@ -28,7 +28,10 @@
     user_exists,
     )
 
-from lpsetup import argparser
+from lpsetup import (
+    argparser,
+    get_version,
+)
 from lpsetup.handlers import (
     handle_lpuser_as_username,
     handle_ssh_keys,
@@ -53,6 +56,31 @@
     )
 
 
+def get_version_path():
+    return '/var/lib/lpsetup/' + get_version()
+
+
+def get_backup_path():
+    return os.path.join(get_version_path(), 'originals')
+
+
+def make_version_dir(mkdirs=mkdirs):
+    mkdirs(get_version_path())
+    mkdirs(get_backup_path())
+
+
+def make_backup(source, get_backup_path=get_backup_path, mkdirs=mkdirs):
+    """Make a backup of a file in the current version's backup location."""
+    # We don't use os.path.join here because both of the input paths are
+    # absolute (i.e., they start with a slash) so we instead construct the
+    # path ourselves in order to "re-root" the otherwise absolute source path
+    # in the backup directory.
+    destination = os.path.normpath(get_backup_path() + os.path.sep + source)
+    destination_path = os.path.dirname(destination)
+    mkdirs(destination_path)
+    shutil.copyfile(source, destination)
+
+
 def write_file_contents(filename, contents, mode, header=None):
     """Write `contents` to filename and chmod it to 0644."""
     assert mode in ('w', 'a'), "Mode should be 'w' or 'a' only."
@@ -121,6 +149,7 @@
     user, full_name, email, lpuser, private_key, public_key, valid_ssh_keys,
     ssh_key_path):
     """Initialize host machine."""
+    make_version_dir()
     initialize_base(user)
     with su(user) as env:
         ssh_dir = os.path.join(env.home, '.ssh')
@@ -143,12 +172,17 @@
     lines = [get_file_header()]
     lines.extend(['{0}\t{1}\n'.format(ip, names)
                   for ip, names in HOSTS_CONTENT])
+    make_backup(HOSTS_FILE)
     for line in lines:
         file_append(HOSTS_FILE, line)
 
 
 def initialize_lxc():
-    """Initialize LXC container."""
+    """Initialize LXC container.
+
+    Note that this step is guarded by the call_initialize_lxc method so that
+    it is only called when lpsetup is running in a container.
+    """
     lxc_os = get_distro()
     # XXX benji 2012-03-19 bug=959352: this is so graphviz will work in an
     # ephemeral container
@@ -163,10 +197,10 @@
         # wait for udev (that is, use start on (local-filesystems)).
         udevtrigger = '/etc/init/udevtrigger.conf'
         if os.path.exists(udevtrigger):
-            shutil.move(udevtrigger, udevtrigger + '.orig')
+            make_backup(udevtrigger)
+            os.remove(udevtrigger)
         networking = '/etc/init/networking.conf'
-        if not os.path.exists(networking + '.orig'):
-            shutil.move(networking, networking + '.orig')
+        make_backup(networking)
         render_to_file('networking.conf', {}, networking)
 
 

=== modified file 'lpsetup/tests/subcommands/test_inithost.py'
--- lpsetup/tests/subcommands/test_inithost.py	2012-07-12 18:23:37 +0000
+++ lpsetup/tests/subcommands/test_inithost.py	2012-07-13 15:18:33 +0000
@@ -162,3 +162,53 @@
             self.ssh_dir, None, None, False, self.temp_filename)
         self.assertTrue(os.path.exists(self.ssh_dir))
         self.assertTrue(os.path.isdir(self.ssh_dir))
+
+
+class VersionTests(unittest.TestCase):
+    """Tests for version and upgrade data storage."""
+
+    def setUp(self):
+        self.directories_made = []
+
+    def mkdirs(self, directory):
+        self.directories_made.append(directory)
+
+    def test_version_dir_creation(self):
+        # If the version path does not exist, it is created.
+        inithost.make_version_dir(mkdirs=self.mkdirs)
+        self.assertIn(inithost.get_version_path(), self.directories_made)
+
+    def test_originals_dir_creation(self):
+        # If the version path contains an "originals" directory for backups of
+        # files that we modify.
+        inithost.make_version_dir(mkdirs=self.mkdirs)
+        self.assertIn(inithost.get_version_path(), self.directories_made)
+
+
+def create(filename, contents=None):
+    """Create an empty file."""
+    f = open(filename, 'a')
+    if contents is not None:
+        f.write(contents)
+    f.close()
+
+
+class BackupTests(unittest.TestCase):
+    """Make sure the backup functions work correctly."""
+
+    def setUp(self):
+        self.tempdir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        shutil.rmtree(self.tempdir, ignore_errors=True)
+
+    def test_backups(self):
+        # When asked to make a backup, make_backup does.
+        source = os.path.join(self.tempdir, 'source')
+        destination_dir = lambda: self.tempdir
+        contents = get_random_string()
+        create(source, contents)
+        inithost.make_backup(source, destination_dir)
+        get_backup_path = destination_dir() + os.path.sep + source
+        self.assertTrue(os.path.exists(get_backup_path))
+        self.assertEqual(file(get_backup_path).read(), contents)