photon: refactor hostname handling and add networkd activator (#958)

6 files changed, 117 insertions, 44 deletions

diff --git a/cloudinit/distros/photon.py b/cloudinit/distros/photon.py
index 61e270c0..4ff90ea6 100644
--- a/cloudinit/distros/photon.py
+++ b/cloudinit/distros/photon.py
@@ -13,13 +13,12 @@ from cloudinit import helpers
 from cloudinit import log as logging
 from cloudinit.settings import PER_INSTANCE
 from cloudinit.distros import rhel_util as rhutil
-from cloudinit.distros.parsers.hostname import HostnameConf
 
 LOG = logging.getLogger(__name__)
 
 
 class Distro(distros.Distro):
-    hostname_conf_fn = '/etc/hostname'
+    systemd_hostname_conf_fn = '/etc/hostname'
     network_conf_dir = '/etc/systemd/network/'
     systemd_locale_conf_fn = '/etc/locale.conf'
     resolve_conf_fn = '/etc/systemd/resolved.conf'
@@ -43,17 +42,18 @@ class Distro(distros.Distro):
         self.osfamily = 'photon'
         self.init_cmd = ['systemctl']
 
-    def exec_cmd(self, cmd, capture=False):
+    def exec_cmd(self, cmd, capture=True):
         LOG.debug('Attempting to run: %s', cmd)
         try:
             (out, err) = subp.subp(cmd, capture=capture)
             if err:
                 LOG.warning('Running %s resulted in stderr output: %s',
                             cmd, err)
-            return True, out, err
+                return True, out, err
+            return False, out, err
         except subp.ProcessExecutionError:
             util.logexc(LOG, 'Command %s failed', cmd)
-            return False, None, None
+            return True, None, None
 
     def generate_fallback_config(self):
         key = 'disable_fallback_netcfg'
@@ -85,41 +85,32 @@ class Distro(distros.Distro):
         # For locale change to take effect, reboot is needed or we can restart
         # systemd-localed. This is equivalent of localectl
         cmd = ['systemctl', 'restart', 'systemd-localed']
-        _ret, _out, _err = self.exec_cmd(cmd)
+        self.exec_cmd(cmd)
 
     def install_packages(self, pkglist):
         # self.update_package_sources()
         self.package_command('install', pkgs=pkglist)
 
-    def _bring_up_interfaces(self, device_names):
-        cmd = ['systemctl', 'restart', 'systemd-networkd', 'systemd-resolved']
-        LOG.debug('Attempting to run bring up interfaces using command %s',
-                  cmd)
-        ret, _out, _err = self.exec_cmd(cmd)
-        return ret
-
     def _write_hostname(self, hostname, filename):
-        conf = None
-        try:
-            # Try to update the previous one
-            # Let's see if we can read it first.
-            conf = HostnameConf(util.load_file(filename))
-            conf.parse()
-        except IOError:
-            pass
-        if not conf:
-            conf = HostnameConf('')
-        conf.set_hostname(hostname)
-        util.write_file(filename, str(conf), mode=0o644)
+        if filename and filename.endswith('/previous-hostname'):
+            util.write_file(filename, hostname)
+        else:
+            ret, _out, err = self.exec_cmd(['hostnamectl', 'set-hostname',
+                                            str(hostname)])
+            if ret:
+                LOG.warning(('Error while setting hostname: %s\n'
+                             'Given hostname: %s', err, hostname))
 
     def _read_system_hostname(self):
-        sys_hostname = self._read_hostname(self.hostname_conf_fn)
-        return (self.hostname_conf_fn, sys_hostname)
+        sys_hostname = self._read_hostname(self.systemd_hostname_conf_fn)
+        return (self.systemd_hostname_conf_fn, sys_hostname)
 
     def _read_hostname(self, filename, default=None):
-        _ret, out, _err = self.exec_cmd(['hostname'])
+        if filename and filename.endswith('/previous-hostname'):
+            return util.load_file(filename).strip()
 
-        return out if out else default
+        _ret, out, _err = self.exec_cmd(['hostname', '-f'])
+        return out.strip() if out else default
 
     def _get_localhost_ip(self):
         return '127.0.1.1'
@@ -128,7 +119,7 @@ class Distro(distros.Distro):
         distros.set_etc_timezone(tz=tz, tz_file=self._find_tz_file(tz))
 
     def package_command(self, command, args=None, pkgs=None):
-        if pkgs is None:
+        if not pkgs:
             pkgs = []
 
         cmd = ['tdnf', '-y']
@@ -142,8 +133,9 @@ class Distro(distros.Distro):
         pkglist = util.expand_package_list('%s-%s', pkgs)
         cmd.extend(pkglist)
 
-        # Allow the output of this to flow outwards (ie not be captured)
-        _ret, _out, _err = self.exec_cmd(cmd, capture=False)
+        ret, _out, err = self.exec_cmd(cmd)
+        if ret:
+            LOG.error('Error while installing packages: %s', err)
 
     def update_package_sources(self):
         self._runner.run('update-sources', self.package_command,
diff --git a/cloudinit/net/activators.py b/cloudinit/net/activators.py
index 84aaafc9..11149548 100644
--- a/cloudinit/net/activators.py
+++ b/cloudinit/net/activators.py
@@ -8,6 +8,7 @@ from cloudinit import subp
 from cloudinit import util
 from cloudinit.net.eni import available as eni_available
 from cloudinit.net.netplan import available as netplan_available
+from cloudinit.net.networkd import available as networkd_available
 from cloudinit.net.network_state import NetworkState
 from cloudinit.net.sysconfig import NM_CFG_FILE
 
@@ -213,12 +214,38 @@ class NetplanActivator(NetworkActivator):
         return _alter_interface(NetplanActivator.NETPLAN_CMD, 'all')
 
 
+class NetworkdActivator(NetworkActivator):
+    @staticmethod
+    def available(target=None) -> bool:
+        """Return true if ifupdown can be used on this system."""
+        return networkd_available(target=target)
+
+    @staticmethod
+    def bring_up_interface(device_name: str) -> bool:
+        """ Return True is successful, otherwise return False """
+        cmd = ['ip', 'link', 'set', 'up', device_name]
+        return _alter_interface(cmd, device_name)
+
+    @staticmethod
+    def bring_up_all_interfaces(network_state: NetworkState) -> bool:
+        """ Return True is successful, otherwise return False """
+        cmd = ['systemctl', 'restart', 'systemd-networkd', 'systemd-resolved']
+        return _alter_interface(cmd, 'all')
+
+    @staticmethod
+    def bring_down_interface(device_name: str) -> bool:
+        """ Return True is successful, otherwise return False """
+        cmd = ['ip', 'link', 'set', 'down', device_name]
+        return _alter_interface(cmd, device_name)
+
+
 # This section is mostly copied and pasted from renderers.py. An abstract
 # version to encompass both seems overkill at this point
 DEFAULT_PRIORITY = [
     IfUpDownActivator,
     NetworkManagerActivator,
     NetplanActivator,
+    NetworkdActivator,
 ]
 
 
diff --git a/cloudinit/net/networkd.py b/cloudinit/net/networkd.py
index 2dffce59..63e3a07f 100644
--- a/cloudinit/net/networkd.py
+++ b/cloudinit/net/networkd.py
@@ -246,7 +246,7 @@ class Renderer(renderer.Renderer):
 
 
 def available(target=None):
-    expected = ['systemctl']
+    expected = ['ip', 'systemctl']
     search = ['/usr/bin', '/bin']
     for p in expected:
         if not subp.which(p, search=search, target=target):
diff --git a/tests/unittests/test_distros/test_photon.py b/tests/unittests/test_distros/test_photon.py
index 775f37ac..1c3145ca 100644
--- a/tests/unittests/test_distros/test_photon.py
+++ b/tests/unittests/test_distros/test_photon.py
@@ -25,11 +25,28 @@ class TestPhoton(CiTestCase):
     def test_get_distro(self):
         self.assertEqual(self.distro.osfamily, 'photon')
 
-    def test_write_hostname(self):
+    @mock.patch("cloudinit.distros.photon.subp.subp")
+    def test_write_hostname(self, m_subp):
         hostname = 'myhostname'
-        hostfile = self.tmp_path('hostfile')
+        hostfile = self.tmp_path('previous-hostname')
         self.distro._write_hostname(hostname, hostfile)
-        self.assertEqual(hostname + '\n', util.load_file(hostfile))
+        self.assertEqual(hostname, util.load_file(hostfile))
+
+        ret = self.distro._read_hostname(hostfile)
+        self.assertEqual(ret, hostname)
+
+        m_subp.return_value = (None, None)
+        hostfile += 'hostfile'
+        self.distro._write_hostname(hostname, hostfile)
+
+        m_subp.return_value = (hostname, None)
+        ret = self.distro._read_hostname(hostfile)
+        self.assertEqual(ret, hostname)
+
+        self.logs.truncate(0)
+        m_subp.return_value = (None, 'bla')
+        self.distro._write_hostname(hostname, None)
+        self.assertIn('Error while setting hostname', self.logs.getvalue())
 
     @mock.patch('cloudinit.net.generate_fallback_config')
     def test_fallback_netcfg(self, m_fallback_cfg):
diff --git a/tests/unittests/test_handler/test_handler_set_hostname.py b/tests/unittests/test_handler/test_handler_set_hostname.py
index 32ca3b7e..1a524c7d 100644
--- a/tests/unittests/test_handler/test_handler_set_hostname.py
+++ b/tests/unittests/test_handler/test_handler_set_hostname.py
@@ -120,8 +120,8 @@ class TestHostname(t_help.FilesystemMockingTestCase):
         contents = util.load_file(distro.hostname_conf_fn)
         self.assertEqual('blah', contents.strip())
 
-    @mock.patch('cloudinit.distros.Distro.uses_systemd', return_value=False)
-    def test_photon_hostname(self, m_uses_systemd):
+    @mock.patch('cloudinit.distros.photon.subp.subp')
+    def test_photon_hostname(self, m_subp):
         cfg1 = {
             'hostname': 'photon',
             'prefer_fqdn_over_hostname': True,
@@ -134,17 +134,31 @@ class TestHostname(t_help.FilesystemMockingTestCase):
         }
 
         ds = None
+        m_subp.return_value = (None, None)
         distro = self._fetch_distro('photon', cfg1)
         paths = helpers.Paths({'cloud_dir': self.tmp})
         cc = cloud.Cloud(ds, paths, {}, distro, None)
-        self.patchUtils(self.tmp)
         for c in [cfg1, cfg2]:
             cc_set_hostname.handle('cc_set_hostname', c, cc, LOG, [])
-            contents = util.load_file(distro.hostname_conf_fn, decode=True)
+            print("\n", m_subp.call_args_list)
             if c['prefer_fqdn_over_hostname']:
-                self.assertEqual(contents.strip(), c['fqdn'])
+                assert [
+                    mock.call(['hostnamectl', 'set-hostname', c['fqdn']],
+                              capture=True)
+                ] in m_subp.call_args_list
+                assert [
+                    mock.call(['hostnamectl', 'set-hostname', c['hostname']],
+                              capture=True)
+                ] not in m_subp.call_args_list
             else:
-                self.assertEqual(contents.strip(), c['hostname'])
+                assert [
+                    mock.call(['hostnamectl', 'set-hostname', c['hostname']],
+                              capture=True)
+                ] in m_subp.call_args_list
+                assert [
+                    mock.call(['hostnamectl', 'set-hostname', c['fqdn']],
+                              capture=True)
+                ] not in m_subp.call_args_list
 
     def test_multiple_calls_skips_unchanged_hostname(self):
         """Only new hostname or fqdn values will generate a hostname call."""
diff --git a/tests/unittests/test_net_activators.py b/tests/unittests/test_net_activators.py
index db825c35..38f2edf2 100644
--- a/tests/unittests/test_net_activators.py
+++ b/tests/unittests/test_net_activators.py
@@ -11,7 +11,8 @@ from cloudinit.net.activators import (
 from cloudinit.net.activators import (
     IfUpDownActivator,
     NetplanActivator,
-    NetworkManagerActivator
+    NetworkManagerActivator,
+    NetworkdActivator
 )
 from cloudinit.net.network_state import parse_net_config_data
 from cloudinit.safeyaml import load
@@ -116,11 +117,17 @@ NETWORK_MANAGER_AVAILABLE_CALLS = [
     (('nmcli',), {'target': None}),
 ]
 
+NETWORKD_AVAILABLE_CALLS = [
+    (('ip',), {'search': ['/usr/bin', '/bin'], 'target': None}),
+    (('systemctl',), {'search': ['/usr/bin', '/bin'], 'target': None}),
+]
+
 
 @pytest.mark.parametrize('activator, available_calls', [
     (IfUpDownActivator, IF_UP_DOWN_AVAILABLE_CALLS),
     (NetplanActivator, NETPLAN_AVAILABLE_CALLS),
     (NetworkManagerActivator, NETWORK_MANAGER_AVAILABLE_CALLS),
+    (NetworkdActivator, NETWORKD_AVAILABLE_CALLS),
 ])
 class TestActivatorsAvailable:
     def test_available(
@@ -140,11 +147,18 @@ NETWORK_MANAGER_BRING_UP_CALL_LIST = [
     ((['nmcli', 'connection', 'up', 'ifname', 'eth1'], ), {}),
 ]
 
+NETWORKD_BRING_UP_CALL_LIST = [
+    ((['ip', 'link', 'set', 'up', 'eth0'], ), {}),
+    ((['ip', 'link', 'set', 'up', 'eth1'], ), {}),
+    ((['systemctl', 'restart', 'systemd-networkd', 'systemd-resolved'], ), {}),
+]
+
 
 @pytest.mark.parametrize('activator, expected_call_list', [
     (IfUpDownActivator, IF_UP_DOWN_BRING_UP_CALL_LIST),
     (NetplanActivator, NETPLAN_CALL_LIST),
     (NetworkManagerActivator, NETWORK_MANAGER_BRING_UP_CALL_LIST),
+    (NetworkdActivator, NETWORKD_BRING_UP_CALL_LIST),
 ])
 class TestActivatorsBringUp:
     @patch('cloudinit.subp.subp', return_value=('', ''))
@@ -159,8 +173,11 @@ class TestActivatorsBringUp:
     def test_bring_up_interfaces(
         self, m_subp, activator, expected_call_list, available_mocks
     ):
+        index = 0
         activator.bring_up_interfaces(['eth0', 'eth1'])
-        assert expected_call_list == m_subp.call_args_list
+        for call in m_subp.call_args_list:
+            assert call == expected_call_list[index]
+            index += 1
 
     @patch('cloudinit.subp.subp', return_value=('', ''))
     def test_bring_up_all_interfaces_v1(
@@ -191,11 +208,17 @@ NETWORK_MANAGER_BRING_DOWN_CALL_LIST = [
     ((['nmcli', 'connection', 'down', 'eth1'], ), {}),
 ]
 
+NETWORKD_BRING_DOWN_CALL_LIST = [
+    ((['ip', 'link', 'set', 'down', 'eth0'], ), {}),
+    ((['ip', 'link', 'set', 'down', 'eth1'], ), {}),
+]
+
 
 @pytest.mark.parametrize('activator, expected_call_list', [
     (IfUpDownActivator, IF_UP_DOWN_BRING_DOWN_CALL_LIST),
     (NetplanActivator, NETPLAN_CALL_LIST),
     (NetworkManagerActivator, NETWORK_MANAGER_BRING_DOWN_CALL_LIST),
+    (NetworkdActivator, NETWORKD_BRING_DOWN_CALL_LIST),
 ])
 class TestActivatorsBringDown:
     @patch('cloudinit.subp.subp', return_value=('', ''))