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

enabling support for multiple protocol instances in redistribution #1114

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

Conversation

rszarecki
Copy link
Contributor

@rszarecki rszarecki commented May 25, 2024

Enable support for source and desination instance of respective protocol.

  1. DEPRECATE /network-instances/network-instance/tables/ and /network-instances/network-instance/table-connections/. Hence this change is breaking.
  2. add new /network-instances/network-instance/route-redistributions. It is similar in concepts and structure to deprecated table-connections, but:
  • alows to specify source and destination instance of protocol. This brings parity to protocol configuration when it is possible to define multiple instances of the same protocol, differentiated by instance name.
  • has more intuitive container name
  • the 5-tuple KEY for route-redistribution is reference to protocols idenitier and name. This simplify model.
module: openconfig-network-instance
  +--rw network-instances
     +--rw network-instance* [name]
        +--rw route-redistributions
           +--rw route-redistribution* [src-protocol src-name dst-protocol dst-name address-family]
              +--rw src-protocol         -> ../config/src-protocol
              +--rw src-instance-name?   -> ../config/src-instance-name
              +--rw dst-protocol?        -> ../config/dst-protocol
              +--rw dst-instance-name?   -> ../config/dst-instance-name
              +--rw address-family?      -> ../config/address-family
              +--rw config
              |  +--rw src-protocol?                 -> ../../../../protocols/protocol/config/identifier
              |  +--rw src-instance-name?            -> ../../../../protocols/protocol/config/name
              |  +--rw address-family?               -> ../../../../protocols/protocol[identifier=current()/../src-protocol]/config/address-family
              |  +--rw dst-protocol?                 -> ../../../../protocols/protocol/config/identifier
              |  +--rw dst-instance-name?            -> ../../../../protocols/protocol/config/name
              |  +--rw disable-metric-propagation?   boolean
              |  +--rw import-policy*                -> /oc-rpol:routing-policy/policy-definitions/policy-definition/name
              |  +--rw default-import-policy?        default-policy-type
              +--ro state
                 +--ro src-protocol?                 -> ../../../../protocols/protocol/config/identifier
                 +--ro src-instance-name?            -> ../../../../protocols/protocol/config/name
                 +--ro address-family?               -> ../../../../protocols/protocol[identifier=current()/../src-protocol]/config/address-family
                 +--ro dst-protocol?                 -> ../../../../protocols/protocol/config/identifier
                 +--ro dst-instance-name?            -> ../../../../protocols/protocol/config/name
                 +--ro disable-metric-propagation?   boolean
                 +--ro import-policy*                -> /oc-rpol:routing-policy/policy-definitions/policy-definition/name
                 +--ro default-import-policy?        default-policy-type

Cisco XR

router bgp 12345 instance FOO
   net 00.0000.0000.12a5.00
   address-family ipv4 unicast
    metric-style wide
    redistribute isis BAAR level 2
!
router isis BAR
   ...

@dplore dplore changed the title enabling multiple protocol instances in enabling multiple protocol instances in table-connections May 25, 2024
@dplore
Copy link
Member

dplore commented May 25, 2024

Instead of refactoring tables and table-connections, how about deprecating both of those completely and introducing a new tree
/network-instances/network-instance/protocol-redistribution

It could look like that you have proposed. I would say since we are using src-instance-name, we no longer need the protocol leaf. A protocols/protocol/config/name will only be one protocol. (no longer a table concept with multiple protocols possible). ie:

  +--rw network-instances
     +--rw network-instance* [name]
        +--rw protocol-redistributions
           +--rw protocol-redistribution* [src-instance-name  dst-instance-name address-family]
              +--rw src-instance-name?   -> ../config/src-instance-name
              +--rw dst-instance-name?   -> ../config/dst-instance-name
              +--rw address-family?      -> ../config/address-family
              +--rw config
              |  +--rw src-instance-name?            -> ../../../../protocols/protocol/config/name
              |  +--rw dst-instance-name?            -> ../../../../protocols/protocol/config/name
              |  +--rw address-family?               -> ../../../../protocols/protocol[identifier=current()/../src-protocol]/config/address-family
              |  [...]
              +--ro state
                 +--ro src-instance-name?            -> ../../../../protocols/protocol/config/name
                 +--ro dst-instance-name?            -> ../../../../protocols/protocol/config/name
                 +--ro address-family?               -> ../../../../protocols/protocol[identifier=current()/../src-protocol]/config/address-family
                 [...]

WDYT?

@rszarecki
Copy link
Contributor Author

@dplore

  1. from my point of view, we can deprecate /network-instances/network-instance/tables/ completly.
  2. we need both src/dst-protocol and src/dst-instance-name. This is because instance name is free-text-string and can be same for multiple protocols. For example: ( src-protocol=BGP, src-instance-name="default", dst-protocol=ISIS, src-instance-name="default").
  3. "table-connections" --> "protocol-redistributions" (or route-redistributions); This would be breaking change. I hope if we keep table-connections AND allow inertpetate lack of instance-name as "the only one existing" (see description), we may be backward compatible.

@nokia1adam
Copy link
Contributor

@dplore

  1. from my point of view, we can deprecate /network-instances/network-instance/tables/ completly.

+1 on deprecating /network-instances/network-instance/tables.

As configuration objects they serve no purpose other than anchoring the endpoints of a table connection, and surely there are simpler ways to accomplish this, especially in light of trying to redistribute routes of a particular protocol instance.

If the configuration of these tables is supposed to be added automatically by the system (and protected from delete), what happens when there is a replace-from-root type operation?

If the tables were state only I would have less objection, but that breaks their utility as a leaf-ref target and takes us back to square one of just using a simpler way to define a table connection.

@rszarecki
Copy link
Contributor Author

In this proposal, table-connections KEYs are leaf-ref to '/protocol/protocols/' directly. Not to fables as it was previously.

@rszarecki rszarecki marked this pull request as ready for review June 25, 2024 20:30
@rszarecki rszarecki requested a review from a team as a code owner June 25, 2024 20:30
@wenovus
Copy link
Contributor

wenovus commented Jun 25, 2024

/gcbrun

@OpenConfigBot
Copy link

OpenConfigBot commented Jun 25, 2024

No major YANG version changes in commit d74af9c

@rszarecki
Copy link
Contributor Author

I refactored it quite heavly, following off-line discussion. Please see PR description.

@rszarecki rszarecki changed the title enabling multiple protocol instances in table-connections enabling support for multiple protocol instances in redistribution Jul 18, 2024
@rszarecki rszarecki requested a review from robshakir July 18, 2024 16:40
@dplore
Copy link
Member

dplore commented Oct 15, 2024

This does support the operational use case to redistribute routes between two network instances, which the current model does not (at least, not as far I can determine). So I am in favor of making this change.

I'd like to ask the community for more feedback. Adding this to the November 2024 openconfig community meeting.

@dplore
Copy link
Member

dplore commented Oct 15, 2024

/gcbrun

@rszarecki
Copy link
Contributor Author

rszarecki commented Oct 15, 2024

This does support the operational use case to redistribute routes between two network instances, which the current model does not (at least, not as far I can determine). So I am in favor of making this change.

I'd like to ask the community for more feedback. Adding this to the November 2024 openconfig community meeting.

Correction: This does support the operational use case to redistribute routes between two protocol instances (of potentially the same protocol like BGP) within the same network-instance.

However main use case is to support cases when protocol instance name is different than "DEFAULT".

@dplore
Copy link
Member

dplore commented Nov 8, 2024

This was presented at the Nov 7, 2024 OC Community meeting to create more visibility. We'd like to see comments from the community on this proposal.

@dplore
Copy link
Member

dplore commented Nov 8, 2024

/gcbrun

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

Successfully merging this pull request may close these issues.

5 participants