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 MODEL] SONiC Yang support for VXLAN #7294

Merged
merged 15 commits into from
Jul 8, 2022
Merged

Conversation

srj102
Copy link
Contributor

@srj102 srj102 commented Apr 12, 2021

Why I did it

SONiC Yang support for VXLAN

How I did it

Added a new sonic-vxlan.yang file.
Please refer to EVPN VXLAN HLD for DB details
https://github.com/Azure/SONiC/tree/master/doc/vxlan/EVPN

How to verify it

Added tests for sonic vxlan yang.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012

Description for the changelog

Added yang file for VXLAN
Added tests for sonic-vxlan.yang

A picture of a cute animal (not mandatory but encouraged)

Added tests for sonic-vxlan.yang
@srj102 srj102 requested a review from lguohan as a code owner April 12, 2021 11:45
@srj102
Copy link
Contributor Author

srj102 commented Apr 12, 2021

Dependent on #6861 for build to go through

@lguohan lguohan added the YANG YANG model related changes label Apr 15, 2021
@lguohan lguohan requested a review from prsunny April 15, 2021 01:26
@zhangyanzhao
Copy link
Collaborator

@srj102 please help to address the conflict and build failure

@zhangyanzhao
Copy link
Collaborator

What is the specific table name for this YANG model? vxlan?

@srj102
Copy link
Contributor Author

srj102 commented Dec 2, 2021

config db - VXLAN_TUNNEL, VXLAN_TUNNEL_MAP, VXLAN_EVPN_NVO
app db - VXLAN_REMOTE_VNI_TABLE, VXLAN_FDB_TABLE
state db - VXLAN_TUNNEL_TABLE

@zhangyanzhao
Copy link
Collaborator

enhancement for previous vxlan YANG tables
config db - VXLAN_TUNNEL, VXLAN_TUNNEL_MAP, VXLAN_EVPN_NVO
app db - VXLAN_REMOTE_VNI_TABLE, VXLAN_FDB_TABLE
state db - VXLAN_TUNNEL_TABLE

error-app-tag invalid-vtep-name;
}
type string {
length 1..10;
Copy link
Collaborator

@qiluo-msft qiluo-msft Dec 9, 2021

Choose a reason for hiding this comment

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

1..10

Where is the limitation from? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for every vlan vni map a vni netdevice of form - is created.
The kernel devices can have a max length of 15 characters.
Keeping aside 5 characters for the "-" we are left with 10.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the detailed explanation! Could you also add it as code comment? the 10 is magic number here.

Copy link
Collaborator

@venkatmahalingam venkatmahalingam Jan 13, 2022

Choose a reason for hiding this comment

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

Using "-" is a must or the user can choose any letter after "vtep" prefix? if any character, please put max 15 chars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The vni device format is -
Max vid is 4 characters and 1 for hyphen leaving us
with 10 characters for tunnelname.

Added a comment in the yang file for the same.

}
}
}
container VXLAN_FDB_TABLE{
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find UT for this 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.

Do we have any infra support for this ? I see a sample_config_db.json but not a sample_app_db or sample_state_db..

Copy link
Collaborator

Choose a reason for hiding this comment

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

No support yet. Could you add this support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the APP and STATE DB table changes and kept only the CONFIG DB changes. Will add the remaining once the support for these tables comes in in the framework.

}

leaf type {
type string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this type has any pattern?


}

leaf vni {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a common type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a common type

}
}
}
container VXLAN_TUNNEL_TABLE {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't find UT 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.

Removed APP and STATE DB related containers and will support later.

src/sonic-yang-models/yang-models/sonic-vxlan.yang Outdated Show resolved Hide resolved
src/sonic-yang-models/yang-models/sonic-vxlan.yang Outdated Show resolved Hide resolved
}
}
}
container VXLAN_REMOTE_VNI_TABLE {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't find UT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed APP and STATE DB related containers and will support later.

}

leaf vni {
type string;
Copy link
Contributor

Choose a reason for hiding this comment

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

VXLAN_TUNNEL_MAP defines vni before, same pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed APP and STATE DB related containers and will support later.

@srj102
Copy link
Contributor Author

srj102 commented Dec 14, 2021

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

import sonic-vlan {
prefix svlan;
}
import sonic-types {
Copy link
Collaborator

@dgsudharsan dgsudharsan Jan 8, 2022

Choose a reason for hiding this comment

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

Please correct alignment #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason is mixing tab and spaces. So it seems aligned in some editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed a few tabs which were present.

max-elements 1;

leaf name {
must "(starts-with(../name, 'vtep'))"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this restriction exists today. Please remove this. Might break backward compatibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Have removed the same.

leaf src_ip {
type inet:ipv4-address;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we also have "dst_ip" in vxlan tunnel. Please update
VXLAN_TUNNEL|{{tunnel_name}}
"src_ip": {{ip_address}}
"dst_ip": {{ip_address}} (OPTIONAL)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont think, we support static vxlan tunnel yet, so dst_ip is not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dst_ip is not handled here.
Even for non BGP P2P tunnel support the configuration will continue to have only the src_ip.

}
}
container VXLAN_REMOTE_VNI_TABLE {
sonic-ext:db-name "APPL_DB";
Copy link
Collaborator

Choose a reason for hiding this comment

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

If i am not wrong currently we define yang models only for config_db and no other DBs. @praveen-li have we decided to use yang in other DBs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed APP and STATE DB related containers and will support later.

max-elements 1;

leaf name {
must "(starts-with(../name, 'vtep'))"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use the string pattern to enforce this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need a list if the number of entries itself is one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this restriction of having to start with vtep.

@zhangyanzhao
Copy link
Collaborator

@qiluo-msft would you please help to merge this PR? Thanks.

@srj102
Copy link
Contributor Author

srj102 commented Jun 9, 2022

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

import sonic-extension {
prefix sonic-ext;
}
import sonic-vlan {
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests are failing in Sonic-utils specifically GCU because of yang known issue: #9312

Please comment out sonic-vlan import and add a note

    // Comment sonic-vlan import here until libyang back-links issue is resolved for VLAN leaf reference.
    //import sonic-vlan {
    //    prefix vlan;
    //}

Copy link
Contributor

Choose a reason for hiding this comment

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

@qiluo-msft for viz

@qiluo-msft
Copy link
Collaborator

@dgsudharsan Please review again.

@qiluo-msft
Copy link
Collaborator

@srj102 @venkatmahalingam would you like to review again? any more comments?

Copy link
Collaborator

@dgsudharsan dgsudharsan left a comment

Choose a reason for hiding this comment

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

}
}
},
"VXLAN_MAP_WITHOUT_VLAN": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see corresponding key in tests/vxlan.json. Can you please check and update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was removed in the last commit as the reference to vlan (import vlan) was failing the builds.
The intent is that this be re-added later once the import vlan is fixed. Hence it is being removed only in the
tests and not the tests_config.

@srj102
Copy link
Contributor Author

srj102 commented Jun 27, 2022

Can you please update the schema in configuration doc? https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-yang-models/doc/Configuration.md

done.

@mssonicbld
Copy link
Collaborator

@srj102 PR conflicts with 202205 branch

@yxieca
Copy link
Contributor

yxieca commented Jul 6, 2023

@srj102 there is cherry-picking conflict, can you help manually cherry-pick this one?

@qiluo-msft
Copy link
Collaborator

@adyeung Could you help ping @srj102 to backport this PR to 202205?

@srj102
Copy link
Contributor Author

srj102 commented Feb 13, 2024

Will backport by Feb 23.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.