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 enabled leaf into isis multi-topology config #1207

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

xandrorel
Copy link

@xandrorel xandrorel commented Oct 23, 2024

Change Scope

This change adds a leaf node to enable isis multi-topology configuration. For some reason the enabled leaf node was not defined under config and some implementations use proprietary leaf nodes to enable isis multi-topology.

module: openconfig-network-instance
  +--rw network-instances
     +--rw network-instance* [name]
        +--rw protocols
           +--rw protocol* [identifier name]
              +--rw isis
                 +--rw global
                    +--rw afi-safi
                       +--rw af* [afi-name safi-name]
                          +--rw multi-topology
                             +--rw config
                             |  +--rw afi-name?    identityref
                             |  +--rw safi-name?   identityref
                             |  +--rw enabled?     boolean      <<< new
                             +--ro state
                                +--ro afi-name?    identityref
                                +--ro safi-name?   identityref
                                +--ro enabled?     boolean

Platform Implementations

Juniper ISIS multi-topology reference

[edit protocols isis]
user@R1# set topologies ipv6-unicast

Arista ISIS config guide

switch(config)# router isis 1
switch(config-router-isis)# address-family ipv6 unicast
switch(config-router-isis-af)# multi-topology

Cisco IOS XR config guide
In Cisco IOS XR isis multi-topology is enabled by default. It can be disabled by single-topology command.

  router isis isp
   net 49.0000.0000.0001.00
   address-family ipv6 unicast
    single-topology

@xandrorel xandrorel marked this pull request as ready for review October 23, 2024 20:21
@xandrorel xandrorel requested a review from a team as a code owner October 23, 2024 20:21
@dplore
Copy link
Member

dplore commented Oct 23, 2024

/gcbrun

@OpenConfigBot
Copy link

OpenConfigBot commented Oct 23, 2024

No major YANG version changes in commit 2a3e9e5

@dplore
Copy link
Member

dplore commented Oct 24, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Oct 25, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Oct 25, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Oct 29, 2024

Hi @xandrorel , We reviewed this in the Oct 29, 2024 OC operators meeting.

It seems the intent of the current OC model is configuring the ISIS afi/safi implies enabling single topology.

ie:
/network-instances/network-instance/protocols/protocol/isis/global/afi-safi/af/config/afi-name

If one configures the ISIS multitopology afi/safi, then multi-topology is implicitly enabled.
ie:
``/network-instances/network-instance/protocols/protocol/isis/global/afi-safi/af/multi-topology/config/afi-name`

This seems "roughly" aligned with the vendor CLI references provided. In each case, there is a configuration for the multitopology and the address family. If configured, it is in use.

Is there any operational use case to configure multi-topology and have it disabled? (as is the case for things like interfaces and bgp sessions).

@s19nal pointed out that the existing model might be more elegant if there was a boolean for multi-topology inside the container /network-instances/network-instance/protocols/protocol/isis/global/afi-safi/af/config. But I am not sure it's worth changing the model to add this since it doesn't create any new, needed functionality?

@nikhil2553
Copy link

Adding the enabled leaf will not add any new functionality but it will make the YANG tree consistent with the state container.

The current tree already looks weird with afi-name and safi-name leaf under the multi-topology container, these are redundant as this container is already under the afi-safi container which has these two leafs. We may not want to remove these leafs now but we should add the enabled leaf under it.

@dplore
Copy link
Member

dplore commented Nov 6, 2024

Quote reply

Ugh, you are correct, there is redundancy in this (git blame shows it goes all the way back to the original model 8 years ago).

I also see the point that the state leaf for enabled already exists which is inconsistent with the config container.

The problem with introducing this enabled leaf is that it is probably in effect, a breaking change. If a existing config doesn't specify this leaf and a device OS update is performed to a new OC release which has this leaf, what will the behavior be? It is ambiguous.

If we leave things as is, it seems clear that if

                          +--rw multi-topology
                             +--rw config
                             |  +--rw afi-name?    identityref
                             |  +--rw safi-name?   identityref

are not present, then multi-topology is not enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

5 participants