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

Enhanced Feature Table state enable/disable for multi-asic platforms. #5358

Merged
merged 7 commits into from
Sep 22, 2020
Merged

Enhanced Feature Table state enable/disable for multi-asic platforms. #5358

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f74afe2
Enhanced Feature Table state enable/disbale for multi-asic platforms.
Sep 10, 2020
2364973
Make sure to retun None for both service list in case of error.
Sep 10, 2020
524eb46
Return empty list as fail condition
Sep 10, 2020
383aecd
Address Review Comments.
Sep 19, 2020
75fa914
Merge remote-tracking branch 'upstream/master' into multiasic_feature…
Sep 19, 2020
084e4ab
Fix merge conflict
Sep 19, 2020
855658f
Address Review Comment.
Sep 19, 2020
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
2 changes: 2 additions & 0 deletions files/build_templates/init_cfg.json.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
"{{feature}}": {
"state": "{{state}}",
"has_timer" : {{has_timer | lower()}},
"has_global_scope": {% if feature + '.service' in installer_services.split(' ') %}true{% else %}false{% endif %},
"has_per_asic_scope": {% if feature + '@.service' in installer_services.split(' ') %}true{% else %}false{% endif %},
"auto_restart": "{{autorestart}}",
"high_mem_alert": "disabled"
}{% if not loop.last %},{% endif -%}
Expand Down
103 changes: 62 additions & 41 deletions files/image_config/hostcfgd/hostcfgd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import copy
import jinja2
import ipaddr as ipaddress
from swsssdk import ConfigDBConnector
from sonic_py_common import device_info

# FILE
PAM_AUTH_CONF = "/etc/pam.d/common-auth-sonic"
Expand Down Expand Up @@ -42,45 +43,6 @@ def obfuscate(data):
return data


def update_feature_state(feature_name, state, has_timer):
feature_suffixes = ["service"] + (["timer"] if ast.literal_eval(has_timer) else [])
if state == "enabled":
start_cmds = []
for suffix in feature_suffixes:
start_cmds.append("sudo systemctl unmask {}.{}".format(feature_name, suffix))
# If feature has timer associated with it, start/enable corresponding systemd .timer unit
# otherwise, start/enable corresponding systemd .service unit
start_cmds.append("sudo systemctl enable {}.{}".format(feature_name, feature_suffixes[-1]))
start_cmds.append("sudo systemctl start {}.{}".format(feature_name, feature_suffixes[-1]))
for cmd in start_cmds:
syslog.syslog(syslog.LOG_INFO, "Running cmd: '{}'".format(cmd))
try:
subprocess.check_call(cmd, shell=True)
except subprocess.CalledProcessError as err:
syslog.syslog(syslog.LOG_ERR, "'{}' failed. RC: {}, output: {}"
.format(err.cmd, err.returncode, err.output))
continue
syslog.syslog(syslog.LOG_INFO, "Feature '{}.{}' is enabled and started"
.format(feature_name, feature_suffixes[-1]))
elif state == "disabled":
stop_cmds = []
for suffix in reversed(feature_suffixes):
stop_cmds.append("sudo systemctl stop {}.{}".format(feature_name, suffix))
stop_cmds.append("sudo systemctl disable {}.{}".format(feature_name, suffix))
stop_cmds.append("sudo systemctl mask {}.{}".format(feature_name, suffix))
for cmd in stop_cmds:
syslog.syslog(syslog.LOG_INFO, "Running cmd: '{}'".format(cmd))
try:
subprocess.check_call(cmd, shell=True)
except subprocess.CalledProcessError as err:
syslog.syslog(syslog.LOG_ERR, "'{}' failed. RC: {}, output: {}"
.format(err.cmd, err.returncode, err.output))
continue
syslog.syslog(syslog.LOG_INFO, "Feature '{}' is stopped and disabled".format(feature_name))
else:
syslog.syslog(syslog.LOG_ERR, "Unexpected state value '{}' for feature '{}'"
.format(state, feature_name))


class Iptables(object):
def __init__(self):
Expand Down Expand Up @@ -280,6 +242,65 @@ class HostConfigDaemon:
self.iptables = Iptables()
self.iptables.load(lpbk_table)

def update_feature_state(self, feature_name, state, feature_table):
has_timer = ast.literal_eval(feature_table[feature_name]['has_timer'])
has_global_scope = ast.literal_eval(feature_table[feature_name]['has_global_scope'])
has_per_asic_scope = ast.literal_eval(feature_table[feature_name]['has_per_asic_scope'])

# Create feature name suffix depending feature is running in host or namespace or in both
feature_name_suffix_list = (([feature_name] if has_global_scope or not device_info.is_multi_npu() else []) +
([(feature_name + '@' + str(asic_inst)) for asic_inst in range(device_info.get_num_npus())
if has_per_asic_scope and device_info.is_multi_npu()]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make is_multi_npu() a class member?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tahmed-dev Done


if not feature_name_suffix_list:
syslog.syslog(syslog.LOG_ERR, "Feature '{}' service not available"
.format(feature_name))

feature_suffixes = ["service"] + (["timer"] if has_timer else [])

if state == "enabled":
start_cmds = []
for feature_name_suffix in feature_name_suffix_list:
for suffix in feature_suffixes:
start_cmds.append("sudo systemctl unmask {}.{}".format(feature_name_suffix, suffix))
Comment on lines +265 to +266
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great work. I know it merged already, but wanted to follow up the story here if we have plan to support enabling/disabling services for particular asics in system?

More specifically, says there are 2 asics in system, we want to disable lldp on asic 1, but not asic 2.

I imagine that to do that, we need

(1) Create entry like FEATURE|lldp|1 with enabled = false and FEATURE|lldp|2 with enabled = true. hostcfgd listens and reacts to the change. It will turn off lldp@1.service but keep lldp@2.service on.

Or (2) Create hostcfgd@1 for asic 1 and hostcfgd@2 for asic 2. hostcfgd@1 listens database@1 CONFIG_DB, and reacts to FEATURE|lldp. Same for hostcfgd@2 which listens database@2 CONFIG_DB. But this one may not work if we want to enable/disable database. If database doesn't exits, hostcfgd@x will have nothing to listen and react.

# If feature has timer associated with it, start/enable corresponding systemd .timer unit
# otherwise, start/enable corresponding systemd .service unit
start_cmds.append("sudo systemctl enable {}.{}".format(feature_name_suffix, feature_suffixes[-1]))
start_cmds.append("sudo systemctl start {}.{}".format(feature_name_suffix, feature_suffixes[-1]))
for cmd in start_cmds:
syslog.syslog(syslog.LOG_INFO, "Running cmd: '{}'".format(cmd))
try:
subprocess.check_call(cmd, shell=True)
except subprocess.CalledProcessError as err:
syslog.syslog(syslog.LOG_ERR, "'{}' failed. RC: {}, output: {}"
.format(err.cmd, err.returncode, err.output))
syslog.syslog(syslog.LOG_ERR, "Feature '{}.{}' failed to be enabled and started"
.format(feature_name, feature_suffixes[-1]))
return
syslog.syslog(syslog.LOG_INFO, "Feature '{}.{}' is enabled and started"
.format(feature_name, feature_suffixes[-1]))
elif state == "disabled":
stop_cmds = []
for feature_name_suffix in feature_name_suffix_list:
for suffix in reversed(feature_suffixes):
stop_cmds.append("sudo systemctl stop {}.{}".format(feature_name_suffix, suffix))
stop_cmds.append("sudo systemctl disable {}.{}".format(feature_name_suffix, suffix))
stop_cmds.append("sudo systemctl mask {}.{}".format(feature_name_suffix, suffix))
for cmd in stop_cmds:
syslog.syslog(syslog.LOG_INFO, "Running cmd: '{}'".format(cmd))
try:
subprocess.check_call(cmd, shell=True)
except subprocess.CalledProcessError as err:
syslog.syslog(syslog.LOG_ERR, "'{}' failed. RC: {}, output: {}"
.format(err.cmd, err.returncode, err.output))
syslog.syslog(syslog.LOG_ERR, "Feature '{}' failed to be stopped and disabled".format(feature_name))
return
syslog.syslog(syslog.LOG_INFO, "Feature '{}' is stopped and disabled".format(feature_name))
else:
syslog.syslog(syslog.LOG_ERR, "Unexpected state value '{}' for feature '{}'"
.format(state, feature_name))


def update_all_feature_states(self):
feature_table = self.config_db.get_table('FEATURE')
for feature_name in feature_table.keys():
Expand All @@ -292,7 +313,7 @@ class HostConfigDaemon:
syslog.syslog(syslog.LOG_WARNING, "Eanble state of feature '{}' is None".format(feature_name))
continue

update_feature_state(feature_name, state, feature_table[feature_name]['has_timer'])
self.update_feature_state(feature_name, state, feature_table)

def aaa_handler(self, key, data):
self.aaacfg.aaa_update(key, data)
Expand Down Expand Up @@ -334,7 +355,7 @@ class HostConfigDaemon:
syslog.syslog(syslog.LOG_WARNING, "Enable state of feature '{}' is None".format(feature_name))
return

update_feature_state(feature_name, state, feature_table[feature_name]['has_timer'])
self.update_feature_state(feature_name, state, feature_table)

def start(self):
# Update all feature states once upon starting
Expand Down