SONiC Yang model support for COPP #7199

amaneti · 2021-03-31T07:38:53Z

Why I did it

Created SONiC Yang model for COPP.
Tables: COPP_GROUP, COPP_TRAP.

How I did it

Defined Yang models for COPP based on Guideline doc:
https://github.com/Azure/SONiC/blob/master/doc/mgmt/SONiC_YANG_Model_Guidelines.md
and
https://github.com/Azure/sonic-utilities/blob/master/doc/Command-Reference.md

How to verify it

$ pytest tests/yang_model_tests/test_yang_model.py -v
============================================================================== test session starts ===============================================================================
platform linux2 -- Python 2.7.13, pytest-3.0.6, py-1.4.32, pluggy-0.4.0 -- /usr/bin/python
cachedir: .cache
rootdir: /sonic/src/sonic-yang-models, inifile:
plugins: cov-2.4.0
collected 1 items

tests/yang_model_tests/test_yang_model.py::Test_yang_models::test_run_tests PASSED

============================================================================ 1 passed in 0.50 seconds ============================================================================


module: sonic-copp
  +--rw sonic-copp
     +--rw COPP_GROUP
     |  +--rw COPP_GROUP_LIST* [name]
     |     +--rw name                    string
     |     +--rw queue?                  uint32
     |     +--rw trap_priority?          uint32
     |     +--rw trap_action             stypes:copp_packet_action
     |     +--rw meter_type              stypes:meter_type
     |     +--rw mode                    enumeration
     |     +--rw color?                  enumeration
     |     +--rw cir?                    uint64
     |     +--rw cbs?                    uint64
     |     +--rw pir?                    uint64
     |     +--rw pbs?                    uint64
     |     +--rw green_action?           stypes:copp_packet_action
     |     +--rw yellow_action?          stypes:copp_packet_action
     |     +--rw red_action?             stypes:copp_packet_action
     |     +--rw genetlink_name?         string
     |     +--rw genetlink_mcgrp_name?   string
     +--rw COPP_TRAP
        +--rw COPP_TRAP_LIST* [name]
           +--rw name          string
           +--rw trap_ids      string
           +--rw trap_group?   -> /sonic-copp/COPP_GROUP/COPP_GROUP_LIST/name

  rpcs:
    +---x get-match-protocols
       +--ro output
          +--ro Match_protocols* [Protocol]
             +--ro Protocol    string

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)

lguohan · 2021-03-31T18:58:39Z

can you split the test cases?

prsunny · 2021-03-31T19:41:22Z

@dgsudharsan to review

amaneti · 2021-04-03T02:47:08Z

retest please

lguohan · 2021-04-08T17:38:29Z

src/sonic-yang-models/yang-models/sonic-copp.yang

+ }
+
+ leaf trap_ids {
+ type string;


should this be enum?

Response here

lguohan · 2021-04-08T17:39:17Z

src/sonic-yang-models/yang-models/sonic-copp.yang

+
+ leaf red_action {
+ type string {
+ pattern "(drop)|(forward)|(copy)|(copy_cancel)|(trap)|(log)|(deny)|(transit)" {


should this be enum?

lguohan · 2021-04-08T17:41:22Z

src/sonic-yang-models/yang-models/sonic-copp.yang

+ }
+ }
+ description
+ "CPU queue id";


-> cpu rx queue id

prsunny · 2021-04-08T18:03:08Z

src/sonic-yang-models/yang-models/sonic-copp.yang

+ }
+
+ leaf trap_ids {
+ type string;


CoPP HLD expects the trap_ids name to be specific string. Should we define a pattern here, something based on this?

trap_ids is a string with comma separated trap_ids.

"arp": { "trap_ids": "arp_req,arp_resp,neigh_discovery", "trap_group": "queue4_group3" },

If we have to define a pattern based on copporch,
trap_ids can be defined as leaf-list in yang i.e array.

leaf-list trap_ids { mandatory true; type enumeration { enum arp_req; enum arp_resp; enum arp_suppress; ... ... enum vrrp; enum vrrpv6; } description "list of trap_ids"; }

But, this would also mandate

changes to copp_cfg.j2 trap_ids fields as below:

"arp": { "trap_ids": [ "arp_req", "arp_resp", "neigh_discovery" ], "trap_group": "queue4_group2" },

changes to coppmgr to handle array of trap_ids. Otherwise, it results into ERR swss#coppmgrd: :- main: Runtime error: type must be string, but is array

ok. im not expecting a change to coppmgr or template.

dgsudharsan · 2021-04-09T04:22:55Z

src/sonic-yang-models/yang-models/sonic-copp.yang

+ leaf queue {
+ type uint8 {
+ range "0..47" {
+ error-message "Invalid value passed for queue";


Any reason why the range is restricted to 48 queues? Is there any restriction imposed in SAI regarding the maximum number of queues?

Removed range "0..47" {

dgsudharsan · 2021-04-09T04:27:46Z

src/sonic-yang-models/yang-models/sonic-copp.yang

+
+ leaf trap_priority {
+ type uint16 {
+ range "0..1023" {


From the SAI headers the range is dependent on platforms. I am not sure why the priority is restricted to 0-1023
*
* @type sai_uint32_t
* @flags CREATE_AND_SET
* @default attrvalue SAI_SWITCH_ATTR_ACL_ENTRY_MINIMUM_PRIORITY
* @validonly SAI_HOSTIF_TRAP_ATTR_PACKET_ACTION == SAI_PACKET_ACTION_TRAP or SAI_HOSTIF_TRAP_ATTR_PACKET_ACTION == SAI_PACKET_ACTION_COPY
*/
SAI_HOSTIF_TRAP_ATTR_TRAP_PRIORITY

Removed range "0..1023" {

dgsudharsan · 2021-04-09T04:33:35Z

src/sonic-yang-models/yang-models/sonic-copp.yang

+ "Red action";
+ }
+
+ leaf genetlink_name {


If genetlink name and mc_group name are exposed to north bound interfaces to be modified what is the purpose of restricting here to a single value?

genetlink_name and genetlink_mcgrp_name are applicable to sFlow only. Current supported values are "psample" and "packets" respectively. Default copp_cfg.j2 does have these defined. However, user may chose to create a new copp_group COPP_GROUP|sflow-group and bind it to COPP_TRAP|sflow. Hence these fields are defined in yang and restricted to allowed patterns (psample, packets).

genetlink_name = genetlink_name ;[Optional] "psample" for sFlow genetlink_mcgrp_name = multicast group name; ;[Optional] "packets" for sFlow

@dgsudharsan, Are you suggesting not to expose these fields through yang ?

We do not expose these to user as these are internal implementations. I am not sure why an external user would be interested in specifying details about internal communication mechanisms. @prsunny Please let me your thoughts on this.

Agree, I think genetlink part can be removed from Yang and it is internal.

Removed leaf genetlink_name and leaf genetlink_mcgrp_name

arlakshm · 2021-04-08T18:05:30Z

src/sonic-yang-models/yang-models/sonic-copp.yang

+ "Control plane policing trap name";
+ }
+
+ leaf trap_ids {


should this mandatory field ?

arlakshm · 2021-04-08T18:07:41Z

src/sonic-yang-models/yang-models/sonic-copp.yang

+ "Red action";
+ }
+
+ leaf genetlink_name {


Why is this leaf needed ?

Response here

amaneti · 2021-04-27T06:36:40Z

retest this please

anshuv-mfst · 2021-05-19T23:18:14Z

@arlakshm , @prsunny - please initiate retest, thanks

anand-kumar-subramanian · 2021-06-10T18:36:11Z

retest please

arlakshm · 2021-06-10T20:27:13Z

@amaneti, can you resolve the conflicts

amaneti · 2021-06-15T16:44:13Z

@arlakshm - conflicts are resolved.

arlakshm · 2021-06-17T04:20:44Z

/azp run

azure-pipelines · 2021-06-17T04:21:02Z

Azure Pipelines successfully started running 1 pipeline(s).

amaneti · 2021-06-18T12:21:15Z

@arlakshm, @anshuv-mfst - rebased, build passed.
@dgsudharsan @prsunny @lguohan - please review.

prsunny · 2021-06-18T21:55:53Z

can you please resolve the conflict?

**- What I did** Created SONiC Yang model for COPP. Tables: COPP_GROUP, COPP_TRAP. **- How I did it** Defined Yang models for COPP based on Guideline doc: https://github.com/Azure/SONiC/blob/master/doc/mgmt/SONiC_YANG_Model_Guidelines.md and https://github.com/Azure/sonic-utilities/blob/master/doc/Command-Reference.md **- How to verify it** $ pytest tests/yang_model_tests/test_yang_model.py -v ============================================================================== test session starts =============================================================================== platform linux2 -- Python 2.7.13, pytest-3.0.6, py-1.4.32, pluggy-0.4.0 -- /usr/bin/python cachedir: .cache rootdir: /sonic/src/sonic-yang-models, inifile: plugins: cov-2.4.0 collected 1 items tests/yang_model_tests/test_yang_model.py::Test_yang_models::test_run_tests PASSED ============================================================================ 1 passed in 0.50 seconds ============================================================================

* defaults to fields: COPP_GROUP queue, trap_priority, color, cir, cbs, green_action, yellow_action, red_action * Mandatory fields: COPP_GROUP trap_action, meter_type, mode and COPP_TRAP|trap_ids * Add more tests

amaneti · 2021-06-20T14:44:19Z

rebased, resolved conflicts.
@dgsudharsan @prsunny @lguohan - please review.

amaneti · 2021-06-20T14:44:50Z

@arlakshm, @anshuv-mfst - rebased, build passed.

anshuv-mfst · 2021-06-29T16:38:49Z

@arlakshm - could you please help with merge, thanks.

* SONiC Yang model support for COPP * Tables: COPP_GROUP, COPP_TRAP.

amaneti requested a review from lguohan as a code owner March 31, 2021 07:38

lguohan added the YANG YANG model related changes label Mar 31, 2021

lguohan requested a review from prsunny March 31, 2021 19:19

amaneti force-pushed the copp_sonic_yang branch from c24745e to e1d95d3 Compare April 1, 2021 05:04

lguohan reviewed Apr 8, 2021

View reviewed changes

prsunny reviewed Apr 8, 2021

View reviewed changes

dgsudharsan reviewed Apr 9, 2021

View reviewed changes

arlakshm reviewed Apr 15, 2021

View reviewed changes

amaneti force-pushed the copp_sonic_yang branch from 5c0980c to 5e6fe39 Compare April 16, 2021 10:30

amaneti force-pushed the copp_sonic_yang branch from 42e1d0f to 442027b Compare April 26, 2021 09:06

dgsudharsan previously approved these changes May 6, 2021

View reviewed changes

prsunny previously approved these changes May 20, 2021

View reviewed changes

lguohan dismissed stale reviews from prsunny and dgsudharsan via 2fcbf84 June 9, 2021 21:12

amaneti force-pushed the copp_sonic_yang branch from 2fcbf84 to 5085ec7 Compare June 11, 2021 09:22

prsunny previously approved these changes Jun 16, 2021

View reviewed changes

dgsudharsan previously approved these changes Jun 17, 2021

View reviewed changes

amaneti dismissed stale reviews from dgsudharsan, prsunny, and arlakshm via af71268 June 18, 2021 05:26

amaneti force-pushed the copp_sonic_yang branch from 5085ec7 to af71268 Compare June 18, 2021 05:26

amaneti added 9 commits June 19, 2021 10:14

[sonic-copp.yang] Fix indentation

e85fa80

[yang] rebase and move copp tests into copp.json

58d6b7e

[yang] rebase test_yang_model.py

2b6ed1a

[yang] Move copp yang tests into copp.json

a5cf376

[yang] sonic-copp yang changes

00155a7

[yang] sonic-copp

f6cd482

* defaults to fields: COPP_GROUP queue, trap_priority, color, cir, cbs, green_action, yellow_action, red_action * Mandatory fields: COPP_GROUP trap_action, meter_type, mode and COPP_TRAP|trap_ids * Add more tests

[yang] Fix yang indentation

58dbba3

[yang] Remove leaf genetlink_name and leaf genetlink_mcgrp_name

481b5ce

amaneti force-pushed the copp_sonic_yang branch from af71268 to 481b5ce Compare June 19, 2021 17:16

arlakshm approved these changes Jun 24, 2021

View reviewed changes

prsunny merged commit 5acf234 into sonic-net:master Jun 29, 2021

carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021

SONiC Yang model support for COPP (sonic-net#7199)

db7a0d4

* SONiC Yang model support for COPP * Tables: COPP_GROUP, COPP_TRAP.

dgsudharsan mentioned this pull request Aug 8, 2024

[COPP:Yang-changes] Fix for config replace issues seen with COPP configurations #19817

Open

SONiC Yang model support for COPP #7199

SONiC Yang model support for COPP #7199

Conversation

amaneti commented Mar 31, 2021 • edited Loading

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)

lguohan commented Mar 31, 2021

prsunny commented Mar 31, 2021

amaneti commented Apr 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

amaneti Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaneti commented Apr 27, 2021

anshuv-mfst commented May 19, 2021

anand-kumar-subramanian commented Jun 10, 2021

arlakshm commented Jun 10, 2021

amaneti commented Jun 15, 2021

arlakshm commented Jun 17, 2021

azure-pipelines bot commented Jun 17, 2021

amaneti commented Jun 18, 2021

prsunny commented Jun 18, 2021

amaneti commented Jun 20, 2021

amaneti commented Jun 20, 2021

anshuv-mfst commented Jun 29, 2021

amaneti commented Mar 31, 2021 •

edited

Loading

prsunny Apr 8, 2021 •

edited

Loading

amaneti Apr 16, 2021 •

edited

Loading