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

VRF-HLD document #242

Closed
wants to merge 9 commits into from
Closed

Conversation

shine4chen
Copy link
Contributor

VRF high level design for SONIC

@msftclas
Copy link

msftclas commented Sep 4, 2018

CLA assistant check
All CLA requirements met.


blackhole = BIT ; Set to 1 if this route is a blackhole (or null0)

Since global vrf name is null, global vrf key will becomes ROUTE_TABLE:prefix.
Copy link

@nikos-github nikos-github Sep 17, 2018

Choose a reason for hiding this comment

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

I suggest you use ROUTE_TABLE:DEFAULT:prefix or ROUTE_TABLE:GLOBAL:prefix or even better, the tableid.

Adding the following logic:

- the Key of NextHop now is changed from only ipaddress to a pair of
(ipaddress, interface_name)

Choose a reason for hiding this comment

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

Should be (ipaddress, tableid) and not (ipaddress, interface_name). Expect to be doing a lot of conversions and lookups if you leave as (ipaddress, interface_name).

Copy link
Contributor

Choose a reason for hiding this comment

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

can we point a route in vrf red to a nexthop (neighbor) in vrf blue? @prsunny , do we need to change the nexthop format in app db?

Choose a reason for hiding this comment

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

@lguohan Yes you can have routes in vrf red with nexthops in vrf blue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nikos-github , yes it is possible but the format in APP_DB is same for route since every route has a nexthop IP with the interface name. Currently orchagent doesn't look for the interface which requires a fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikos-github When set neigh to SAI layer (ipaddress, interface_name) would be more comfortable. And relatedAPP_DB/ASIC_DB needn't be changed.

> routerCnt  ,neighCnt and rifCnt  is decreased to zero.

> When device binds to specified VRF, the ip address of slave device will be
> removed and kernel will delete all neigh associated slave device.

Choose a reason for hiding this comment

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

I believe on enslavement, connected and local routes are automatically moved to the table associated with the VRF device. Is that not the case with the latest kernel? Can you explain how you plan to achieve the above removal/behavior you mention?

Copy link
Contributor

@jipanyang jipanyang left a comment

Choose a reason for hiding this comment

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

Could you update the MD file and make it more structured and hierarchical? It is difficult to read in current format.

@lguohan
Copy link
Contributor

lguohan commented Sep 19, 2018

@jipanyang , I have re-formatted the doc, please comment.

@shine4chen
Copy link
Contributor Author

@lguohan thanks for help to format md file.

@shine4chen shine4chen closed this Sep 20, 2018
@lguohan
Copy link
Contributor

lguohan commented Sep 20, 2018

why close this PR?

@shine4chen shine4chen reopened this Sep 20, 2018
@shine4chen
Copy link
Contributor Author

@lguohan sorry It is missoperation. reopen again.


```
"VRF": {
"VRF-blue": {

Choose a reason for hiding this comment

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

Any reason why you are using VRF-blue instead of vrf-blue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any reason why you are using VRF-blue instead of vrf-blue?

I think vrf-blue is okay. I will revise it @nikos-github

```
"VRF": {
"VRF-blue": {
"fall_through":"1" //enable global fib lookup while vrf fib lookup missed

Choose a reason for hiding this comment

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

Please change fall_through to fallback. The term fall_through tends to be used for routemaps but for vrfs the industry established term is fallback.

There should be no restriction that the fallback be the global/main table even if that may be the only one we plan to support (and I don't see why we plan to support only global/main as fallback).

Your document needs to specify how the fallback is specified. I assume the table id will be used since linux doesn't have a name for the global/main table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikos-github Fallback term is better.I will revise it. The fallback global feature is very useful for specify VRF user access internet through global/main route which is defined by RFC4364. Some enterprise user still use this to access internet on vpn environment. Different vrf route leak is beyond this document scope.

list of 0 or more ip prefixes;

;Status: stable
key = INTF_TABLE:ifname:IPprefix ; an instance of this key will be repeated for
Copy link

@nikos-github nikos-github Oct 2, 2018

Choose a reason for hiding this comment

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

What is the advantage of having multiple keys?

Please provide how the json looks for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikos-github There is two reason why break up app-intf-table into app-intf-table and app-intf-prefix-table.

  1. Multiple ip addresses can be configured on one interface.So I put interface common attribute into app-intf-table and keep ip-prefix specific attribute on app-intf-prefix-table.
  2. Interface can be put to specific VRF before ip address is configured on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jason file looks like below:
"Ethernet1":{
"mtu":"1500”,
"vrf":"vrf-red"
},
"Ethernet0|11.11.11.1/24": {},
"Ethernet1|12.12.12.1/24": {},
"Ethernet2|13.13.13.1/24": {},
"Ethernet3|14.14.14.1/24": {},

shine and others added 7 commits January 18, 2019 01:21
1.	Support loopback interface as part of VRF interface.
2.	In SAI , VR is treated same ad VRF, so in SONiC code VRF will be implemented as VR.
3.	Consider the configuration dependency between VRF binding event and interface ip address configuration. The proposal is to add VRF name before the ip address configuration.
4.	When cold restart, vrfMgrd will flush all vrf related setting in linux kernel.
5.	In the overall diagram, make the color scheme consistent for all modified module.
6.	Add VRF show command.
7.	Will not delete VRF object until all routerintfs related to VRF object have unbind from this VRF by user.
1.	Support loopback interface as part of VRF interface.
2.	In SAI , VR is treated same ad VRF, so in SONiC code VRF will be implemented as VR.
3.	Consider the configuration dependency between VRF binding event and interface ip address configuration. The proposal is to add VRF name before the ip address configuration.
4.	When cold restart, vrfMgrd will flush all vrf related setting in linux kernel.
5.	In the overall diagram, make the color scheme consistent for all modified module.
6.	Add VRF show command.
7.	Will not delete VRF object until all routerintfs related to VRF object have unbind from this VRF by user.
This reverts commit c0a86c3.
@shine4chen
Copy link
Contributor Author

@lguohan @prsunny @jipanyang please help to approve this document if no more question.

@shine4chen
Copy link
Contributor Author

Nephos plan to submit this feature include warm-reboot support on 201906 branch. @lguohan @prsunny @jipanyang

@prsunny
Copy link
Contributor

prsunny commented Jan 31, 2019

Closing in favor of this commit

@prsunny prsunny closed this Jan 31, 2019
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.

6 participants