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

sonic yang changes for BUM storm control #7355

Merged
merged 7 commits into from
Dec 16, 2021

Conversation

mohan-selvaraj
Copy link
Contributor

@mohan-selvaraj mohan-selvaraj commented Apr 16, 2021

Why I did it

Sonic yang model for BUM storm control

How I did it

Added yang model and the corresponding test cases.

How to verify it

yang model test case for storm control

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

  • 201811
  • 201911
  • 202006
  • 202012

Description for the changelog

Addition of storm control yang model and corresponding tests to verify the same.

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

Rate is defined in bits-per-second. All three types of storm-control can be
enabled on an interface.";

revision 2019-09-03 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update the revision date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

container PORT_STORM_CONTROL {
list PORT_STORM_CONTROL_LIST {
key "ifname storm_type";
/*scommon:key-pattern "PORT_STORM_CONTROL|{ifname}:{storm_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 do we have ":" in the key pattern, we should use "|"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

},
"Ethernet0|unknown-unicast": {
"kbps": "20000"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please write the actual UT cases as well e.g add cases in src/sonic-yang-models/tests/yang_model_tests/tests for this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

enum unknown-multicast;
}
}
leaf kbps {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can we use rate instead of kbps field name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rate might indicate the % of linerate. kbps would be a constant value irrespective of line-rate. Hence chose kbps.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry...I meant meaningful name than kbps e.g storm_limit..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Do you recommend changing this in all the instances (CLI and orchagent) or only here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend changing it in all places, but that can be done in a separate PR.

@zhangyanzhao
Copy link
Collaborator

@mohan-selvaraj could you please join the YANG subgroup meeting and review with team? Thanks.

@zhangyanzhao
Copy link
Collaborator

@mohan-selvaraj please help to fix the conflict asap.

@qiluo-msft qiluo-msft merged commit 36673c1 into sonic-net:master Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
YANG YANG model related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants