Skip to content

Commit a1057b2

Browse files
dgsudharsandgsudharsan
authored
[config reload]Config Reload Enhancement (sonic-net#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
1 parent 04d0b34 commit a1057b2

File tree

10 files changed

+69
-157
lines changed

10 files changed

+69
-157
lines changed

‎config/main.py

Lines changed: 1 addition & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -870,23 +870,8 @@ def _get_sonic_services():
870870
return (unit.strip() for unit in out.splitlines())
871871

872872

873-
def _get_delayed_sonic_units(get_timers=False):
874-
rc1, _ = clicommon.run_command("systemctl list-dependencies --plain sonic-delayed.target | sed '1d'", return_cmd=True)
875-
rc2, _ = clicommon.run_command("systemctl is-enabled {}".format(rc1.replace("\n", " ")), return_cmd=True)
876-
timer = [line.strip() for line in rc1.splitlines()]
877-
state = [line.strip() for line in rc2.splitlines()]
878-
services = []
879-
for unit in timer:
880-
if state[timer.index(unit)] == "enabled":
881-
if not get_timers:
882-
services.append(re.sub('\.timer$', '', unit, 1))
883-
else:
884-
services.append(unit)
885-
return services
886-
887-
888873
def _reset_failed_services():
889-
for service in itertools.chain(_get_sonic_services(), _get_delayed_sonic_units()):
874+
for service in _get_sonic_services():
890875
clicommon.run_command("systemctl reset-failed {}".format(service))
891876

892877

@@ -905,12 +890,6 @@ def _restart_services():
905890
click.echo("Reloading Monit configuration ...")
906891
clicommon.run_command("sudo monit reload")
907892

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

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

1495-
if not _delay_timers_elapsed():
1496-
click.echo("Relevant services are not up. Retry later or use -f to avoid system checks")
1497-
sys.exit(CONFIG_RELOAD_NOT_READY)
1498-
14991474
if not _swss_ready():
15001475
click.echo("SwSS container is not ready. Retry later or use -f to avoid system checks")
15011476
sys.exit(CONFIG_RELOAD_NOT_READY)

‎scripts/db_migrator.py

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ def __init__(self, namespace, socket=None):
4545
none-zero values.
4646
build: sequentially increase within a minor version domain.
4747
"""
48-
self.CURRENT_VERSION = 'version_4_0_1'
48+
self.CURRENT_VERSION = 'version_4_0_2'
4949

5050
self.TABLE_NAME = 'VERSIONS'
5151
self.TABLE_KEY = 'DATABASE'
@@ -575,6 +575,17 @@ def migrate_port_qos_map_global(self):
575575
self.configDB.set_entry('PORT_QOS_MAP', 'global', {"dscp_to_tc_map": dscp_to_tc_map_table_names[0]})
576576
log.log_info("Created entry for global DSCP_TO_TC_MAP {}".format(dscp_to_tc_map_table_names[0]))
577577

578+
def migrate_feature_timer(self):
579+
'''
580+
Migrate feature 'has_timer' field to 'delayed'
581+
'''
582+
feature_table = self.configDB.get_table('FEATURE')
583+
for feature, config in feature_table.items():
584+
state = config.get('has_timer')
585+
if state is not None:
586+
config['delayed'] = state
587+
config.pop('has_timer')
588+
self.configDB.set_entry('FEATURE', feature, config)
578589
def migrate_route_table(self):
579590
"""
580591
Handle route table migration. Migrations handled:
@@ -926,9 +937,17 @@ def version_4_0_0(self):
926937
def version_4_0_1(self):
927938
"""
928939
Version 4_0_1.
940+
"""
941+
self.migrate_feature_timer()
942+
self.set_version('version_4_0_2')
943+
return 'version_4_0_2'
944+
945+
def version_4_0_2(self):
946+
"""
947+
Version 4_0_2.
929948
This is the latest version for master branch
930949
"""
931-
log.log_info('Handling version_4_0_1')
950+
log.log_info('Handling version_4_0_2')
932951
return None
933952

934953
def get_version(self):

‎sonic_package_manager/service_creator/feature.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ def update(self,
105105
old_manifest: Manifest,
106106
new_manifest: Manifest):
107107
""" Migrate feature configuration. It can be that non-configurable
108-
feature entries have to be updated. e.g: "has_timer" for example if
108+
feature entries have to be updated. e.g: "delayed" for example if
109109
the new feature introduces a service timer or name of the service has
110110
changed, but user configurable entries are not changed).
111111
@@ -227,12 +227,12 @@ def get_default_feature_entries(state=None, owner=None) -> Dict[str, str]:
227227

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

232232
return {
233233
'has_per_asic_scope': str(manifest['service']['asic-service']),
234234
'has_global_scope': str(manifest['service']['host-service']),
235-
'has_timer': str(manifest['service']['delayed']),
235+
'delayed': str(manifest['service']['delayed']),
236236
'check_up_status': str(manifest['service']['check_up_status']),
237237
'support_syslog_rate_limit': str(manifest['service']['syslog']['support-rate-limit']),
238238
}

‎tests/config_test.py

Lines changed: 1 addition & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,6 @@
115115
Reloading Monit configuration ...
116116
"""
117117

118-
reload_config_with_untriggered_timer_output="""\
119-
Relevant services are not up. Retry later or use -f to avoid system checks
120-
"""
121-
122118
def mock_run_command_side_effect(*args, **kwargs):
123119
command = args[0]
124120

@@ -155,41 +151,6 @@ def mock_run_command_side_effect_disabled_timer(*args, **kwargs):
155151
else:
156152
return '', 0
157153

158-
def mock_run_command_side_effect_untriggered_timer(*args, **kwargs):
159-
command = args[0]
160-
161-
if kwargs.get('display_cmd'):
162-
click.echo(click.style("Running command: ", fg='cyan') + click.style(command, fg='green'))
163-
164-
if kwargs.get('return_cmd'):
165-
if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'":
166-
return 'snmp.timer', 0
167-
elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'":
168-
return 'swss', 0
169-
elif command == "systemctl is-enabled snmp.timer":
170-
return 'enabled', 0
171-
elif command == "systemctl show snmp.timer --property=LastTriggerUSecMonotonic --value":
172-
return '0', 0
173-
else:
174-
return '', 0
175-
176-
def mock_run_command_side_effect_gnmi(*args, **kwargs):
177-
command = args[0]
178-
179-
if kwargs.get('display_cmd'):
180-
click.echo(click.style("Running command: ", fg='cyan') + click.style(command, fg='green'))
181-
182-
if kwargs.get('return_cmd'):
183-
if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'":
184-
return 'gnmi.timer', 0
185-
elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'":
186-
return 'swss', 0
187-
elif command == "systemctl is-enabled gnmi.timer":
188-
return 'enabled', 0
189-
else:
190-
return '', 0
191-
192-
193154
# Load sonic-cfggen from source since /usr/local/bin/sonic-cfggen does not have .py extension.
194155
sonic_cfggen = load_module_from_source('sonic_cfggen', '/usr/local/bin/sonic-cfggen')
195156

@@ -235,32 +196,6 @@ def test_config_reload(self, get_cmd_module, setup_single_broadcom_asic):
235196

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

238-
def test_config_reload_untriggered_timer(self, get_cmd_module, setup_single_broadcom_asic):
239-
with mock.patch("utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_side_effect_untriggered_timer)) as mock_run_command:
240-
(config, show) = get_cmd_module
241-
242-
jsonfile_config = os.path.join(mock_db_path, "config_db.json")
243-
jsonfile_init_cfg = os.path.join(mock_db_path, "init_cfg.json")
244-
245-
# create object
246-
config.INIT_CFG_FILE = jsonfile_init_cfg
247-
config.DEFAULT_CONFIG_DB_FILE = jsonfile_config
248-
249-
db = Db()
250-
runner = CliRunner()
251-
obj = {'config_db': db.cfgdb}
252-
253-
# simulate 'config reload' to provoke load_sys_info option
254-
result = runner.invoke(config.config.commands["reload"], ["-l", "-y"], obj=obj)
255-
256-
print(result.exit_code)
257-
print(result.output)
258-
traceback.print_tb(result.exc_info[2])
259-
260-
assert result.exit_code == 1
261-
262-
assert "\n".join([l.rstrip() for l in result.output.split('\n')][:2]) == reload_config_with_untriggered_timer_output
263-
264199
@classmethod
265200
def teardown_class(cls):
266201
print("TEARDOWN")
@@ -293,25 +228,7 @@ def test_load_minigraph(self, get_cmd_module, setup_single_broadcom_asic):
293228
assert "\n".join([l.rstrip() for l in result.output.split('\n')]) == load_minigraph_command_output
294229
# Verify "systemctl reset-failed" is called for services under sonic.target
295230
mock_run_command.assert_any_call('systemctl reset-failed swss')
296-
# Verify "systemctl reset-failed" is called for services under sonic-delayed.target
297-
mock_run_command.assert_any_call('systemctl reset-failed snmp')
298-
assert mock_run_command.call_count == 11
299-
300-
def test_load_minigraph_with_gnmi_timer(self, get_cmd_module, setup_single_broadcom_asic):
301-
with mock.patch("utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_side_effect_gnmi)) as mock_run_command:
302-
(config, show) = get_cmd_module
303-
runner = CliRunner()
304-
result = runner.invoke(config.config.commands["load_minigraph"], ["-y"])
305-
print(result.exit_code)
306-
print(result.output)
307-
traceback.print_tb(result.exc_info[2])
308-
assert result.exit_code == 0
309-
assert "\n".join([l.rstrip() for l in result.output.split('\n')]) == load_minigraph_command_output
310-
# Verify "systemctl reset-failed" is called for services under sonic.target
311-
mock_run_command.assert_any_call('systemctl reset-failed swss')
312-
# Verify "systemctl reset-failed" is called for services under sonic-delayed.target
313-
mock_run_command.assert_any_call('systemctl reset-failed gnmi')
314-
assert mock_run_command.call_count == 11
231+
assert mock_run_command.call_count == 8
315232

316233
def test_load_minigraph_with_port_config_bad_format(self, get_cmd_module, setup_single_broadcom_asic):
317234
with mock.patch(

‎tests/counterpoll_input/config_db.json

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2235,31 +2235,31 @@
22352235
"auto_restart": "enabled",
22362236
"state": "enabled",
22372237
"has_global_scope": "True",
2238-
"has_timer": "False"
2238+
"delayed": "False"
22392239
},
22402240
"pmon": {
22412241
"has_per_asic_scope": "False",
22422242
"high_mem_alert": "disabled",
22432243
"auto_restart": "enabled",
22442244
"state": "enabled",
22452245
"has_global_scope": "True",
2246-
"has_timer": "False"
2246+
"delayed": "False"
22472247
},
22482248
"sflow": {
22492249
"has_per_asic_scope": "False",
22502250
"high_mem_alert": "disabled",
22512251
"auto_restart": "enabled",
22522252
"state": "disabled",
22532253
"has_global_scope": "True",
2254-
"has_timer": "False"
2254+
"delayed": "False"
22552255
},
22562256
"database": {
22572257
"has_per_asic_scope": "True",
22582258
"high_mem_alert": "disabled",
22592259
"auto_restart": "disabled",
22602260
"state": "enabled",
22612261
"has_global_scope": "True",
2262-
"has_timer": "False"
2262+
"delayed": "False"
22632263
},
22642264
"telemetry": {
22652265
"has_per_asic_scope": "False",
@@ -2268,79 +2268,79 @@
22682268
"auto_restart": "enabled",
22692269
"state": "enabled",
22702270
"has_global_scope": "True",
2271-
"has_timer": "True"
2271+
"delayed": "True"
22722272
},
22732273
"snmp": {
22742274
"has_per_asic_scope": "False",
22752275
"high_mem_alert": "disabled",
22762276
"auto_restart": "enabled",
22772277
"state": "enabled",
22782278
"has_global_scope": "True",
2279-
"has_timer": "True"
2279+
"delayed": "True"
22802280
},
22812281
"bgp": {
22822282
"has_per_asic_scope": "True",
22832283
"high_mem_alert": "disabled",
22842284
"auto_restart": "enabled",
22852285
"state": "enabled",
22862286
"has_global_scope": "False",
2287-
"has_timer": "False"
2287+
"delayed": "False"
22882288
},
22892289
"radv": {
22902290
"has_per_asic_scope": "False",
22912291
"high_mem_alert": "disabled",
22922292
"auto_restart": "enabled",
22932293
"state": "enabled",
22942294
"has_global_scope": "True",
2295-
"has_timer": "False"
2295+
"delayed": "False"
22962296
},
22972297
"mgmt-framework": {
22982298
"has_per_asic_scope": "False",
22992299
"high_mem_alert": "disabled",
23002300
"auto_restart": "enabled",
23012301
"state": "enabled",
23022302
"has_global_scope": "True",
2303-
"has_timer": "True"
2303+
"delayed": "True"
23042304
},
23052305
"nat": {
23062306
"has_per_asic_scope": "False",
23072307
"high_mem_alert": "disabled",
23082308
"auto_restart": "enabled",
23092309
"state": "disabled",
23102310
"has_global_scope": "True",
2311-
"has_timer": "False"
2311+
"delayed": "False"
23122312
},
23132313
"teamd": {
23142314
"has_per_asic_scope": "True",
23152315
"high_mem_alert": "disabled",
23162316
"auto_restart": "enabled",
23172317
"state": "enabled",
23182318
"has_global_scope": "False",
2319-
"has_timer": "False"
2319+
"delayed": "False"
23202320
},
23212321
"dhcp_relay": {
23222322
"has_per_asic_scope": "False",
23232323
"high_mem_alert": "disabled",
23242324
"auto_restart": "enabled",
23252325
"state": "enabled",
23262326
"has_global_scope": "True",
2327-
"has_timer": "False"
2327+
"delayed": "False"
23282328
},
23292329
"swss": {
23302330
"has_per_asic_scope": "True",
23312331
"high_mem_alert": "disabled",
23322332
"auto_restart": "enabled",
23332333
"state": "enabled",
23342334
"has_global_scope": "False",
2335-
"has_timer": "False"
2335+
"delayed": "False"
23362336
},
23372337
"syncd": {
23382338
"has_per_asic_scope": "True",
23392339
"high_mem_alert": "disabled",
23402340
"auto_restart": "enabled",
23412341
"state": "enabled",
23422342
"has_global_scope": "False",
2343-
"has_timer": "False"
2343+
"delayed": "False"
23442344
}
23452345
},
23462346
"DSCP_TO_TC_MAP": {
@@ -2669,4 +2669,4 @@
26692669
"size": "56368"
26702670
}
26712671
}
2672-
}
2672+
}

‎tests/db_migrator_input/config_db/feature-expected.json

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,23 +3,23 @@
33
"auto_restart": "disabled",
44
"has_global_scope": "False",
55
"has_per_asic_scope": "True",
6-
"has_timer": "False",
6+
"delayed": "False",
77
"high_mem_alert": "disabled",
88
"state": "enabled"
99
},
1010
"FEATURE|syncd": {
1111
"auto_restart": "enabled",
1212
"has_global_scope": "False",
1313
"has_per_asic_scope": "True",
14-
"has_timer": "False",
14+
"delayed": "False",
1515
"high_mem_alert": "disabled",
1616
"state": "enabled"
1717
},
1818
"FEATURE|telemetry": {
1919
"auto_restart": "enabled",
2020
"has_global_scope": "False",
2121
"has_per_asic_scope": "True",
22-
"has_timer": "False",
22+
"delayed": "True",
2323
"high_mem_alert": "disabled",
2424
"state": "enabled"
2525
}

0 commit comments

Comments
 (0)