Merge lp:~hazmat/pyjuju/formula-upgrade-removes-obsolete into lp:pyjuju

[Kapil Thangavelu]

On upgrade old formula files and directories not present in the new formula are removed. Also adds a dirent manifest for formulas. In retrospect this might have been achievable more simply with just removing the old formula dir while hook execution is stopped, and extracting the new one.. i had already implemented most of this before i realized that though..

[Gustavo Niemeyer (niemeyer)]

[1]

25 + stale_file_path = os.path.join(formula_dir, "old-file")
26 + fh = open(stale_file_path, "w")
27 + fh.write("hello world")
28 + fh.close()

That's not a stale file, but a file created dynamically. I'm a bit concerned
about killing arbitrary files that the formula created, as it's basically saying
that the formula directory is necessarily read-only. Is that what we want?

review: Needs Information

[Kapil Thangavelu (hazmat)]

I talked with gustavo about this, and we settled on an explicit manifest of install files to be used as a delta, to avoid killing any formula state files. alternatively we'd have to specify a state dir for formulas like 'var'.

Unmerged revisions

265. By Kapil Thangavelu on 2011-07-05

stale formula files are removed on formula upgrade.

264. By Kapil Thangavelu on 2011-07-05

dynamically computed manifests for formulas

=== modified file 'ensemble/agents/tests/test_unit.py'
--- ensemble/agents/tests/test_unit.py	2011-05-26 17:01:24 +0000
+++ ensemble/agents/tests/test_unit.py	2011-07-05 17:39:27 +0000
@@ -1,6 +1,7 @@
 import argparse
 import logging
 import os
+import stat
 import yaml
 
 from twisted.internet.defer import (
@@ -501,16 +502,51 @@
         yield self.agent.workflow.fire_transition("stop")
 
     @inlineCallbacks
+    def test_upgrade_removes_stale_files(self):
+        """A formula upgrade removes any old formula files."""
+        self.agent.set_watch_enabled(False)
+        yield self.agent.startService()
+        yield self.mark_formula_upgrade()
+
+        # Add a stale entries which should get removed.
+        formula_dir = os.path.join(self.agent.unit_directory, "formula")
+
+        stale_file_path = os.path.join(formula_dir, "old-file")
+        fh = open(stale_file_path, "w")
+        fh.write("hello world")
+        fh.close()
+
+        stale_hook_path = os.path.join(formula_dir, "hooks", "stop")
+        fh = open(stale_hook_path, "w")
+        fh.write("#!/bin/bash\necho done")
+        fh.close()
+
+        stale_dir = os.path.join(formula_dir, "empty")
+        os.mkdir(stale_dir)
+
+        stale = [stale_file_path, stale_hook_path, stale_dir]
+
+        # Perform the upgrade and verify
+        hook_done = self.wait_on_hook(
+            "upgrade-formula", executor=self.agent.executor)
+
+        upgrade = FormulaUpgradeOperation(self.agent)
+        value = yield upgrade.run()
+        self.assertTrue(value)
+        yield hook_done
+
+        for stale_path in stale:
+            self.assertFalse(os.path.exists(stale_path))
+
+    @inlineCallbacks
     def test_agent_upgrade(self):
         """The agent can succesfully upgrade its formula."""
         self.agent.set_watch_enabled(False)
         yield self.agent.startService()
-
         yield self.mark_formula_upgrade()
 
         hook_done = self.wait_on_hook(
             "upgrade-formula", executor=self.agent.executor)
-        self.write_hook("upgrade-formula", "#!/bin/bash\nexit 0")
         output = self.capture_logging("unit.upgrade", level=logging.DEBUG)
 
         # Do the upgrade
@@ -589,8 +625,19 @@
         self.agent.set_watch_enabled(False)
         yield self.agent.startService()
 
-        # Upload a new version of the unit's formula
+        # Upload a new version of the unit's formula, with a bad
+        # upgrade hook.
         repository = self.increment_formula(self.formula)
+        formula_dir = repository.find("mysql").path
+        os.mkdir(os.path.join(formula_dir, "hooks"))
+        upgrade_hook_path = os.path.join(
+            formula_dir, "hooks", "upgrade-formula")
+
+        fp = open(upgrade_hook_path, "w")
+        fp.write("#!/bin/bash\nexit 1")
+        fp.close()
+        os.chmod(upgrade_hook_path, stat.S_IRWXU)
+
         formula, formula_state = yield self.publish_formula(
             repository.find("mysql").path)
 
@@ -600,7 +647,6 @@
 
         hook_done = self.wait_on_hook(
             "upgrade-formula", executor=self.agent.executor)
-        self.write_hook("upgrade-formula", "#!/bin/bash\nexit 1")
         output = self.capture_logging("unit.upgrade", level=logging.DEBUG)
 
         # Do the upgrade
 
=== modified file 'ensemble/agents/unit.py'
--- ensemble/agents/unit.py	2011-05-24 18:07:03 +0000
+++ ensemble/agents/unit.py	2011-07-05 17:39:27 +0000
@@ -6,6 +6,7 @@
 from twisted.internet.defer import inlineCallbacks, returnValue
 
 from ensemble.errors import EnsembleError
+from ensemble.formula import get_formula_from_path
 from ensemble.state.service import ServiceStateManager
 from ensemble.hooks.protocol import UnitSettingsFactory
 from ensemble.hooks.executor import HookExecutor
@@ -195,6 +196,21 @@
             shutil.rmtree(self._formula_directory)
         return result
 
+    def _remove_stale(self, new_formula, formula_dir):
+        new_entries = set(new_formula.manifest())
+        old_entries = set(formula_dir.manifest())
+
+        stale_entries = list(old_entries - new_entries)
+        stale_entries.sort(lambda x, y: cmp(len(x), len(y)))
+        stale_entries.reverse()
+
+        for entry in stale_entries:
+            entry_path = os.path.join(formula_dir.path, entry)
+            if entry_path.endswith("/"):
+                os.rmdir(entry_path)
+            else:
+                os.remove(entry_path)
+
     def run(self):
         d = self._run()
         d.addBoth(self._remove_tree)
@@ -254,13 +270,13 @@
         yield self._agent.unit_state.set_formula_id(service_formula_id)
 
         # Extract formula
+        formula_dir = os.path.join(self._agent.unit_directory, "formula")
         self._log.debug("Extracting new formula.")
-        formula.extract_to(
-            os.path.join(self._agent.unit_directory, "formula"))
+        self._remove_stale(formula, get_formula_from_path(formula_dir))
+        formula.extract_to(formula_dir)
 
         # Upgrade
         self._log.debug("Invoking upgrade transition.")
-
         success = yield self._agent.workflow.fire_transition(
             "upgrade_formula")
 
 
=== modified file 'ensemble/formula/base.py'
--- ensemble/formula/base.py	2010-08-18 15:23:10 +0000
+++ ensemble/formula/base.py	2011-07-05 17:39:27 +0000
@@ -33,3 +33,10 @@
         if self._sha256 is None:
             self._sha256 = self.compute_sha256()
         return self._sha256
+
+    def manifest(self):
+        """Return a file and directory manifest as a list of entries.
+
+        The entries are relative to the formula.
+        """
+        self._unsupported("manifest()")
 
=== modified file 'ensemble/formula/bundle.py'
--- ensemble/formula/bundle.py	2011-06-05 20:19:18 +0000
+++ ensemble/formula/bundle.py	2011-07-05 17:39:27 +0000
@@ -64,3 +64,10 @@
         path"""
         dn = tempfile.mkdtemp(prefix="tmp")
         return self.extract_to(dn)
+
+    def manifest(self):
+        """Return a file and directory manifest as a list of entries.
+
+        The entries are relative to the formula.
+        """
+        return ZipFile(self.path, "r").namelist()
 
=== modified file 'ensemble/formula/directory.py'
--- ensemble/formula/directory.py	2011-06-09 07:10:07 +0000
+++ ensemble/formula/directory.py	2011-07-05 17:39:27 +0000
@@ -1,6 +1,7 @@
+import itertools
 import os
+import tempfile
 import zipfile
-import tempfile
 
 from ensemble.formula.config import ConfigOptions
 from ensemble.formula.metadata import MetaData
@@ -77,3 +78,22 @@
         Compute sha256, based on the bundle.
         """
         return self.as_bundle().compute_sha256()
+
+    def manifest(self):
+        """Return a file and directory manifest as a list of entries.
+
+        The entries are relative to the formula.
+        """
+        entries = []
+        for dirpath, dirnames, filenames in os.walk(self.path):
+            relative_path = dirpath[len(self.path):]
+            dirnames = ["%s/" % d for d in dirnames]
+            for dirent in itertools.chain(dirnames, filenames):
+                entry = "/".join((relative_path, dirent))
+                if entry.startswith("/"):
+                    entry = entry[1:]
+                if self._ignore(entry):
+                    continue
+                entries.append(entry)
+
+        return entries
 
=== modified file 'ensemble/formula/tests/test_base.py'
--- ensemble/formula/tests/test_base.py	2010-09-09 18:34:47 +0000
+++ ensemble/formula/tests/test_base.py	2011-07-05 17:39:27 +0000
@@ -28,6 +28,10 @@
         self.assertUnsupported(lambda: self.formula.compute_sha256(),
                                "compute_sha256()")
 
+    def test_manifest_is_unsupported(self):
+        self.assertUnsupported(lambda: self.formula.manifest(),
+                               "manifest()")
+
     def test_get_sha256_fails_if_not_preset(self):
         """
         get_sha256() will internally call compute_sha256() if
 
=== modified file 'ensemble/formula/tests/test_bundle.py'
--- ensemble/formula/tests/test_bundle.py	2011-05-06 04:45:17 +0000
+++ ensemble/formula/tests/test_bundle.py	2011-07-05 17:39:27 +0000
@@ -131,3 +131,10 @@
         fn = os.path.split(f2.path)[1]
         # verify that it used the expected prefix
         self.assertStartsWith(fn, "tmp")
+
+    def test_manifest(self):
+        """A formula manifest contains all directories and files contained."""
+        bundle = FormulaBundle(self.filename)
+        self.assertEqual(bundle.manifest(),
+                         ["config.yaml", "metadata.yaml", "src/",
+                          "src/hello.c", "empty/"])
 
=== modified file 'ensemble/formula/tests/test_directory.py'
--- ensemble/formula/tests/test_directory.py	2011-05-06 04:45:17 +0000
+++ ensemble/formula/tests/test_directory.py	2011-07-05 17:39:27 +0000
@@ -56,7 +56,11 @@
         included = [info.filename for info in zf.infolist()]
         self.assertEqual(
             set(included),
-            set(("metadata.yaml", "empty/", "src/", "src/hello.c", "config.yaml")))
+            set(("metadata.yaml",
+                 "empty/",
+                 "src/",
+                 "src/hello.c",
+                 "config.yaml")))
 
     def test_as_bundle(self):
         directory = FormulaDirectory(self.sample_dir1)
@@ -131,3 +135,15 @@
         self.assertEquals(directory.config.get_serialization_data(),
                           sample_yaml_data)
 
+    def test_manifest(self):
+        """A formula manifest contains all directories and files contained."""
+        directory = FormulaDirectory(self.sample_dir1)
+        self.assertEqual(directory.manifest(), ["metadata.yaml"])
+        directory = FormulaDirectory(sample_directory)
+        self.assertEqual(
+            set(directory.manifest()),
+            set(("metadata.yaml",
+                 "empty/",
+                 "src/",
+                 "src/hello.c",
+                 "config.yaml")))
 
=== modified file 'ensemble/unit/tests/test_lifecycle.py'
--- ensemble/unit/tests/test_lifecycle.py	2011-05-26 11:20:43 +0000
+++ ensemble/unit/tests/test_lifecycle.py	2011-07-05 17:39:27 +0000
@@ -62,7 +62,16 @@
         self.unit_directory = os.path.join(self.ensemble_directory,
             "units",
             self.states["unit"].unit_name.replace("/", "-"))
+
         os.makedirs(os.path.join(self.unit_directory, "formula", "hooks"))
+        fp = open(os.path.join(
+            self.unit_directory, "formula", "metadata.yaml"), "w")
+        fp.write(yaml.dump(
+            {"ensemble": "formula",
+             "revision": 1,
+             "name": "mysql",
+             "description": "db",
+             "summary": "db"}))
         os.makedirs(os.path.join(self.ensemble_directory, "state"))
 
     def write_hook(self, name, text, no_exec=False):