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

Augmented attributes interface-ref and metric fields to mpls-static lsp #1021

Merged
merged 4 commits into from
Apr 17, 2024

Conversation

Shashank-arista
Copy link
Contributor

@Shashank-arista Shashank-arista commented Dec 18, 2023

  • (M) release/models/mpls/openconfig-mpls-static.yang
  • (M) release/models/mpls/openconfig-mpls-types.yang
  • (M) release/models/mpls/openconfig-mpls.yang
  • (M) release/models/mpls/openconfig-mpls-igp.yang
  • (M) release/models/mpls/openconfig-mpls-te.yang

Change Scope

  • Augmented with below attributes interface-ref, and metric in the path /network-instances/network-instance/mpls/lsps/static-lsps/static-lsp
  +--rw network-instances
     +--rw network-instance [name]
        +--rw mpls
           +--rw lsps
              +--rw static-lsps
                 +--rw static-lsp [name]
                    +--rw ingress
                    |  +--rw config
                    |  |  +--rw interface?        -> /oc-if:interfaces/interface/name
                    |  |  +--rw subinterface?     -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index
                    |  |  +--rw metric?           uint8
                    |  +--ro state
                    |     +--ro interface?        -> /oc-if:interfaces/interface/name
                    |     +--ro subinterface?     -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index
                    |     +--ro metric?           uint8
                    +--rw transit
                    |  +--rw config
                    |  |  +--rw interface?        -> /oc-if:interfaces/interface/name
                    |  |  +--rw subinterface?     -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index
                    |  |  +--rw metric?           uint8
                    |  +--ro state
                    |     +--ro interface?        -> /oc-if:interfaces/interface/name
                    |     +--ro subinterface?     -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index
                    |     +--ro metric?           uint8
                    +--rw egress
                       +--rw config
                       |  +--rw interface?        -> /oc-if:interfaces/interface/name
                       |  +--rw subinterface?     -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index
                       |  +--rw metric?           uint8
                       +--ro state
                          +--ro interface?        -> /oc-if:interfaces/interface/name
                          +--ro subinterface?     -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index
                          +--ro metric?           uint8 

Platform Implementations

@OpenConfigBot
Copy link

OpenConfigBot commented Dec 18, 2023

No major YANG version changes in commit e6ab4f0

@Shashank-arista Shashank-arista force-pushed the mplsStatic branch 2 times, most recently from c5f7fea to 7ae0875 Compare December 19, 2023 07:33
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.

Please add some additional description where pointed out. I will help with an additional implementation reference.

release/models/mpls/openconfig-mpls-types.yang Outdated Show resolved Hide resolved
release/models/mpls/openconfig-mpls-static.yang Outdated Show resolved Hide resolved
release/models/mpls/openconfig-mpls-static.yang Outdated Show resolved Hide resolved
@dplore
Copy link
Member

dplore commented Feb 15, 2024

JunOS static-label-switched-path supports options for setting the next hop as an interface, and setting metric. There is no support for payload-type.

Cisco XROS static lsp configuration supports interface as next hop. It does not appear to support metric or payload-type.

So based on three references (EOS as submitted and XROS and JunOS), it seems we have at least two which support metric and nexthop, but only Arista has a concept of 'payload-type'.

Why is payload-type needed?

@dplore dplore self-assigned this Feb 15, 2024
@dplore
Copy link
Member

dplore commented Mar 19, 2024

@Shashank-arista ping for any comments, especially regarding payload type.

@dplore
Copy link
Member

dplore commented Mar 22, 2024

/gcbrun

@Shashank-arista
Copy link
Contributor Author

@Shashank-arista ping for any comments, especially regarding payload type.

Hi @dplore, Sorry for the delay in getting back on this. I tried to investigate more on payload-type requirement. Arista CLI needs a payload-type mandatorily, although today most of hardware platforms depend on auto-decide for inferring it. But still, there is a use case where payload-type would be necessary/used on a certain platform where we would need to skip applying egress ACLs for certain labels which relies on payload-type for the functionality.

[no] mpls static top-label [ intf ] { { swap-label } | { pop [ payload-type { ipv4 | ipv6 } [ access-list bypass ] ] } } [ metric ]

So, it seems payload-type is something not a mandatory attribute but would still be required in certain specific scenario.

@wenovus
Copy link
Contributor

wenovus commented Apr 3, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Apr 5, 2024

@Shashank-arista ping for any comments, especially regarding payload type.

Hi @dplore, Sorry for the delay in getting back on this. I tried to investigate more on payload-type requirement. Arista CLI needs a payload-type mandatorily, although today most of hardware platforms depend on auto-decide for inferring it. But still, there is a use case where payload-type would be necessary/used on a certain platform where we would need to skip applying egress ACLs for certain labels which relies on payload-type for the functionality.

[no] mpls static top-label [ intf ] { { swap-label } | { pop [ payload-type { ipv4 | ipv6 } [ access-list bypass ] ] } } [ metric ]

So, it seems payload-type is something not a mandatory attribute but would still be required in certain specific scenario.

Ok. In this scenario since we haven't identified at least 2 vendors that support payload type, nor the intent to add a payload type, nor specific operational use cases I suggest the payload type be removed from the OC model. It could added via an Arista specific yang augment.

@Shashank-arista
Copy link
Contributor Author

@Shashank-arista ping for any comments, especially regarding payload type.

Hi @dplore, Sorry for the delay in getting back on this. I tried to investigate more on payload-type requirement. Arista CLI needs a payload-type mandatorily, although today most of hardware platforms depend on auto-decide for inferring it. But still, there is a use case where payload-type would be necessary/used on a certain platform where we would need to skip applying egress ACLs for certain labels which relies on payload-type for the functionality.
[no] mpls static top-label [ intf ] { { swap-label } | { pop [ payload-type { ipv4 | ipv6 } [ access-list bypass ] ] } } [ metric ]
So, it seems payload-type is something not a mandatory attribute but would still be required in certain specific scenario.

Ok. In this scenario since we haven't identified at least 2 vendors that support payload type, nor the intent to add a payload type, nor specific operational use cases I suggest the payload type be removed from the OC model. It could added via an Arista specific yang augment.

Sure, @dplore. I have updated the pull request by removing the payload type related changes from OC model. Please review, thanks.

@dplore
Copy link
Member

dplore commented Apr 10, 2024

/gcbrun

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.

This LGTM. Last call for comments! I plan to merge this on April 17th, 2024.

@dplore dplore changed the title Augmented attributes interface-ref, payload-type and metric fields to mpls-static lsp Augmented attributes interface-ref and metric fields to mpls-static lsp Apr 10, 2024
@dplore dplore merged commit 7e546de into openconfig:master Apr 17, 2024
14 checks passed
dplore pushed a commit that referenced this pull request Apr 25, 2024
…sp (#1021)

* Add interface-ref and metric fields to mpls-static lsp
@Shashank-arista Shashank-arista deleted the mplsStatic branch June 19, 2024 10:26
romeyod pushed a commit to romeyod/aftNextHop that referenced this pull request Sep 19, 2024
…sp (openconfig#1021)

* Add interface-ref and metric fields to mpls-static lsp
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