Merge ~ajorgens/cloud-init:octal-modes into cloud-init:master

[Andrew Jorgensen]

write_file(s): Print file modes as octal, not decimal

Unix file modes are usually represented as octal, but they were being
interpreted as decimal, for example 0o644 would be printed as '420'.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:93c6d05f2195d141ad71ea634992ad457472900b
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~ajorgens/cloud-init/+git/cloud-init/+merge/325424/+edit-commit-message

https://jenkins.ubuntu.com/server/job/cloud-init-ci/509/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/509/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/509/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/509/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/509/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/509/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/509/rebuild

review: Needs Fixing (continuous-integration)

[Andrew Jorgensen (ajorgens)]

Well, dang, I thought this was going to be the easy one to get merged. Why is the mode None? *sigh*

[Scott Moser (smoser)]

Andrew,

Thanks for re-submitting this MP.

I've reviewed a little bit and found a few things
a.) as seen in the c-i logs, the change to 'decode_perms' can fail if the input was a string.
  >>> "%o" % "0755"
  TypeError: %o format: an integer is required, not str

b.) it seems to have exposed an error in append_file() that i'm not sure how it ever worked.

For your MP to be accepted, we need to fix 'a'

I've put a suggested change like:
 http://paste.ubuntu.com/24841624/

Basically that just tries to do '%o' but uses '%r' if that fails.

Thoughts?

review: Needs Fixing

5a9acd1... by Andrew Jorgensen on 2017-06-12

Additional fixes and test case from smoser@

[Andrew Jorgensen (ajorgens)]

Both a and b are fixed in 5a9acd1

review: Needs Resubmitting

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/514/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/514
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/514
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/514
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/514
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/514

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/514/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

Thanks, this looks good to me.
I'm asking Chad for a quick review also.

review: Approve

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:5a9acd191621c59285f67d4f667afad87d93854e
https://jenkins.ubuntu.com/server/job/cloud-init-ci/515/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/515
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/515
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/515
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/515
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/515

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/515/rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

Looks good and thanks for the unit tests. Since we now have a with_logs option in CiTest class, let's use that instead of mocks.

Here's a slight extension to our unit test logging setup to also make sure we check log level on some of these logs.
http://paste.ubuntu.com/24843185/

Approved if folks think this is a good idea with the following patch.

review: Approve

2f7132a... by Andrew Jorgensen on 2017-06-12

Unit test fixes from csmith and smoser

[Andrew Jorgensen (ajorg)]

Ready to merge AFAICT.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:2f7132a0fa8ae12f9771b5e3c44f93ef1685ea75
https://jenkins.ubuntu.com/server/job/cloud-init-ci/519/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/519
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/519
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/519
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/519
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/519

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/519/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

I approve this.
We really ought to change the signature of decode_perms to not take a 'log' input but rather just to use logging.
Andrew, interested in doing a follow up for that?

review: Approve

[Andrew Jorgensen (ajorgens)]

> Andrew, interested in doing a follow up for that?

Probably. I've got a rather long queue of patches to post, but I've added this to the TODO list.

diff --git a/cloudinit/config/cc_write_files.py b/cloudinit/config/cc_write_files.py
index 72e1cdd..1835a31 100644
--- a/cloudinit/config/cc_write_files.py
+++ b/cloudinit/config/cc_write_files.py
@@ -53,6 +53,7 @@ import six
 from cloudinit.settings import PER_INSTANCE
 from cloudinit import util
 
+
 frequency = PER_INSTANCE
 
 DEFAULT_OWNER = "root:root"
@@ -119,7 +120,14 @@ def decode_perms(perm, default, log):
             # Force to string and try octal conversion
             return int(str(perm), 8)
     except (TypeError, ValueError):
-        log.warn("Undecodable permissions %s, assuming %s", perm, default)
+        reps = []
+        for r in (perm, default):
+            try:
+                reps.append("%o" % r)
+            except TypeError:
+                reps.append("%r" % r)
+        log.warning(
+            "Undecodable permissions {0}, returning default {1}".format(*reps))
         return default
 
 
diff --git a/cloudinit/util.py b/cloudinit/util.py
index 415ca37..ec68925 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -1751,8 +1751,12 @@ def write_file(filename, content, mode=0o644, omode="wb", copy_mode=False):
     else:
         content = decode_binary(content)
         write_type = 'characters'
+    try:
+        mode_r = "%o" % mode
+    except TypeError:
+        mode_r = "%r" % mode
     LOG.debug("Writing to %s - %s: [%s] %s %s",
-              filename, omode, mode, len(content), write_type)
+              filename, omode, mode_r, len(content), write_type)
     with SeLinuxGuard(path=filename):
         with open(filename, omode) as fh:
             fh.write(content)
diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py
index e78abce..569f1ae 100644
--- a/tests/unittests/helpers.py
+++ b/tests/unittests/helpers.py
@@ -97,11 +97,13 @@ class CiTestCase(TestCase):
         super(CiTestCase, self).setUp()
         if self.with_logs:
             # Create a log handler so unit tests can search expected logs.
-            logger = logging.getLogger()
+            self.logger = logging.getLogger()
             self.logs = six.StringIO()
+            formatter = logging.Formatter('%(levelname)s: %(message)s')
             handler = logging.StreamHandler(self.logs)
-            self.old_handlers = logger.handlers
-            logger.handlers = [handler]
+            handler.setFormatter(formatter)
+            self.old_handlers = self.logger.handlers
+            self.logger.handlers = [handler]
 
     def tearDown(self):
         if self.with_logs:
diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
index 42f49e0..b17f389 100644
--- a/tests/unittests/test_datasource/test_azure.py
+++ b/tests/unittests/test_datasource/test_azure.py
@@ -263,7 +263,8 @@ fdescfs            /dev/fd          fdescfs rw              0 0
             {}, distro=None, paths=self.paths)
         self.assertFalse(dsrc.get_data())
         self.assertEqual(
-            "Non-Azure DMI asset tag '{0}' discovered.\n".format(nonazure_tag),
+            "DEBUG: Non-Azure DMI asset tag '{0}' discovered.\n".format(
+                nonazure_tag),
             self.logs.getvalue())
 
     def test_basic_seed_dir(self):
diff --git a/tests/unittests/test_handler/test_handler_ntp.py b/tests/unittests/test_handler/test_handler_ntp.py
index 3a9f7f7..c4299d9 100644
--- a/tests/unittests/test_handler/test_handler_ntp.py
+++ b/tests/unittests/test_handler/test_handler_ntp.py
@@ -216,7 +216,8 @@ class TestNtp(FilesystemMockingTestCase):
         """When no ntp section is defined handler logs a warning and noops."""
         cc_ntp.handle('cc_ntp', {}, None, None, [])
         self.assertEqual(
-            'Skipping module named cc_ntp, not present or disabled by cfg\n',
+            'DEBUG: Skipping module named cc_ntp, '
+            'not present or disabled by cfg\n',
             self.logs.getvalue())
 
     def test_ntp_handler_schema_validation_allows_empty_ntp_config(self):
diff --git a/tests/unittests/test_handler/test_handler_write_files.py b/tests/unittests/test_handler/test_handler_write_files.py
index fb252d1..88a4742 100644
--- a/tests/unittests/test_handler/test_handler_write_files.py
+++ b/tests/unittests/test_handler/test_handler_write_files.py
@@ -1,10 +1,10 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
-from cloudinit.config.cc_write_files import write_files
+from cloudinit.config.cc_write_files import write_files, decode_perms
 from cloudinit import log as logging
 from cloudinit import util
 
-from ..helpers import FilesystemMockingTestCase
+from ..helpers import CiTestCase, FilesystemMockingTestCase
 
 import base64
 import gzip
@@ -98,6 +98,33 @@ class TestWriteFiles(FilesystemMockingTestCase):
         self.assertEqual(len(expected), flen_expected)
 
 
+class TestDecodePerms(CiTestCase):
+
+    with_logs = True
+
+    def test_none_returns_default(self):
+        """If None is passed as perms, then default should be returned."""
+        default = object()
+        found = decode_perms(None, default, self.logger)
+        self.assertEqual(default, found)
+
+    def test_integer(self):
+        """A valid integer should return itself."""
+        found = decode_perms(0o755, None, self.logger)
+        self.assertEqual(0o755, found)
+
+    def test_valid_octal_string(self):
+        """A string should be read as octal."""
+        found = decode_perms("644", None, self.logger)
+        self.assertEqual(0o644, found)
+
+    def test_invalid_octal_string_returns_default_and_warns(self):
+        """A string with invalid octal should warn and return default."""
+        found = decode_perms("999", None, self.logger)
+        self.assertIsNone(found)
+        self.assertIn("WARNING: Undecodable", self.logs.getvalue())
+
+
 def _gzip_bytes(data):
     buf = six.BytesIO()
     fp = None