Open-Network-Models-and-Interfaces-ONMI / TAPI

LF ONMI Transport API Repository (TAPI)
https://github.com/Open-Network-Models-and-Interfaces-ONMI/TAPI/wiki
Apache License 2.0
95 stars 80 forks source link

Why is tapi-common:context a presence container? #501

Closed rgaglian closed 4 months ago

rgaglian commented 3 years ago

RFC7950 defines Presence containers as:

" YANG supports two styles of containers, those that exist only for organizing the hierarchy of data nodes and those whose presence in the data tree has an explicit meaning."

The context container is the root for the TAPI API and it is not really clear what is the "explicit meaning" of its presence. Making this container a presence container has an important impact on implementations.

container context {
    uses tapi-context;
    presence "Root container for all TAPI interaction";
    description "none";
}

Could we remove the presence requirement for this container in the root tree or explain the "explicit meaning" of its presence?

amazzini commented 3 years ago

By design it is intended to be always present, regardless existence of children. It is unclear which are the consequences on implementations, could you kindly provide more information?

rgaglian commented 3 years ago

Maybe the disconnect is that a presence container is not a container that will "always be present" but one which its presence needs to convert into a conditional statement from the northbound system.

For example, this is a valid output for the current TAPI API when the context container is not present: rogaglia@ROGAGLIA-M-71BU logs % curl -vvv -u admin:admin http://127.0.0.1:7080/restconf/data/tapi-common:context

Having a presence container so high in the tree has a performance impact as validating its presence means a call to the full config/state tree. We saw this performance impact during the recent OIF interop tests (where we had to deviate from it) where also some controllers did not allow a GET into the /restconf/data/tapi-common:context container to check its presence.

In another example, this is a valid call for the current implementation where the presence of the context container must be explicitness set if no child presence (contrary to the target design):

admin@ncs(config)# commit dry-run outformat native native { device { name optical-controller-0 data RESTCONF POST :: /restconf/data { "tapi-common:context":{ } } } }

amazzini commented 3 years ago

In TAPI we agreed that for elementary exchange the context instance must be present. The key requirement is that the context instance must be created by the TAPI server, not by the TAPI client. As far as filtering (depth/fields) is anyway necessary for implementation, it seems unclear why all the tree shall be retrieved to fulfil Restconf compliance.

rgaglian commented 3 years ago

I understand what you want to achieve but the "presence" sub-stamente does not achieve your goal (as demonstrated above) and causes real problems based on the IETF RFCs 7950 and 8040. Probably the real question is in which circumstances the TAPI server running/operations datastores would be complete empty (which is the case you are addressing) even without any leaf with default values, if this is realistic (I do not think so) and how you want to notify the client about its expecting behavior. Regarding your last comment, I am not aware how a client can "create" a container; I am familiar on how to create/update/delete leafs, lists, leaf-lists but not containers (with the exception when you add the presence sub-statement). So, maybe you are trying to address a non-problem. Why is this unnecessarily "presence" sub-statement so high in the chain a problem? A generic RESTCONF client, which is not hardcoded for TAPI, is not aware if there is additional logic associated with a give "presence", so it needs to validate this presence by performing a GET into the presence container. Unfortunately, many RESTCONF servers do not support the different "filter" options and therefore you end up with an large response. All in all, the issues we saw in the interop are the sum of some missing RESTCONF features in some servers plus presence container. So, we end up with more calls that needed and a performance penalty.

amazzini commented 3 years ago

The TAPI team agreed to remove the "presence" statement. So far a patch version of 2.1.3 is not planned, hence the modification will be available on next 2.3 major delivery.

amazzini commented 4 months ago

Presence statement has been removed from version 2.3.1