Merge lp:~larsks/cloud-init/fix-systemd-detection into lp:~cloud-init-dev/cloud-init/trunk

[Lars Kellogg-Stedman]

check for systemd using sd_booted() semantics

The existing cloud-init code determines if systemd is in use by looking at the
distribution name and version. This is prone to error because:

- RHEL derivatives other than CentOS (e.g., Scientific Linux) will fail this test, and
- Distributions that are not derived from RHEL also use systemd

This patch makes cloud-init use the same logic that is used in systemd's
sd_booted() method
(http://www.freedesktop.org/software/systemd/man/sd_booted.html)

[Dan Watkins (daniel-thewatkins)]

Hi Lars,

Thanks for taking the time to do this, this looks like a good improvement.

Some tests for this would be good, to ensure that we don't regress here[0]. TestGenericDistro in tests/unittests/test_distros/test_generic.py already has the means to do filesystem mocking in place, so it shouldn't be too painful.

Dan

[0] If I weren't looking at the code too carefully, I'd replace what you've written with os.path.isdir; but that would be incorrect because it follows symbolic links. Having something that makes us notice if someone does this would be valuable.

[lp:~larsks/cloud-init/fix-systemd-detection]

1112. By Lars Kellogg-Stedman on 2015-06-03

transform paths in functions taking more than a single argument

Patch FilesystemMockingTestcase.patchOS to support methods taking more
than a single path argument. This is required in order to properly mock
`os.symlink`, which takes two path arguments.

1113. By Lars Kellogg-Stedman on 2015-06-03

add tests for systemd detection

This adds the following tests in test_distros.test_generic:

- test_systemd_in_use

  Test the situation in which /run/systemd/system exists.

- test_systemd_not_in_use

  Test the situation in which /run/systemd/system does not exists.

- test_systemd_symlink

  This tests the situation in which /run/systemd/system exists but is a
  *symlink* to a directory, which according to sd_booted() should return
  false.

[Lars Kellogg-Stedman (larsks)]

Dan,

Thanks for the review. I hope this addresses your suggestions.

[Dan Watkins (daniel-thewatkins)]

Looks good, thanks!

review: Approve

=== modified file 'cloudinit/distros/__init__.py'
--- cloudinit/distros/__init__.py	2015-05-19 15:21:34 +0000
+++ cloudinit/distros/__init__.py	2015-06-03 17:20:32 +0000
@@ -27,6 +27,7 @@
 import abc
 import os
 import re
+import stat
 
 from cloudinit import importer
 from cloudinit import log as logging
@@ -89,6 +90,13 @@
         self._write_hostname(writeable_hostname, self.hostname_conf_fn)
         self._apply_hostname(writeable_hostname)
 
+    def uses_systemd(self):
+        try:
+            res = os.lstat('/run/systemd/system')
+            return stat.S_ISDIR(res.st_mode)
+        except:
+            return False
+
     @abc.abstractmethod
     def package_command(self, cmd, args=None, pkgs=None):
         raise NotImplementedError()

=== modified file 'cloudinit/distros/rhel.py'
--- cloudinit/distros/rhel.py	2015-05-15 20:28:24 +0000
+++ cloudinit/distros/rhel.py	2015-06-03 17:20:32 +0000
@@ -111,14 +111,6 @@
             rhel_util.update_sysconfig_file(self.network_conf_fn, net_cfg)
         return dev_names
 
-    def uses_systemd(self):
-        # Fedora 18 and RHEL 7 were the first adopters in their series
-        (dist, vers) = util.system_info()['dist'][:2]
-        major = (int)(vers.split('.')[0])
-        return ((dist.startswith('Red Hat Enterprise Linux') and major >= 7)
-                or (dist.startswith('CentOS Linux') and major >= 7)
-                or (dist.startswith('Fedora') and major >= 18))
-
     def apply_locale(self, locale, out_fn=None):
         if self.uses_systemd():
             if not out_fn:

=== modified file 'tests/unittests/helpers.py'
--- tests/unittests/helpers.py	2015-03-04 12:29:29 +0000
+++ tests/unittests/helpers.py	2015-06-03 17:20:32 +0000
@@ -248,13 +248,15 @@
 
     def patchOS(self, new_root):
         patch_funcs = {
-            os.path: ['isfile', 'exists', 'islink', 'isdir'],
-            os: ['listdir'],
+            os.path: [('isfile', 1), ('exists', 1),
+                      ('islink', 1), ('isdir', 1)],
+            os: [('listdir', 1), ('mkdir', 1),
+                 ('lstat', 1), ('symlink', 2)],
         }
         for (mod, funcs) in patch_funcs.items():
-            for f in funcs:
+            for f, nargs in funcs:
                 func = getattr(mod, f)
-                trap_func = retarget_many_wrapper(new_root, 1, func)
+                trap_func = retarget_many_wrapper(new_root, nargs, func)
                 self.patched_funcs.enter_context(
                     mock.patch.object(mod, f, trap_func))
 

=== modified file 'tests/unittests/test_distros/test_generic.py'
--- tests/unittests/test_distros/test_generic.py	2015-01-23 00:53:04 +0000
+++ tests/unittests/test_distros/test_generic.py	2015-06-03 17:20:32 +0000
@@ -194,6 +194,29 @@
                          {'primary': 'http://fs-primary-intel',
                           'security': 'http://security-mirror2-intel'})
 
+    def test_systemd_in_use(self):
+        cls = distros.fetch("ubuntu")
+        d = cls("ubuntu", {}, None)
+        self.patchOS(self.tmp)
+        self.patchUtils(self.tmp)
+        os.makedirs('/run/systemd/system')
+        self.assertTrue(d.uses_systemd())
+
+    def test_systemd_not_in_use(self):
+        cls = distros.fetch("ubuntu")
+        d = cls("ubuntu", {}, None)
+        self.patchOS(self.tmp)
+        self.patchUtils(self.tmp)
+        self.assertFalse(d.uses_systemd())
+
+    def test_systemd_symlink(self):
+        cls = distros.fetch("ubuntu")
+        d = cls("ubuntu", {}, None)
+        self.patchOS(self.tmp)
+        self.patchUtils(self.tmp)
+        os.makedirs('/run/systemd')
+        os.symlink('/', '/run/systemd/system')
+        self.assertFalse(d.uses_systemd())
 
 # def _get_package_mirror_info(mirror_info, availability_zone=None,
 #                             mirror_filter=util.search_for_mirror):