Skip to content
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

Add gre and mpls containers under next-hops aft entry state. Add ttl and tos under gre, mpls and ip-in-ip aft entry state for telemetry. #879

Closed
wants to merge 7 commits into from

Conversation

romeyod
Copy link
Contributor

@romeyod romeyod commented May 25, 2023

  • (M) aft/openconfig-aft-common.yang
  • (M) aft/openconfig-aft-ethernet.yang
  • (M) aft/openconfig-aft-ipv4.yang
  • (M) aft/openconfig-aft-ipv6.yang
  • (M) aft/openconfig-aft-mpls.yang
  • (M) aft/openconfig-aft-pf.yang
  • (M) aft/openconfig-aft-state-synced.yang
  • (M) aft/openconfig-aft.yang

Change Scope

  • Add gre and mpls containers under next-hops aft entry state.
  • Add ttl and tos under gre, mpls and ip-in-ip aft entry state for telemetry.
  • This change is backwards compatible

Platform Implementations

Relevant pyang output:

module: openconfig-network-instance
  +--rw network-instances
     +--rw network-instance* [name]
        +--ro afts
        |  +--ro next-hops
        |     +--ro next-hop* [index]
        |        +--ro index            -> ../state/index
        |        +--ro state
        |        |  +--ro index?                       uint64
        |        |  +--ro programmed-index?            uint64
        |        |  +--ro ip-address?                  oc-inet:ip-address
        |        |  +--ro mac-address?                 oc-yang:mac-address
        |        |  +--ro pop-top-label?               boolean
        |        |  +--ro pushed-mpls-label-stack*     oc-mplst:mpls-label
        |        |  +--ro encapsulate-header?          oc-aftt:encapsulation-header-type
        |        |  +--ro decapsulate-header?          oc-aftt:encapsulation-header-type
        |        |  +--ro origin-protocol?             identityref
        |        |  +--ro lsp-name?                    string
        |        |  +--ro counters
        |        |  |  +--ro packets-forwarded?   oc-yang:counter64
        |        |  |  +--ro octets-forwarded?    oc-yang:counter64
        |        |  +--ro vni-label?                   oc-evpn-types:evi-id
        |        |  +--ro tunnel-src-ip-address?       oc-inet:ip-address
        |        |  +--ro oc-aftni:network-instance?   oc-ni:network-instance-ref
        |        +--ro ip-in-ip
        |        |  +--ro state
        |        |     +--ro src-ip?   oc-inet:ip-address
        |        |     +--ro dst-ip?   oc-inet:ip-address
        |        |     +--ro ttl?      uint32
        |        |     +--ro tos?      uint16
        |        +--ro gre
        |        |  +--ro state
        |        |     +--ro ttl?   uint32
        |        |     +--ro tos?   uint16
        |        +--ro mpls
        |        |  +--ro state
        |        |     +--ro ttl?   uint32
        |        |     +--ro tos?   uint16
        |        +--ro interface-ref
        |           +--ro state
        |              +--ro interface?      -> /oc-if:interfaces/interface/name
        |              +--ro subinterface?   -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index 

@romeyod romeyod requested a review from a team as a code owner May 25, 2023 20:52
@OpenConfigBot
Copy link

OpenConfigBot commented May 25, 2023

Major YANG version changes in commit a69ea22:

@OpenConfigBot
Copy link

OpenConfigBot commented May 25, 2023

Compatibility Report for commit a69ea22:
pyangbind@6b85c2b

Copy link
Contributor

@robshakir robshakir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these new fields intended for programming or solely telemetry? It'd be good to understand this.

added some questions in-line to clarify some of the new fields.

release/models/aft/openconfig-aft-common.yang Outdated Show resolved Hide resolved
release/models/aft/openconfig-aft-common.yang Outdated Show resolved Hide resolved
release/models/aft/openconfig-aft-common.yang Outdated Show resolved Hide resolved
@romeyod
Copy link
Contributor Author

romeyod commented May 26, 2023

are these new fields intended for programming or solely telemetry? It'd be good to understand this.

added some questions in-line to clarify some of the new fields.

Solely for a Telemetry based use case.

@romeyod romeyod changed the title Add ttl and tos in NH AFT entry state and Add next-hop-group-name in NHG AFT entry state Add ttl and tos in NH AFT entry state and Add next-hop-group-name and auto-size in NHG AFT entry state Jul 13, 2023
@romeyod romeyod changed the title Add ttl and tos in NH AFT entry state and Add next-hop-group-name and auto-size in NHG AFT entry state Add gre and mpls containers under next-hops aft entry state. Add ttl and tos under gre, mpls and ip-in-ip aft entry state for telemetry. Aug 7, 2023
Copy link
Member

@dplore dplore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romeyod could you clarify which feature you are supporting?

Please include how one would configure this feature (using existing or new OC as required, or if not supported in OC, the relevant vendor CLI ) to demonstrate when these AFT fields would be populated.

@romeyod
Copy link
Contributor Author

romeyod commented Aug 18, 2023

@romeyod could you clarify which feature you are supporting?

Please include how one would configure this feature (using existing or new OC as required, or if not supported in OC, the relevant vendor CLI ) to demonstrate when these AFT fields would be populated.

@dplore We want to support next-hop-group state and next-hop content streaming for telemetry. For full view of the encapsulation header, streaming TTL and TOS field along with the already existing AFT state.

I did not see any existing OC configs for this. Example: https://openconfig.net/projects/models/schemadocs/yangdoc/openconfig-aft.html#mod-openconfig-aft

Arista documentation for the CLI

Example CLI config snippet:

switch(config)# nexthop-group NH-1
switch(config-nexthop-group-NH-1)# size 4
switch(config-nexthop-group-NH-1)# entry 0 tunnel-destination 10.13.4.4
switch(config-nexthop-group-NH-1)# entry 1 tunnel-destination 10.15.4.22
switch(config-nexthop-group-NH-1)# entry 2 tunnel-destination 10.15.5.37
switch(config-nexthop-group-NH-1)# show active
 nexthop-group NH-1
   size 4
   ttl 64
   entry 0 tunnel-destination 10.13.4.4
   entry 1 tunnel-destination 10.15.4.22
   entry 2 tunnel-destination 10.15.5.37
switch(config-nexthop-group-NH-1)#

Let me know if I didnt understand your query correctly. Thanks

release/models/aft/openconfig-aft-common.yang Outdated Show resolved Hide resolved
release/models/aft/openconfig-aft-common.yang Outdated Show resolved Hide resolved

container mpls {
description
"When specified, the packet has an MPLS header applied to it before

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If multiple labels are getting added and each label has its own tos/ttl values then what should be the values?
Should these mpls specific leaves tos/ttl kept under "pushed-mpls-label-stack"?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romeyod any comments on this suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed following up on this, let me get back to you about this. [ my understanding is this TTL/TOS is for the next-hop and need to clarify what it means wrt mpls labels ]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krishna-juniper Thanks for the suggestion

pushed-mpls-label-stack is of type oc-mplst:mpls-label which is also used in other models. Example: popped-mpls-label-stack, start-label-value more occurences

Instead of adding ttl/tos to oc-mplst:mpls-label and affecting unrelated models, we can do the following:
define a typedef encap-outer-header-state with ttl and tos
and
in place of mpls container, under next-hop/state at the same level as pushed-mpls-label-stack we define

leaf-list pushed-mpls-label-header-state {
    type encap-outer-header-state;
}

This new leaf-list pushed-mpls-label-header-state will have ordered entries with tos/ttl corresponding to pushed-mpls-label-stack

Let me know what you think about this proposal.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach assumes implicit correlation between labels and ttl/tos. (labels coming from pushed-mpls-label-stack, ttl/tos coming from pushed-mpls-label-header-state).
By adding label stack as part of new container(container for encap-outer-header-state + pushed labels) we can avoid this implicit correlation at the cost of redundant information.
what are your thoughts on this approach?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this approach supported on any system? Please can we provide multi-vendor examples if we are going to make such a change?

Note that changes around how the MPLS label stack is being pushed have implications in gRIBI and are backwards compatible. I'd like to make sure we're supporting a use case that is actually supported here, since it also has overhead for those that don't care about this.

I also don't really like this typedef being a single leaf -- i think we rather need to think about how this is actually user friendly to parse.

Copy link
Contributor Author

@romeyod romeyod Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you @robshakir. We should not be make existing pushed-mpls-label-stack backward incompatible in anyway. Hence I proposed a new leaf/leaf list pushed-mpls-label-header-state that focuses only on the ttl/tos use case.

I also don't really like this typedef being a single leaf -- i think we rather need to think about how this is actually user friendly to parse.
I don't completely understand this comment. The new typedef encap-outer-header-state will have ttl and tos leafs.

I do think we need to work together and come up with a more user friendly layout. Will appreciate any inputs or existing precedence references.

* (M) aft/openconfig-aft-common.yang
* (M) aft/openconfig-aft-ethernet.yang
* (M) aft/openconfig-aft-ipv4.yang
* (M) aft/openconfig-aft-ipv6.yang
* (M) aft/openconfig-aft-mpls.yang
* (M) aft/openconfig-aft-pf.yang
* (M) aft/openconfig-aft-state-synced.yang
* (M) aft/openconfig-aft.yang
  - Add next-hop-group-name in NHG AFT entry state
  - Add ttl and tos in NH AFT entry state
…and tos under gre, mpls and ip-in-ip aft entry state. Remove nexthop-group-name and autosiz changes from this PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants