[yang]: Add Yang models for BGP monitor#9752
Conversation
Signed-off-by: Gang Lv ganglv@microsoft.com
|
Tested on KVM env. BTW, I saw in production that |
| range "0..1"; | ||
| } | ||
| description "Route reflector client"; | ||
| } |
There was a problem hiding this comment.
I am not sure if nhopself and rrclient are required fields. Could you please confirm?
There was a problem hiding this comment.
Per bgpcfgd code, these are required fields.
|
@venkatmahalingam Could you help review? |
Sure, will do. |
| } | ||
|
|
||
| container sonic-bgp-monitor { | ||
| container BGP_MONITORS { |
There was a problem hiding this comment.
This table fields seem to be duplicate from the table BGP_NEIGHBOR, please clarify why do we need a separate table, also, please use the grouping to group the fields and use them in multiple places.
There was a problem hiding this comment.
This table fields seem to be duplicate from the table BGP_NEIGHBOR, please clarify why do we need a separate table
This is a good question, and I believe it is large than the PR trying to solve, ie, add the missing yang model for BGP_MONITORS table, which is existing for long,.
This PR will help minigraph parser unit test improvement, which validate each minigraph generated ConfigDB by SONiC Yang model.
This PR will also unblock the generic config updater end-to-end test.
We can create an issue that BGP_NEIGHBOR and BGP_MONITORS tables should be unified.
| } | ||
| } | ||
| }, | ||
|
|
There was a problem hiding this comment.
I think, we should add YANG for BGP_PEER_RANGE table as well, please check and create the PR for review.
Any insight for bgpcfgd issue? |
Signed-off-by: Gang Lv ganglv@microsoft.com
| "holdtime": "180", | ||
| "keepalive": "60", | ||
| "local_addr": "10.0.0.2", | ||
| "name":"BGPMonitor123", |
There was a problem hiding this comment.
Does this test case validate 'name' field? having the digit in the name is invalid?
There was a problem hiding this comment.
https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-config-engine/minigraph.py#L823
It seems that the name must be 'BGPMonitor'
Why I did it end2end test is blocked by Yang model for BGP monitor. How I did it Create new yang files for BGP monitor, and add UT. How to verify it Follow the steps in #9711. Run UT for sonic-yang-models. Signed-off-by: Gang Lv ganglv@microsoft.com
I think this is because the fields under BGP_MONITORS are all create-only. The patch steps are adding each field one by one, this is not correct, The field needs to be added when the parent is created, if they are to be updated The parent needs to be deleted and added back. related issue: sonic-net/sonic-utilities#2029 |
|
Hi Mohamed, I agree with you on the create-only conclusion. It is not an urgent need to fix as it can work now after I modify the patch. |
Signed-off-by: Gang Lv ganglv@microsoft.com
Why I did it
end2end test is blocked by Yang model for BGP monitor.
How I did it
Create new yang files for BGP monitor, and add UT.
How to verify it
Follow the steps in #9711.
Run UT for sonic-yang-models.
Which release branch to backport (provide reason below if selected)
Description for the changelog
Fix #9711
A picture of a cute animal (not mandatory but encouraged)