sonic-yang-models updates for MPLS#7881
Conversation
qbdwlr
force-pushed
the
yangMpls
branch
2 times, most recently
from
afcd489 to
cefc1ca
Compare
There was a problem hiding this comment.
Can you add Yang model tests for mpls enable/disable on RIFs? Most of tests are in sonic-yang-models/tests/yang_model_tests/tests
There was a problem hiding this comment.
@kperumalbfn I looked at the types of tests currently under sonic-yang-models. It looked like it was mostly parameter validation. Since this mpls knob has two fixed values, I wasn't sure what more needed to be checked for that. Do you have something specific in mind? Can you provide a pointer to an example?
There was a problem hiding this comment.
I think it will be good to have a test for one type of interface. SAI default value for SAI_ROUTER_INTERFACE_ATTR_ADMIN_MPLS_STATE is disabled, we can use that. There are examples for NAT_ZONE in sonic-yang-models/tests/yang_model_tests/tests/interface.json
There was a problem hiding this comment.
@kperumalbfn Type for mpls attribute has been changed from string to enum, which I believe should eliminate the need for valid/invalid attribute value tests. No explicit default should be defined in yang models due to lack of support for this atttribute in many existing platforms.
There was a problem hiding this comment.
Please add tests for MPLS CRM.
There was a problem hiding this comment.
@kperumalbfn I looked at the existing CRM tests in sonic-yang-models. It appears that there exist currently generic parameter checking that is applicable for all CRM types. I don't think there needs to be anything specific for MPLS, since it can already make use of the generic CRM tests.
There was a problem hiding this comment.
@kperumalbfn I have added CRM tests for the 2 MPLS CRM types as requested.
There was a problem hiding this comment.
Default value for all the interface types?
There was a problem hiding this comment.
@kperumalbfn I don't think we want to add a default value here, do we? What happens if the platform does not support this attribute? I think we do not want to explicitly set the attribute in that case.
There was a problem hiding this comment.
As discussed in the meeting, please define typedef for enable/disable and use it here.
There was a problem hiding this comment.
@venkatmahalingam There seems to be some inconsistency with regards to typedefs in sonic-yang-models. I see instances of enable/disable enum in sonic-device_metadata.yang and sonic-flex_counter.yang without a generic typedef being referenced. Also, I see that sonic-types.yang now requires a revision. I would suggest that a new revision of sonic-types.yang that incorporates a generic enable/disable typedef should be outside of the scope of this PR and done as a general cleanup of the yang_models to avoid churn on yang revisions.
There was a problem hiding this comment.
why can't we add typedef for enable/disable in the sonic-types.yang and use it in this YANG? we can correct typedef in the other modules later?
There was a problem hiding this comment.
@venkatmahalingam Presumably any addition to sonic-types.yang requires a new revision for that file. It makes more sense to bundle multiple changes to sonic-types.yang together to avoid revision churn. This should be done as general cleanup prior to new release and should not be included in the scope of this PR.
|
@qbdwlr can you pls share some update on the review comments handling? it is blocking dpb from being executed on master. |
|
This PR is needed to solve the DPB break in the current master. DPB is nonfunctional for more than two months now. |
|
@qbdwlr would you please help to address this PR? #7881 (comment) @caizhenghui-juniper please help to drive the PR merge. Thanks. |
|
@zhangyanzhao @liat-grozovik There is nothing preventing this PR from being merged in its current state. It may be merged now to resolve DPB issue. |
|
We will create a new PR to track the "why can't we add typedef for enable/disable in the sonic-types.yang and use it in this YANG? we can correct typedef in the other modules later?" issue. @venkatmahalingam please approve this PR and Guohan will merge. |
SONiC YANG model support in buildimage for MPLS: sonic-yang-model support for MPLS enable/disable sonic-yang-model support for MPLS CRM thresholds
8 participants
What I did
SONiC YANG model support in buildimage for MPLS:
Why I did it
SONiC yang-models support for MPLS
How I verified it
Unit-tests in sonic-swss/tests/test_mpls.py and sonic-utilities/tests
System tests in sonic-mgmt
Details if related
Refer to MPLS HLD sonic-net/SONiC#706