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.

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

[copporch] Add safeguard during policer attribute update #2977

Merged
merged 12 commits into from
Dec 10, 2023

Conversation

vivekrnv
Copy link
Contributor

@vivekrnv vivekrnv commented Dec 2, 2023

What I did

Add safeguard during policer attribute update in copporch when the config is trying to change CREATE_AND_UPDATE attributes by not allowing CREATE_ONLY attributes to pass.

If user tries to update a CREATE_ONLY attribute, it follows the current design of orchagent crash.

Why I did it

During fast-boot, coppmgr does a DEL folowed by SET on the diverged COPP_TABLE key, But while reading the notification, the DEL is optimized and only SET is seen and this would lead to orchagent crash even though, user is only changing CREATE_AND_UPDATE attributes.

How I verified it

Update the copp_cfg.j2 file to update the CIR and CBS, followed by a fast boot, This should simulate divergence in the copp config and the scenario explained above is hit and orchagent did not crash

<entries applied during re-conciliation>
2023-12-01.02:45:53.378712|COPP_TABLE:queue4_group3|SET|cbs:100|cir:100|meter_type:packets|mode:sr_tcm|queue:4|red_action:drop|trap_action:trap|trap_priority:4|trap_ids:dhcp,dhcpv6,lldp,udld
2023-12-01.02:45:53.378747|COPP_TABLE:queue4_group2|SET|cbs:1000|cir:600|meter_type:packets|mode:sr_tcm|queue:4|red_action:drop|trap_action:copy|trap_priority:4|trap_ids:arp_req,arp_resp,neigh_discovery
2023-12-01.02:45:53.378763|COPP_TABLE:default|SET|cbs:600|cir:600|meter_type:packets|mode:sr_tcm|queue:0|red_action:drop
2023-12-01.02:45:53.378778|COPP_TABLE:queue1_group1|SET|cbs:6000|cir:6000|meter_type:packets|mode:sr_tcm|queue:1|red_action:drop|trap_action:trap|trap_priority:1|trap_ids:ip2me
2023-12-01.02:45:53.378791|COPP_TABLE:queue4_group1|SET|cbs:6000|cir:6000|meter_type:packets|mode:sr_tcm|queue:4|red_action:drop|trap_action:trap|trap_priority:4|trap_ids:bgp,bgpv6,lacp

<New SET entry processed by copporch>
2023-12-01.02:46:37.465125|COPP_TABLE:queue4_group3|SET|cbs:500|cir:500|meter_type:packets|mode:sr_tcm|queue:4|red_action:drop|trap_action:trap|trap_priority:4|trap_ids:dhcp,dhcpv6,lldp,udld

Added Unit Test

Details if related

@vivekrnv vivekrnv requested a review from prsunny as a code owner December 2, 2023 01:11
@vivekrnv vivekrnv requested a review from dgsudharsan December 2, 2023 01:11
dgsudharsan
dgsudharsan previously approved these changes Dec 2, 2023
@dgsudharsan
Copy link
Collaborator

@prsunny , @StormLiangMS This change is required as a bug fix for 202305. Without this fix, lets assume we are changing some default policer field in copp_cfg.j2 (e.g. Updating CBS/CIR value as in sonic-net/sonic-buildimage#17132) which was present in previous version (202205), then coppmgr logic will do a del of policer and then set with new fields. However due to optimization in orchagent, DEL will be ignored and the SET will set all the policer fields which includes create only fields leading to crash.
To avoid that, this PR checks if create only fields are not modified, skip it.

Updating the CREATE_ONLY attributes of the policer will cause a crash
If modified, throw an error log and proceed with changeable attributes
*/
if(policer_attr.id == SAI_POLICER_ATTR_METER_TYPE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't we just check for CREATE_ONLY attribute and skip setting that (Why to check if value is changed or not)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

coppmgr pushes all the fv's down to copporch. if we don't save the old attributes that were created, we can't log an error saying user is trying to change a CREATE_ONLY attribute.

orchagent/copporch.cpp Outdated Show resolved Hide resolved
prsunny
prsunny previously approved these changes Dec 7, 2023
Signed-off-by: Vivek Reddy <vkarri@nvidia.com>
@dgsudharsan dgsudharsan merged commit d357e6f into sonic-net:master Dec 10, 2023
15 checks passed
@dgsudharsan
Copy link
Collaborator

@StormLiangMS Tested on top of 202305 commit sonic-net/sonic-buildimage@688245a

mssonicbld pushed a commit to mssonicbld/sonic-swss that referenced this pull request Dec 20, 2023
)

Add safeguard during policer attribute update in copporch when the config is trying to change CREATE_AND_UPDATE attributes by not allowing CREATE_ONLY attributes to pass to SAI
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202305: #2996

mssonicbld pushed a commit that referenced this pull request Dec 20, 2023
Add safeguard during policer attribute update in copporch when the config is trying to change CREATE_AND_UPDATE attributes by not allowing CREATE_ONLY attributes to pass to SAI
mssonicbld pushed a commit to mssonicbld/sonic-swss that referenced this pull request Feb 2, 2024
)

Add safeguard during policer attribute update in copporch when the config is trying to change CREATE_AND_UPDATE attributes by not allowing CREATE_ONLY attributes to pass to SAI
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202311: #3034

mssonicbld pushed a commit that referenced this pull request Feb 2, 2024
Add safeguard during policer attribute update in copporch when the config is trying to change CREATE_AND_UPDATE attributes by not allowing CREATE_ONLY attributes to pass to SAI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants