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 next-hop-group-name in NHG AFT entry state #966

Merged
merged 15 commits into from
Oct 27, 2023

Conversation

romeyod
Copy link
Contributor

@romeyod romeyod commented Sep 26, 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 next-hop-group-name in NHG AFT entry state
  • 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-hop-groups
        |  |  +--ro next-hop-group* [id]
        |  |     +--ro id             -> ../state/id
        |  |     +--ro state
        |  |     |  +--ro id?                      uint64
        |  |     |  +--ro next-hop-group-name?     string
        |  |     |  +--ro programmed-id?           uint64
        |  |     |  +--ro color?                   uint64
        |  |     |  +--ro backup-next-hop-group?   -> ../../../next-hop-group/state/id
        |  |     +--ro next-hops
        |  |     |  +--ro next-hop* [index]
        |  |     |     +--ro index    -> ../state/index
        |  |     |     +--ro state
        |  |     |        +--ro index?    -> ../../../../../../next-hops/next-hop/state/index
        |  |     |        +--ro weight?   uint64
        |  |     +--ro conditional
        |  |        +--ro condition* [id]
        |  |           +--ro id                  -> ../state/id
        |  |           +--ro state
        |  |           |  +--ro id?               uint64
        |  |           |  +--ro dscp*             oc-inet:dscp
        |  |           |  +--ro next-hop-group?   -> ../../../../../next-hop-group/state/id
        |  |           +--ro input-interfaces
        |  |              +--ro input-interface* [id]
        |  |                 +--ro id       -> ../state/id
        |  |                 +--ro state
        |  |                    +--ro id?             string
        |  |                    +--ro interface?      -> /oc-if:interfaces/interface/name
        |  |                    +--ro subinterface?   -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index

romeyod and others added 10 commits May 25, 2023 11:01
* (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.
@romeyod romeyod requested a review from a team as a code owner September 26, 2023 16:18
@OpenConfigBot
Copy link

OpenConfigBot commented Sep 26, 2023

Major YANG version changes in commit 8ed9df3:

@dplore
Copy link
Member

dplore commented Oct 17, 2023

@romeyod thanks for this. Is there an expectation that an operator can configure the next-hop-group-name? I don't see a leaf for configuration in the OC models already for this. If not configured, how is the name generated? Does the name have any other function or is it merely informational? Can you provide an operational use case why someone or something would want to use a name for the next-hop-group?

@dplore dplore self-assigned this Oct 17, 2023
@sulrich
Copy link
Contributor

sulrich commented Oct 19, 2023

@dplore -

a few misc. asides on this topic.

  • we see immediate utility for at least streaming out the next-hop-group name for implementations that support this. this may or may not be a mandatory attribute for the NHG depending on the implementation. but it's largely informational in the context of the AFT. it may be configured via other mechanisms.
  • as this is a leaf addition to the AFT we're never really configuring anything. (there aren't any config containers in the /network-instances/.../afts/... hierarchy.)

existing examples of NHG name specification:

  • nokia
  • some impementations might have named static LSPs which are effectively NHGs,
    example.
  • junos
  • FRR

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.

Thanks for breaking this apart -- I think that with the documentation links that @sulrich shared, and the clarification that this is optional this seems reasonable to include.

As and when gRIBI adopts an updated AFT model, it will be deviated out there such that it is not configurable via the gRIBI API.

@dplore - was there other feedback here that should be considered pre-merge?

@dplore
Copy link
Member

dplore commented Oct 24, 2023

@dplore - was there other feedback here that should be considered pre-merge?

No other feedback received to date. Agreed that the references do show naming next-hop-group is common. I do think it's ok to add only the AFT state here. Config of next-hop-group with naming (whether static or via other means could be in a separate PR when someone has the use case. It doesn't need to block this PR for AFT.

@dplore
Copy link
Member

dplore commented Oct 24, 2023

/gcbrun

@wenovus
Copy link
Contributor

wenovus commented Oct 25, 2023

/gcbrun

@wenovus
Copy link
Contributor

wenovus commented Oct 25, 2023

It's possible this PR's CI is failing because it's based on a very old version of openconfig/public: romeyod/aftNextHop@nhg-Name...openconfig:public:master

I recommend merging into the latest branch -- I suspect that might fix this issue if my invocation run did not.

@dplore
Copy link
Member

dplore commented Oct 25, 2023

@romeyod please refresh / merge your pull request with HEAD from openconfig:master and this should fix the CI checks.

@romeyod
Copy link
Contributor Author

romeyod commented Oct 27, 2023

@dplore I have done the refresh now. Let me know if that was correct. Thanks

@dplore
Copy link
Member

dplore commented Oct 27, 2023

@dplore I have done the refresh now. Let me know if that was correct. Thanks

Looks great, thank you.

@dplore dplore merged commit 0426774 into openconfig:master Oct 27, 2023
13 checks passed
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.

6 participants