Skip to content

Commit

Permalink
[config reload]Config Reload Enhancement (#2693) (#2863)
Browse files Browse the repository at this point in the history 
Backporting #2693

What I did
Code changes for HLD: sonic-net/SONiC#1203
Removed the timer based checks for config reload
Added db_migrator to migrate from "has_timer" to "delayed"
Modified package-manager to migrate from "has_timer" to "delayed"

How I did it
Modified relevant files

How to verify it
Added UT to verify
  • Loading branch information
@dgsudharsan
dgsudharsan authored Jun 10, 2023
1 parent d69aae4 commit 1246bc8
Show file tree
Hide file tree
Showing 10 changed files with 70 additions and 157 deletions.
27 changes: 1 addition & 26 deletions config/main.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -869,23 +869,8 @@ def _get_sonic_services():
return (unit.strip() for unit in out.splitlines())


def _get_delayed_sonic_units(get_timers=False):
rc1, _ = clicommon.run_command("systemctl list-dependencies --plain sonic-delayed.target | sed '1d'", return_cmd=True)
rc2, _ = clicommon.run_command("systemctl is-enabled {}".format(rc1.replace("\n", " ")), return_cmd=True)
timer = [line.strip() for line in rc1.splitlines()]
state = [line.strip() for line in rc2.splitlines()]
services = []
for unit in timer:
if state[timer.index(unit)] == "enabled":
if not get_timers:
services.append(re.sub('\.timer$', '', unit, 1))
else:
services.append(unit)
return services


def _reset_failed_services():
for service in itertools.chain(_get_sonic_services(), _get_delayed_sonic_units()):
for service in _get_sonic_services():
clicommon.run_command("systemctl reset-failed {}".format(service))


Expand All @@ -904,12 +889,6 @@ def _restart_services():
click.echo("Reloading Monit configuration ...")
clicommon.run_command("sudo monit reload")

def _delay_timers_elapsed():
for timer in _get_delayed_sonic_units(get_timers=True):
out, _ = clicommon.run_command("systemctl show {} --property=LastTriggerUSecMonotonic --value".format(timer), return_cmd=True)
if out.strip() == "0":
return False
return True

def _per_namespace_swss_ready(service_name):
out, _ = clicommon.run_command("systemctl show {} --property ActiveState --value".format(service_name), return_cmd=True)
Expand Down Expand Up @@ -1526,10 +1505,6 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form
click.echo("System is not up. Retry later or use -f to avoid system checks")
sys.exit(CONFIG_RELOAD_NOT_READY)

if not _delay_timers_elapsed():
click.echo("Relevant services are not up. Retry later or use -f to avoid system checks")
sys.exit(CONFIG_RELOAD_NOT_READY)

if not _swss_ready():
click.echo("SwSS container is not ready. Retry later or use -f to avoid system checks")
sys.exit(CONFIG_RELOAD_NOT_READY)
Expand Down
24 changes: 22 additions & 2 deletions scripts/db_migrator.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def __init__(self, namespace, socket=None):
none-zero values.
build: sequentially increase within a minor version domain.
"""
self.CURRENT_VERSION = 'version_4_0_1'
self.CURRENT_VERSION = 'version_4_0_2'

self.TABLE_NAME = 'VERSIONS'
self.TABLE_KEY = 'DATABASE'
Expand Down Expand Up @@ -575,6 +575,18 @@ def migrate_port_qos_map_global(self):
self.configDB.set_entry('PORT_QOS_MAP', 'global', {"dscp_to_tc_map": dscp_to_tc_map_table_names[0]})
log.log_info("Created entry for global DSCP_TO_TC_MAP {}".format(dscp_to_tc_map_table_names[0]))

def migrate_feature_timer(self):
'''
Migrate feature 'has_timer' field to 'delayed'
'''
feature_table = self.configDB.get_table('FEATURE')
for feature, config in feature_table.items():
state = config.get('has_timer')
if state is not None:
config['delayed'] = state
config.pop('has_timer')
self.configDB.set_entry('FEATURE', feature, config)

def update_edgezone_aggregator_config(self):
"""
Update cable length configuration in ConfigDB for T0 neighbor interfaces
Expand Down Expand Up @@ -909,9 +921,17 @@ def version_4_0_0(self):
def version_4_0_1(self):
"""
Version 4_0_1.
"""
self.migrate_feature_timer()
self.set_version('version_4_0_2')
return 'version_4_0_2'

def version_4_0_2(self):
"""
Version 4_0_2.
This is the latest version for master branch
"""
log.log_info('Handling version_4_0_1')
log.log_info('Handling version_4_0_2')
return None

def get_version(self):
Expand Down
6 changes: 3 additions & 3 deletions sonic_package_manager/service_creator/feature.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ def update(self,
old_manifest: Manifest,
new_manifest: Manifest):
""" Migrate feature configuration. It can be that non-configurable
feature entries have to be updated. e.g: "has_timer" for example if
feature entries have to be updated. e.g: "delayed" for example if
the new feature introduces a service timer or name of the service has
changed, but user configurable entries are not changed).
Expand Down Expand Up @@ -227,12 +227,12 @@ def get_default_feature_entries(state=None, owner=None) -> Dict[str, str]:

@staticmethod
def get_non_configurable_feature_entries(manifest) -> Dict[str, str]:
""" Get non-configurable feature table entries: e.g. 'has_timer' """
""" Get non-configurable feature table entries: e.g. 'delayed' """

return {
'has_per_asic_scope': str(manifest['service']['asic-service']),
'has_global_scope': str(manifest['service']['host-service']),
'has_timer': str(manifest['service']['delayed']),
'delayed': str(manifest['service']['delayed']),
'check_up_status': str(manifest['service']['check_up_status']),
'support_syslog_rate_limit': str(manifest['service']['syslog']['support-rate-limit']),
}
85 changes: 1 addition & 84 deletions tests/config_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,6 @@
Reloading Monit configuration ...
"""

reload_config_with_untriggered_timer_output="""\
Relevant services are not up. Retry later or use -f to avoid system checks
"""

def mock_run_command_side_effect(*args, **kwargs):
command = args[0]

Expand Down Expand Up @@ -154,41 +150,6 @@ def mock_run_command_side_effect_disabled_timer(*args, **kwargs):
else:
return '', 0

def mock_run_command_side_effect_untriggered_timer(*args, **kwargs):
command = args[0]

if kwargs.get('display_cmd'):
click.echo(click.style("Running command: ", fg='cyan') + click.style(command, fg='green'))

if kwargs.get('return_cmd'):
if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'":
return 'snmp.timer', 0
elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'":
return 'swss', 0
elif command == "systemctl is-enabled snmp.timer":
return 'enabled', 0
elif command == "systemctl show snmp.timer --property=LastTriggerUSecMonotonic --value":
return '0', 0
else:
return '', 0

def mock_run_command_side_effect_gnmi(*args, **kwargs):
command = args[0]

if kwargs.get('display_cmd'):
click.echo(click.style("Running command: ", fg='cyan') + click.style(command, fg='green'))

if kwargs.get('return_cmd'):
if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'":
return 'gnmi.timer', 0
elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'":
return 'swss', 0
elif command == "systemctl is-enabled gnmi.timer":
return 'enabled', 0
else:
return '', 0


# Load sonic-cfggen from source since /usr/local/bin/sonic-cfggen does not have .py extension.
sonic_cfggen = load_module_from_source('sonic_cfggen', '/usr/local/bin/sonic-cfggen')

Expand Down Expand Up @@ -234,32 +195,6 @@ def test_config_reload(self, get_cmd_module, setup_single_broadcom_asic):

assert "\n".join([l.rstrip() for l in result.output.split('\n')][:1]) == reload_config_with_sys_info_command_output

def test_config_reload_untriggered_timer(self, get_cmd_module, setup_single_broadcom_asic):
with mock.patch("utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_side_effect_untriggered_timer)) as mock_run_command:
(config, show) = get_cmd_module

jsonfile_config = os.path.join(mock_db_path, "config_db.json")
jsonfile_init_cfg = os.path.join(mock_db_path, "init_cfg.json")

# create object
config.INIT_CFG_FILE = jsonfile_init_cfg
config.DEFAULT_CONFIG_DB_FILE = jsonfile_config

db = Db()
runner = CliRunner()
obj = {'config_db': db.cfgdb}

# simulate 'config reload' to provoke load_sys_info option
result = runner.invoke(config.config.commands["reload"], ["-l", "-y"], obj=obj)

print(result.exit_code)
print(result.output)
traceback.print_tb(result.exc_info[2])

assert result.exit_code == 1

assert "\n".join([l.rstrip() for l in result.output.split('\n')][:2]) == reload_config_with_untriggered_timer_output

@classmethod
def teardown_class(cls):
print("TEARDOWN")
Expand Down Expand Up @@ -292,25 +227,7 @@ def test_load_minigraph(self, get_cmd_module, setup_single_broadcom_asic):
assert "\n".join([l.rstrip() for l in result.output.split('\n')]) == load_minigraph_command_output
# Verify "systemctl reset-failed" is called for services under sonic.target
mock_run_command.assert_any_call('systemctl reset-failed swss')
# Verify "systemctl reset-failed" is called for services under sonic-delayed.target
mock_run_command.assert_any_call('systemctl reset-failed snmp')
assert mock_run_command.call_count == 11

def test_load_minigraph_with_gnmi_timer(self, get_cmd_module, setup_single_broadcom_asic):
with mock.patch("utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_side_effect_gnmi)) as mock_run_command:
(config, show) = get_cmd_module
runner = CliRunner()
result = runner.invoke(config.config.commands["load_minigraph"], ["-y"])
print(result.exit_code)
print(result.output)
traceback.print_tb(result.exc_info[2])
assert result.exit_code == 0
assert "\n".join([l.rstrip() for l in result.output.split('\n')]) == load_minigraph_command_output
# Verify "systemctl reset-failed" is called for services under sonic.target
mock_run_command.assert_any_call('systemctl reset-failed swss')
# Verify "systemctl reset-failed" is called for services under sonic-delayed.target
mock_run_command.assert_any_call('systemctl reset-failed gnmi')
assert mock_run_command.call_count == 11
assert mock_run_command.call_count == 8

def test_load_minigraph_with_port_config_bad_format(self, get_cmd_module, setup_single_broadcom_asic):
with mock.patch(
Expand Down
30 changes: 15 additions & 15 deletions tests/counterpoll_input/config_db.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2235,31 +2235,31 @@
"auto_restart": "enabled",
"state": "enabled",
"has_global_scope": "True",
"has_timer": "False"
"delayed": "False"
},
"pmon": {
"has_per_asic_scope": "False",
"high_mem_alert": "disabled",
"auto_restart": "enabled",
"state": "enabled",
"has_global_scope": "True",
"has_timer": "False"
"delayed": "False"
},
"sflow": {
"has_per_asic_scope": "False",
"high_mem_alert": "disabled",
"auto_restart": "enabled",
"state": "disabled",
"has_global_scope": "True",
"has_timer": "False"
"delayed": "False"
},
"database": {
"has_per_asic_scope": "True",
"high_mem_alert": "disabled",
"auto_restart": "disabled",
"state": "enabled",
"has_global_scope": "True",
"has_timer": "False"
"delayed": "False"
},
"telemetry": {
"has_per_asic_scope": "False",
Expand All @@ -2268,79 +2268,79 @@
"auto_restart": "enabled",
"state": "enabled",
"has_global_scope": "True",
"has_timer": "True"
"delayed": "True"
},
"snmp": {
"has_per_asic_scope": "False",
"high_mem_alert": "disabled",
"auto_restart": "enabled",
"state": "enabled",
"has_global_scope": "True",
"has_timer": "True"
"delayed": "True"
},
"bgp": {
"has_per_asic_scope": "True",
"high_mem_alert": "disabled",
"auto_restart": "enabled",
"state": "enabled",
"has_global_scope": "False",
"has_timer": "False"
"delayed": "False"
},
"radv": {
"has_per_asic_scope": "False",
"high_mem_alert": "disabled",
"auto_restart": "enabled",
"state": "enabled",
"has_global_scope": "True",
"has_timer": "False"
"delayed": "False"
},
"mgmt-framework": {
"has_per_asic_scope": "False",
"high_mem_alert": "disabled",
"auto_restart": "enabled",
"state": "enabled",
"has_global_scope": "True",
"has_timer": "True"
"delayed": "True"
},
"nat": {
"has_per_asic_scope": "False",
"high_mem_alert": "disabled",
"auto_restart": "enabled",
"state": "disabled",
"has_global_scope": "True",
"has_timer": "False"
"delayed": "False"
},
"teamd": {
"has_per_asic_scope": "True",
"high_mem_alert": "disabled",
"auto_restart": "enabled",
"state": "enabled",
"has_global_scope": "False",
"has_timer": "False"
"delayed": "False"
},
"dhcp_relay": {
"has_per_asic_scope": "False",
"high_mem_alert": "disabled",
"auto_restart": "enabled",
"state": "enabled",
"has_global_scope": "True",
"has_timer": "False"
"delayed": "False"
},
"swss": {
"has_per_asic_scope": "True",
"high_mem_alert": "disabled",
"auto_restart": "enabled",
"state": "enabled",
"has_global_scope": "False",
"has_timer": "False"
"delayed": "False"
},
"syncd": {
"has_per_asic_scope": "True",
"high_mem_alert": "disabled",
"auto_restart": "enabled",
"state": "enabled",
"has_global_scope": "False",
"has_timer": "False"
"delayed": "False"
}
},
"DSCP_TO_TC_MAP": {
Expand Down Expand Up @@ -2669,4 +2669,4 @@
"size": "56368"
}
}
}
}
6 changes: 3 additions & 3 deletions tests/db_migrator_input/config_db/feature-expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,23 @@
"auto_restart": "disabled",
"has_global_scope": "False",
"has_per_asic_scope": "True",
"has_timer": "False",
"delayed": "False",
"high_mem_alert": "disabled",
"state": "enabled"
},
"FEATURE|syncd": {
"auto_restart": "enabled",
"has_global_scope": "False",
"has_per_asic_scope": "True",
"has_timer": "False",
"delayed": "False",
"high_mem_alert": "disabled",
"state": "enabled"
},
"FEATURE|telemetry": {
"auto_restart": "enabled",
"has_global_scope": "False",
"has_per_asic_scope": "True",
"has_timer": "False",
"delayed": "True",
"high_mem_alert": "disabled",
"state": "enabled"
}
Expand Down
Loading

0 comments on commit 1246bc8

Please sign in to comment.