[DPB] Allow BGP_NEIGHBOR YANG model with just IP as the key #7992
Conversation
… in kernel Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Signed-off-by: Venkatesan Mahalingam <[email protected]>
Not sure what you mean here, can you elaborate? |
|
@praveen-li The two forms of Yang model are for two different bgp daemons is what my impression is. Where each daemon takes different configuration format. What I do know is that this yang model was originally written with a different configuration format in mind than what SONiC loads by default on master right now. I am working under the assumption that we need to support both formats depending on the @venkatmahalingam Its fine with me if this gets pushed as a hotfix to stabilize DPB on platforms not using the frr form of the DPB config, if its good with other maintainers. However, we do need a long-term solution for this yang model. |
Signed-off-by: Venkatesan Mahalingam <[email protected]>
| @@ -868,6 +868,11 @@ | |||
| }, | |||
| "BGP_NEIGHBOR": { | |||
| "default|192.168.1.1": { | |||
| "asn": "65200", | |||
| "holdtime": "180", | |||
There was a problem hiding this comment.
"BGP_NEIGHBOR": {
"default|192.168.1.1": {
"asn": "65200",
"holdtime": "180",
"keepalive": "60",
"local_addr": "10.0.0.2",
"name":"PEER2"
}
},
@praveen-li These fields are present in the grouping, looks like libyang is not able to parse it, what's your suggestion? shall we remove these fields and merge the PR to unblock DPB testing? or you can fix the infra to handle the fields from grouping as well?
Read JSON Section: SAMPLE_CONFIG_DB_JSON
sonic_yang(3):All Keys are not parsed in BGP_NEIGHBOR
dict_keys(['default|192.168.1.1'])
sonic_yang(3):exceptionList:['Value not found for neighbor in default|192.168.1.1', "'asn'"]
sonic_yang(3):Data Loading Failed:All Keys are not parsed in BGP_NEIGHBOR
dict_keys(['default|192.168.1.1'])
There was a problem hiding this comment.
@venkatmahalingam: Yeah, [current translation doesn't allow all the variation of leaf, only choice and leaf is allowed.] if you are using grouping, please add support at:
https://github.com/Azure/sonic-buildimage/blob/5acf2348ba88c69bad3300adabd58b270e5bc01f/src/sonic-yang-mgmt/sonic_yang_ext.py#L209
Also, about the other BGP_NEIGHBOR table
-- where the relevant config go, as I understand BGP_NEIGHBOR table in config_db is different.
-- which daemon processes it, does it make use of current YANG models.
-- please point to the code, which is using this BGP_NEIGHBOR yang model.
There was a problem hiding this comment.
New generic BGP config models are being processed by the below frrcfgd daemon in BGP container.
https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-frr-mgmt-framework/frrcfgd/frrcfgd.py
@praveen-li I'm not familiar with libyang infra code, can you please add support for grouping?
There was a problem hiding this comment.
@venkatmahalingam: can you please point to the function or relevant line in frrcfgd.py, where it is doing config validation against yang. Also from where it reads the config i.e. source of config?.
There was a problem hiding this comment.
@praveen-li There is no config validation against YANG in frrcfgd, frrcfgd simply listens for config-DB events and handles them, all configs validations are expected to handle in SONiC YANGs before pushing to config-DB.
| description "Keepalive interval"; | ||
| } | ||
|
|
||
| leaf local_addr { |
There was a problem hiding this comment.
Should we make sure that local address and Peer address belongs to same address family?
There was a problem hiding this comment.
I dont see any easy way to check this, do you see easy YANG check?
|
@praveen-li Can you look at my replies for your comments, if you are okay, can you approve this PR? |
|
@alexrallen If you don't have any more comments, please approve this PR. |
There was a problem hiding this comment.
This looks fine to me.
With the current state of this PR it will allow Yang to accept the bgpcfgd form of CONFIG_DB neighbor entries which will fix the root cause of #7959 so given that I would support merging it.
However....
- The frrcfgd form is still broken and will be until our Yang implementation is updated to accept the
groupingkeyword per @praveen-li comment. This is an outstanding bug. - We still need to intelligently differentiate between what form of BGP config the user is using via the DEVICE_METADATA table. This can be a future enhancement.
I will refactor issue #7959 to scope only for the bgpcfgd issue so it may be closed following merging this PR in recognition that the default BGP neighbor template in SONiC should now work and allow DPB to function.
However, I will also raise a new bug to ensure that the issues with the grouping keyword are tracked.
Let's discuss this in the next week's YANG meeting. |
|
@lguohan Please help to merge this PR. |
|
@anshuv-mfst @lguohan Please merge this PR. |
|
@anshuv-mfst Please help to merge this PR. |
|
@anshuv-mfst Is there anything that we are still waiting on this PR? If not can you please help merge this? |
|
@praveen-li I am not sure what is the criteria for merge. That's why I asked @anshuv-mfst. If we need some more approvals who should be reviewing it? |
|
Hi @shi-su - could you please review, approve and help with merge. Thanks! |
Why I did it
[DPB] BGP_NEIGHBOR yang model causing DPB errors.
How I did it
Allowed the BGP_NEIGHBOR list with just IP as the key to allow the current BGP neighbor table in the community.
How to verify it
Built the SONIC YANG models.
Which release branch to backport (provide reason below if selected)