From 4bc1b7305199fffc78439ab1ad1cdb8272988d52 Mon Sep 17 00:00:00 2001 From: Vendula Poncova Date: Fri, 3 Jul 2020 12:21:11 +0200 Subject: [PATCH] Use the structure for time sources in the Timezone module Modify the Timezone module to work with the DBus structures for time sources instead of the strings. Rename the DBus property NTPServers to TimeSources. --- pyanaconda/modules/timezone/installation.py | 14 ++- pyanaconda/modules/timezone/timezone.py | 44 ++++--- .../modules/timezone/timezone_interface.py | 34 +++--- .../pyanaconda_tests/module_timezone_test.py | 107 ++++++++++++++---- 4 files changed, 141 insertions(+), 58 deletions(-) diff --git a/pyanaconda/modules/timezone/installation.py b/pyanaconda/modules/timezone/installation.py index 6383df1103..c3ea4d7179 100644 --- a/pyanaconda/modules/timezone/installation.py +++ b/pyanaconda/modules/timezone/installation.py @@ -145,12 +145,14 @@ def run(self): return chronyd_conf_path = os.path.normpath(self._sysroot + ntp.NTP_CONFIG_FILE) - pools, servers = ntp.internal_to_pools_and_servers(self._ntp_servers) if os.path.exists(chronyd_conf_path): log.debug("Modifying installed chrony configuration") try: - ntp.save_servers_to_config(pools, servers, conf_file_path=chronyd_conf_path) + ntp.save_servers_to_config( + self._ntp_servers, + conf_file_path=chronyd_conf_path + ) except ntp.NTPconfigError as ntperr: log.warning("Failed to save NTP configuration: %s", ntperr) @@ -160,9 +162,11 @@ def run(self): log.debug("Creating chrony configuration based on the " "configuration from installation environment") try: - ntp.save_servers_to_config(pools, servers, - conf_file_path=ntp.NTP_CONFIG_FILE, - out_file_path=chronyd_conf_path) + ntp.save_servers_to_config( + self._ntp_servers, + conf_file_path=ntp.NTP_CONFIG_FILE, + out_file_path=chronyd_conf_path + ) except ntp.NTPconfigError as ntperr: log.warning("Failed to save NTP configuration without chrony package: %s", ntperr) diff --git a/pyanaconda/modules/timezone/timezone.py b/pyanaconda/modules/timezone/timezone.py index 0678072978..ff89d1ea77 100644 --- a/pyanaconda/modules/timezone/timezone.py +++ b/pyanaconda/modules/timezone/timezone.py @@ -18,10 +18,12 @@ # Red Hat, Inc. # from pyanaconda.core.configuration.anaconda import conf +from pyanaconda.core.constants import TIME_SOURCE_SERVER from pyanaconda.core.dbus import DBus from pyanaconda.core.signal import Signal from pyanaconda.modules.common.base import KickstartService from pyanaconda.modules.common.constants.services import TIMEZONE +from pyanaconda.modules.common.structures.timezone import TimeSourceData from pyanaconda.timezone import NTP_PACKAGE from pyanaconda.modules.common.containers import TaskContainer from pyanaconda.modules.common.structures.requirement import Requirement @@ -48,8 +50,8 @@ def __init__(self): self.ntp_enabled_changed = Signal() self._ntp_enabled = True - self.ntp_servers_changed = Signal() - self._ntp_servers = [] + self.time_sources_changed = Signal() + self._time_sources = [] # FIXME: temporary workaround until PAYLOAD module is available self._ntp_excluded = False @@ -70,7 +72,17 @@ def process_kickstart(self, data): self.set_timezone(data.timezone.timezone) self.set_is_utc(data.timezone.isUtc) self.set_ntp_enabled(not data.timezone.nontp) - self.set_ntp_servers(data.timezone.ntpservers) + + servers = [] + + for hostname in data.timezone.ntpservers: + server = TimeSourceData() + server.type = TIME_SOURCE_SERVER + server.hostname = hostname + server.options = ["iburst"] + servers.append(server) + + self.set_time_sources(servers) def setup_kickstart(self, data): """Set up the kickstart data.""" @@ -78,8 +90,12 @@ def setup_kickstart(self, data): data.timezone.isUtc = self.is_utc data.timezone.nontp = not self.ntp_enabled - if self.ntp_enabled: - data.timezone.ntpservers = list(self.ntp_servers) + if not self.ntp_enabled: + return + + data.timezone.ntpservers = [ + server.hostname for server in self.time_sources + ] @property def timezone(self): @@ -115,15 +131,15 @@ def set_ntp_enabled(self, ntp_enabled): log.debug("NTP is set to %s.", ntp_enabled) @property - def ntp_servers(self): - """Return a list of NTP servers.""" - return self._ntp_servers + def time_sources(self): + """Return a list of time sources.""" + return self._time_sources - def set_ntp_servers(self, servers): - """Set NTP servers.""" - self._ntp_servers = list(servers) - self.ntp_servers_changed.emit() - log.debug("NTP servers are set to %s.", servers) + def set_time_sources(self, servers): + """Set time sources.""" + self._time_sources = list(servers) + self.time_sources_changed.emit() + log.debug("Time sources are set to: %s", servers) def collect_requirements(self): """Return installation requirements for this module. @@ -168,6 +184,6 @@ def install_with_tasks(self): ConfigureNTPTask( sysroot=conf.target.system_root, ntp_enabled=self.ntp_enabled, - ntp_servers=self.ntp_servers + ntp_servers=self.time_sources ) ] diff --git a/pyanaconda/modules/timezone/timezone_interface.py b/pyanaconda/modules/timezone/timezone_interface.py index 03c5003f1e..f36e0b3723 100644 --- a/pyanaconda/modules/timezone/timezone_interface.py +++ b/pyanaconda/modules/timezone/timezone_interface.py @@ -17,13 +17,15 @@ # License and may only be used or replicated with the express permission of # Red Hat, Inc. # -from pyanaconda.modules.common.constants.services import TIMEZONE -from pyanaconda.modules.common.containers import TaskContainer from dasbus.server.property import emits_properties_changed from dasbus.typing import * # pylint: disable=wildcard-import -from pyanaconda.modules.common.base import KickstartModuleInterface from dasbus.server.interface import dbus_interface +from pyanaconda.modules.common.base import KickstartModuleInterface +from pyanaconda.modules.common.constants.services import TIMEZONE +from pyanaconda.modules.common.containers import TaskContainer +from pyanaconda.modules.common.structures.timezone import TimeSourceData + @dbus_interface(TIMEZONE.interface_name) class TimezoneInterface(KickstartModuleInterface): @@ -34,7 +36,7 @@ def connect_signals(self): self.watch_property("Timezone", self.implementation.timezone_changed) self.watch_property("IsUTC", self.implementation.is_utc_changed) self.watch_property("NTPEnabled", self.implementation.ntp_enabled_changed) - self.watch_property("NTPServers", self.implementation.ntp_servers_changed) + self.watch_property("TimeSources", self.implementation.time_sources_changed) @property def Timezone(self) -> Str: @@ -91,22 +93,26 @@ def SetNTPEnabled(self, ntp_enabled: Bool): self.implementation.set_ntp_enabled(ntp_enabled) @property - def NTPServers(self) -> List[Str]: - """A list of NTP servers. + def TimeSources(self) -> List[Structure]: + """A list of time sources. - :return: a list of servers + :return: a list of time source data + :rtype: a list of structures of the type TimeSourceData """ - return self.implementation.ntp_servers + return TimeSourceData.to_structure_list( + self.implementation.time_sources + ) @emits_properties_changed - def SetNTPServers(self, servers: List[Str]): - """Set the NTP servers. + def SetTimeSources(self, sources: List[Structure]): + """Set the time sources. - Example: [ntp.cesnet.cz] - - :param servers: a list of servers + :param sources: a list of time sources + :type sources: a list of structures of the type TimeSourceData """ - self.implementation.set_ntp_servers(servers) + self.implementation.set_time_sources( + TimeSourceData.from_structure_list(sources) + ) def ConfigureNTPServiceEnablementWithTask(self, ntp_excluded: Bool) -> ObjPath: """Enable or disable NTP service. diff --git a/tests/nosetests/pyanaconda_tests/module_timezone_test.py b/tests/nosetests/pyanaconda_tests/module_timezone_test.py index f991f1e992..bb751d6f4b 100644 --- a/tests/nosetests/pyanaconda_tests/module_timezone_test.py +++ b/tests/nosetests/pyanaconda_tests/module_timezone_test.py @@ -23,8 +23,13 @@ from shutil import copytree, copyfile from unittest.mock import Mock, patch +from dasbus.structure import compare_data +from dasbus.typing import * # pylint: disable=wildcard-import + +from pyanaconda.core.constants import TIME_SOURCE_SERVER, TIME_SOURCE_POOL from pyanaconda.modules.common.constants.services import TIMEZONE from pyanaconda.modules.common.errors.installation import TimezoneConfigurationError +from pyanaconda.modules.common.structures.timezone import TimeSourceData from pyanaconda.modules.timezone.installation import ConfigureNTPTask, ConfigureTimezoneTask, \ ConfigureNTPServiceEnablementTask from pyanaconda.modules.common.structures.kickstart import KickstartReport @@ -33,7 +38,7 @@ from pyanaconda.ntp import NTP_CONFIG_FILE, NTPconfigError from tests.nosetests.pyanaconda_tests import check_kickstart_interface, \ patch_dbus_publish_object, PropertiesChangedCallback, check_task_creation, \ - patch_dbus_get_proxy, check_task_creation_list + patch_dbus_get_proxy, check_task_creation_list, check_dbus_property from pyanaconda.timezone import NTP_SERVICE @@ -50,6 +55,14 @@ def setUp(self): self.callback = PropertiesChangedCallback() self.timezone_interface.PropertiesChanged.connect(self.callback) + def _check_dbus_property(self, *args, **kwargs): + check_dbus_property( + self, + TIMEZONE, + self.timezone_interface, + *args, **kwargs + ) + def kickstart_properties_test(self): """Test kickstart properties.""" self.assertEqual(self.timezone_interface.KickstartCommands, ["timezone"]) @@ -76,12 +89,24 @@ def ntp_property_test(self): self.assertEqual(self.timezone_interface.NTPEnabled, False) self.callback.assert_called_once_with(TIMEZONE.interface_name, {'NTPEnabled': False}, []) - def ntp_servers_property_test(self): - """Test the NTPServers property.""" - self.timezone_interface.SetNTPServers(["ntp.cesnet.cz"]) - self.assertEqual(self.timezone_interface.NTPServers, ["ntp.cesnet.cz"]) - self.callback.assert_called_once_with( - TIMEZONE.interface_name, {'NTPServers': ["ntp.cesnet.cz"]}, []) + def time_sources_property_test(self): + """Test the TimeSources property.""" + server = { + "type": get_variant(Str, TIME_SOURCE_SERVER), + "hostname": get_variant(Str, "ntp.cesnet.cz"), + "options": get_variant(List[Str], ["iburst"]), + } + + pool = { + "type": get_variant(Str, TIME_SOURCE_POOL), + "hostname": get_variant(Str, "0.fedora.pool.ntp.org"), + "options": get_variant(List[Str], []), + } + + self._check_dbus_property( + "TimeSources", + [server, pool] + ) def _test_kickstart(self, ks_in, ks_out): check_kickstart_interface(self, self.timezone_interface, ks_in, ks_out) @@ -162,10 +187,19 @@ def install_with_tasks_configured_test(self, publisher): self.timezone_interface.SetNTPEnabled(False) # --nontp and --ntpservers are mutually exclusive in kicstart but # there is no such enforcement in the module so for testing this is ok - self.timezone_interface.SetNTPServers([ - "clock1.example.com", - "clock2.example.com", - ]) + + server = TimeSourceData() + server.type = TIME_SOURCE_SERVER + server.hostname = "clock1.example.com" + server.options = ["iburst"] + + pool = TimeSourceData() + pool.type = TIME_SOURCE_POOL + pool.hostname = "clock2.example.com" + + self.timezone_interface.SetTimeSources( + TimeSourceData.to_structure_list([server, pool]) + ) task_classes = [ ConfigureTimezoneTask, @@ -182,10 +216,9 @@ def install_with_tasks_configured_test(self, publisher): # ConfigureNTPTask obj = task_objs[1] self.assertEqual(obj.implementation._ntp_enabled, False) - self.assertEqual(obj.implementation._ntp_servers, [ - "clock1.example.com", - "clock2.example.com", - ]) + self.assertEqual(len(obj.implementation._ntp_servers), 2) + self.assertTrue(compare_data(obj.implementation._ntp_servers[0], server)) + self.assertTrue(compare_data(obj.implementation._ntp_servers[1], pool)) @patch_dbus_publish_object def configure_ntp_service_enablement_default_test(self, publisher): @@ -354,13 +387,13 @@ class NTPTasksTestCase(unittest.TestCase): def ntp_task_success_test(self): """Test the success cases for NTP setup D-Bus task.""" - self._test_ntp_inputs(False, False, ["unique.ntp.server", "another.unique.server"]) - self._test_ntp_inputs(False, True, ["unique.ntp.server", "another.unique.server"]) + self._test_ntp_inputs(False, False) + self._test_ntp_inputs(False, True) def ntp_overwrite_test(self): """Test overwriting existing config for NTP setup D-Bus task.""" - self._test_ntp_inputs(True, True, ["unique.ntp.server", "another.unique.server"]) - self._test_ntp_inputs(True, False, ["unique.ntp.server", "another.unique.server"]) + self._test_ntp_inputs(True, True) + self._test_ntp_inputs(True, False) def ntp_save_failure_test(self): """Test failure when saving NTP config in D-Bus task.""" @@ -368,6 +401,25 @@ def ntp_save_failure_test(self): self._test_ntp_exception(True) self._test_ntp_exception(False) + def _get_test_sources(self): + """Get a list of sources""" + server = TimeSourceData() + server.type = TIME_SOURCE_SERVER + server.hostname = "unique.ntp.server" + server.options = ["iburst"] + + pool = TimeSourceData() + pool.type = TIME_SOURCE_POOL + pool.hostname = "another.unique.server" + + return [server, pool] + + def _get_expected_lines(self): + return [ + "server unique.ntp.server iburst\n", + "pool another.unique.server\n" + ] + @patch("pyanaconda.modules.timezone.installation.ntp.save_servers_to_config", side_effect=NTPconfigError) def _test_ntp_exception(self, make_chronyd, mock_save): @@ -376,11 +428,14 @@ def _test_ntp_exception(self, make_chronyd, mock_save): with self.assertLogs("anaconda.modules.timezone.installation", level="WARNING"): self._execute_task(sysroot, True, ["ntp.example.com"]) - def _test_ntp_inputs(self, make_chronyd, ntp_enabled, ntp_servers): + def _test_ntp_inputs(self, make_chronyd, ntp_enabled): + ntp_servers = self._get_test_sources() + expected_lines = self._get_expected_lines() + with tempfile.TemporaryDirectory() as sysroot: self._setup_environment(sysroot, make_chronyd) self._execute_task(sysroot, ntp_enabled, ntp_servers) - self._validate_ntp_config(sysroot, make_chronyd, ntp_enabled, ntp_servers) + self._validate_ntp_config(sysroot, make_chronyd, ntp_enabled, expected_lines) def _setup_environment(self, sysroot, make_chronyd): os.mkdir(sysroot + "/etc") @@ -395,12 +450,14 @@ def _execute_task(self, sysroot, ntp_enabled, ntp_servers): ) task.run() - def _validate_ntp_config(self, sysroot, was_present, was_enabled, expected_servers): + def _validate_ntp_config(self, sysroot, was_present, was_enabled, expected_lines): if was_enabled: with open(sysroot + NTP_CONFIG_FILE) as fobj: - all_lines = "\n".join(fobj.readlines()) - for server in expected_servers: - self.assertIn(server, all_lines) + all_lines = fobj.readlines() + + for line in expected_lines: + self.assertIn(line, all_lines) + elif not was_present: self.assertFalse(os.path.exists(sysroot + NTP_CONFIG_FILE)) -- 2.23.0