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

Yang models for static route support on south-ocnos #73

Merged
merged 3 commits into from
Dec 27, 2022

Conversation

KazuyaAnazawa
Copy link
Contributor

@KazuyaAnazawa KazuyaAnazawa commented Dec 20, 2022

This PR adds new yang models of goldstone-routing, goldstone-static-route, and goldstone-nexthop to support static route on south-ocnos.
As discussed in issue 70, these models are defined considering the followings:

  • These models are based on FRR yang models since south-frr will be introduced in the future south-frr: south daemon that controls FRR #50.
  • The models should have extensibility to support other routing protocols such as OSPFv2 although the first target is static-route.
    • Only necessary attributes for static-route are defined in this phase.
    • vrf is not considered in proposed model since the concept of network-instance has not been introduced in goldstone.

In addition to these three models, we may need another yang model which represents consolidated active routes in the system like RIB/FIB.
In FRR, such structure is introduced here, but corresponding model has not been introduced yet.
We may need to decide if we had better to add such structure in current phase (of targeting on static-route support as the first step).

@ishidawataru @ipi-claytonpascoal
Could you please review the models, and continue to discuss the above?

@KazuyaAnazawa KazuyaAnazawa marked this pull request as draft December 20, 2022 00:52
@KazuyaAnazawa KazuyaAnazawa self-assigned this Dec 20, 2022
yang/goldstone-routing.yang Outdated Show resolved Hide resolved
yang/goldstone-static-route.yang Outdated Show resolved Hide resolved
yang/goldstone-static-route.yang Outdated Show resolved Hide resolved
yang/goldstone-nexthop.yang Outdated Show resolved Hide resolved
yang/goldstone-nexthop.yang Outdated Show resolved Hide resolved
yang/goldstone-nexthop.yang Outdated Show resolved Hide resolved
yang/goldstone-nexthop.yang Outdated Show resolved Hide resolved
yang/goldstone-nexthop.yang Outdated Show resolved Hide resolved
yang/goldstone-static-route.yang Outdated Show resolved Hide resolved
yang/goldstone-nexthop.yang Outdated Show resolved Hide resolved

grouping static-route-prefix-attributes {
list path-list {
key "table-id distance";

Choose a reason for hiding this comment

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

I do not know the advantage of having a path list indexed by table-id and distance, but it sounds strange to me. I have not seen also this kind of structure in other vendors' routing modules.
Even in FRR, table-id and distance are not mandatory for static-route configuration.
I would like to suggest removing the path-list and moving the attributes distance to the next-hop entry configuration and tag to the route configuration context.

Copy link
Contributor Author

@KazuyaAnazawa KazuyaAnazawa Dec 21, 2022

Choose a reason for hiding this comment

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

@ipi-claytonpascoal
I tried to consider the model based on your comments.
Removed the path-list and moved distance to next-hop entry.
Tag may be used when we support OSPFv2, so it is ignored in this phase.
Do you agree with this structure?

module: goldstone-static-route

  augment /gs-rt:routing/gs-rt:control-plane-protocol:
    +--rw static-route
       +--rw route-list* [prefix]
          +--rw prefix      -> ../config/prefix
          +--rw config
          |  +--rw prefix?   inet:ipv4-prefix
          +--ro state
          |  +--ro prefix?   inet:ipv4-prefix
          +--rw nexthops
             +--rw nexthop* [nexthop-type gateway interface]
                +--rw nexthop-type    -> ../config/nexthop-type
                +--rw gateway         -> ../config/gateway
                +--rw interface       -> ../config/interface
                +--rw config
                |  +--rw nexthop-type      nexthop-type
                |  +--rw gateway?          optional-ip-address
                |  +--rw interface?        gs-if:interface-ref
                |  +--rw distance?         gs-rt:administrative-distance
                |  +--rw blackhole-type?   blackhole-type
                |  +--rw onlink?           boolean
                +--ro state
                   +--ro nexthop-type      nexthop-type
                   +--ro gateway?          optional-ip-address
                   +--ro interface?        gs-if:interface-ref
                   +--ro distance?         gs-rt:administrative-distance
                   +--ro blackhole-type?   blackhole-type
                   +--ro onlink?           boolean
                   +--ro active?           boolean
                   +--ro fib?              boolean

Choose a reason for hiding this comment

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

It sounds pretty good!
I just added my considerations regarding nexthop list keys above

description
"Nexthop is active.";
}

Choose a reason for hiding this comment

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

What is the difference between active and fib for the nexthop-state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ipi-claytonpascoal
Thank you for your question.
When I looked into OcNOS, there are two attributes, fib-installed and active.
I intended to represent each attribute as fib and active in the model.

container state {
config false;
uses nexthop-config;
uses nexthop-state;

Choose a reason for hiding this comment

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

Maybe the operational data for static-route/nexthop would be better placed in a centralized RIB/FIB structure that consolidates all future protocols routing base.
Is there any plan in this direction?

Copy link
Contributor Author

@KazuyaAnazawa KazuyaAnazawa Dec 22, 2022

Choose a reason for hiding this comment

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

@ipi-claytonpascoal
I thought your question here is related to what I described here.

We may need to decide if we had better to add such structure in current phase (of targeting on static-route support as the first step).

I agree with your comment as OcNOS, FRR, and OpenConfig adopt such style and structure.

@ishidawataru
Do you have any advice or comments about this?

Copy link
Contributor

@ishidawataru ishidawataru Dec 22, 2022

Choose a reason for hiding this comment

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

@KazuyaAnazawa First, let's keep the following config/state structure (OpenConfig style) in all containers of the Goldstone primitive models.
Following this structure makes the model structure more predictable when reading.

grouping b-config {
}

grouping b-state {
// can be empty
}

grouping b-top {
  container b {
    container config {
      uses b-config;
    }
    container state {
      config false;
      uses b-config;
      uses b-state;
    }
  }
}

grouping a-config {
}

grouping a-state {
// can be empty
}

container a {
  container config {
    uses a-config;
  }
  container state {
    config false;
    uses a-config;
    uses a-state;
  }

  uses b-top;
}

At this moment, I think it's OK to keep operational information on the static route and next-hop in the corresponding state containers.
We will need to define a model for a RIB (and maybe FIB (it looks like OpenConfig doesn't have a FIB model)) when we support dynamic routing protocols. When we do that, we can consider whether it's better to move operational info to the RIB structure.

Goldstone primitive YANG model is an internal API for Goldstone management components. We're free to change it when we need to.

Also, I think we will end up refining the models when we start implementing the south daemon that supports them.
Let's don't put too much time into just defining a model. We can discuss this again when we have an actual implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ishidawataru Thank you so much for your valuable comments and advise.
I first try to define the models so that we can keep OpenConfig style, and submit the early version of the models.

@KazuyaAnazawa
Copy link
Contributor Author

@ishidawataru @ipi-claytonpascoal
I fixed to follow OpenConfig style for each model on 544b224 and c987033.
Comments regarding path-list, table-id, and tag are addressed on ced1738.

Sorry to keep persisting you, but could you check the fixed models?

Copy link

@ipi-claytonpascoal ipi-claytonpascoal left a comment

Choose a reason for hiding this comment

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

Understood and agree with the comments.
Routing models seem fine to me

Comment on lines 81 to 86
leaf nexthop-type {
type nexthop-type;
mandatory true;
description
"The nexthop type.";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this since we decided to use an index for the key of nexthops.

Comment on lines 26 to 47
typedef nexthop-type {
type enumeration {
enum "interface" {
description
"Specific interface.";
}
enum "ip4" {
description
"IPv4 address.";
}
enum "ip4-interface" {
description
"IPv4 address and interface.";
}
enum "blackhole" {
description
"Unreachable or prohibited.";
}
}
description
"Nexthop types.";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

@KazuyaAnazawa
Copy link
Contributor Author

@ishidawataru Thank you for your review.
As you pointed out, now the index is used for the key for nexthops.
So I removed nexthop-type and push the commit again.

@ishidawataru
Copy link
Contributor

Looks good to me. Please clean up the commits.

@KazuyaAnazawa
Copy link
Contributor Author

@ishidawataru Thank you for your review.
Cleaned up the commits.

@ishidawataru ishidawataru marked this pull request as ready for review December 27, 2022 04:46
@ishidawataru ishidawataru merged commit 1169ea0 into master Dec 27, 2022
@ishidawataru ishidawataru deleted the yang-for-static-route branch December 27, 2022 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants