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 version 1.1 #408

Merged
merged 7 commits into from Jul 17, 2019
Merged

VRF HLD version 1.1 #408

merged 7 commits into from Jul 17, 2019

Conversation

preetham-singh
Copy link
Contributor

Updating VRF HLD to create version 1.1

@msftclas
Copy link

msftclas commented Jun 19, 2019

CLA assistant check
All CLA requirements met.

Removed test plan since its tracked as separate document
@jeffreyzfzeng
Copy link
Contributor

In the VRF diagram, in each VRF, site-1 and site-2 are repeated. Should they be site-3 and site-4? Or it means a multi-homing configuration? Please clarify.

doc/sonic-vrf-hld-v1.0.md Outdated Show resolved Hide resolved
doc/sonic-vrf-hld-v1.0.md Show resolved Hide resolved
doc/sonic-vrf-hld-v1.0.md Outdated Show resolved Hide resolved
doc/sonic-vrf-hld-v1.0.md Show resolved Hide resolved
doc/sonic-vrf-hld-v1.0.md Outdated Show resolved Hide resolved
doc/sonic-vrf-hld-v1.0.md Outdated Show resolved Hide resolved
doc/sonic-vrf-hld-v1.0.md Outdated Show resolved Hide resolved
doc/sonic-vrf-hld-v1.0.md Outdated Show resolved Hide resolved
doc/sonic-vrf-hld-v1.0.md Outdated Show resolved Hide resolved
doc/sonic-vrf-hld-v1.0.md Outdated Show resolved Hide resolved
Removed test plan since its tracked as separate document
Addressing comments
Removed test plan since its tracked as separate document
Addressing comments
Prefix lists within route maps are used to match route prefixes in source VRF and various action can be applied on these matching prefixes.
If route map action is permit, these routes will be installed into destination VRF.

Leaked routes will be automatically deleted when corresponding routes are deleted in source VRF.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide and describe the use case for VRF route leak similar to multiple VRF deployment use-case?

@lguohan
Copy link
Contributor

lguohan commented Jun 26, 2019

remove the version number in the file name.

@@ -238,7 +277,7 @@ Add vrf-binding information in config_db.json file.

With this approach, there is no redundant vrf info configured with an interface where multiple IP addresses are configured.

Logically IP address configuration must be processed after interface binding to vrf is processed. In intfmgrd/intfOrch process intf-bind-vrf event must be handled before IP address event. So interface-name entry in config_db.json is necessary even user doesn't use VRF feature. e.g. `"Ethernet2":{}` in the above example configuration. For version upgrade compatibility we need to add a script, this script will convert old config_db.json to new config_db.json at bootup, then the new config_db.json would contain the interface-name entry for interfaces associated in the global VRF table.
Logically IP address configuration must be processed after interface binding to vrf is processed. In intfmgrd/intfOrch process intf-bind-vrf event must be handled before IP address event. So interface-name entry in config_db.json is necessary even though user doesn't use VRF feature. e.g. `"Ethernet2":{}` in the above example configuration. For version upgrade compatibility we need to add a script, this script will convert old config_db.json to new config_db.json at bootup automatically, then the new config_db.json would contain the interface-name entry for interfaces associated in the global VRF table.

Choose a reason for hiding this comment

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

@preetham-singh @ben-gale @prsunny Regarding the comment I made in the community meeting last Tuesday on changing config_db.json when it comes to interfaces belonging in the global table and also mentioned in here https://github.com/Azure/SONiC/blob/master/doc/sonic-vrf-hld-v1.0.md

"Ethernet2":{}, // it means this interface belongs to global vrf. It is necessary even user doesnt use vrf.

In order to preserve backwards compatibility with existing config_db.json files and deployments where the config_db.json is checked for deviations, when saving the contents of config db from redis back into config_db.json, default entries like the the entry above can be removed. Basically it shouldn't be a requirement for the user to either provide that in their config_db.json or have this saved back.

Such approach is being followed already by some BGP cmds I introduced and implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on discussion on this topic in the Sonic user group, it was agreed upon that in certain cases where the feature demands it, it is expected to have a config_db change as long as there is a provision to migrate the old config_db.json to the new image version to ensure backward compatibility. This means, after a save of the migrated config_db, the saved json file could be different from the old one and the expectation is that the monitoring application must accommodate to the new schema defined in the migrated image version.

Choose a reason for hiding this comment

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

Prince - what about the reverse direction (i.e. downgrade)? I assume the user is expected to manually 'repair' their configuration?

Is there really no way that this could be handled differently?

Copy link
Contributor

Choose a reason for hiding this comment

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

For the downgrade, the user has to manually repair. In general, it is not an automated support. W.r.t VRF, if you are asking whether it could handled differently, yes it is possible but with the complexities of supporting move feature. Moreover, some of the code to support the config_db approach like the db_migrator, interface changes etc were already in-place for VNet feature. That's why we want to take this path.

@prsunny prsunny merged commit 56a4b71 into sonic-net:master Jul 17, 2019
rajendra-dendukuri pushed a commit to rajendra-dendukuri/SONiC that referenced this pull request Jul 29, 2019
* Updating VRF HLD Version 1.0

* VRF HLD v1.1

* Addressing comments from Nephos:
Removed test plan since its tracked as separate document

* Addressing comments from Nephos:
Removed test plan since its tracked as separate document
Addressing comments

* Addressing comments from Nephos:
Removed test plan since its tracked as separate document
Addressing comments

* Addressing few review comments from VRF HLD review

* Rename VRF HLD file to not have version numbre in file name
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.

None yet

8 participants