[dhcp_relay] Disable dhcp_relay for ToRRouter switches type by the feature manager by shlomibitton · Pull Request #7789 · sonic-net/sonic-buildimage

shlomibitton · 2021-06-03T15:39:20Z

Signed-off-by: Shlomi Bitton shlomibi@nvidia.com

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.

Which release branch to backport (provide reason below if selected)

201811
201911
202006
202012

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

liat-grozovik · 2021-06-13T13:50:51Z

@dgsudharsan i am unable to assign you as a reviewer but appreciate if you can review and add comments.

shlomibitton · 2021-06-13T13:52:18Z

@liat-grozovik he did the internal review and approve it :)

liat-grozovik

LGTM beside of the small comment which is mostly documentation of a so called WA

liat-grozovik · 2021-06-13T13:56:01Z

src/sonic-host-services/scripts/hostcfgd

what do you mean by WA? to ensure upgrade and backwards compatibility?

MSFT have a production issue, they want only ToRRouter switches to enable dhcp relay service.
The previous approach was to disable the feature by not hooking dhcp packets, the new approach allow the user to enable it if it is configured to do so but still address the MSFT requirement to enable it by default only for ToRRouter.
If it is not a ToRRouter the user will have to override the configuration to enable it.

prsunny · 2021-06-17T21:55:58Z

lgtm, did you test if rule gets added if the feature is enabled at a later stage? @dgsudharsan to review

shlomibitton · 2021-06-21T09:39:46Z

lgtm, did you test if rule gets added if the feature is enabled at a later stage? @dgsudharsan to review

@prsunny Yes, for ToRRouter it will be enabled by default, for other types it will be disabled by default.
But, if the user enabled the feature with a non ToRRouter switch it will stay enabled.
I added a UT cases to verify it as well.

liat-grozovik · 2021-06-23T17:57:18Z

@prsunny is it ok to merge?

files/image_config/copp/copp_cfg.j2

tahmed-dev · 2021-06-28T19:20:20Z

src/sonic-host-services/scripts/hostcfgd

Please move this to state field in the init_cfg.json similar to mux feature in the test cases.

I tried to do so on init_cfg.json but this is not possible since this file is used on build stage and the value of "device_metadata['localhost']['type']" is not exist, so the approach here is to disable dhcp_relay by default.
hostcfgd will check on runtime if this is a ToRRouter switch and enable it accordingly.
If not, it will be disabled as desired, but if the user explicitly request to enable it, this will be accepted as well.

The content of the state will be evaluated when loading the init_cfg.j2 for the first time. Also, the build J2 template will not render contents inside append call. Something like the following:

{% do features.append(("dhcp_relay", "{% if 'type' in DEVICE_METADATA['localhost'] and DEVICE_METADATA['localhost']['type'] == 'ToRRouter' %}enabled{% else %}disabled{% endif %}", false, "enabled")) %}

Then we can remove the change in hostcfgd all together.

@tahmed-dev can you please check the changes now?

tahmed-dev · 2021-06-28T19:20:46Z

src/sonic-host-services/tests/hostcfgd/test_vectors.py

Here is example of mux feature.

Since the dhcp_relay is disabled by default from init_cfg.json there is no reason to add this condition, it will be always disabled.
The UT should check if ToRRouter switched to 'enabled' state by hostcfgd, and if a non ToRRouter switch, which a user manually enabled, will stay enabled.

The point is to move the code from hostcfg to init_cfg.json.j2

liat-grozovik · 2021-06-29T20:05:15Z

@shlomibitton please help to resolve conflicts due to recent changes in hostcfgd.

shlomibitton · 2021-06-30T10:19:51Z

@shlomibitton please help to resolve conflicts due to recent changes in hostcfgd.

@liat-grozovik resolved the conflicts

Enable dhcp_relay trap from COPP manager by default. If the user want to enable the feature for non ToRRouter switches, manual enablement is required. 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. Change default values for 'dhcp_relay' on test cases to fit the change on 'init_cfg.json.j2'

liat-grozovik · 2021-07-04T13:25:00Z

/azp run

azure-pipelines · 2021-07-04T13:25:16Z

Azure Pipelines successfully started running 1 pipeline(s).

tahmed-dev · 2021-07-07T23:34:19Z

src/sonic-host-services/tests/hostcfgd/test_vectors.py

+                        "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 %}"


why use -ve logic? curious.

…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.

…ature manager (sonic-net#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.

shlomibitton requested a review from lguohan as a code owner June 3, 2021 15:39

shlomibitton mentioned this pull request Jun 6, 2021

DHCP relay for IPv6 HLD sonic-net/SONiC#765

Merged

liat-grozovik requested a review from tahmed-dev June 13, 2021 13:50

liat-grozovik reviewed Jun 13, 2021

View reviewed changes

liat-grozovik previously approved these changes Jun 23, 2021

View reviewed changes

dgsudharsan previously approved these changes Jun 23, 2021

View reviewed changes

tahmed-dev suggested changes Jun 28, 2021

View reviewed changes

shlomibitton dismissed stale reviews from dgsudharsan and liat-grozovik via 232bb83 June 30, 2021 10:10

shlomibitton requested review from liat-grozovik and tahmed-dev June 30, 2021 10:19

Fix quotes

4d043dd

tahmed-dev reviewed Jul 7, 2021

View reviewed changes

tahmed-dev approved these changes Jul 7, 2021

View reviewed changes

liat-grozovik added the Request for 202106 Branch label Jul 8, 2021

liat-grozovik merged commit 776a446 into sonic-net:master Jul 8, 2021

shlomibitton deleted the disable_dhcp_default branch July 20, 2021 08:52

judyjoseph added the Included in 202106 Branch label Aug 4, 2021

shlomibitton mentioned this pull request Dec 19, 2021

Revert "[sonic-package-manager] support sonic-cli-gen and packages wi… sonic-net/sonic-utilities#1972

Merged

Conversation

shlomibitton commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

Uh oh!

liat-grozovik commented Jun 13, 2021

Uh oh!

shlomibitton commented Jun 13, 2021

Uh oh!

liat-grozovik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shlomibitton Jun 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsunny commented Jun 17, 2021

Uh oh!

shlomibitton commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liat-grozovik commented Jun 23, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liat-grozovik commented Jun 29, 2021

Uh oh!

shlomibitton commented Jun 30, 2021

Uh oh!

liat-grozovik commented Jul 4, 2021

Uh oh!

azure-pipelines bot commented Jul 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shlomibitton commented Jun 3, 2021 •

edited

Loading

shlomibitton Jun 13, 2021 •

edited

Loading

shlomibitton commented Jun 21, 2021 •

edited

Loading