Merge lp:~ericsnowcurrently/landscape-charm/fix-1610413-collect-inner-model into lp:~landscape/landscape-charm/tools

[Eric Snow]

Make collect-logs work with Juju 2.

Testing instructions:

1. Deploy landscape
2. install juju-2.0 on the landscape-server/0 unit
3. enable the "juju-2.0" feature flag
4. autopilot
5. run collect-logs

[lp:~ericsnowcurrently/landscape-charm/fix-1610413-collect-inner-model]

28. By Eric Snow on 2016-08-17

Revert revision #27.

29. By Eric Snow on 2016-08-17

Revert revision #26.

Unmerged revisions

=== modified file 'collect-logs'
--- collect-logs	2016-08-17 14:14:30 +0000
+++ collect-logs	2016-08-17 14:14:30 +0000
@@ -2,6 +2,7 @@
 
 from argparse import ArgumentParser, ArgumentDefaultsHelpFormatter
 import errno
+import functools
 import logging
 import multiprocessing
 import os
@@ -25,16 +26,24 @@
             "/var/lib/juju/containers/juju-*-lxc-template"]
 LANDSCAPE_JUJU_HOME = "/var/lib/landscape/juju-homes"
 
-
-def juju_status():
+JUJU1 = "juju"
+# XXX This is going to break once juju-2.1 happens.
+# See https://bugs.launchpad.net/juju-core/+bug/1613864.
+JUJU2 = "juju-2.0"
+JUJU = JUJU1
+
+
+def juju_status(juju):
     """Return a juju status structure."""
-    cmd = ["juju", "status", "--format=yaml"]
+    cmd = [juju, "status", "--format=yaml"]
     output = check_output(cmd).decode("utf-8").strip()
     return yaml.load(output)
 
 
-def get_units(status=juju_status()):
+def get_units(juju, status=None):
     """Return a list with all units."""
+    if status is None:
+        status = juju_status(juju)
     units = []
     if "services" in status:
         applications = status["services"]
@@ -52,12 +61,12 @@
     return units
 
 
-def _create_ps_output_file(unit):
+def _create_ps_output_file(juju, unit):
     """List running processes and redirect them to a file."""
     ps_cmd = "ps fauxww | sudo tee /var/log/ps-fauxww.txt"
     try:
         log.info("Collecting ps output on unit {}".format(unit))
-        check_output(["juju", "ssh", unit, ps_cmd], stderr=STDOUT)
+        check_output([juju, "ssh", unit, ps_cmd], stderr=STDOUT)
     except CalledProcessError as e:
         log.warning(
             "Failed to collect running processes on unit {}".format(unit))
@@ -65,7 +74,7 @@
         log.warning(e.returncode)
 
 
-def _create_log_tarball(unit):
+def _create_log_tarball(juju, unit):
     exclude = " ".join(["--exclude=%s" % x for x in EXCLUDED])
     logs = "$(sudo sh -c \"ls -1d %s 2>/dev/null\")" % " ".join(LOGS)
     # note, tar commands can fail since we are backing up log files
@@ -80,7 +89,7 @@
         tar_cmd, exclude, logsuffix, logs)
     try:
         log.info("Creating tarball on unit {}".format(unit))
-        check_output(["juju", "ssh", unit, cmd], stderr=STDOUT)
+        check_output([juju, "ssh", unit, cmd], stderr=STDOUT)
     except CalledProcessError as e:
         log.warning(
             "Failed to create remote log tarball on unit {}".format(unit))
@@ -88,7 +97,7 @@
         log.warning(e.returncode)
 
 
-def collect_logs():
+def collect_logs(juju=JUJU):
     """
     Remotely, on each unit, create a tarball with the requested log files
     or directories, if they exist. If a requested log does not exist on a
@@ -96,19 +105,19 @@
     After each tarball is created, it's downloaded to the current directory
     and expanded, and the tarball is then deleted.
     """
-    units = get_units()
+    units = get_units(juju)
     # include bootstrap
     units.append("0")
 
     log.info("Collecting running processes for all units including bootstrap")
-    map(_create_ps_output_file, units)
+    map((lambda u: _create_ps_output_file(juju, u)), units)
 
     log.info("Creating remote tarball in parallel for units %s" % (
         ",".join(units)))
-    map(_create_log_tarball, units)
+    map((lambda u: _create_log_tarball(juju, u)), units)
     log.info("Downloading logs from units")
 
-    _mp_map(download_log_from_unit, units)
+    _mp_map(functools.partial(download_log_from_unit, juju), units)
 
 
 def _mp_map(func, args):
@@ -129,6 +138,7 @@
         return units[0]
 
 
+<<<<<<< TREE
 def disable_ssh_proxy(landscape_unit):
     """
     Workaround for #1607076: disable the proxy-ssh juju environment setting
@@ -148,77 +158,117 @@
         log.warning("Error was:\n{}".format(e.output))
 
 def collect_inner_logs():
+=======
+def find_inner_juju(juju, landscape_unit):
+    """Return the juju dir and binary path, if any."""
+    # Identify the most recent juju "home" that landscape is using.
+    cmd = "sudo ls -rt {}/".format(LANDSCAPE_JUJU_HOME)
+    try:
+        output = check_output(["juju", "ssh", landscape_unit, cmd]).strip()
+    except CalledProcessError:
+        return None, None
+    if output.startswith("sudo: "):
+        _, _, output = output.partition("\r\n")
+    models = [m for m in output.split() if m and m.isdigit()]
+    if not models:
+        return None, None
+    model = models[-1]
+    juju_dir = os.path.join(LANDSCAPE_JUJU_HOME, model)
+
+    # Try Juju 2.
+    juju_env = "JUJU_DATA={}".format(juju_dir)
+    cmd = "sudo {} {} status".format(juju_env, JUJU2)
+    if call([juju, "ssh", landscape_unit, cmd]) == 0:
+        log.info("using Juju 2 for inner model")
+        return juju_env, JUJU2
+
+    # Try Juju 1.
+    juju_env = "JUJU_HOME={}".format(juju_dir)
+    cmd = "sudo {} {} status".format(juju_env, JUJU1)
+    if call([juju, "ssh", landscape_unit, cmd]) == 0:
+        log.info("using Juju 1 for inner model")
+        return juju_env, JUJU1
+
+    # We didn't find an inner model.
+    return None, None
+
+
+def collect_inner_logs(juju=JUJU):
+>>>>>>> MERGE-SOURCE
     """Collect logs from an inner landscape[-server]/0 unit."""
-    units = get_units()
+    units = get_units(juju)
     landscape_unit = get_landscape_unit(units)
     if not landscape_unit:
         log.info("No landscape[-server]/N found, skipping")
         return
-    log.info("Found landscape unit %s" % landscape_unit)
+    log.info("Found landscape unit {}".format(landscape_unit))
 
-    # Make sure that there *is* an inner model.
-    poke_inner_env = (
-        "sudo JUJU_HOME=%s/`sudo ls -rt %s/ | tail -1` juju status" %
-        (LANDSCAPE_JUJU_HOME, LANDSCAPE_JUJU_HOME))
-    try:
-        check_output(["juju", "ssh", landscape_unit, poke_inner_env])
-    except CalledProcessError:
-        log.info("No active inner environment found on %s, skipping" %
-                 landscape_unit)
+    # Look up the inner model.
+    juju_env, inner_juju = find_inner_juju(juju, landscape_unit)
+    if inner_juju is None:
+        log.info(("No active inner environment found on {}, skipping"
+                  ).format(landscape_unit))
         return
 <<<<<<< TREE
     disable_ssh_proxy(landscape_unit)
+    call(["juju", "scp", PRG, "%s:%s" % (landscape_unit, collect_logs)])
+    call(["juju", "ssh", landscape_unit, "sudo rm -rf /tmp/inner-logs.tar.*"])
+    log.info("Collecting Logs on inner environment")
+    check_call(
+        ["juju", "ssh", landscape_unit, "sudo -u landscape %s" % collect_run])
 =======
 
     # Prepare to get the logs from the inner model.
     collect_logs = "/tmp/collect-logs"
->>>>>>> MERGE-SOURCE
-    call(["juju", "scp", PRG, "%s:%s" % (landscape_unit, collect_logs)])
-    call(["juju", "ssh", landscape_unit, "sudo rm -rf /tmp/inner-logs.tar.*"])
+    tarball = "/tmp/inner-logs.tar.gz"
+    check_call([juju, "scp",
+                PRG,
+                "{}:{}".format(landscape_unit, collect_logs)])
+    check_call([juju, "ssh", landscape_unit,
+                "sudo rm -rf /tmp/inner-logs.tar.*"])
 
     # Collect the logs for the inner model.
-    log.info("Collecting Logs on inner environment")
-    collect_run = (
-        "JUJU_HOME=%s/`sudo ls -rt %s/ | tail -1`"
-        " %s /tmp/inner-logs.tar.gz" % (
-            LANDSCAPE_JUJU_HOME, LANDSCAPE_JUJU_HOME, collect_logs))
-    check_call(["juju", "ssh",
+    log.info("Collecting logs on inner environment")
+    cmd = ("{} {} --inner --juju {} {}"
+           ).format(juju_env, collect_logs, inner_juju, tarball)
+    check_call([juju, "ssh",
                 landscape_unit,
-                "sudo -u landscape %s" % collect_run,
+                "sudo -u landscape {}".format(cmd),
                 ])
 
     # Copy the inner logs into a local directory.
+>>>>>>> MERGE-SOURCE
     log.info("Copying inner environment back")
+    source = "{}:{}".format(landscape_unit, tarball)
+    cwd = os.getcwd()
+    target = os.path.join(cwd, os.path.basename(tarball))
     try:
-        check_call(["juju", "scp",
-                    "%s:/tmp/inner-logs.tar.gz" % landscape_unit,
-                   ".",
-                   ])
-        cwd = os.getcwd()
+        check_call([juju, "scp", source, target])
+
         inner_dir = "landscape-0-inner-logs"
         os.mkdir(inner_dir)
         os.chdir(inner_dir)
         try:
-            check_call(["tar", "-zxf", "%s/inner-logs.tar.gz" % cwd])
+            check_call(["tar", "-zxf", target])
         finally:
             os.chdir(cwd)
     finally:
         try:
-            os.remove("inner-logs.tar.gz")
+            os.remove(target)
         except OSError as e:
             if e.errno != errno.ENOENT:
                 log.warning(
-                    "failed to remove inner logs tarball: {}".format(err))
-
-
-def download_log_from_unit(unit):
+                    "failed to remove inner logs tarball: {}".format(e))
+
+
+def download_log_from_unit(juju, unit):
     log.info("Downloading tarball from unit %s" % unit)
     unit_filename = unit.replace("/", "-")
     if unit == "0":
         unit_filename = "bootstrap"
     try:
         remote_filename = "logs_%s.tar.gz" % unit_filename
-        call(["juju", "scp", "%s:/tmp/%s" % (unit, remote_filename), "."])
+        call([juju, "scp", "%s:/tmp/%s" % (unit, remote_filename), "."])
         os.mkdir(unit_filename)
         call(["tar", "-C", unit_filename, "-xzf", remote_filename])
         os.unlink(remote_filename)
@@ -267,13 +317,17 @@
                    "inner autopilot cloud is detected, include that.")
     parser = ArgumentParser(description=description,
                             formatter_class=ArgumentDefaultsHelpFormatter)
+    parser.add_argument("--inner", action="store_true", default=False,
+                        help="Collect logs for an inner model.")
+    parser.add_argument("--juju", default=JUJU,
+                        help="The Juju binary to use.")
     parser.add_argument("tarfile", help="Full path to tarfile to create.")
     parser.add_argument("extrafiles", help="Optional full path to extra "
                         "logfiles to include, space separated", nargs="*")
     return parser
 
 
-def main(tarfile, extrafiles):
+def main(tarfile, extrafiles, juju=JUJU, inner=False):
     # we need the absolute path because we will be changing
     # the cwd
     tmpdir = mkdtemp()
@@ -281,11 +335,12 @@
     # logs are collected inside a temporary directory
     os.chdir(tmpdir)
     try:
-        collect_logs()
-        try:
-            collect_inner_logs()
-        except:
-            log.warning("Collecting inner logs failed, continuing")
+        collect_logs(juju)
+        if not inner:
+            try:
+                collect_inner_logs(juju)
+            except:
+                log.warning("Collecting inner logs failed, continuing")
         # we create the final tarball outside of tmpdir to we can
         # add the extrafiles to the tarball root
         os.chdir(cwd)
@@ -302,4 +357,4 @@
     parser = get_option_parser()
     args = parser.parse_args(sys.argv[1:])
     tarfile = os.path.abspath(args.tarfile)
-    main(tarfile, args.extrafiles)
+    main(tarfile, args.extrafiles, args.juju, args.inner)

=== modified file 'test_collect-logs.py'
--- test_collect-logs.py	2016-08-17 14:14:30 +0000
+++ test_collect-logs.py	2016-08-17 14:14:30 +0000
@@ -92,22 +92,35 @@
 
         script.main(tarfile, extrafiles)
 
-        script.collect_logs.assert_called_once_with()
-        script.collect_inner_logs.assert_called_once_with()
+        script.collect_logs.assert_called_once_with("juju")
+        script.collect_inner_logs.assert_called_once_with("juju")
         script.bundle_logs.assert_called_once_with(
             self.tempdir, tarfile, extrafiles)
         self.assertFalse(os.path.exists(self.tempdir))
 
     def test_in_correct_directories(self):
-        script.collect_logs.side_effect = lambda: self.assert_cwd(self.tempdir)
+        script.collect_logs.side_effect = (
+            lambda _: self.assert_cwd(self.tempdir))
         script.collect_inner_logs.side_effect = (
-            lambda: self.assert_cwd(self.tempdir))
+            lambda _: self.assert_cwd(self.tempdir))
         script.bundle_logs.side_effect = lambda *a: self.assert_cwd(self.cwd)
         tarfile = "/tmp/logs.tgz"
         extrafiles = ["spam.py"]
 
         script.main(tarfile, extrafiles)
 
+    def test_inner(self):
+        tarfile = "/tmp/logs.tgz"
+        extrafiles = ["spam.py"]
+
+        script.main(tarfile, extrafiles, inner=True)
+
+        script.collect_logs.assert_called_once_with("juju")
+        script.collect_inner_logs.assert_not_called()
+        script.bundle_logs.assert_called_once_with(
+            self.tempdir, tarfile, extrafiles)
+        self.assertFalse(os.path.exists(self.tempdir))
+
     def test_cleanup(self):
         tarfile = "/tmp/logs.tgz"
         extrafiles = ["spam.py"]
@@ -124,7 +137,7 @@
         with self.assertRaises(FakeError):
             script.main(tarfile, extrafiles)
 
-        script.collect_logs.assert_called_once_with()
+        script.collect_logs.assert_called_once_with("juju")
         script.collect_inner_logs.assert_not_called()
         script.bundle_logs.assert_not_called()
         self.assertFalse(os.path.exists(self.tempdir))
@@ -136,8 +149,8 @@
 
         script.main(tarfile, extrafiles)
 
-        script.collect_logs.assert_called_once_with()
-        script.collect_inner_logs.assert_called_once_with()
+        script.collect_logs.assert_called_once_with("juju")
+        script.collect_inner_logs.assert_called_once_with("juju")
         script.bundle_logs.assert_called_once_with(
             self.tempdir, tarfile, extrafiles)
         self.assertFalse(os.path.exists(self.tempdir))
@@ -150,8 +163,8 @@
         with self.assertRaises(FakeError):
             script.main(tarfile, extrafiles)
 
-        script.collect_logs.assert_called_once_with()
-        script.collect_inner_logs.assert_called_once_with()
+        script.collect_logs.assert_called_once_with("juju")
+        script.collect_inner_logs.assert_called_once_with("juju")
         script.bundle_logs.assert_called_once_with(
             self.tempdir, tarfile, extrafiles)
         self.assertFalse(os.path.exists(self.tempdir))
@@ -192,9 +205,9 @@
     def test_success(self):
         script.call.side_effect = self._call_side_effect
 
-        script.collect_logs()
+        script.collect_logs("juju")
 
-        script.get_units.assert_called_once_with()
+        script.get_units.assert_called_once_with("juju")
         expected = []
         units = self.units + ["0"]
         # for _create_ps_output_file()
@@ -246,9 +259,9 @@
         script.get_units.side_effect = FakeError()
 
         with self.assertRaises(FakeError):
-            script.collect_logs()
+            script.collect_logs("juju")
 
-        script.get_units.assert_called_once_with()
+        script.get_units.assert_called_once_with("juju")
         script.check_output.assert_not_called()
         script.call.assert_not_called()
 
@@ -258,9 +271,9 @@
                                            ]
 
         with self.assertRaises(FakeError):
-            script.collect_logs()
+            script.collect_logs("juju")
 
-        script.get_units.assert_called_once_with()
+        script.get_units.assert_called_once_with("juju")
         self.assertEqual(script.check_output.call_count, 2)
         script.call.assert_not_called()
 
@@ -276,9 +289,9 @@
             return self._call_side_effect(cmd)
         script.call.side_effect = call_side_effect
 
-        script.collect_logs()
+        script.collect_logs("juju")
 
-        script.get_units.assert_called_once_with()
+        script.get_units.assert_called_once_with("juju")
         units = self.units + ["0"]
         self.assertEqual(script.check_output.call_count, len(units) * 2)
         self.assertEqual(script.call.call_count, len(units) * 2 - 1)
@@ -306,6 +319,8 @@
             "haproxy/0",
             ]
         script.get_units.return_value = self.units[:]
+        script.check_output.return_value = "0\n"
+        script.call.return_value = 0
 
         os.chdir(self.tempdir)
 
@@ -313,50 +328,110 @@
         self.assert_cwd(self.tempdir)
         self.assertFalse(os.path.exists("inner-logs.tar.gz"))
 
-    def test_success(self):
-        def check_call_side_effect(cmd):
-            if script.check_call.call_count == 2:
-                self.assert_cwd(self.tempdir)
-                self._touch_tempfile("inner-logs.tar.gz")
-            elif script.check_call.call_count == 3:
-                cwd = os.path.join(self.tempdir, "landscape-0-inner-logs")
-                self.assert_cwd(cwd)
-            return None
-        script.check_call.side_effect = check_call_side_effect
-
-        script.collect_inner_logs()
-
-        # get_units()
-        script.get_units.assert_called_once_with()
-        # check_output()
-        cmd = ("sudo JUJU_HOME=/var/lib/landscape/juju-homes/"
-               "`sudo ls -rt /var/lib/landscape/juju-homes/ | tail -1`"
-               " juju status")
-        script.check_output.assert_called_once_with(
-            ["juju", "ssh", "landscape-server/0", cmd])
-        # call()
-        expected = [
-            mock.call(["juju", "scp",
-                       os.path.join(os.path.dirname(__file__), "collect-logs"),
-                       "landscape-server/0:/tmp/collect-logs",
-                       ]),
-            mock.call(["juju", "ssh",
-                       "landscape-server/0",
-                       "sudo rm -rf /tmp/inner-logs.tar.*",
-                       ]),
-            ]
-        self.assertEqual(script.call.call_count, len(expected))
-        script.call.assert_has_calls(expected, any_order=True)
-        # check_call()
-        cmd = ("sudo -u landscape"
-               " JUJU_HOME=/var/lib/landscape/juju-homes/"
-               "`sudo ls -rt /var/lib/landscape/juju-homes/ | tail -1`"
-               " /tmp/collect-logs /tmp/inner-logs.tar.gz")
-        expected = [
-            mock.call(["juju", "ssh", "landscape-server/0", cmd]),
-            mock.call(["juju", "scp",
-                       "landscape-server/0:/tmp/inner-logs.tar.gz",
-                       ".",
+    def test_juju_2(self):
+        def check_call_side_effect(cmd):
+            if script.check_call.call_count == 4:
+                self.assert_cwd(self.tempdir)
+                self._touch_tempfile("inner-logs.tar.gz")
+            elif script.check_call.call_count == 5:
+                cwd = os.path.join(self.tempdir, "landscape-0-inner-logs")
+                self.assert_cwd(cwd)
+            return None
+        script.check_call.side_effect = check_call_side_effect
+
+        script.collect_inner_logs("juju")
+
+        # get_units()
+        script.get_units.assert_called_once_with("juju")
+        # check_output()
+        script.check_output.assert_called_once_with(
+            ["juju", "ssh", "landscape-server/0",
+             "sudo ls -rt /var/lib/landscape/juju-homes/"])
+        # call()
+        expected = [
+            mock.call(["juju", "ssh", "landscape-server/0",
+                       ("sudo JUJU_DATA=/var/lib/landscape/juju-homes/0 "
+                        "juju-2.0 status"),
+                       ]),
+            ]
+        self.assertEqual(script.call.call_count, len(expected))
+        script.call.assert_has_calls(expected, any_order=True)
+        # check_call()
+        cmd = ("sudo -u landscape"
+               " JUJU_DATA=/var/lib/landscape/juju-homes/0"
+               " /tmp/collect-logs --inner --juju juju-2.0"
+               " /tmp/inner-logs.tar.gz")
+        expected = [
+            mock.call(["juju", "scp",
+                       os.path.join(os.path.dirname(__file__), "collect-logs"),
+                       "landscape-server/0:/tmp/collect-logs",
+                       ]),
+            mock.call(["juju", "ssh",
+                       "landscape-server/0",
+                       "sudo rm -rf /tmp/inner-logs.tar.*",
+                       ]),
+            mock.call(["juju", "ssh", "landscape-server/0", cmd]),
+            mock.call(["juju", "scp",
+                       "landscape-server/0:/tmp/inner-logs.tar.gz",
+                       os.path.join(self.tempdir, "inner-logs.tar.gz"),
+                       ]),
+            mock.call(["tar", "-zxf", self.tempdir + "/inner-logs.tar.gz"]),
+            ]
+        self.assertEqual(script.check_call.call_count, len(expected))
+        script.check_call.assert_has_calls(expected, any_order=True)
+        self.assert_clean()
+
+    def test_juju_1(self):
+        def check_call_side_effect(cmd):
+            if script.check_call.call_count == 4:
+                self.assert_cwd(self.tempdir)
+                self._touch_tempfile("inner-logs.tar.gz")
+            elif script.check_call.call_count == 5:
+                cwd = os.path.join(self.tempdir, "landscape-0-inner-logs")
+                self.assert_cwd(cwd)
+            return None
+        script.check_call.side_effect = check_call_side_effect
+        script.call.side_effect = [1, 0]
+
+        script.collect_inner_logs("juju")
+
+        # get_units()
+        script.get_units.assert_called_once_with("juju")
+        # check_output()
+        script.check_output.assert_called_once_with(
+            ["juju", "ssh", "landscape-server/0",
+             "sudo ls -rt /var/lib/landscape/juju-homes/"])
+        # call()
+        expected = [
+            mock.call(["juju", "ssh", "landscape-server/0",
+                       ("sudo JUJU_DATA=/var/lib/landscape/juju-homes/0 "
+                        "juju-2.0 status"),
+                       ]),
+            mock.call(["juju", "ssh", "landscape-server/0",
+                       ("sudo JUJU_HOME=/var/lib/landscape/juju-homes/0 "
+                        "juju status"),
+                       ]),
+            ]
+        self.assertEqual(script.call.call_count, len(expected))
+        script.call.assert_has_calls(expected, any_order=True)
+        # check_call()
+        cmd = ("sudo -u landscape"
+               " JUJU_HOME=/var/lib/landscape/juju-homes/0"
+               " /tmp/collect-logs --inner --juju juju"
+               " /tmp/inner-logs.tar.gz")
+        expected = [
+            mock.call(["juju", "scp",
+                       os.path.join(os.path.dirname(__file__), "collect-logs"),
+                       "landscape-server/0:/tmp/collect-logs",
+                       ]),
+            mock.call(["juju", "ssh",
+                       "landscape-server/0",
+                       "sudo rm -rf /tmp/inner-logs.tar.*",
+                       ]),
+            mock.call(["juju", "ssh", "landscape-server/0", cmd]),
+            mock.call(["juju", "scp",
+                       "landscape-server/0:/tmp/inner-logs.tar.gz",
+                       os.path.join(self.tempdir, "inner-logs.tar.gz"),
                        ]),
             mock.call(["tar", "-zxf", self.tempdir + "/inner-logs.tar.gz"]),
             ]
@@ -368,21 +443,19 @@
         self.units[0] = "landscape/0"
         script.get_units.return_value = self.units[:]
 
-        script.collect_inner_logs()
+        script.collect_inner_logs("juju")
 
-        cmd = ("sudo JUJU_HOME=/var/lib/landscape/juju-homes/"
-               "`sudo ls -rt /var/lib/landscape/juju-homes/ | tail -1`"
-               " juju status")
         script.check_output.assert_called_once_with(
-            ["juju", "ssh", "landscape/0", cmd])
+            ["juju", "ssh", "landscape/0",
+             "sudo ls -rt /var/lib/landscape/juju-homes/"])
         self.assert_clean()
 
     def test_no_units(self):
         script.get_units.return_value = []
 
-        script.collect_inner_logs()
+        script.collect_inner_logs("juju")
 
-        script.get_units.assert_called_once_with()
+        script.get_units.assert_called_once_with("juju")
         script.check_output.assert_not_called()
         script.call.assert_not_called()
         script.check_call.assert_not_called()
@@ -392,9 +465,23 @@
         del self.units[0]
         script.get_units.return_value = self.units[:]
 
-        script.collect_inner_logs()
-
-        script.get_units.assert_called_once_with()
+        script.collect_inner_logs("juju")
+
+        script.get_units.assert_called_once_with("juju")
+        script.check_output.assert_not_called()
+        script.call.assert_not_called()
+        script.check_call.assert_not_called()
+        self.assert_clean()
+
+    def test_no_juju_homes(self):
+        script.get_units.return_value = []
+        script.check_output.return_value = ""
+
+        script.collect_inner_logs("juju")
+
+        script.get_units.assert_called_once_with("juju")
+
+        script.get_units.assert_called_once_with("juju")
         script.check_output.assert_not_called()
         script.call.assert_not_called()
         script.check_call.assert_not_called()
@@ -404,7 +491,7 @@
         script.get_units.side_effect = FakeError()
 
         with self.assertRaises(FakeError):
-            script.collect_inner_logs()
+            script.collect_inner_logs("juju")
 
         self.assertEqual(script.get_units.call_count, 1)
         script.check_output.assert_not_called()
@@ -417,7 +504,7 @@
         script.check_output.side_effect = FakeError()
 
         with self.assertRaises(FakeError):
-            script.collect_inner_logs()
+            script.collect_inner_logs("juju")
 
         self.assertEqual(script.get_units.call_count, 1)
         self.assertEqual(script.check_output.call_count, 1)
@@ -426,11 +513,11 @@
         self.assert_cwd(self.tempdir)
         self.assert_clean()
 
-    def test_call_failure_1(self):
+    def test_call_juju2_failure(self):
         script.call.side_effect = FakeError()
 
         with self.assertRaises(FakeError):
-            script.collect_inner_logs()
+            script.collect_inner_logs("juju")
 
         self.assertEqual(script.get_units.call_count, 1)
         self.assertEqual(script.check_output.call_count, 1)
@@ -439,13 +526,51 @@
         self.assert_cwd(self.tempdir)
         self.assert_clean()
 
-    def test_call_failure_2(self):
-        script.call.side_effect = [None,
+    def test_call_juju1_failure(self):
+        script.call.side_effect = [1,
                                    FakeError(),
                                    ]
 
         with self.assertRaises(FakeError):
-            script.collect_inner_logs()
+            script.collect_inner_logs("juju")
+
+        self.assertEqual(script.get_units.call_count, 1)
+        self.assertEqual(script.check_output.call_count, 1)
+        self.assertEqual(script.call.call_count, 2)
+        script.check_call.assert_not_called()
+        self.assert_cwd(self.tempdir)
+        self.assert_clean()
+
+    def test_call_juju2_nonzero_return(self):
+        script.call.side_effect = [1,
+                                   mock.DEFAULT,
+                                   ]
+
+        script.collect_inner_logs("juju")
+
+        self.assertEqual(script.get_units.call_count, 1)
+        self.assertEqual(script.check_output.call_count, 1)
+        self.assertEqual(script.call.call_count, 2)
+        self.assertEqual(script.check_call.call_count, 5)
+        self.assert_clean()
+
+    def test_call_juju1_nonzero_return(self):
+        script.call.side_effect = [mock.DEFAULT,
+                                   1,
+                                   ]
+
+        script.collect_inner_logs("juju")
+
+        self.assertEqual(script.get_units.call_count, 1)
+        self.assertEqual(script.check_output.call_count, 1)
+        self.assertEqual(script.call.call_count, 1)
+        self.assertEqual(script.check_call.call_count, 5)
+        self.assert_clean()
+
+    def test_call_all_nonzero_return(self):
+        script.call.return_value = 1
+
+        script.collect_inner_logs("juju")
 
         self.assertEqual(script.get_units.call_count, 1)
         self.assertEqual(script.check_output.call_count, 1)
@@ -457,11 +582,11 @@
         script.check_call.side_effect = FakeError()
 
         with self.assertRaises(FakeError):
-            script.collect_inner_logs()
+            script.collect_inner_logs("juju")
 
         self.assertEqual(script.get_units.call_count, 1)
         self.assertEqual(script.check_output.call_count, 1)
-        self.assertEqual(script.call.call_count, 2)
+        self.assertEqual(script.call.call_count, 1)
         self.assertEqual(script.check_call.call_count, 1)
         self.assert_clean()
 
@@ -471,31 +596,62 @@
                                          ]
 
         with self.assertRaises(FakeError):
-            script.collect_inner_logs()
+            script.collect_inner_logs("juju")
 
         self.assertEqual(script.get_units.call_count, 1)
         self.assertEqual(script.check_output.call_count, 1)
-        self.assertEqual(script.call.call_count, 2)
+        self.assertEqual(script.call.call_count, 1)
         self.assertEqual(script.check_call.call_count, 2)
         self.assert_clean()
 
     def test_check_call_failure_3(self):
+        script.check_call.side_effect = [None,
+                                         None,
+                                         FakeError(),
+                                         ]
+
+        with self.assertRaises(FakeError):
+            script.collect_inner_logs("juju")
+
+        self.assertEqual(script.get_units.call_count, 1)
+        self.assertEqual(script.check_output.call_count, 1)
+        self.assertEqual(script.call.call_count, 1)
+        self.assertEqual(script.check_call.call_count, 3)
+        self.assert_clean()
+
+    def test_check_call_failure_4(self):
+        script.check_call.side_effect = [None,
+                                         None,
+                                         None,
+                                         FakeError(),
+                                         ]
+
+        with self.assertRaises(FakeError):
+            script.collect_inner_logs("juju")
+
+        self.assertEqual(script.get_units.call_count, 1)
+        self.assertEqual(script.check_output.call_count, 1)
+        self.assertEqual(script.call.call_count, 1)
+        self.assertEqual(script.check_call.call_count, 4)
+        self.assert_clean()
+
+    def test_check_call_failure_5(self):
         def check_call_side_effect(cmd):
-            if script.check_call.call_count == 1:
+            if script.check_call.call_count in (1, 2, 3):
                 return None
-            if script.check_call.call_count == 2:
+            if script.check_call.call_count == 4:
                 self._touch_tempfile("inner-logs.tar.gz")
                 return None
             raise FakeError()
         script.check_call.side_effect = check_call_side_effect
 
         with self.assertRaises(FakeError):
-            script.collect_inner_logs()
+            script.collect_inner_logs("juju")
 
         self.assertEqual(script.get_units.call_count, 1)
         self.assertEqual(script.check_output.call_count, 1)
-        self.assertEqual(script.call.call_count, 2)
-        self.assertEqual(script.check_call.call_count, 3)
+        self.assertEqual(script.call.call_count, 1)
+        self.assertEqual(script.check_call.call_count, 5)
         self.assert_clean()