From ec10c3f1cb23e49b0f1a963632fc0ca3d216d855 Mon Sep 17 00:00:00 2001 From: Alexander Allen Date: Wed, 23 Feb 2022 20:23:34 +0000 Subject: [PATCH 01/10] Move hostcfgd back to ConfigDBConnector for subscribing to updates --- src/sonic-host-services/scripts/hostcfgd | 81 ++++++++++-------------- 1 file changed, 34 insertions(+), 47 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index 9b39fb5eb807..833732d3a567 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -987,6 +987,9 @@ class HostConfigDaemon: # Initialize AAACfg self.hostname_cache="" self.aaacfg = AaaCfg() + + # Initialize cache + self.cache = {} def load(self): @@ -995,6 +998,8 @@ class HostConfigDaemon: tacacs_server = self.config_db.get_table('TACPLUS_SERVER') radius_global = self.config_db.get_table('RADIUS') radius_server = self.config_db.get_table('RADIUS_SERVER') + self.cache = {'AAA': aaa, 'TACPLUS': tacacs_global, 'TACPLUS_SERVER': tacacs_server, + 'RADIUS': radius_global, 'RADIUS_SERVER': radius_server} self.aaacfg.load(aaa, tacacs_global, tacacs_server, radius_global, radius_server) try: @@ -1097,39 +1102,39 @@ class HostConfigDaemon: systemctl_cmd = "sudo systemctl is-system-running --wait --quiet" subprocess.call(systemctl_cmd, shell=True) - def subscribe(self, table, callback, pri): - try: - if table not in self.callbacks: - self.callbacks[table] = [] - subscriber = SubscriberStateTable(self.dbconn, table, TableConsumable.DEFAULT_POP_BATCH_SIZE, pri) - self.selector.addSelectable(subscriber) # Add to the Selector - self.subscriber_map[subscriber.getFd()] = (subscriber, table) # Maintain a mapping b/w subscriber & fd + def register_callbacks(self): - self.callbacks[table].append(callback) - except Exception as err: - syslog.syslog(syslog.LOG_ERR, "Subscribe to table {} failed with error {}".format(table, err)) + def make_callback(func): + if data is None: + op = "DEL" + else: + op = "SET" - def register_callbacks(self): - self.subscribe('KDUMP', lambda table, key, op, data: self.kdump_handler(key, op, data), HOSTCFGD_MAX_PRI) + def callback(table, key, data): + return func(key, op, data) + + return callback + + self.config_db.subscribe('KDUMP', make_callback(self.config_db.kdump_handler), init=True) # Handle FEATURE updates before other tables - self.subscribe('FEATURE', lambda table, key, op, data: self.feature_handler.handle(key, op, data), HOSTCFGD_MAX_PRI-1) + self.config_db.subscribe('FEATURE', make_callback(self.config_db.feature_handler.handle), init=True) # Handle AAA, TACACS and RADIUS related tables - self.subscribe('AAA', lambda table, key, op, data: self.aaa_handler(key, op, data), HOSTCFGD_MAX_PRI-2) - self.subscribe('TACPLUS', lambda table, key, op, data: self.tacacs_global_handler(key, op, data), HOSTCFGD_MAX_PRI-2) - self.subscribe('TACPLUS_SERVER', lambda table, key, op, data: self.tacacs_server_handler(key, op, data), HOSTCFGD_MAX_PRI-2) - self.subscribe('RADIUS', lambda table, key, op, data: self.radius_global_handler(key, op, data), HOSTCFGD_MAX_PRI-2) - self.subscribe('RADIUS_SERVER', lambda table, key, op, data: self.radius_server_handler(key, op, data), HOSTCFGD_MAX_PRI-2) + self.config_db.subscribe('AAA', make_callback(self.config_db.aaa_handler), init=True) + self.config_db.subscribe('TACPLUS', make_callback(self.config_db.tacacs_global_handler), init=True) + self.config_db.subscribe('TACPLUS_SERVER', make_callback(self.config_db.tacacs_server_handler, init=True) + self.config_db.subscribe('RADIUS', make_callback(self.config_db.radius_global_handler), init=True) + self.config_db.subscribe('RADIUS_SERVER', make_callback(self.config_db.radius_server_handler), init=True) # Handle IPTables configuration - self.subscribe('LOOPBACK_INTERFACE', lambda table, key, op, data: self.lpbk_handler(key, op, data), HOSTCFGD_MAX_PRI-3) + self.config_db.subscribe('LOOPBACK_INTERFACE', make_callback(self.config_db.lpbk_handler), init=True) # Handle NTP & NTP_SERVER updates - self.subscribe('NTP', lambda table, key, op, data: self.ntp_global_handler(key, op, data), HOSTCFGD_MAX_PRI-4) - self.subscribe('NTP_SERVER', lambda table, key, op, data: self.ntp_server_handler(key, op, data), HOSTCFGD_MAX_PRI-4) + self.config_db.subscribe('NTP', make_callback(self.config_db.ntp_global_handler), init=True) + self.config_db.subscribe('NTP_SERVER', make_callback(self.config_db.ntp_server_handler), init=True) # Handle updates to src intf changes in radius - self.subscribe('MGMT_INTERFACE', lambda table, key, op, data: self.mgmt_intf_handler(key, op, data), HOSTCFGD_MAX_PRI-5) - self.subscribe('VLAN_INTERFACE', lambda table, key, op, data: self.vlan_intf_handler(key, op, data), HOSTCFGD_MAX_PRI-5) - self.subscribe('VLAN_SUB_INTERFACE', lambda table, key, op, data: self.vlan_sub_intf_handler(key, op, data), HOSTCFGD_MAX_PRI-5) - self.subscribe('PORTCHANNEL_INTERFACE', lambda table, key, op, data: self.portchannel_intf_handler(key, op, data), HOSTCFGD_MAX_PRI-5) - self.subscribe('INTERFACE', lambda table, key, op, data: self.phy_intf_handler(key, op, data), HOSTCFGD_MAX_PRI-5) + self.config_db.subscribe('MGMT_INTERFACE', make_callback(self.config_db.mgmt_intf_handler), init=True) + self.config_db.subscribe('VLAN_INTERFACE', make_callback(self.config_db.vlan_intf_handler), init=True) + self.config_db.subscribe('VLAN_SUB_INTERFACE', make_callback(self.config_db.vlan_sub_intf_handler), init=True) + self.config_db.subscribe('PORTCHANNEL_INTERFACE', make_callback(self.config_db.portchannel_intf_handler), init=True) + self.config_db.subscribe('INTERFACE', make_callback(self.config_db.phy_intf_handler), init=True) syslog.syslog(syslog.LOG_INFO, "Waiting for systemctl to finish initialization") @@ -1137,28 +1142,10 @@ class HostConfigDaemon: syslog.syslog(syslog.LOG_INFO, "systemctl has finished initialization -- proceeding ...") - def start(self): - while True: - state, selectable_ = self.selector.select(DEFAULT_SELECT_TIMEOUT) - if state == self.selector.TIMEOUT: - continue - elif state == self.selector.ERROR: - syslog.syslog(syslog.LOG_ERR, - "error returned by select") - continue + self.config_db.listen(start=False) - fd = selectable_.getFd() - # Get the Corresponding subscriber & table - subscriber, table = self.subscriber_map.get(fd, (None, "")) - if not subscriber: - syslog.syslog(syslog.LOG_ERR, - "No Subscriber object found for fd: {}, subscriber map: {}".format(fd, subscriber_map)) - continue - key, op, fvs = subscriber.pop() - # Get the registered callback - cbs = self.callbacks.get(table, None) - for callback in cbs: - callback(table, key, op, dict(fvs)) + def start(self): + self.config_db.process(cache=self.cache) def main(): From f860ca14c86bb87c5cd2e252e41ee97fc984f35e Mon Sep 17 00:00:00 2001 From: Alexander Allen Date: Thu, 24 Feb 2022 19:00:24 +0000 Subject: [PATCH 02/10] Update unit tests --- src/sonic-host-services/scripts/hostcfgd | 72 ++++++++++------- .../tests/common/mock_configdb.py | 79 +++---------------- .../tests/hostcfgd/hostcfgd_radius_test.py | 6 +- .../tests/hostcfgd/hostcfgd_tacacs_test.py | 6 +- .../tests/hostcfgd/hostcfgd_test.py | 14 ++-- 5 files changed, 60 insertions(+), 117 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index 833732d3a567..992f53d18b82 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -11,7 +11,6 @@ import signal import jinja2 from sonic_py_common import device_info -from swsscommon.swsscommon import SubscriberStateTable, DBConnector, Select from swsscommon.swsscommon import ConfigDBConnector, TableConsumable # FILE @@ -195,6 +194,20 @@ class FeatureHandler(object): else: self.resync_feature_state(self._cached_config[feature_name]) + def update_all_features_config(self, feature_table): + for feature_name in feature_table.keys(): + if not feature_name: + syslog.syslog(syslog.LOG_WARNING, "Feature is None") + continue + + feature = Feature(feature_name, feature_table[feature_name], self._device_config) + self._cached_config.setdefault(feature_name, feature) + + self.update_feature_auto_restart(feature) + + self.update_feature_state(feature) + self.resync_feature_state(feature) + def sync_state_field(self): """ Summary: @@ -956,14 +969,8 @@ class HostConfigDaemon: # before moving forward self.config_db = ConfigDBConnector() self.config_db.connect(wait_for_init=True, retry_on=True) - self.dbconn = DBConnector(CFG_DB, 0) - self.selector = Select() syslog.syslog(syslog.LOG_INFO, 'ConfigDB connect success') - self.select = Select() - self.callbacks = dict() - self.subscriber_map = dict() - # Load DEVICE metadata configurations self.device_config = {} self.device_config['DEVICE_METADATA'] = self.config_db.get_table('DEVICE_METADATA') @@ -993,6 +1000,9 @@ class HostConfigDaemon: def load(self): + features = self.config_db.get_table('FEATURE') + self.feature_handler.update_all_features_config(features) + aaa = self.config_db.get_table('AAA') tacacs_global = self.config_db.get_table('TACPLUS') tacacs_server = self.config_db.get_table('TACPLUS_SERVER') @@ -1002,6 +1012,14 @@ class HostConfigDaemon: 'RADIUS': radius_global, 'RADIUS_SERVER': radius_server} self.aaacfg.load(aaa, tacacs_global, tacacs_server, radius_global, radius_server) + lpbk_table = self.config_db.get_table('LOOPBACK_INTERFACE') + self.iptables.load(lpbk_table) + + # Load NTP configurations + ntp_server = self.config_db.get_table('NTP_SERVER') + ntp_global = self.config_db.get_table('NTP') + self.ntpcfg.load(ntp_global, ntp_server) + try: dev_meta = self.config_db.get_table('DEVICE_METADATA') if 'localhost' in dev_meta: @@ -1105,36 +1123,34 @@ class HostConfigDaemon: def register_callbacks(self): def make_callback(func): - if data is None: - op = "DEL" - else: - op = "SET" - def callback(table, key, data): + if data is None: + op = "DEL" + else: + op = "SET" return func(key, op, data) - return callback - self.config_db.subscribe('KDUMP', make_callback(self.config_db.kdump_handler), init=True) + self.config_db.subscribe('KDUMP', make_callback(self.kdump_handler)) # Handle FEATURE updates before other tables - self.config_db.subscribe('FEATURE', make_callback(self.config_db.feature_handler.handle), init=True) + self.config_db.subscribe('FEATURE', make_callback(self.feature_handler.handle)) # Handle AAA, TACACS and RADIUS related tables - self.config_db.subscribe('AAA', make_callback(self.config_db.aaa_handler), init=True) - self.config_db.subscribe('TACPLUS', make_callback(self.config_db.tacacs_global_handler), init=True) - self.config_db.subscribe('TACPLUS_SERVER', make_callback(self.config_db.tacacs_server_handler, init=True) - self.config_db.subscribe('RADIUS', make_callback(self.config_db.radius_global_handler), init=True) - self.config_db.subscribe('RADIUS_SERVER', make_callback(self.config_db.radius_server_handler), init=True) + self.config_db.subscribe('AAA', make_callback(self.aaa_handler)) + self.config_db.subscribe('TACPLUS', make_callback(self.tacacs_global_handler)) + self.config_db.subscribe('TACPLUS_SERVER', make_callback(self.tacacs_server_handler)) + self.config_db.subscribe('RADIUS', make_callback(self.radius_global_handler)) + self.config_db.subscribe('RADIUS_SERVER', make_callback(self.radius_server_handler)) # Handle IPTables configuration - self.config_db.subscribe('LOOPBACK_INTERFACE', make_callback(self.config_db.lpbk_handler), init=True) + self.config_db.subscribe('LOOPBACK_INTERFACE', make_callback(self.lpbk_handler)) # Handle NTP & NTP_SERVER updates - self.config_db.subscribe('NTP', make_callback(self.config_db.ntp_global_handler), init=True) - self.config_db.subscribe('NTP_SERVER', make_callback(self.config_db.ntp_server_handler), init=True) + self.config_db.subscribe('NTP', make_callback(self.ntp_global_handler)) + self.config_db.subscribe('NTP_SERVER', make_callback(self.ntp_server_handler)) # Handle updates to src intf changes in radius - self.config_db.subscribe('MGMT_INTERFACE', make_callback(self.config_db.mgmt_intf_handler), init=True) - self.config_db.subscribe('VLAN_INTERFACE', make_callback(self.config_db.vlan_intf_handler), init=True) - self.config_db.subscribe('VLAN_SUB_INTERFACE', make_callback(self.config_db.vlan_sub_intf_handler), init=True) - self.config_db.subscribe('PORTCHANNEL_INTERFACE', make_callback(self.config_db.portchannel_intf_handler), init=True) - self.config_db.subscribe('INTERFACE', make_callback(self.config_db.phy_intf_handler), init=True) + self.config_db.subscribe('MGMT_INTERFACE', make_callback(self.mgmt_intf_handler)) + self.config_db.subscribe('VLAN_INTERFACE', make_callback(self.vlan_intf_handler)) + self.config_db.subscribe('VLAN_SUB_INTERFACE', make_callback(self.vlan_sub_intf_handler)) + self.config_db.subscribe('PORTCHANNEL_INTERFACE', make_callback(self.portchannel_intf_handler)) + self.config_db.subscribe('INTERFACE', make_callback(self.phy_intf_handler)) syslog.syslog(syslog.LOG_INFO, "Waiting for systemctl to finish initialization") diff --git a/src/sonic-host-services/tests/common/mock_configdb.py b/src/sonic-host-services/tests/common/mock_configdb.py index 138869dc3bee..80a0912f580b 100644 --- a/src/sonic-host-services/tests/common/mock_configdb.py +++ b/src/sonic-host-services/tests/common/mock_configdb.py @@ -4,9 +4,10 @@ class MockConfigDb(object): """ STATE_DB = None CONFIG_DB = None + event_queue = [] def __init__(self, **kwargs): - pass + self.handlers = {} @staticmethod def set_config_db(test_config_db): @@ -44,75 +45,13 @@ def set_entry(self, key, field, data): def get_table(self, table_name): return MockConfigDb.CONFIG_DB[table_name] + def subscribe(self, table_name, callback): + self.handlers[table_name] = callback -class MockSelect(): - - event_queue = [] - - @staticmethod - def set_event_queue(Q): - MockSelect.event_queue = Q - - @staticmethod - def get_event_queue(): - return MockSelect.event_queue - - @staticmethod - def reset_event_queue(): - MockSelect.event_queue = [] - - def __init__(self): - self.sub_map = {} - self.TIMEOUT = "TIMEOUT" - self.ERROR = "ERROR" - - def addSelectable(self, subscriber): - self.sub_map[subscriber.table] = subscriber - - def select(self, TIMEOUT): - if not MockSelect.get_event_queue(): - raise TimeoutError - table, key = MockSelect.get_event_queue().pop(0) - self.sub_map[table].nextKey(key) - return "OBJECT", self.sub_map[table] - - -class MockSubscriberStateTable(): - - FD_INIT = 0 - - @staticmethod - def generate_fd(): - curr = MockSubscriberStateTable.FD_INIT - MockSubscriberStateTable.FD_INIT = curr + 1 - return curr - - @staticmethod - def reset_fd(): - MockSubscriberStateTable.FD_INIT = 0 - - def __init__(self, conn, table, pop, pri): - self.fd = MockSubscriberStateTable.generate_fd() - self.next_key = '' - self.table = table - - def getFd(self): - return self.fd - - def nextKey(self, key): - self.next_key = key - - def pop(self): - table = MockConfigDb.CONFIG_DB.get(self.table, {}) - if self.next_key not in table: - op = "DEL" - fvs = {} - else: - op = "SET" - fvs = table.get(self.next_key, {}) - return self.next_key, op, fvs + def listen(self, start): + pass + def process(self, cache): + for e in MockConfigDb.event_queue: + self.handlers[e[0]](e[0], e[1], self.get_entry(e[0], e[1])) -class MockDBConnector(): - def __init__(self, db, val): - pass diff --git a/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py b/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py index 4e3d18648100..b7c5ff6fc286 100644 --- a/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py +++ b/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py @@ -10,8 +10,7 @@ from parameterized import parameterized from unittest import TestCase, mock from tests.hostcfgd.test_radius_vectors import HOSTCFGD_TEST_RADIUS_VECTOR -from tests.common.mock_configdb import MockConfigDb, MockSubscriberStateTable -from tests.common.mock_configdb import MockSelect, MockDBConnector +from tests.common.mock_configdb import MockConfigDb test_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) @@ -33,9 +32,6 @@ # Mock swsscommon classes hostcfgd.ConfigDBConnector = MockConfigDb -hostcfgd.SubscriberStateTable = MockSubscriberStateTable -hostcfgd.Select = MockSelect -hostcfgd.DBConnector = MockDBConnector class TestHostcfgdRADIUS(TestCase): diff --git a/src/sonic-host-services/tests/hostcfgd/hostcfgd_tacacs_test.py b/src/sonic-host-services/tests/hostcfgd/hostcfgd_tacacs_test.py index 3cc3504d606b..8935ccb5c91f 100644 --- a/src/sonic-host-services/tests/hostcfgd/hostcfgd_tacacs_test.py +++ b/src/sonic-host-services/tests/hostcfgd/hostcfgd_tacacs_test.py @@ -10,8 +10,7 @@ from parameterized import parameterized from unittest import TestCase, mock from tests.hostcfgd.test_tacacs_vectors import HOSTCFGD_TEST_TACACS_VECTOR -from tests.common.mock_configdb import MockConfigDb, MockSubscriberStateTable -from tests.common.mock_configdb import MockSelect, MockDBConnector +from tests.common.mock_configdb import MockConfigDb test_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) modules_path = os.path.dirname(test_path) @@ -32,9 +31,6 @@ # Mock swsscommon classes hostcfgd.ConfigDBConnector = MockConfigDb -hostcfgd.SubscriberStateTable = MockSubscriberStateTable -hostcfgd.Select = MockSelect -hostcfgd.DBConnector = MockDBConnector class TestHostcfgdTACACS(TestCase): """ diff --git a/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py b/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py index bbce866e2331..2538ddc2d81e 100644 --- a/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py +++ b/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py @@ -8,8 +8,7 @@ from unittest import TestCase, mock from .test_vectors import HOSTCFGD_TEST_VECTOR, HOSTCFG_DAEMON_CFG_DB -from tests.common.mock_configdb import MockConfigDb, MockSubscriberStateTable -from tests.common.mock_configdb import MockSelect, MockDBConnector +from tests.common.mock_configdb import MockConfigDb from pyfakefs.fake_filesystem_unittest import patchfs from deepdiff import DeepDiff @@ -24,9 +23,6 @@ hostcfgd_path = os.path.join(scripts_path, 'hostcfgd') hostcfgd = load_module_from_source('hostcfgd', hostcfgd_path) hostcfgd.ConfigDBConnector = MockConfigDb -hostcfgd.SubscriberStateTable = MockSubscriberStateTable -hostcfgd.Select = MockSelect -hostcfgd.DBConnector = MockDBConnector class TestHostcfgd(TestCase): @@ -209,7 +205,7 @@ def tearDown(self): @patchfs def test_feature_events(self, fs): fs.create_dir(hostcfgd.FeatureHandler.SYSTEMD_SYSTEM_DIR) - MockSelect.event_queue = [('FEATURE', 'dhcp_relay'), + MockConfigDb.event_queue = [('FEATURE', 'dhcp_relay'), ('FEATURE', 'mux'), ('FEATURE', 'telemetry')] daemon = hostcfgd.HostConfigDaemon() @@ -240,7 +236,7 @@ def test_feature_events(self, fs): # Change the state to disabled MockConfigDb.CONFIG_DB['FEATURE']['telemetry']['state'] = 'disabled' - MockSelect.event_queue = [('FEATURE', 'telemetry')] + MockConfigDb.event_queue = [('FEATURE', 'telemetry')] try: daemon.start() except TimeoutError: @@ -255,7 +251,7 @@ def test_feature_events(self, fs): def test_loopback_events(self): MockConfigDb.set_config_db(HOSTCFG_DAEMON_CFG_DB) - MockSelect.event_queue = [('NTP', 'global'), + MockConfigDb.event_queue = [('NTP', 'global'), ('NTP_SERVER', '0.debian.pool.ntp.org'), ('LOOPBACK_INTERFACE', 'Loopback0|10.184.8.233/32')] daemon = hostcfgd.HostConfigDaemon() @@ -279,7 +275,7 @@ def test_kdump_event(self): daemon = hostcfgd.HostConfigDaemon() daemon.register_callbacks() assert MockConfigDb.CONFIG_DB['KDUMP']['config'] - MockSelect.event_queue = [('KDUMP', 'config')] + MockConfigDb.event_queue = [('KDUMP', 'config')] with mock.patch('hostcfgd.subprocess') as mocked_subprocess: popen_mock = mock.Mock() attrs = {'communicate.return_value': ('output', 'error')} From 0fbaeec810c747626aed9aa5cf653d3fa0b3ea0a Mon Sep 17 00:00:00 2001 From: Alexander Allen Date: Fri, 25 Feb 2022 23:17:47 +0000 Subject: [PATCH 03/10] Fix a couple bugs and review feedback --- src/sonic-host-services/scripts/hostcfgd | 49 ++++++++++++++++-------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index 992f53d18b82..26fced83d14e 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -1,4 +1,4 @@ -#!/usr/bin/env python3 +#!/usr/bin/python3 import ast import copy @@ -203,7 +203,7 @@ class FeatureHandler(object): feature = Feature(feature_name, feature_table[feature_name], self._device_config) self._cached_config.setdefault(feature_name, feature) - self.update_feature_auto_restart(feature) + self.update_feature_auto_restart(feature, feature_name) self.update_feature_state(feature) self.resync_feature_state(feature) @@ -399,6 +399,10 @@ class Iptables(object): ''' return (isinstance(key, tuple)) + def load(self, lpbk_table): + for row in lpbk_table: + self.iptables_handler(row, lpbk_table[row]) + def command(self, chain, ip, ver, op): cmd = 'iptables' if ver == '4' else 'ip6tables' cmd += ' -t mangle --{} {} -p tcp --tcp-flags SYN SYN'.format(op, chain) @@ -905,6 +909,15 @@ class NtpCfg(object): self.ntp_global = {} self.ntp_servers = set() + def load(self, ntp_global_conf, ntp_server_conf): + syslog.syslog(syslog.LOG_INFO, "NtpCfg load ...") + + for row in ntp_global_conf: + self.ntp_global_update(row, ntp_global_conf[row], True) + + # Force reload on init + self.ntp_server_update(0, None, load=True) + def handle_ntp_source_intf_chg(self, intf_name): # if no ntp server configured, do nothing if not self.ntp_servers: @@ -947,16 +960,19 @@ class NtpCfg(object): cmd = 'service ntp restart' run_cmd(cmd) - def ntp_server_update(self, key, op): + def ntp_server_update(self, key, op, load=False): syslog.syslog(syslog.LOG_INFO, 'ntp server update key {}'.format(key)) restart_config = False - if op == "SET" and key not in self.ntp_servers: - restart_config = True - self.ntp_servers.add(key) - elif op == "DEL" and key in self.ntp_servers: + if not load: + if op == "SET" and key not in self.ntp_servers: + restart_config = True + self.ntp_servers.add(key) + elif op == "DEL" and key in self.ntp_servers: + restart_config = True + self.ntp_servers.remove(key) + else: restart_config = True - self.ntp_servers.remove(key) if restart_config: cmd = 'systemctl restart ntp-config' @@ -1001,23 +1017,22 @@ class HostConfigDaemon: def load(self): features = self.config_db.get_table('FEATURE') - self.feature_handler.update_all_features_config(features) - aaa = self.config_db.get_table('AAA') tacacs_global = self.config_db.get_table('TACPLUS') tacacs_server = self.config_db.get_table('TACPLUS_SERVER') radius_global = self.config_db.get_table('RADIUS') radius_server = self.config_db.get_table('RADIUS_SERVER') - self.cache = {'AAA': aaa, 'TACPLUS': tacacs_global, 'TACPLUS_SERVER': tacacs_server, - 'RADIUS': radius_global, 'RADIUS_SERVER': radius_server} - self.aaacfg.load(aaa, tacacs_global, tacacs_server, radius_global, radius_server) - lpbk_table = self.config_db.get_table('LOOPBACK_INTERFACE') - self.iptables.load(lpbk_table) - - # Load NTP configurations ntp_server = self.config_db.get_table('NTP_SERVER') ntp_global = self.config_db.get_table('NTP') + + self.cache = {'FEATURES': features, 'AAA': aaa, 'TACPLUS': tacacs_global, 'TACPLUS_SERVER': tacacs_server, + 'RADIUS': radius_global, 'RADIUS_SERVER': radius_server, 'LOOPBACK_INTERFACE': lpbk_table, 'NTP_SERVER': ntp_server, + 'NTP': ntp_global} + + self.feature_handler.update_all_features_config(features) + self.aaacfg.load(aaa, tacacs_global, tacacs_server, radius_global, radius_server) + self.iptables.load(lpbk_table) self.ntpcfg.load(ntp_global, ntp_server) try: From 18a294d8754af73c7fa85854a9c1048cd2552998 Mon Sep 17 00:00:00 2001 From: Alexander Allen Date: Mon, 7 Mar 2022 19:13:35 +0000 Subject: [PATCH 04/10] Fix ntp_global_update signature. Remove unused import. --- src/sonic-host-services/scripts/hostcfgd | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index 26fced83d14e..230b035ca379 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -11,7 +11,7 @@ import signal import jinja2 from sonic_py_common import device_info -from swsscommon.swsscommon import ConfigDBConnector, TableConsumable +from swsscommon.swsscommon import ConfigDBConnector # FILE PAM_AUTH_CONF = "/etc/pam.d/common-auth-sonic" @@ -913,7 +913,7 @@ class NtpCfg(object): syslog.syslog(syslog.LOG_INFO, "NtpCfg load ...") for row in ntp_global_conf: - self.ntp_global_update(row, ntp_global_conf[row], True) + self.ntp_global_update(row, ntp_global_conf[row], load=True) # Force reload on init self.ntp_server_update(0, None, load=True) @@ -931,7 +931,7 @@ class NtpCfg(object): cmd = 'systemctl restart ntp-config' run_cmd(cmd) - def ntp_global_update(self, key, data): + def ntp_global_update(self, key, data, load=False): syslog.syslog(syslog.LOG_INFO, 'NTP GLOBAL Update') orig_src = self.ntp_global.get('src_intf', '') orig_src_set = set(orig_src.split(";")) @@ -944,6 +944,9 @@ class NtpCfg(object): # Update the Local Cache self.ntp_global = data + # If initial load don't restart daemon + if load: return + # check if ntp server configured, if not, do nothing if not self.ntp_servers: syslog.syslog(syslog.LOG_INFO, "No ntp server when global config change, do nothing") From 3b5e76d9751fc724397fe0054e4120120ce16127 Mon Sep 17 00:00:00 2001 From: Alexander Allen Date: Wed, 16 Mar 2022 02:15:43 +0000 Subject: [PATCH 05/10] Update hostcfgd with review feedback --- src/sonic-host-services/scripts/hostcfgd | 35 +++++++------------ .../tests/common/mock_configdb.py | 5 +-- 2 files changed, 13 insertions(+), 27 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index 230b035ca379..99af3e829254 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -1014,24 +1014,16 @@ class HostConfigDaemon: self.hostname_cache="" self.aaacfg = AaaCfg() - # Initialize cache - self.cache = {} - - - def load(self): - features = self.config_db.get_table('FEATURE') - aaa = self.config_db.get_table('AAA') - tacacs_global = self.config_db.get_table('TACPLUS') - tacacs_server = self.config_db.get_table('TACPLUS_SERVER') - radius_global = self.config_db.get_table('RADIUS') - radius_server = self.config_db.get_table('RADIUS_SERVER') - lpbk_table = self.config_db.get_table('LOOPBACK_INTERFACE') - ntp_server = self.config_db.get_table('NTP_SERVER') - ntp_global = self.config_db.get_table('NTP') - - self.cache = {'FEATURES': features, 'AAA': aaa, 'TACPLUS': tacacs_global, 'TACPLUS_SERVER': tacacs_server, - 'RADIUS': radius_global, 'RADIUS_SERVER': radius_server, 'LOOPBACK_INTERFACE': lpbk_table, 'NTP_SERVER': ntp_server, - 'NTP': ntp_global} + def load(self, init_data): + features = init_data['FEATURE'] + aaa = init_data['AAA'] + tacacs_global = init_data['TACPLUS'] + tacacs_server = init_data['TACPLUS_SERVER'] + radius_global = init_data['RADIUS'] + radius_server = init_data['RADIUS_SERVER'] + lpbk_table = init_data['LOOPBACK_INTERFACE'] + ntp_server = init_data['NTP_SERVER'] + ntp_global = init_data['NTP'] self.feature_handler.update_all_features_config(features) self.aaacfg.load(aaa, tacacs_global, tacacs_server, radius_global, radius_server) @@ -1039,7 +1031,7 @@ class HostConfigDaemon: self.ntpcfg.load(ntp_global, ntp_server) try: - dev_meta = self.config_db.get_table('DEVICE_METADATA') + dev_meta = init_data['DEVICE_METADATA'] if 'localhost' in dev_meta: if 'hostname' in dev_meta['localhost']: self.hostname_cache = dev_meta['localhost']['hostname'] @@ -1176,10 +1168,8 @@ class HostConfigDaemon: syslog.syslog(syslog.LOG_INFO, "systemctl has finished initialization -- proceeding ...") - self.config_db.listen(start=False) - def start(self): - self.config_db.process(cache=self.cache) + self.config_db.listen(init=self.load) def main(): @@ -1188,7 +1178,6 @@ def main(): signal.signal(signal.SIGHUP, signal_handler) daemon = HostConfigDaemon() daemon.register_callbacks() - daemon.load() daemon.start() if __name__ == "__main__": diff --git a/src/sonic-host-services/tests/common/mock_configdb.py b/src/sonic-host-services/tests/common/mock_configdb.py index 80a0912f580b..367e442efad1 100644 --- a/src/sonic-host-services/tests/common/mock_configdb.py +++ b/src/sonic-host-services/tests/common/mock_configdb.py @@ -48,10 +48,7 @@ def get_table(self, table_name): def subscribe(self, table_name, callback): self.handlers[table_name] = callback - def listen(self, start): - pass - - def process(self, cache): + def listen(self, init=None): for e in MockConfigDb.event_queue: self.handlers[e[0]](e[0], e[1], self.get_entry(e[0], e[1])) From 17dccfddd327fa70721325f35e29a177d4277446 Mon Sep 17 00:00:00 2001 From: Alexander Allen Date: Thu, 24 Mar 2022 04:36:52 +0000 Subject: [PATCH 06/10] Respond to review feedback --- src/sonic-host-services/scripts/hostcfgd | 43 ++++++------------- .../tests/hostcfgd/hostcfgd_test.py | 3 +- 2 files changed, 14 insertions(+), 32 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index 99af3e829254..62bdb5d94325 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -194,27 +194,12 @@ class FeatureHandler(object): else: self.resync_feature_state(self._cached_config[feature_name]) - def update_all_features_config(self, feature_table): - for feature_name in feature_table.keys(): - if not feature_name: - syslog.syslog(syslog.LOG_WARNING, "Feature is None") - continue - - feature = Feature(feature_name, feature_table[feature_name], self._device_config) - self._cached_config.setdefault(feature_name, feature) - - self.update_feature_auto_restart(feature, feature_name) - - self.update_feature_state(feature) - self.resync_feature_state(feature) - - def sync_state_field(self): + def sync_state_field(self, feature_table): """ Summary: Updates the state field in the FEATURE|* tables as the state field might have to be rendered based on DEVICE_METADATA table """ - feature_table = self._config_db.get_table('FEATURE') for feature_name in feature_table.keys(): if not feature_name: syslog.syslog(syslog.LOG_WARNING, "Feature is None") @@ -887,15 +872,13 @@ class KdumpCfg(object): memory = self.kdump_defaults["memory"] if data.get("memory") is not None: memory = data.get("memory") - if data.get("memory") is not None: - run_cmd("sonic-kdump-config --memory " + memory) + run_cmd("sonic-kdump-config --memory " + memory) # Num dumps num_dumps = self.kdump_defaults["num_dumps"] if data.get("num_dumps") is not None: num_dumps = data.get("num_dumps") - if data.get("num_dumps") is not None: - run_cmd("sonic-kdump-config --num_dumps " + num_dumps) + run_cmd("sonic-kdump-config --num_dumps " + num_dumps) class NtpCfg(object): """ @@ -913,10 +896,10 @@ class NtpCfg(object): syslog.syslog(syslog.LOG_INFO, "NtpCfg load ...") for row in ntp_global_conf: - self.ntp_global_update(row, ntp_global_conf[row], load=True) + self.ntp_global_update(row, ntp_global_conf[row], is_load=True) # Force reload on init - self.ntp_server_update(0, None, load=True) + self.ntp_server_update(0, None, is_load=True) def handle_ntp_source_intf_chg(self, intf_name): # if no ntp server configured, do nothing @@ -931,7 +914,7 @@ class NtpCfg(object): cmd = 'systemctl restart ntp-config' run_cmd(cmd) - def ntp_global_update(self, key, data, load=False): + def ntp_global_update(self, key, data, is_load=False): syslog.syslog(syslog.LOG_INFO, 'NTP GLOBAL Update') orig_src = self.ntp_global.get('src_intf', '') orig_src_set = set(orig_src.split(";")) @@ -945,7 +928,7 @@ class NtpCfg(object): self.ntp_global = data # If initial load don't restart daemon - if load: return + if is_load: return # check if ntp server configured, if not, do nothing if not self.ntp_servers: @@ -963,11 +946,11 @@ class NtpCfg(object): cmd = 'service ntp restart' run_cmd(cmd) - def ntp_server_update(self, key, op, load=False): + def ntp_server_update(self, key, op, is_load=False): syslog.syslog(syslog.LOG_INFO, 'ntp server update key {}'.format(key)) restart_config = False - if not load: + if not is_load: if op == "SET" and key not in self.ntp_servers: restart_config = True self.ntp_servers.add(key) @@ -996,14 +979,12 @@ class HostConfigDaemon: # Initialize KDump Config and set the config to default if nothing is provided self.kdumpCfg = KdumpCfg(self.config_db) - self.kdumpCfg.load(self.config_db.get_table('KDUMP')) # Initialize IpTables self.iptables = Iptables() # Intialize Feature Handler self.feature_handler = FeatureHandler(self.config_db, self.device_config) - self.feature_handler.sync_state_field() # Initialize Ntp Config Handler self.ntpcfg = NtpCfg() @@ -1024,14 +1005,16 @@ class HostConfigDaemon: lpbk_table = init_data['LOOPBACK_INTERFACE'] ntp_server = init_data['NTP_SERVER'] ntp_global = init_data['NTP'] + kdump = init_data['KDUMP'] - self.feature_handler.update_all_features_config(features) + self.feature_handler.sync_state_field(features) self.aaacfg.load(aaa, tacacs_global, tacacs_server, radius_global, radius_server) self.iptables.load(lpbk_table) self.ntpcfg.load(ntp_global, ntp_server) + self.kdumpCfg.load(kdump) try: - dev_meta = init_data['DEVICE_METADATA'] + dev_meta = self.config_db.get_table('DEVICE_METADATA') if 'localhost' in dev_meta: if 'hostname' in dev_meta['localhost']: self.hostname_cache = dev_meta['localhost']['hostname'] diff --git a/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py b/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py index 2538ddc2d81e..30ff5a5e5c05 100644 --- a/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py +++ b/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py @@ -101,8 +101,8 @@ def test_hostcfgd_feature_handler(self, test_name, test_data, fs): feature_handler = hostcfgd.FeatureHandler(MockConfigDb(), device_config) # sync the state field and Handle Feature Updates - feature_handler.sync_state_field() features = MockConfigDb.CONFIG_DB['FEATURE'] + feature_handler.sync_state_field(features) for key, fvs in features.items(): feature_handler.handle(key, 'SET', fvs) @@ -274,7 +274,6 @@ def test_kdump_event(self): MockConfigDb.set_config_db(HOSTCFG_DAEMON_CFG_DB) daemon = hostcfgd.HostConfigDaemon() daemon.register_callbacks() - assert MockConfigDb.CONFIG_DB['KDUMP']['config'] MockConfigDb.event_queue = [('KDUMP', 'config')] with mock.patch('hostcfgd.subprocess') as mocked_subprocess: popen_mock = mock.Mock() From 10308aca9a83ce60082694247f339e899c43486c Mon Sep 17 00:00:00 2001 From: Alexander Allen Date: Thu, 24 Mar 2022 20:11:02 +0000 Subject: [PATCH 07/10] Fix python shebang and try-except --- src/sonic-host-services/scripts/hostcfgd | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index 83f8a9e3833d..e235dc9f42aa 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -1,4 +1,4 @@ -#!/usr/bin/python3 +#!/usr/bin/env python3 import ast import copy @@ -1033,13 +1033,10 @@ class HostConfigDaemon: self.ntpcfg.load(ntp_global, ntp_server) self.kdumpCfg.load(kdump) - try: - dev_meta = self.config_db.get_table('DEVICE_METADATA') - if 'localhost' in dev_meta: - if 'hostname' in dev_meta['localhost']: - self.hostname_cache = dev_meta['localhost']['hostname'] - except Exception as e: - pass + dev_meta = self.config_db.get_table('DEVICE_METADATA') + if 'localhost' in dev_meta: + if 'hostname' in dev_meta['localhost']: + self.hostname_cache = dev_meta['localhost']['hostname'] # Update AAA with the hostname self.aaacfg.hostname_update(self.hostname_cache) From 8be6fda20e4bad5f0f780312ac5a9d558dd287e6 Mon Sep 17 00:00:00 2001 From: Alexander Allen Date: Fri, 25 Mar 2022 16:51:21 +0000 Subject: [PATCH 08/10] Add MockDbConnector --- src/sonic-host-services/tests/common/mock_configdb.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/sonic-host-services/tests/common/mock_configdb.py b/src/sonic-host-services/tests/common/mock_configdb.py index 367e442efad1..f6374d1bd37d 100644 --- a/src/sonic-host-services/tests/common/mock_configdb.py +++ b/src/sonic-host-services/tests/common/mock_configdb.py @@ -52,3 +52,7 @@ def listen(self, init=None): for e in MockConfigDb.event_queue: self.handlers[e[0]](e[0], e[1], self.get_entry(e[0], e[1])) + +class MockDBConnector(): + def __init__(self, db, val): + pass From a850870151d2eabf0e2c46339d95d6d602c05921 Mon Sep 17 00:00:00 2001 From: Alexander Allen Date: Mon, 28 Mar 2022 21:09:19 +0000 Subject: [PATCH 09/10] Update argument and fix sync_state_field method --- src/sonic-host-services/scripts/hostcfgd | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index e235dc9f42aa..04767ea8600e 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -214,8 +214,11 @@ class FeatureHandler(object): continue feature = Feature(feature_name, feature_table[feature_name], self._device_config) - if not feature.compare_state(feature_name, feature_table.get(feature_name, {})): - self.resync_feature_state(feature) + + self._cached_config.setdefault(feature_name, feature) + self.update_feature_auto_restart(feature, feature_name) + self.update_feature_state(feature) + self.resync_feature_state(feature) def update_feature_state(self, feature): cached_feature = self._cached_config[feature.name] @@ -1169,7 +1172,7 @@ class HostConfigDaemon: "systemctl has finished initialization -- proceeding ...") def start(self): - self.config_db.listen(init=self.load) + self.config_db.listen(init_data_handler=self.load) def main(): From 57189d6a0b72da09aef7815894ad03bc8634e6b4 Mon Sep 17 00:00:00 2001 From: Alexander Allen Date: Mon, 28 Mar 2022 23:23:23 +0000 Subject: [PATCH 10/10] update tests with argument change --- src/sonic-host-services/tests/common/mock_configdb.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sonic-host-services/tests/common/mock_configdb.py b/src/sonic-host-services/tests/common/mock_configdb.py index f6374d1bd37d..f0b12b11abf9 100644 --- a/src/sonic-host-services/tests/common/mock_configdb.py +++ b/src/sonic-host-services/tests/common/mock_configdb.py @@ -48,7 +48,7 @@ def get_table(self, table_name): def subscribe(self, table_name, callback): self.handlers[table_name] = callback - def listen(self, init=None): + def listen(self, init_data_handler=None): for e in MockConfigDb.event_queue: self.handlers[e[0]](e[0], e[1], self.get_entry(e[0], e[1]))