From f53480dfe3efb21d6c47a2479e774cfb14050010 Mon Sep 17 00:00:00 2001 From: Tamer Ahmed Date: Tue, 18 Aug 2020 11:26:05 -0700 Subject: [PATCH 1/3] [services] Fix Delay Start of SNMP And Telemetry SNMP and Telemetry services are not critical to switch startup. They also cause fast-reboot not to meet timing requirements. In order to delay start those service are associated with systemd timer units, however when hostcfgd initiate service start, it start the service and not the timer. This PR fixes this issue by starting the timer associated with systemd unit. signed-off-by: Tamer Ahmed --- files/image_config/hostcfgd/hostcfgd | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/files/image_config/hostcfgd/hostcfgd b/files/image_config/hostcfgd/hostcfgd index 3a895a729b19..9351e43f844f 100755 --- a/files/image_config/hostcfgd/hostcfgd +++ b/files/image_config/hostcfgd/hostcfgd @@ -42,11 +42,12 @@ def obfuscate(data): def update_feature_state(feature_name, state): + feature_suffix = "timer" if feature_name in ["snmp", "telemetry"] else "service" if state == "enabled": start_cmds = [] - start_cmds.append("sudo systemctl unmask {}.service".format(feature_name)) - start_cmds.append("sudo systemctl enable {}.service".format(feature_name)) - start_cmds.append("sudo systemctl start {}.service".format(feature_name)) + start_cmds.append("sudo systemctl unmask {}.{}".format(feature_name, feature_suffix)) + start_cmds.append("sudo systemctl enable {}.{}".format(feature_name, feature_suffix)) + start_cmds.append("sudo systemctl start {}.{}".format(feature_name, feature_suffix)) for cmd in start_cmds: syslog.syslog(syslog.LOG_INFO, "Running cmd: '{}'".format(cmd)) try: @@ -55,12 +56,12 @@ def update_feature_state(feature_name, state): 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)) + syslog.syslog(syslog.LOG_INFO, "Feature '{}.{}' is enabled and started".format(feature_name, feature_suffix)) elif state == "disabled": stop_cmds = [] - stop_cmds.append("sudo systemctl stop {}.service".format(feature_name)) - stop_cmds.append("sudo systemctl disable {}.service".format(feature_name)) - stop_cmds.append("sudo systemctl mask {}.service".format(feature_name)) + stop_cmds.append("sudo systemctl stop {}.{}".format(feature_name, feature_suffix)) + stop_cmds.append("sudo systemctl disable {}.{}".format(feature_name, feature_suffix)) + stop_cmds.append("sudo systemctl mask {}.{}".format(feature_name, feature_suffix)) for cmd in stop_cmds: syslog.syslog(syslog.LOG_INFO, "Running cmd: '{}'".format(cmd)) try: @@ -71,7 +72,8 @@ def update_feature_state(feature_name, state): 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)) + syslog.syslog(syslog.LOG_ERR, "Unexpected state value '{}' for feature '{}.{}'" + .format(state, feature_name, feature_suffix)) class Iptables(object): From ff3df0d27fbe32f1974e27d95eab05c2087396b4 Mon Sep 17 00:00:00 2001 From: Tamer Ahmed Date: Wed, 19 Aug 2020 10:35:50 -0700 Subject: [PATCH 2/3] add delay start info to init_cfg template --- files/build_templates/init_cfg.json.j2 | 35 +++++++++++++------------- files/image_config/hostcfgd/hostcfgd | 8 +++--- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/files/build_templates/init_cfg.json.j2 b/files/build_templates/init_cfg.json.j2 index 334593a9d3cc..20d450378ab6 100644 --- a/files/build_templates/init_cfg.json.j2 +++ b/files/build_templates/init_cfg.json.j2 @@ -17,26 +17,27 @@ {% endfor %} } }, -{%- set features = [("bgp", "enabled", "enabled"), - ("database", "enabled", "disabled"), - ("dhcp_relay", "enabled", "enabled"), - ("lldp", "enabled", "enabled"), - ("pmon", "enabled", "enabled"), - ("radv", "enabled", "enabled"), - ("snmp", "enabled", "enabled"), - ("swss", "enabled", "enabled"), - ("syncd", "enabled", "enabled"), - ("teamd", "enabled", "enabled")] %} -{%- if include_iccpd == "y" %}{% do features.append(("iccpd", "disabled", "enabled")) %}{% endif %} -{%- if include_mgmt_framework == "y" %}{% do features.append(("mgmt-framework", "enabled", "enabled")) %}{% endif %} -{%- if include_nat == "y" %}{% do features.append(("nat", "disabled", "enabled")) %}{% endif %} -{%- if include_restapi == "y" %}{% do features.append(("restapi", "enabled", "enabled")) %}{% endif %} -{%- if include_sflow == "y" %}{% do features.append(("sflow", "disabled", "enabled")) %}{% endif %} -{%- if include_system_telemetry == "y" %}{% do features.append(("telemetry", "enabled", "enabled")) %}{% endif %} +{%- set features = [("bgp", "enabled", false, "enabled"), + ("database", "enabled", false, "disabled"), + ("dhcp_relay", "enabled", false, "enabled"), + ("lldp", "enabled", false, "enabled"), + ("pmon", "enabled", false, "enabled"), + ("radv", "enabled", false, "enabled"), + ("snmp", "enabled", true, "enabled"), + ("swss", "enabled", false, "enabled"), + ("syncd", "enabled", false, "enabled"), + ("teamd", "enabled", false, "enabled")] %} +{%- if include_iccpd == "y" %}{% do features.append(("iccpd", "disabled", false, "enabled")) %}{% endif %} +{%- if include_mgmt_framework == "y" %}{% do features.append(("mgmt-framework", "enabled", false, "enabled")) %}{% endif %} +{%- if include_nat == "y" %}{% do features.append(("nat", "disabled", false, "enabled")) %}{% endif %} +{%- if include_restapi == "y" %}{% do features.append(("restapi", "enabled", false, "enabled")) %}{% endif %} +{%- if include_sflow == "y" %}{% do features.append(("sflow", "disabled", false, "enabled")) %}{% endif %} +{%- if include_system_telemetry == "y" %}{% do features.append(("telemetry", "enabled", true, "enabled")) %}{% endif %} "FEATURE": { -{%- for feature, state, autorestart in features %} +{%- for feature, state, delay_start, autorestart in features %} "{{feature}}": { "state": "{{state}}", + "delay_start" : {{delay_start | lower()}}, "auto_restart": "{{autorestart}}", "high_mem_alert": "disabled" }{% if not loop.last %},{% endif -%} diff --git a/files/image_config/hostcfgd/hostcfgd b/files/image_config/hostcfgd/hostcfgd index 9351e43f844f..22755006badc 100755 --- a/files/image_config/hostcfgd/hostcfgd +++ b/files/image_config/hostcfgd/hostcfgd @@ -41,8 +41,8 @@ def obfuscate(data): return data -def update_feature_state(feature_name, state): - feature_suffix = "timer" if feature_name in ["snmp", "telemetry"] else "service" +def update_feature_state(feature_name, state, delay_start=False): + feature_suffix = "timer" if delay_start else "service" if state == "enabled": start_cmds = [] start_cmds.append("sudo systemctl unmask {}.{}".format(feature_name, feature_suffix)) @@ -286,7 +286,7 @@ class HostConfigDaemon: syslog.syslog(syslog.LOG_WARNING, "Eanble state of feature '{}' is None".format(feature_name)) continue - update_feature_state(feature_name, state) + update_feature_state(feature_name, state, feature_table[feature_name]['delay_start']) def aaa_handler(self, key, data): self.aaacfg.aaa_update(key, data) @@ -328,7 +328,7 @@ class HostConfigDaemon: syslog.syslog(syslog.LOG_WARNING, "Enable state of feature '{}' is None".format(feature_name)) return - update_feature_state(feature_name, state) + update_feature_state(feature_name, state, feature_table[feature_name]['delay_start']) def start(self): # Update all feature states once upon starting From 26e556475aa9c9742ecf06c26f39e6f4dade36c5 Mon Sep 17 00:00:00 2001 From: Tamer Ahmed Date: Wed, 19 Aug 2020 12:28:57 -0700 Subject: [PATCH 3/3] review comment --- files/build_templates/init_cfg.json.j2 | 5 +++-- files/image_config/hostcfgd/hostcfgd | 8 ++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/files/build_templates/init_cfg.json.j2 b/files/build_templates/init_cfg.json.j2 index 20d450378ab6..6db4fa8893d1 100644 --- a/files/build_templates/init_cfg.json.j2 +++ b/files/build_templates/init_cfg.json.j2 @@ -34,10 +34,11 @@ {%- if include_sflow == "y" %}{% do features.append(("sflow", "disabled", false, "enabled")) %}{% endif %} {%- if include_system_telemetry == "y" %}{% do features.append(("telemetry", "enabled", true, "enabled")) %}{% endif %} "FEATURE": { -{%- for feature, state, delay_start, autorestart in features %} +{# has_timer field if set, will start the feature systemd .timer unit instead of .service unit #} +{%- for feature, state, has_timer, autorestart in features %} "{{feature}}": { "state": "{{state}}", - "delay_start" : {{delay_start | lower()}}, + "has_timer" : {{has_timer | lower()}}, "auto_restart": "{{autorestart}}", "high_mem_alert": "disabled" }{% if not loop.last %},{% endif -%} diff --git a/files/image_config/hostcfgd/hostcfgd b/files/image_config/hostcfgd/hostcfgd index 22755006badc..2817e37a8f4c 100755 --- a/files/image_config/hostcfgd/hostcfgd +++ b/files/image_config/hostcfgd/hostcfgd @@ -41,8 +41,8 @@ def obfuscate(data): return data -def update_feature_state(feature_name, state, delay_start=False): - feature_suffix = "timer" if delay_start else "service" +def update_feature_state(feature_name, state, has_timer=False): + feature_suffix = "timer" if has_timer else "service" if state == "enabled": start_cmds = [] start_cmds.append("sudo systemctl unmask {}.{}".format(feature_name, feature_suffix)) @@ -286,7 +286,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]['delay_start']) + update_feature_state(feature_name, state, feature_table[feature_name]['has_timer']) def aaa_handler(self, key, data): self.aaacfg.aaa_update(key, data) @@ -328,7 +328,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]['delay_start']) + update_feature_state(feature_name, state, feature_table[feature_name]['has_timer']) def start(self): # Update all feature states once upon starting