Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disable systemd auto-restart of dependent services for spineRouters #83

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Disable systemd auto-restart of dependent services for spineRouters #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion scripts/featured
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,20 @@ class FeatureHandler(object):
Returns:
None.
"""
restart_field_str = "always" if "enabled" in feature_config.auto_restart else "no"
# As per the current code(due to various dependencies) SWSS service stop/start also stops/starts the dependent services(syncd, teamd, bgpd etc)
# There is an issue seen of syncd service getting stopped twice upon a critical process crash in syncd service due to above reason.
# Also early start of syncd service has traffic impact on VOQ chassis.
# to fix the above issue, we are disabling the auto restart of syncd service as it will be started by swss service.
# This change can be extended to other dependent services as well in future and also on pizza box platforms.

device_type = self._device_config.get('DEVICE_METADATA', {}).get('localhost', {}).get('type')
deepak-singhal0408 marked this conversation as resolved.
Show resolved Hide resolved
is_dependent_service = feature_config.name in ['syncd', 'gbsyncd']
if device_type == 'SpineRouter' and is_dependent_service:
deepak-singhal0408 marked this conversation as resolved.
Show resolved Hide resolved
syslog.syslog(syslog.LOG_INFO, "Skipped setting Restart field in systemd for {}".format(feature_config.name))
deepak-singhal0408 marked this conversation as resolved.
Show resolved Hide resolved
restart_field_str = "no"
else:
restart_field_str = "always" if "enabled" in feature_config.auto_restart else "no"

feature_systemd_config = "[Service]\nRestart={}\n".format(restart_field_str)
feature_names, feature_suffixes = self.get_multiasic_feature_instances(feature_config)

Expand Down
17 changes: 11 additions & 6 deletions tests/featured/featured_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def checks_config_table(self, feature_table, expected_table):

return True if not ddiff else False

def checks_systemd_config_file(self, feature_table, feature_systemd_name_map=None):
def checks_systemd_config_file(self, device_type, feature_table, feature_systemd_name_map=None):
"""Checks whether the systemd configuration file of each feature was created or not
and whether the `Restart=` field in the file is set correctly or not.

Expand All @@ -71,6 +71,7 @@ def checks_systemd_config_file(self, feature_table, feature_systemd_name_map=Non
'auto_restart.conf')

for feature_name in feature_table:
is_dependent_feature = True if feature_name in ['syncd', 'gbsyncd'] else False
auto_restart_status = feature_table[feature_name].get('auto_restart', 'disabled')
if "enabled" in auto_restart_status:
auto_restart_status = "enabled"
Expand All @@ -86,7 +87,10 @@ def checks_systemd_config_file(self, feature_table, feature_systemd_name_map=Non

with open(feature_systemd_config_file_path) as systemd_config_file:
status = systemd_config_file.read().strip()
assert status == '[Service]\nRestart={}'.format(truth_table[auto_restart_status])
if device_type == 'SpineRouter' and is_dependent_feature:
assert status == '[Service]\nRestart=no'
else:
assert status == '[Service]\nRestart={}'.format(truth_table[auto_restart_status])

def get_state_db_set_calls(self, feature_table):
"""Returns a Mock call objects which recorded the `set` calls to `FEATURE` table in `STATE_DB`.
Expand Down Expand Up @@ -143,6 +147,7 @@ def test_sync_state_field(self, test_scenario_name, config_data, fs):
device_config = {}
device_config['DEVICE_METADATA'] = MockConfigDb.CONFIG_DB['DEVICE_METADATA']
device_config.update(config_data['device_runtime_metadata'])
device_type = MockConfigDb.CONFIG_DB['DEVICE_METADATA']['localhost']['type']

feature_handler = featured.FeatureHandler(MockConfigDb(), feature_state_table_mock,
device_config, False)
Expand All @@ -169,13 +174,13 @@ def test_sync_state_field(self, test_scenario_name, config_data, fs):

feature_table_state_db_calls = self.get_state_db_set_calls(feature_table)

self.checks_systemd_config_file(config_data['config_db']['FEATURE'], feature_systemd_name_map)
self.checks_systemd_config_file(device_type, config_data['config_db']['FEATURE'], feature_systemd_name_map)
mocked_subprocess.check_call.assert_has_calls(config_data['enable_feature_subprocess_calls'],
any_order=True)
mocked_subprocess.check_call.assert_has_calls(config_data['daemon_reload_subprocess_call'],
any_order=True)
feature_state_table_mock.set.assert_has_calls(feature_table_state_db_calls)
self.checks_systemd_config_file(config_data['config_db']['FEATURE'], feature_systemd_name_map)
self.checks_systemd_config_file(device_type, config_data['config_db']['FEATURE'], feature_systemd_name_map)

@parameterized.expand(FEATURED_TEST_VECTOR)
@patchfs
Expand Down Expand Up @@ -207,6 +212,7 @@ def test_handler(self, test_scenario_name, config_data, fs):
device_config = {}
device_config['DEVICE_METADATA'] = MockConfigDb.CONFIG_DB['DEVICE_METADATA']
device_config.update(config_data['device_runtime_metadata'])
device_type = MockConfigDb.CONFIG_DB['DEVICE_METADATA']['localhost']['type']
feature_handler = featured.FeatureHandler(MockConfigDb(), feature_state_table_mock,
device_config, False)
feature_handler.is_delayed_enabled = True
Expand All @@ -220,7 +226,7 @@ def test_handler(self, test_scenario_name, config_data, fs):
feature_names, _ = feature_handler.get_multiasic_feature_instances(feature)
feature_systemd_name_map[feature_name] = feature_names

self.checks_systemd_config_file(config_data['config_db']['FEATURE'], feature_systemd_name_map)
self.checks_systemd_config_file(device_type, config_data['config_db']['FEATURE'], feature_systemd_name_map)
mocked_subprocess.check_call.assert_has_calls(config_data['enable_feature_subprocess_calls'],
any_order=True)
mocked_subprocess.check_call.assert_has_calls(config_data['daemon_reload_subprocess_call'],
Expand Down Expand Up @@ -467,4 +473,3 @@ def test_portinit_timeout(self, mock_syslog, get_runtime):
call(['sudo', 'systemctl', 'enable', 'telemetry.service']),
call(['sudo', 'systemctl', 'start', 'telemetry.service'])]
mocked_subprocess.check_call.assert_has_calls(expected)

115 changes: 102 additions & 13 deletions tests/featured/test_vectors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,8 +532,14 @@
"auto_restart": "enabled",
"high_mem_alert": "disabled"
},


"syncd": {
"state": "enabled",
"delayed": "False",
"has_global_scope": "True",
"has_per_asic_scope": "True",
"auto_restart": "enabled",
"high_mem_alert": "disabled"
},
},
},
"expected_config_db": {
Expand Down Expand Up @@ -562,12 +568,24 @@
"high_mem_alert": "disabled",
"state": "enabled"
},
"syncd": {
"auto_restart": "enabled",
"has_global_scope": "True",
"has_per_asic_scope": "True",
"delayed": "False",
"high_mem_alert": "disabled",
"state": "enabled"
},
},
},
"enable_feature_subprocess_calls": [
call(["sudo", "systemctl", "stop", "bgp.service"]),
call(["sudo", "systemctl", "disable", "bgp.service"]),
call(["sudo", "systemctl", "mask", "bgp.service"]),
call(["sudo", "systemctl", "unmask", "syncd.service"]),
call(["sudo", "systemctl", "enable", "syncd.service"]),
call(["sudo", "systemctl", "start", "syncd.service"]),

],
"daemon_reload_subprocess_call": [
call(["sudo", "systemctl", "daemon-reload"]),
Expand Down Expand Up @@ -620,8 +638,14 @@
"auto_restart": "enabled",
"high_mem_alert": "disabled"
},


"syncd": {
"state": "enabled",
"delayed": "False",
"has_global_scope": "True",
"has_per_asic_scope": "True",
"auto_restart": "enabled",
"high_mem_alert": "disabled"
},
},
},
"expected_config_db": {
Expand Down Expand Up @@ -650,12 +674,24 @@
"high_mem_alert": "disabled",
"state": "enabled"
},
"syncd": {
"auto_restart": "enabled",
"has_global_scope": "True",
"has_per_asic_scope": "True",
"delayed": "False",
"high_mem_alert": "disabled",
"state": "enabled"
},
},
},
"enable_feature_subprocess_calls": [
call(["sudo", "systemctl", "stop", "bgp.service"]),
call(["sudo", "systemctl", "disable", "bgp.service"]),
call(["sudo", "systemctl", "mask", "bgp.service"]),
call(["sudo", "systemctl", "unmask", "syncd.service"]),
call(["sudo", "systemctl", "enable", "syncd.service"]),
call(["sudo", "systemctl", "start", "syncd.service"]),

],
"daemon_reload_subprocess_call": [
call(["sudo", "systemctl", "daemon-reload"]),
Expand Down Expand Up @@ -708,8 +744,14 @@
"auto_restart": "enabled",
"high_mem_alert": "disabled"
},


"syncd": {
"state": "enabled",
"delayed": "False",
"has_global_scope": "True",
"has_per_asic_scope": "True",
"auto_restart": "enabled",
"high_mem_alert": "disabled"
},
},
},
"expected_config_db": {
Expand Down Expand Up @@ -738,6 +780,14 @@
"high_mem_alert": "disabled",
"state": "enabled"
},
"syncd": {
"auto_restart": "enabled",
"has_global_scope": "True",
"has_per_asic_scope": "True",
"delayed": "False",
"high_mem_alert": "disabled",
"state": "enabled"
},
},
},
"enable_feature_subprocess_calls": [
Expand All @@ -747,7 +797,10 @@
call(["sudo", "systemctl", "start", "teamd.service"]),
call(["sudo", "systemctl", "enable", "teamd.service"]),
call(["sudo", "systemctl", "unmask", "teamd.service"]),

call(["sudo", "systemctl", "unmask", "syncd.service"]),
call(["sudo", "systemctl", "enable", "syncd.service"]),
call(["sudo", "systemctl", "start", "syncd.service"]),

],
"daemon_reload_subprocess_call": [
call(["sudo", "systemctl", "daemon-reload"]),
Expand Down Expand Up @@ -800,8 +853,14 @@
"auto_restart": "enabled",
"high_mem_alert": "disabled"
},


"syncd": {
"state": "enabled",
"delayed": "False",
"has_global_scope": "True",
"has_per_asic_scope": "True",
"auto_restart": "enabled",
"high_mem_alert": "disabled"
},
},
},
"expected_config_db": {
Expand Down Expand Up @@ -830,6 +889,14 @@
"high_mem_alert": "disabled",
"state": "enabled"
},
"syncd": {
"auto_restart": "enabled",
"has_global_scope": "True",
"has_per_asic_scope": "True",
"delayed": "False",
"high_mem_alert": "disabled",
"state": "enabled"
},
},
},
"enable_feature_subprocess_calls": [
Expand All @@ -839,7 +906,10 @@
call(["sudo", "systemctl", "start", "teamd.service"]),
call(["sudo", "systemctl", "enable", "teamd.service"]),
call(["sudo", "systemctl", "unmask", "teamd.service"]),

call(["sudo", "systemctl", "unmask", "syncd.service"]),
call(["sudo", "systemctl", "enable", "syncd.service"]),
call(["sudo", "systemctl", "start", "syncd.service"]),

],
"daemon_reload_subprocess_call": [
call(["sudo", "systemctl", "daemon-reload"]),
Expand Down Expand Up @@ -893,8 +963,14 @@
"auto_restart": "enabled",
"high_mem_alert": "disabled"
},


"syncd": {
"state": "enabled",
"delayed": "False",
"has_global_scope": "False",
"has_per_asic_scope": "True",
"auto_restart": "enabled",
"high_mem_alert": "disabled"
},
},
},
"expected_config_db": {
Expand Down Expand Up @@ -923,6 +999,14 @@
"high_mem_alert": "disabled",
"state": "enabled"
},
"syncd": {
"auto_restart": "enabled",
"has_global_scope": "False",
"has_per_asic_scope": "True",
"delayed": "False",
"high_mem_alert": "disabled",
"state": "enabled"
},
},
},
"enable_feature_subprocess_calls": [
Expand All @@ -944,7 +1028,12 @@
call(["sudo", "systemctl", "stop", "lldp@1.service"]),
call(["sudo", "systemctl", "disable", "lldp@1.service"]),
call(["sudo", "systemctl", "mask", "lldp@1.service"]),

call(["sudo", "systemctl", "start", "syncd@0.service"]),
call(["sudo", "systemctl", "enable", "syncd@0.service"]),
call(["sudo", "systemctl", "unmask", "syncd@0.service"]),
call(["sudo", "systemctl", "start", "syncd@1.service"]),
call(["sudo", "systemctl", "enable", "syncd@1.service"]),
call(["sudo", "systemctl", "unmask", "syncd@1.service"]),
],
"daemon_reload_subprocess_call": [
call(["sudo", "systemctl", "daemon-reload"]),
Expand Down
Loading