From 8de0139f811a1abd33e0b03e4bc89bb2b8cdd597 Mon Sep 17 00:00:00 2001 From: Shreenidhi Shedi Date: Tue, 3 Aug 2021 17:42:10 +0530 Subject: [PATCH 1/2] distros/photon.py: refactor hostname handling Signed-off-by: Shreenidhi Shedi --- cloudinit/distros/photon.py | 53 ++++++++++--------- tests/unittests/test_distros/test_photon.py | 23 ++++++-- .../test_handler/test_handler_set_hostname.py | 26 ++++++--- 3 files changed, 67 insertions(+), 35 deletions(-) diff --git a/cloudinit/distros/photon.py b/cloudinit/distros/photon.py index 61e270c0890..95b6e1bf2b5 100644 --- a/cloudinit/distros/photon.py +++ b/cloudinit/distros/photon.py @@ -13,13 +13,12 @@ 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 @@ def __init__(self, name, cfg, paths): 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,7 +85,7 @@ def apply_locale(self, locale, out_fn=None): # 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() @@ -95,31 +95,31 @@ 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) + ret, _out, err = self.exec_cmd(cmd) + if ret: + LOG.error('Failed to bringup interfaces: %s', err) 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 +128,7 @@ def set_timezone(self, tz): 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 +142,9 @@ def package_command(self, command, args=None, pkgs=None): 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/tests/unittests/test_distros/test_photon.py b/tests/unittests/test_distros/test_photon.py index 775f37ac948..1c3145caf17 100644 --- a/tests/unittests/test_distros/test_photon.py +++ b/tests/unittests/test_distros/test_photon.py @@ -25,11 +25,28 @@ def test_network_renderer(self): 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 32ca3b7e23b..1a524c7d409 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 @@ def test_write_hostname_sles(self, m_uses_systemd): 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 @@ def test_photon_hostname(self, m_uses_systemd): } 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.""" From 71ebd2ed39b775dac518ca3913c4ec125481a60b Mon Sep 17 00:00:00 2001 From: Shreenidhi Shedi Date: Fri, 6 Aug 2021 15:23:24 +0530 Subject: [PATCH 2/2] Add networkd activator Signed-off-by: Shreenidhi Shedi --- cloudinit/distros/photon.py | 9 --------- cloudinit/net/activators.py | 27 ++++++++++++++++++++++++++ cloudinit/net/networkd.py | 2 +- tests/unittests/test_net_activators.py | 27 ++++++++++++++++++++++++-- 4 files changed, 53 insertions(+), 12 deletions(-) diff --git a/cloudinit/distros/photon.py b/cloudinit/distros/photon.py index 95b6e1bf2b5..4ff90ea6673 100644 --- a/cloudinit/distros/photon.py +++ b/cloudinit/distros/photon.py @@ -91,15 +91,6 @@ 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) - if ret: - LOG.error('Failed to bringup interfaces: %s', err) - return ret - def _write_hostname(self, hostname, filename): if filename and filename.endswith('/previous-hostname'): util.write_file(filename, hostname) diff --git a/cloudinit/net/activators.py b/cloudinit/net/activators.py index 84aaafc9101..11149548b6f 100644 --- a/cloudinit/net/activators.py +++ b/cloudinit/net/activators.py @@ -8,6 +8,7 @@ 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 @@ def bring_down_all_interfaces(network_state: NetworkState) -> bool: 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 2dffce59b67..63e3a07f54f 100644 --- a/cloudinit/net/networkd.py +++ b/cloudinit/net/networkd.py @@ -246,7 +246,7 @@ def _render_content(self, ns): 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_net_activators.py b/tests/unittests/test_net_activators.py index db825c35737..38f2edf22f6 100644 --- a/tests/unittests/test_net_activators.py +++ b/tests/unittests/test_net_activators.py @@ -11,7 +11,8 @@ 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 @@ def test_none_available(self, unavailable_mocks): (('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 @@ def test_available( ((['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 @@ def test_bring_up_interface( 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 @@ def test_bring_up_all_interfaces_v2( ((['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=('', ''))