-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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-MGMT MDB extensions : OTN #1701
base: master
Are you sure you want to change the base?
Sonic-MGMT MDB extensions : OTN #1701
Conversation
@gandhiinfinera Join Zoom Meeting |
@kwangsuk Regards, |
@gandhiinfinera Join Zoom Meeting Please contact @sneelam20 to add your presentation to the agenda in the Sep/25 meeting. |
@sneelam20 The HLD has the changes to support for multiple Redis DB for each slot in a Chassis platform Related documentation: Attached a PPT for your reference: Please add it to the agenda in the Sep/25 meeting. |
Please add review of this HLD PR for Multi Redis DB in next meeting agenda. |
The gNMI support requires the gNMI server which is provided as a part of sonic-telemetry container. We would like to rename this container as the sonic-gnmi container as now it can perform configurations as well through the gNMI server. | ||
Will introduce a new container sonic-mgmt-common to host the common code that is used both in the mgmt-framework and sonic-telemetry container. This new repo will compile into static libraries that will be used in the other two repos. This way sonic-telemetry repo can be compiled without the mgmt-framework being present in the code base. | ||
The gNMI support requires the gNMI server which is provided as a part of sonic-gnmi container. | ||
A new container sonic-mgmt-common to host the common code that is used both in the mgmt-framework and sonic-gnmi container. This new repo will compile into static libraries that will be used in the other two repos. This way sonic-gnmi repo can be compiled without the mgmt-framework being present in the code base. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new container sonic-mgmt-common to host .....
Please change as "new repo".. Looks like a typo in the original draft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine will correct this
A new container sonic-mgmt-common to host .....
Please change as "new repo".. Looks like a typo in the original draft
@@ -1053,7 +1054,7 @@ definition files (both YANG generated and manual) along with link to open Swagge | |||
|
|||
##### 3.2.2.5 gNMI server | |||
|
|||
1. gNMI server is part of the telemetry process that supports telemtry as well as gNMI. | |||
1. gNMI server is part of the sonic-gnmi process that supports telemtry as well as gNMI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the process name is still "telemetry" -- https://github.com/sonic-net/sonic-buildimage/blob/master/dockers/docker-sonic-gnmi/gnmi-native.sh#L107C16-L107C25. Please double check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the process name is still "telemetry" -- https://github.com/sonic-net/sonic-buildimage/blob/master/dockers/docker-sonic-gnmi/gnmi-native.sh#L107C16-L107C25. Please double check
Will check and correct it
@@ -1823,6 +1825,7 @@ Transformer has a built-in default transformer method to perform static, simple | |||
|
|||
Additionally, for more complex translations of non-ABNF YANG models, i.e. OpenConfig models, Transformer also allows developers to overload the default method by specifying a callback fucntion in YANG extensions, to perform translations with developer-supplied translation codes as callback functions. Transformer dynamically invokes those functions instead of using default method. Each transformer callback must be defined to support two-way translation, i.e, YangToDb_<transformer_callback> and DbToYang_<transformer_callback>, which are invoked by Transformer core. | |||
|
|||
Transformer also has callback funtion in YANG extensions to get namespace associated with the uri path. This will be mainly useful in case of Multi ASIC platform with multiple Redis DBs. The namespace callback function will be specific to every YANG model defined with its annotations. Translib/transformer shall use "HOST" DB as default for YANG models without namespace extension defined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace callback function will be specific to every YANG model
Term "yang model" is ambiguous in this context.. Should this callback be attached per yang module (i.e, top level -- similar to "post transformer") or can it be attached at any subpath level? If latter, will descendants of that path also inherit this callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The namespaceFunc will be attached to subpath level and its descendants will inherit this CB
namespace callback function will be specific to every YANG model
Term "yang model" is ambiguous in this context.. Should this callback be attached per yang module (i.e, top level -- similar to "post transformer") or can it be attached at any subpath level? If latter, will descendants of that path also inherit this callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If descendants inherit the callback , what happens when a descendant list has a different table/key/Db annotation ? is there a way to terminate the inheritance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this callback is being inherited, will all the inherited callbacks be invoked or will the parent callback take care of returning the namespaces at the uri and its children? Is this call done before the translib transaction starts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If descendants inherit the callback , what happens when a descendant list has a different table/key/Db annotation ? is there a way to terminate the inheritance?
Parent and descendant will be always belong to a specific namespace, the descendant may have different table/key/DB which will be used to select a DB/Table from the selected namespace,
We dont see a need to terminate this inheritance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this callback is being inherited, will all the inherited callbacks be invoked or will the parent callback take care of returning the namespaces at the uri and its children? Is this call done before the translib transaction starts?
Please note by namespace we here mean the slot/ASIC based redisDB and not namespace keyword in yang model.
Parent callback will ensure namespaces for child paths. We dont see a need for defining a new namespace function for children
Namespace CB will be invoked before transactions starts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If descendants inherit the callback , what happens when a descendant list has a different table/key/Db annotation ? is there a way to terminate the inheritance?
Parent and descendant will be always belong to a specific namespace, the descendant may have different table/key/DB which will be used to select a DB/Table from the selected namespace, We dont see a need to terminate this inheritance.
For a use-case where a descendent yang node belongs to a different or default/Host namespace then inheritance can be terminated using a name-space annotation right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If descendants inherit the callback , what happens when a descendant list has a different table/key/Db annotation ? is there a way to terminate the inheritance?
Parent and descendant will be always belong to a specific namespace, the descendant may have different table/key/DB which will be used to select a DB/Table from the selected namespace, We dont see a need to terminate this inheritance.
For a use-case where a descendent yang node belongs to a different or default/Host namespace then inheritance can be terminated using a name-space annotation right?
The descendant namespace should be same as parent, name-space annotation termination is not a supported usecase
@@ -1922,7 +1925,87 @@ The table-transformer is inherited to all descendant nodes unless another one is | |||
11. `sonic-ext:get-validate [function]`: | |||
A special hook to validate YANG nodes, to populate data read from database, allows developers to instruct Transformer to choose a YANG node among multiple nodes, while constructing the response payload. | |||
Typically used to check the “when” condition to validate YANG node among multiple nodes to choose only valid nodes from sibling nodes. | |||
12. `sonic-ext:get-namespace [function]`: | |||
Multiple DB extension, populates associated namespaces with the YANG path | |||
Used by Translib service to get all DB's associated with the YANG path. In case of single DB, this extension shall not be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get all DB's associated with the YANG path
Can a path be associated with multiple namespaces at a time? If yes, how do you format the GET response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes a path without a key can be associated with multiple namespaces, in this case output will be aggregation of all responses.
get all DB's associated with the YANG path
Can a path be associated with multiple namespaces at a time? If yes, how do you format the GET response?
deviation /oc-opt-amp:optical-amplifier/oc-opt-amp:amplifiers/oc-opt-amp:amplifier { | ||
deviate add { | ||
sonic-ext:key-transformer "oc_name_key_xfmr"; | ||
sonic-ext:get-namespace "oc_name_get_namespace_xfmr"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example is be confusing.. You are attaching ket-transformer and get-namespace without specifying db-name and table-name. Does it mean the get-namespace can be a replacement for db-name/table-name??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will correct the example, get-namespace will not replace the db-name/table-name
This example is be confusing.. You are attaching ket-transformer and get-namespace without specifying db-name and table-name. Does it mean the get-namespace can be a replacement for db-name/table-name??
22. Status being returned from DB access layer. | ||
23. Translib then invokes the processWrite API on the App module. | ||
24. App modules perform writes of the translated data to the DB access layer. | ||
25. DB access layer validates the writes using CVL and then caches them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you pass the namespace info to CVL?
CVL can also open new connections to StateDB, ApplDB etc if the config depends on some state info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Namespace details will be available at the DB instance passed, will investigate the cases for new connections to StateDBapplDB in CVL case and updated sequence accordingly
How do you pass the namespace info to CVL? CVL can also open new connections to StateDB, ApplDB etc if the config depends on some state info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So write/set to a non-ConfigDb is going to be allowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So write/set to a non-ConfigDb is going to be allowed?
No, the proposed change is to extend selection of RedisDB instance. However existing CVL,WRITE validation remain same
16. App module returns after initializing local data | ||
17. App module calls Transformer function to translate the request from cached YGOT structures into Redis ABNF format. It also gets all the keys that will be affected as part of this request. | ||
18. App module returns the list of keys that it wants to keep a watch on along with the status. | ||
19. Translib infra invokes the start transaction request exposed by the DB access layer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translib also needs validate whether all keys belong to the same namespace. gNMI Set request can include multiple operations, each belonging to different yang modules. Transactional write cannot be ensured if op1 maps to global namespace and op2 maps to a specific namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SET sequence will be updated to support rollback of transactional write when any of the subsequent operation in set fails
Translib also needs validate whether all keys belong to the same namespace. gNMI Set request can include multiple operations, each belonging to different yang modules. Transactional write cannot be ensured if op1 maps to global namespace and op2 maps to a specific namespace.
19. App module fills the YGOT structure with the data from the Redis DB and validated the filled YGOT structure for the syntax. | ||
20. App module converts the YGOT structures to JSON format. | ||
21. IETF JSON payload is returned to the Translib infra. | ||
22. Translib infra aggregates all the responses from app module with different namesapce DB associated with uri path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the RESTCONF and gNMI specs. Cannot aggregate the values data if the path points to a specific leaf.
For example -- what will be the ourtput of GET /interfaces/interface[name=Ethernet0]/state/mtu if this path was mapped to multiple namespaces? Client is supposed to receive only one value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above explanation is generic handling, in case of path with specific keys the namespace will return unique namespace only and hence it will be single response.
This breaks the RESTCONF and gNMI specs. Cannot aggregate the values data if the path points to a specific leaf.
For example -- what will be the ourtput of GET /interfaces/interface[name=Ethernet0]/state/mtu if this path was mapped to multiple namespaces? Client is supposed to receive only one value.
22. If it is successful, control goes to next step, else error is returned to DB access layer. The next step is to ensure that keys are present in Redis DB for Update/Delete operation. But keys should not be present for Create operation. | ||
23. Status is returned after checking keys. | ||
24. CVL gets dependent data from incoming Redis payload. For example if ACL_TABLE and ACL_RULE is getting created in a single request. | ||
25. Otherwise dependent should be present in Redis DB, query is sent to Redis to fetch it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How this step will receive the namespace info?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The namespace information is used in Step-10, where Transformer will get DB from the associated namespace.
Here in step 25, the validation remain specific to the DB alone
How this step will receive the namespace info?
24. CVL gets dependent data from incoming Redis payload. For example if ACL_TABLE and ACL_RULE is getting created in a single request. | ||
25. Otherwise dependent should be present in Redis DB, query is sent to Redis to fetch it. | ||
26. Redis returns response to the query. | ||
27. Finally request data and dependent is merged and validateSemantics() is called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can different namespaces have same table names in their DBs? If yes, there may be a need for namespace specific sonic yangs. Are you assuming all the validations (must expressions, enums, custom validations etc) be same across all namespaces??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, different namespaces shall have the same table names in their DB. The proposed change is to replicate the current Redis DBs into multiple namespaces
all the validations (must expressions, enums, custom validations etc) be same across all namespaces
Can different namespaces have same table names in their DBs? If yes, there may be a need for namespace specific sonic yangs. Are you assuming all the validations (must expressions, enums, custom validations etc) be same across all namespaces??
..... | ||
deviation /oc-opt-amp:optical-amplifier/oc-opt-amp:amplifiers/oc-opt-amp:amplifier/oc-opt-amp:state { | ||
deviate add { | ||
sonic-ext:db-name "STATE_DB"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example has DB-name annotation , what is the role of namespace in such cases. Does DB-name override the DBs returned from namespace callback or vice-versa? I guess both DB-name annotation and namespace callback shouldn't co-exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example has DB-name annotation , what is the role of namespace in such cases. Does DB-name override the DBs returned from namespace callback or vice-versa? I guess both DB-name annotation and namespace callback shouldn't co-exist
The HLD is to support multiple RedisDB instances in case of a chassis, where separate RedisDB instance shall be present for each slot. Namespace call back will be used to select the RedisDB instance. Each RedisDB instace will contain all existing SONIC DBs like Config_DB, State_DB etc..,
Now in this case we need two level of selection
- Namespace : like ASIC0, ASIC1
Newly introduced Namespace CB shall be used to select the RedisDB instance - DB/Table name: Config_DB (TableA)
Existing Annotation will be continued to select the DB/Table in the selected RedisDB instance
Newly introduced Namespace annotation will co-exist with the existing DB-name, Table annotation
sonic-ext:db-name "STATE_DB"; | ||
sonic-ext:table-name "OSC"; | ||
sonic-ext:key-transformer "osc_key_xfmr"; | ||
sonic-ext:get-namespace "oc_name_get_namespace_xfmr"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If name-space callback is going to be inherited why is the child yang node again annotated with name-space callback and the same one as that of the parent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If name-space callback is going to be inherited why is the child yang node again annotated with name-space callback and the same one as that of the parent?
Yes, will remove the name-space annotation for child node
// If Key is present in the xpath add the associated namespace and return | ||
if ockey != "" { | ||
//db.GetMDBNameFromEntity converts the slot number in the key to namespace | ||
dbName := db.GetMDBNameFromEntity(ockey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the context of the NB/request URI and the Redis DB table this list maps to being identified in the generic function, GetMDBNameFromEntity? There can be multiple oc lists with same key name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the context of the NB/request URI and the Redis DB table this list maps to being identified in the generic function, GetMDBNameFromEntity? There can be multiple oc lists with same key name.
GetMDBNameFromEntity is a generic function operating on the string passed as input, which will determine the Namespace. Yang specific namespace annotation will have the logic to get the slot specific string from table key and pass it to GetMDBNameFromEntity.
31. REST Status returned from the Request handler. | ||
32. REST response is sent by the REST gateway to the REST client. | ||
8. Translib infra invoke App module getNamespace API to get namespace associated with the uri | ||
9. App module invokes transformer service getNamespaceMapping to retrieve namespace based on Yang model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getNamespaceMapping returns an array of namespaces only mapped at the request URI only? or does it also return the namespaces of all its children?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getNamespaceMapping returns an array of namespaces only mapped at the request URI only? or does it also return the namespaces of all its children?
Namespace will be based on request URI and children shall use the same namespace
7. App module queries Transformer to get all namespace associated with the uri path | ||
8. Transformer shall use Yang specific "Transformer Spec" to get all namespace associated with uri path | ||
9. Transformer return namespace associated with the uri path | ||
10. App module return namespace retrived by the transformer layer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If name-space can be annotated to any yang-node/subpath in a given yang-module and the associated namespace(s) is retrieved based on the request URL target ,at the very beginning in translib layer before the processing of the query is done in transformer layer what happens in the following case?
Yang-module openconfig-x has following structure with list node-C annotated with name-space and the REST GET request URL is at list node-A(openconfig-x/container-As/list-A) that has no namespace associated with it and is parent of list node-C :
openconfig-x
container-As
list-A
...
...
container-Cs (child of list-A)
list-C name-space-annotation
As per the points mentioned in this section only the default namespace will be returned since the URL is at list-A. Transformer infra processing/traversal of the child yang hierarchy happens much later in processGet(point 16.) and the node annotated with name-space will be known/reached much later. How will this be handled? The name-space seems to rely only on URL/reuest target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If name-space can be annotated to any yang-node/subpath in a given yang-module and the associated namespace(s) is retrieved based on the request URL target ,at the very beginning in translib layer before the processing of the query is done in transformer layer what happens in the following case?
Yang-module openconfig-x has following structure with list node-C annotated with name-space and the REST GET request URL is at list node-A(openconfig-x/container-As/list-A) that has no namespace associated with it and is parent of list node-C : openconfig-x container-As list-A ... ... container-Cs (child of list-A) list-C name-space-annotation
As per the points mentioned in this section only the default namespace will be returned since the URL is at list-A. Transformer infra processing/traversal of the child yang hierarchy happens much later in processGet(point 16.) and the node annotated with name-space will be known/reached much later. How will this be handled? The name-space seems to rely only on URL/reuest target.
The mentioned example is a bug, as we should defile namespace annotation at parent level i,e list-A
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This defies your comment - #1701 (comment)
that namespace annotation can be at yang subpath level. So in effect namespace has to be annotated at top level node yang module hence its per yang module similar to pre/post-xfmrt.
Then are you saying there cannot be a use-case where list-A belongs to default/Host name space and list-C belongs to a different namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This defies your comment - #1701 (comment) that namespace annotation can be at yang subpath level. So in effect namespace has to be annotated at top level node yang module hence its per yang module similar to pre/post-xfmrt. Then are you saying there cannot be a use-case where list-A belongs to default/Host name space and list-C belongs to a different namespace?
Updated the misleading comment, yes following usecase is not supported
"Parent list-A belongs to default/Host name space and its child list-C belongs to a different namespace"
10. Transformer invokes Yang model based exception to retrieve the namespace associated with uri | ||
11. Transformer returns namespace associated with th uri | ||
12. Appmodule returns namespace | ||
13. Translib infra gets all DBs and selects DB for the namespace associated with the uri |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If name-space can be annotated to any yang-node/subpath for a given yang-module and the namepspace selection is based just on URL what happens if a yang node present in payload has a namespace annotated but the target URL yang node has no annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If name-space can be annotated to any yang-node/subpath for a given yang-module and the namepspace selection is based just on URL what happens if a yang node present in payload has a namespace annotated but the target URL yang node has no annotation?
With the new approach all OpenConfig yang models should have Namespace annotation, however if a Yang model does not have namespace annotation a default namespace function will be used for it, which will redirect to "host" RedisDB instance, in the above mentioned case namespace should be selected based on payload
SONiC Mgmt support for Multiple DB extensions