Update the description of ip-ttl and dscp - #1511
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the descriptions for the dscp and ip-ttl leaves in openconfig-aft-common.yang to clarify encapsulation behavior. The review feedback correctly identifies a syntax error in the dscp description caused by a curly double quote, as well as capitalization and spacing inconsistencies regarding 'Hop Limit' in the ip-ttl description.
1deda9e to
7825e90
Compare
7825e90 to
64a0e50
Compare
|
OC operator review July 14th. Thanks for your contribution. Can you add references to this in the PR description? Do we have an RFC that specifics this or 2 vendor implementations that support this behavior? |
| in the outer header during packet encapsulation. When this leaf is | ||
| not set, the TTL value of the inner packet is copied over as the | ||
| outer packet's IP TTL value during encapsulation."; | ||
| not set, the IP TTL / Hop Limit value of the inner packet is copied over as the |
There was a problem hiding this comment.
HI @tasaleh ,
if your intent is towards v6 version of IP ttl , can we add it as a different field so that it is consistent with v6 terminology ?
There was a problem hiding this comment.
Hello @navaneethyv,
We have raised this point in this PR 1492. The response was to keep using ip-ttl with V6. Please see the comments from @earies and @ ElodinLaarz
This is the reason why we propose this description change to capture the new usage.
There was a problem hiding this comment.
I would prefer this to be consistent to the rfc8200 for IPv6. If it is "Hop Limit" for IPv6, I feel the OC should match.
There was a problem hiding this comment.
However, just saw that #1492 is merged now after multiple discussions with the following reasoning and looks fine to me.
Summary: The deviation is purely a YANG modeling compromise. In pull/1492, the slightly inaccurate naming (ip-ttl instead of hop-limit for IPv6) was accepted to maximize code reuse by importing an existing module's structural grouping. Doing something different for IPv6 will add additional complexities without much benefit
| description | ||
| "DSCP value to use for the UDP header of the encapsulated | ||
| packet."; | ||
| "This leaf reflects the configured/default DSCP value that is used |
There was a problem hiding this comment.
This makes some assumptions about the behavior of DSCP value in this leaf. Can you please add 2 supporting implementations for this ?
|
OC review 28th. Added a few comments, + requesting for supporting implementations from 2 vendors. Can you please sign this to ensure the check passes [ this is a presubmit check] |
3 participants
Update the description of the following leaves:
ip-ttl: Since this leave models both TTL (IPv4) and Hop Limit (IPv6) values, then, we need to indicate this explicitly in the leaf's description.dscp: Update the description of the behavior when the leaf is not configured. When the leaf is not configured, the value of the inner packet will be copied over as the outer packet's value during encapsulation.