Merge lp:~ursinha/landscape-charm/collect-logs-ignore-file-changed into lp:~landscape/landscape-charm/tools

[Ursula Junque]

Fixes bug 1623678, by re-adding the mechanism to ignore when tar returns 1 because files changed while tarfile is being created.

This branch fixes bug 1623678, by re-adding the mechanism to ignore when tar returns 1 because files changed while tarfile is being created.

I've done two things:
- Removed the tar --warning=no-file-changed option as that was only masking the output, tar exit code was still 1 causing collect-logs to silently skip tarball creation for such cases. Removing it allows me to check the exception output for that specific message and ignore it.
- Added mechanism to check if tar failure was due to files changing and deliberately proceed, ignoring this error in particular.

These two together restore previous and expected behavior.

[Andreas Hasenack (ahasenack)]

+1!

review: Approve

=== modified file 'collect-logs'
--- collect-logs	2016-08-31 15:32:04 +0000
+++ collect-logs	2016-09-14 22:10:11 +0000
@@ -246,11 +246,9 @@
     log.info("Creating tarball on unit {}".format(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
-    # that are actively being written, ignoring errors seems best
-    # for now.
-    # http://serverfault.com/questions/560741/
-    tar_cmd = "sudo tar --ignore-failed-read --warning=no-file-changed"
+    # --ignore-failed-read avoids failure for unreadable files (not for files
+    # being written)
+    tar_cmd = "sudo tar --ignore-failed-read"
     logsuffix = unit.replace("/", "-")
     if unit == "0":
         logsuffix = "bootstrap"
@@ -263,6 +261,11 @@
         try:
             check_output(args, stderr=STDOUT, env=juju.env)
         except CalledProcessError as e:
+            # Note: tar commands can fail since we are backing up log files
+            # that are actively being written, ignoring such errors is what we
+            # can and should do now. Everything else we might retry as usual.
+            if e.returncode == 1 and "file changed as we read it" in e.output:
+                break
             log.warning(
                 "Failed to archive log files on unit {}".format(unit))
             log.warning(e.output)

=== modified file 'test_collect-logs.py'
--- test_collect-logs.py	2016-08-29 18:26:55 +0000
+++ test_collect-logs.py	2016-09-14 22:10:11 +0000
@@ -317,7 +317,7 @@
             tarfile = "/tmp/logs_{}.tar".format(unit.replace("/", "-")
                                                 if unit != "0"
                                                 else "bootstrap")
-            cmd = ("sudo tar --ignore-failed-read --warning=no-file-changed"
+            cmd = ("sudo tar --ignore-failed-read"
                    " --exclude=/var/lib/landscape/client/package/hash-id"
                    " --exclude=/var/lib/juju/containers/juju-*-lxc-template"
                    " -cf {}"
@@ -388,7 +388,7 @@
             tarfile = "/tmp/logs_{}.tar".format(unit.replace("/", "-")
                                                 if unit != "0"
                                                 else "bootstrap")
-            cmd = ("sudo tar --ignore-failed-read --warning=no-file-changed"
+            cmd = ("sudo tar --ignore-failed-read"
                    " --exclude=/var/lib/landscape/client/package/hash-id"
                    " --exclude=/var/lib/juju/containers/juju-*-lxc-template"
                    " -cf {}"