Merge ~smoser/cloud-init:feature/net-render-priority into cloud-init:master

[Scott Moser]

net: add renderers for automatically selecting the renderer.

Previously, the distro had hard coded which network renderer it would
use. This adds support for just picking the right renderer based
on what is available.

Now, that can be set via a priority in system_info, but should
generally work. That config looks like:
 system_info:
   network:
     renderers: ["eni", "sysconfig"]

When no renderers are found, a specific RendererNotFoundError is raised.
stages.py is modified to catch that and log it at error level. This
path should not really be exercised, but could occur if for example an
Ubuntu system did not have ifupdown, or a rhel system did not have
sysconfig. In such a system previously we would have quietly rendered
ENI configuration but that would have been ignored. This is one step
better in that we at least log the error.

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

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

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

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

review: Needs Fixing

[Scott Moser (smoser)]

please re-review.

review: Resubmit

[Ryan Harper (raharper)]

Add no renderer found exception and unittest as well.

review: Needs Fixing

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

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

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

review: Approve (continuous-integration)

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

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

i think i've convinced you of /etc/network/interfaces versus /etc/network/interfaces.d
we both agree that currently our detection of "which networking system is used" is remedial, and will need improvement over time.

the tests you asked for are added now i think.

review: Resubmit

[Ryan Harper (raharper)]

That looks right; thanks.

review: Approve

[Scott Moser (smoser)]

The change to raise a RuntimeError if there is no network renderer I think can cause a problem.
I'm trying to think through this, but in a situation where there is no ifupdown or sysconfig, previously we'd just write some files that were not going to do anything.

Ie, on ubuntu if you installed systemd-networkd and configured it to do something, and uninstalled ifupdown, then cloud-init would run and write ENI config, but if the system was configured to do the right thing ("dhcp on eth0" or whatever that was) then it might continue to work.

However, raising a RuntimeError here will throw exception up all the way through main.

currently cloudinit/stages.py's "apply_network_config" does handle a NotImplementedError in apply_network_config.

So that leaves me thinking of 2 options:
a.) change the raised RuntimeError to a NotImplementedError (which seems sane)
b.) change cloudinit/stages.py:apply_network_config to similarly catch the RunTimeException.

I think for now, 'a' seems the best case because a RuntimeError seems like there couldb e more reasons for that to occur.

I'll also change the LOG.warn there to log what message it was (currently it didn't even print the exception's message.

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

FAILED: Continuous integration, rev:9048420e719ef1eb67456e403edf2d51d77a1df6
https://jenkins.ubuntu.com/server/job/cloud-init-ci/135/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/135/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/135/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/135/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/135/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/135/console

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

review: Needs Fixing (continuous-integration)

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

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

in #cloud-init (https://irclogs.ubuntu.com/2017/03/17/%23cloud-init.html) rharper and I discussed and he was OK with what was here.

I've re-written the commit message to describe all changes made and squashed to trunk.
I'm pulling this now.

[Scott Moser (smoser)]

review: Approve

diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
index f3d395b..803ac74 100755
--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -22,6 +22,7 @@ from cloudinit import log as logging
 from cloudinit import net
 from cloudinit.net import eni
 from cloudinit.net import network_state
+from cloudinit.net import renderers
 from cloudinit import ssh_util
 from cloudinit import type_utils
 from cloudinit import util
@@ -50,6 +51,7 @@ class Distro(object):
     hostname_conf_fn = "/etc/hostname"
     tz_zone_dir = "/usr/share/zoneinfo"
     init_cmd = ['service']  # systemctl, service etc
+    renderer_configs = {}
 
     def __init__(self, name, cfg, paths):
         self._paths = paths
@@ -69,6 +71,17 @@ class Distro(object):
     def _write_network_config(self, settings):
         raise NotImplementedError()
 
+    def _supported_write_network_config(self, network_config):
+        priority = util.get_cfg_by_path(
+            self._cfg, ('network', 'renderers'), None)
+
+        name, render_cls = renderers.select(priority=priority)
+        LOG.debug("Selected renderer '%s' from priority list: %s",
+                  name, priority)
+        renderer = render_cls(config=self.renderer_configs.get(name))
+        renderer.render_network_config(network_config=network_config)
+        return []
+
     def _find_tz_file(self, tz):
         tz_file = os.path.join(self.tz_zone_dir, str(tz))
         if not os.path.isfile(tz_file):
diff --git a/cloudinit/distros/debian.py b/cloudinit/distros/debian.py
index 48ccec8..1101f02 100644
--- a/cloudinit/distros/debian.py
+++ b/cloudinit/distros/debian.py
@@ -13,8 +13,6 @@ import os
 from cloudinit import distros
 from cloudinit import helpers
 from cloudinit import log as logging
-from cloudinit.net import eni
-from cloudinit.net.network_state import parse_net_config_data
 from cloudinit import util
 
 from cloudinit.distros.parsers.hostname import HostnameConf
@@ -38,11 +36,18 @@ ENI_HEADER = """# This file is generated from information provided by
 # network: {config: disabled}
 """
 
+NETWORK_CONF_FN = "/etc/network/interfaces.d/50-cloud-init.cfg"
+
 
 class Distro(distros.Distro):
     hostname_conf_fn = "/etc/hostname"
     locale_conf_fn = "/etc/default/locale"
-    network_conf_fn = "/etc/network/interfaces.d/50-cloud-init.cfg"
+    renderer_configs = {
+        'eni': {
+            'eni_path': NETWORK_CONF_FN,
+            'eni_header': ENI_HEADER,
+        }
+    }
 
     def __init__(self, name, cfg, paths):
         distros.Distro.__init__(self, name, cfg, paths)
@@ -51,12 +56,6 @@ class Distro(distros.Distro):
         # should only happen say once per instance...)
         self._runner = helpers.Runners(paths)
         self.osfamily = 'debian'
-        self._net_renderer = eni.Renderer({
-            'eni_path': self.network_conf_fn,
-            'eni_header': ENI_HEADER,
-            'links_path_prefix': None,
-            'netrules_path': None,
-        })
 
     def apply_locale(self, locale, out_fn=None):
         if not out_fn:
@@ -76,14 +75,12 @@ class Distro(distros.Distro):
         self.package_command('install', pkgs=pkglist)
 
     def _write_network(self, settings):
-        util.write_file(self.network_conf_fn, settings)
+        util.write_file(NETWORK_CONF_FN, settings)
         return ['all']
 
     def _write_network_config(self, netconfig):
-        ns = parse_net_config_data(netconfig)
-        self._net_renderer.render_network_state("/", ns)
         _maybe_remove_legacy_eth0()
-        return []
+        return self._supported_write_network_config(netconfig)
 
     def _bring_up_interfaces(self, device_names):
         use_all = False
diff --git a/cloudinit/distros/rhel.py b/cloudinit/distros/rhel.py
index 7498c63..372c7d0 100644
--- a/cloudinit/distros/rhel.py
+++ b/cloudinit/distros/rhel.py
@@ -11,8 +11,6 @@
 from cloudinit import distros
 from cloudinit import helpers
 from cloudinit import log as logging
-from cloudinit.net.network_state import parse_net_config_data
-from cloudinit.net import sysconfig
 from cloudinit import util
 
 from cloudinit.distros import net_util
@@ -49,16 +47,13 @@ class Distro(distros.Distro):
         # should only happen say once per instance...)
         self._runner = helpers.Runners(paths)
         self.osfamily = 'redhat'
-        self._net_renderer = sysconfig.Renderer()
         cfg['ssh_svcname'] = 'sshd'
 
     def install_packages(self, pkglist):
         self.package_command('install', pkgs=pkglist)
 
     def _write_network_config(self, netconfig):
-        ns = parse_net_config_data(netconfig)
-        self._net_renderer.render_network_state("/", ns)
-        return []
+        return self._supported_write_network_config(netconfig)
 
     def _write_network(self, settings):
         # TODO(harlowja) fix this... since this is the ubuntu format
diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
index ea649cc..1cf98ef 100755
--- a/cloudinit/net/__init__.py
+++ b/cloudinit/net/__init__.py
@@ -364,4 +364,9 @@ def get_interfaces_by_mac(devs=None):
             ret[mac] = name
     return ret
 
+
+class RendererNotFoundError(RuntimeError):
+    pass
+
+
 # vi: ts=4 expandtab
diff --git a/cloudinit/net/eni.py b/cloudinit/net/eni.py
index efa034b..9d39a2b 100644
--- a/cloudinit/net/eni.py
+++ b/cloudinit/net/eni.py
@@ -500,4 +500,18 @@ def network_state_to_eni(network_state, header=None, render_hwaddress=False):
         network_state, render_hwaddress=render_hwaddress)
     return header + contents
 
+
+def available(target=None):
+    expected = ['ifquery', 'ifup', 'ifdown']
+    search = ['/sbin', '/usr/sbin']
+    for p in expected:
+        if not util.which(p, search=search, target=target):
+            return False
+    eni = util.target_path(target, 'etc/network/interfaces')
+    if not os.path.is_file(eni):
+        return False
+
+    return True
+
+
 # vi: ts=4 expandtab
diff --git a/cloudinit/net/renderer.py b/cloudinit/net/renderer.py
index 3a19243..a5b2b57 100644
--- a/cloudinit/net/renderer.py
+++ b/cloudinit/net/renderer.py
@@ -7,6 +7,7 @@
 
 import six
 
+from .network_state import parse_net_config_data
 from .udev import generate_udev_rule
 
 
@@ -36,4 +37,8 @@ class Renderer(object):
                                                  iface['mac_address']))
         return content.getvalue()
 
+    def render_network_config(self, network_config, target=None):
+        return self.render_network_state(
+            network_state=parse_net_config_data(network_config), target=target)
+
 # vi: ts=4 expandtab
diff --git a/cloudinit/net/renderers.py b/cloudinit/net/renderers.py
new file mode 100644
index 0000000..5ad8455
--- /dev/null
+++ b/cloudinit/net/renderers.py
@@ -0,0 +1,51 @@
+# This file is part of cloud-init. See LICENSE file for license information.
+
+from . import eni
+from . import RendererNotFoundError
+from . import sysconfig
+
+NAME_TO_RENDERER = {
+    "eni": eni,
+    "sysconfig": sysconfig,
+}
+
+DEFAULT_PRIORITY = ["eni", "sysconfig"]
+
+
+def search(priority=None, target=None, first=False):
+    if priority is None:
+        priority = DEFAULT_PRIORITY
+
+    available = NAME_TO_RENDERER
+
+    unknown = [i for i in priority if i not in available]
+    if unknown:
+        raise ValueError(
+            "Unknown renderers provided in priority list: %s" % unknown)
+
+    found = []
+    for name in priority:
+        render_mod = available[name]
+        if render_mod.available(target):
+            cur = (name, render_mod.Renderer)
+            if first:
+                return cur
+            found.append(cur)
+
+    return found
+
+
+def select(priority=None, target=None):
+    found = search(priority, target=target, first=True)
+    if not found:
+        if priority is None:
+            priority = DEFAULT_PRIORITY
+        tmsg = ""
+        if target and target != "/":
+            tmsg = " in target=%s" % target
+        raise RendererNotFoundError(
+            "No available network renderers found%s. Searched "
+            "through list: %s" % (tmsg, priority))
+    return found
+
+# vi: ts=4 expandtab
diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py
index 4eeaaa8..117b515 100644
--- a/cloudinit/net/sysconfig.py
+++ b/cloudinit/net/sysconfig.py
@@ -404,4 +404,21 @@ class Renderer(renderer.Renderer):
             netrules_path = util.target_path(target, self.netrules_path)
             util.write_file(netrules_path, netrules_content)
 
+
+def available(target=None):
+    expected = ['ifup', 'ifdown']
+    search = ['/sbin', '/usr/sbin']
+    for p in expected:
+        if not util.which(p, search=search, target=target):
+            return False
+
+    expected_paths = [
+        'etc/sysconfig/network-scripts/network-functions',
+        'etc/sysconfig/network-scripts/ifdown-eth']
+    for p in expected_paths:
+        if not os.path.isfile(util.target_path(target, p)):
+            return False
+    return True
+
+
 # vi: ts=4 expandtab
diff --git a/cloudinit/settings.py b/cloudinit/settings.py
index 692ff5e..dbafead 100644
--- a/cloudinit/settings.py
+++ b/cloudinit/settings.py
@@ -46,6 +46,7 @@ CFG_BUILTIN = {
             'templates_dir': '/etc/cloud/templates/',
         },
         'distro': 'ubuntu',
+        'network': {'renderers': None},
     },
     'vendor_data': {'enabled': True, 'prefix': []},
 }
diff --git a/cloudinit/stages.py b/cloudinit/stages.py
index 5bed903..1216543 100644
--- a/cloudinit/stages.py
+++ b/cloudinit/stages.py
@@ -646,9 +646,13 @@ class Init(object):
                  src, bring_up, netcfg)
         try:
             return self.distro.apply_network_config(netcfg, bring_up=bring_up)
+        except net.RendererNotFoundError as e:
+            LOG.error("Unable to render networking. Network config is "
+                      "likely broken: %s", e)
+            return
         except NotImplementedError:
             LOG.warn("distro '%s' does not implement apply_network_config. "
-                     "networking may not be configured properly." %
+                     "networking may not be configured properly.",
                      self.distro)
             return
 
diff --git a/cloudinit/util.py b/cloudinit/util.py
index 7196a7c..82f2f76 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -2099,21 +2099,36 @@ def get_mount_info(path, log=LOG):
         return parse_mount(path)
 
 
-def which(program):
-    # Return path of program for execution if found in path
-    def is_exe(fpath):
-        return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
-
-    _fpath, _ = os.path.split(program)
-    if _fpath:
-        if is_exe(program):
+def is_exe(fpath):
+    # return boolean indicating if fpath exists and is executable.
+    return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
+
+
+def which(program, search=None, target=None):
+    target = target_path(target)
+
+    if os.path.sep in program:
+        # if program had a '/' in it, then do not search PATH
+        # 'which' does consider cwd here. (cd / && which bin/ls) = bin/ls
+        # so effectively we set cwd to / (or target)
+        if is_exe(target_path(target, program)):
             return program
-    else:
-        for path in os.environ.get("PATH", "").split(os.pathsep):
-            path = path.strip('"')
-            exe_file = os.path.join(path, program)
-            if is_exe(exe_file):
-                return exe_file
+
+    if search is None:
+        paths = [p.strip('"') for p in
+                 os.environ.get("PATH", "").split(os.pathsep)]
+        if target == "/":
+            search = paths
+        else:
+            search = [p for p in paths if p.startswith("/")]
+
+    # normalize path input
+    search = [os.path.abspath(p) for p in search]
+
+    for path in search:
+        ppath = os.path.sep.join((path, program))
+        if is_exe(target_path(target, ppath)):
+            return ppath
 
     return None
 
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index dca44b3..902204a 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -4,6 +4,7 @@ from cloudinit import net
 from cloudinit.net import cmdline
 from cloudinit.net import eni
 from cloudinit.net import network_state
+from cloudinit.net import renderers
 from cloudinit.net import sysconfig
 from cloudinit.sources.helpers import openstack
 from cloudinit import util
@@ -1050,6 +1051,50 @@ class TestEniRoundTrip(CiTestCase):
             expected, [line for line in found if line])
 
 
+class TestNetRenderers(CiTestCase):
+    @mock.patch("cloudinit.net.renderers.sysconfig.available")
+    @mock.patch("cloudinit.net.renderers.eni.available")
+    def test_eni_and_sysconfig_available(self, m_eni_avail, m_sysc_avail):
+        m_eni_avail.return_value = True
+        m_sysc_avail.return_value = True
+        found = renderers.search(priority=['sysconfig', 'eni'], first=False)
+        names = [f[0] for f in found]
+        self.assertEqual(['sysconfig', 'eni'], names)
+
+    @mock.patch("cloudinit.net.renderers.eni.available")
+    def test_search_returns_empty_on_none(self, m_eni_avail):
+        m_eni_avail.return_value = False
+        found = renderers.search(priority=['eni'], first=False)
+        self.assertEqual([], found)
+
+    @mock.patch("cloudinit.net.renderers.sysconfig.available")
+    @mock.patch("cloudinit.net.renderers.eni.available")
+    def test_first_in_priority(self, m_eni_avail, m_sysc_avail):
+        # available should only be called until one is found.
+        m_eni_avail.return_value = True
+        m_sysc_avail.side_effect = Exception("Should not call me")
+        found = renderers.search(priority=['eni', 'sysconfig'], first=True)
+        self.assertEqual(['eni'], [found[0]])
+
+    @mock.patch("cloudinit.net.renderers.sysconfig.available")
+    @mock.patch("cloudinit.net.renderers.eni.available")
+    def test_select_positive(self, m_eni_avail, m_sysc_avail):
+        m_eni_avail.return_value = True
+        m_sysc_avail.return_value = False
+        found = renderers.select(priority=['sysconfig', 'eni'])
+        self.assertEqual('eni', found[0])
+
+    @mock.patch("cloudinit.net.renderers.sysconfig.available")
+    @mock.patch("cloudinit.net.renderers.eni.available")
+    def test_select_none_found_raises(self, m_eni_avail, m_sysc_avail):
+        # if select finds nothing, should raise exception.
+        m_eni_avail.return_value = False
+        m_sysc_avail.return_value = False
+
+        self.assertRaises(net.RendererNotFoundError, renderers.select,
+                          priority=['sysconfig', 'eni'])
+
+
 def _gzip_data(data):
     with io.BytesIO() as iobuf:
         gzfp = gzip.GzipFile(mode="wb", fileobj=iobuf)