Skip to content

Commit

Permalink
[dhcp_relay] Disable dhcp_relay for ToRRouter switches type by the fe…
Browse files Browse the repository at this point in the history 
…ature manager (#7789)

- Why I did it
Currently dhcp packets are disabled by the COPP manager for non ToRRouter type switches.
Even if the feature is enabled, DHCP packets wont hook to the CPU since the COPP manager will not trap this packets.
This change is to disable dhcp_relay by default for non ToRRouter switches from init_cfg.json.
With this approach, if the user want to enable the feature for non ToRRouter switches, manual enablement is required by the 'feature' configuration.
This is to keep the current approach for MSFT production issue with dhcp relay for non ToRRouter switched and allow the user to decide if to use it or not.

- How I did it
Configure dhcp_relay 'disabled' by default on init_cfg.json for non ToRRouter switches.
Remove the exclusion of dhcp packets on copp_cfg.json

- How to verify it
Enable dhcp_relay feature on a non ToRRouter switch.
Unit-tests modified so the default values on mocked CONFIG DB in 'test_vectors.py' for dhcp_relay will be 'disabled'.
This is by the change for 'init_cfg.json.j2'.
For ToRRouter the state will change from 'disabled' to 'enabled'.
Another test case added for a 'ToR' switch type, this is to test the state is 'enabled' if the user configured it to be so.
  • Loading branch information
@shlomibitton @judyjoseph
shlomibitton authored and judyjoseph committed Aug 4, 2021
1 parent 0f3657f commit 88e94d3
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 11 deletions.
2 changes: 1 addition & 1 deletion files/build_templates/init_cfg.json.j2
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@
},
{%- set features = [("bgp", "enabled", false, "enabled"),
("database", "always_enabled", false, "always_enabled"),
("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")] %}
{% do features.append(("dhcp_relay", "{% if not (DEVICE_METADATA is defined and DEVICE_METADATA['localhost'] is defined and DEVICE_METADATA['localhost']['type'] is defined and DEVICE_METADATA['localhost']['type'] != 'ToRRouter') %}enabled{% else %}disabled{% endif %}", false, "enabled")) %}
{%- if sonic_asic_platform == "vs" %}{% do features.append(("gbsyncd", "enabled", false, "enabled")) %}{% endif %}
{%- if include_iccpd == "y" %}{% do features.append(("iccpd", "disabled", false, "enabled")) %}{% endif %}
{%- if include_mgmt_framework == "y" %}{% do features.append(("mgmt-framework", "enabled", true, "enabled")) %}{% endif %}
Expand Down
4 changes: 1 addition & 3 deletions files/image_config/copp/copp_cfg.j2
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,10 @@
"trap_ids": "lldp",
"trap_group": "queue4_group3"
},
{% if not (DEVICE_METADATA is defined and DEVICE_METADATA['localhost'] is defined and DEVICE_METADATA['localhost']['type'] is defined and DEVICE_METADATA['localhost']['type'] != "ToRRouter") %}
"dhcp": {
"dhcp_relay": {
"trap_ids": "dhcp,dhcpv6",
"trap_group": "queue4_group3"
},
{% endif %}
"udld": {
"trap_ids": "udld",
"trap_group": "queue4_group3"
Expand Down
104 changes: 97 additions & 7 deletions src/sonic-host-services/tests/hostcfgd/test_vectors.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"has_timer": "False",
"high_mem_alert": "disabled",
"set_owner": "kube",
"state": "enabled"
"state": "{% if not (DEVICE_METADATA is defined and DEVICE_METADATA['localhost'] is defined and DEVICE_METADATA['localhost']['type'] is defined and DEVICE_METADATA['localhost']['type'] != 'ToRRouter') %}enabled{% else %}disabled{% endif %}"
},
"mux": {
"auto_restart": "enabled",
Expand Down Expand Up @@ -125,7 +125,7 @@
"has_timer": "False",
"high_mem_alert": "disabled",
"set_owner": "kube",
"state": "enabled"
"state": "{% if not (DEVICE_METADATA is defined and DEVICE_METADATA['localhost'] is defined and DEVICE_METADATA['localhost']['type'] is defined and DEVICE_METADATA['localhost']['type'] != 'ToRRouter') %}enabled{% else %}disabled{% endif %}"
},
"mux": {
"auto_restart": "enabled",
Expand Down Expand Up @@ -157,7 +157,7 @@
"has_timer": "False",
"high_mem_alert": "disabled",
"set_owner": "kube",
"state": "enabled"
"state": "disabled"
},
"mux": {
"auto_restart": "enabled",
Expand All @@ -181,9 +181,6 @@
},
},
"expected_subprocess_calls": [
call("sudo systemctl unmask dhcp_relay.service", shell=True),
call("sudo systemctl enable dhcp_relay.service", shell=True),
call("sudo systemctl start dhcp_relay.service", shell=True),
call("sudo systemctl stop mux.service", shell=True),
call("sudo systemctl disable mux.service", shell=True),
call("sudo systemctl mask mux.service", shell=True),
Expand Down Expand Up @@ -213,6 +210,99 @@
"memory": "0M-2G:256M,2G-4G:320M,4G-8G:384M,8G-:448M"
}
},
"FEATURE": {
"dhcp_relay": {
"auto_restart": "enabled",
"has_global_scope": "True",
"has_per_asic_scope": "False",
"has_timer": "False",
"high_mem_alert": "disabled",
"set_owner": "kube",
"state": "{% if not (DEVICE_METADATA is defined and DEVICE_METADATA['localhost'] is defined and DEVICE_METADATA['localhost']['type'] is defined and DEVICE_METADATA['localhost']['type'] != 'ToRRouter') %}enabled{% else %}disabled{% endif %}"
},
"mux": {
"auto_restart": "enabled",
"has_global_scope": "True",
"has_per_asic_scope": "False",
"has_timer": "False",
"high_mem_alert": "disabled",
"set_owner": "local",
"state": "{% if 'subtype' in DEVICE_METADATA['localhost'] and DEVICE_METADATA['localhost']['subtype'] == 'DualToR' %}enabled{% else %}always_disabled{% endif %}"
},
"telemetry": {
"auto_restart": "enabled",
"has_global_scope": "True",
"has_per_asic_scope": "False",
"has_timer": "True",
"high_mem_alert": "disabled",
"set_owner": "kube",
"state": "enabled",
"status": "enabled"
},
},
},
"expected_config_db": {
"FEATURE": {
"dhcp_relay": {
"auto_restart": "enabled",
"has_global_scope": "True",
"has_per_asic_scope": "False",
"has_timer": "False",
"high_mem_alert": "disabled",
"set_owner": "kube",
"state": "disabled"
},
"mux": {
"auto_restart": "enabled",
"has_global_scope": "True",
"has_per_asic_scope": "False",
"has_timer": "False",
"high_mem_alert": "disabled",
"set_owner": "local",
"state": "always_disabled"
},
"telemetry": {
"auto_restart": "enabled",
"has_global_scope": "True",
"has_per_asic_scope": "False",
"has_timer": "True",
"high_mem_alert": "disabled",
"set_owner": "kube",
"state": "enabled",
"status": "enabled"
},
},
},
"expected_subprocess_calls": [
call("sudo systemctl stop mux.service", shell=True),
call("sudo systemctl disable mux.service", shell=True),
call("sudo systemctl mask mux.service", shell=True),
call("sudo systemctl unmask telemetry.service", shell=True),
call("sudo systemctl unmask telemetry.timer", shell=True),
call("sudo systemctl enable telemetry.timer", shell=True),
call("sudo systemctl start telemetry.timer", shell=True),
],
"popen_attributes": {
'communicate.return_value': ('output', 'error')
},
},
],
[
"SingleToRCase_DHCP_Relay_Enabled",
{
"config_db": {
"DEVICE_METADATA": {
"localhost": {
"type": "ToR",
}
},
"KDUMP": {
"config": {
"enabled": "false",
"num_dumps": "3",
"memory": "0M-2G:256M,2G-4G:320M,4G-8G:384M,8G-:448M"
}
},
"FEATURE": {
"dhcp_relay": {
"auto_restart": "enabled",
Expand Down Expand Up @@ -318,7 +408,7 @@
"has_timer": "False",
"high_mem_alert": "disabled",
"set_owner": "kube",
"state": "enabled"
"state": "{% if not (DEVICE_METADATA is defined and DEVICE_METADATA['localhost'] is defined and DEVICE_METADATA['localhost']['type'] is defined and DEVICE_METADATA['localhost']['type'] != 'ToRRouter') %}enabled{% else %}disabled{% endif %}"
},
"mux": {
"auto_restart": "enabled",
Expand Down

0 comments on commit 88e94d3

Please sign in to comment.