-
Notifications
You must be signed in to change notification settings - Fork 664
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
VXLAN config and show utilities #870
Conversation
This pull request introduces 2 alerts when merging 9165184b710f39d5da4beae4854d991f66a88ddd into a0f3b93 - view on LGTM.com new alerts:
|
@Sj102, can you fix the lgtm warnings |
This pull request introduces 3 alerts when merging 831931bddefdb2fa6d712dc5c6efa3ce6d113ec0 into eccefa4 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 819b757481fb2692e3259c9be1385f438f57d50f into fab196e - view on LGTM.com new alerts:
|
config/main.py
Outdated
"""Del VXLAN""" | ||
db = ctx.obj['db'] | ||
|
||
vxlan_keys = db.keys('CONFIG_DB', "EVPN_NVO|*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In sonic-swss-commin PR339, this table is named as VXLAN_EVPN_NVO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to be in sync with PR339
config/main.py
Outdated
def add_vxlan_evpn_nvo(ctx, nvo_name, vxlan_name): | ||
"""Add NVO""" | ||
db = ctx.obj['db'] | ||
vxlan_keys = db.keys('CONFIG_DB', "EVPN_NVO|*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In sonic-swss-commin PR339, this table is named as VXLAN_EVPN_NVO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to be in sync with PR 339
This pull request introduces 2 alerts and fixes 5 when merging 0541c729f6c5d4b312b4b7c6199b315aecebf217 into fd7781b - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert when merging 316171402fcf3d136471f56d885bff48a2654493 into fd7781b - view on LGTM.com new alerts:
|
config/main.py
Outdated
break | ||
|
||
if (found == 1): | ||
print "VNI {} mapped to Vrf {}, Please remove VRF VNI mapping".format(vni, vrf_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it throw error or is it a warning? If it is error use ctx.fail().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a utility function which is used by other functions and hence returning True or False.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that this line is the cause of errors during test phase. Probably because of Python version
See an error log after build, i.e. make target/python-wheels/sonic_utilities-1.2-py3-none-any.whl
:
============================= test session starts ==============================
platform linux -- Python 3.7.3, pytest-3.10.1, py-1.7.0, pluggy-0.8.0
rootdir: /sonic/src/sonic-utilities, inifile: pytest.ini
plugins: cov-2.6.0
collected 142 items / 22 errors
==================================== ERRORS ====================================
________________ ERROR collecting tests/chassis_modules_test.py ________________
/usr/lib/python3/dist-packages/_pytest/python.py:450: in _importtestmodule
mod = self.fspath.pyimport(ensuresyspath=importmode)
/usr/lib/python3/dist-packages/py/_path/local.py:668: in pyimport
__import__(modname)
<frozen importlib._bootstrap>:983: in _find_and_load
???
<frozen importlib._bootstrap>:967: in _find_and_load_unlocked
???
<frozen importlib._bootstrap>:668: in _load_unlocked
???
<frozen importlib._bootstrap>:638: in _load_backward_compatible
???
/usr/lib/python3/dist-packages/_pytest/assertion/rewrite.py:294: in load_module
six.exec_(co, mod.__dict__)
tests/chassis_modules_test.py:9: in <module>
import show.main as show
show/main.py:9: in <module>
import utilities_common.cli as clicommon
E File "/sonic/src/sonic-utilities/utilities_common/cli.py", line 359
E print "VNI {} mapped to Vrf {}, Please remove VRF VNI mapping".format(vni, vrf_key)
E ^
E SyntaxError: invalid syntax
____________________ ERROR collecting tests/console_test.py ____________________
/usr/lib/python3/dist-packages/_pytest/python.py:450: in _importtestmodule
mod = self.fspath.pyimport(ensuresyspath=importmode)
/usr/lib/python3/dist-packages/py/_path/local.py:668: in pyimport
__import__(modname)
<frozen importlib._bootstrap>:983: in _find_and_load
???
<frozen importlib._bootstrap>:967: in _find_and_load_unlocked
???
<frozen importlib._bootstrap>:668: in _load_unlocked
???
<frozen importlib._bootstrap>:638: in _load_backward_compatible
???
/usr/lib/python3/dist-packages/_pytest/assertion/rewrite.py:294: in load_module
six.exec_(co, mod.__dict__)
tests/console_test.py:9: in <module>
import config.main as config
config/main.py:23: in <module>
import utilities_common.cli as clicommon
E File "/sonic/src/sonic-utilities/utilities_common/cli.py", line 359
E print "VNI {} mapped to Vrf {}, Please remove VRF VNI mapping".format(vni, vrf_key)
E ^
E SyntaxError: invalid syntax
... totally 22 times the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the reason is #1128
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment. resolved now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review comments provided
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Suggest moving this functionality to separate file, like "vxlan.py" as there is lots of code being added.
- Also can you provide some sample config/show output in the description so that can be used as reference?
This pull request introduces 2 alerts when merging 9095051bd556093ff36d76390e6eced9861b781b into d5eb2f8 - view on LGTM.com new alerts:
|
Unit tests required |
Config and show commands added to click infra for VXLAN objects. Please refer to sonic-net/SONiC#437 for the commands. The fast-reboot script is changed to not clear VXLAN tunnel state table during a warm reboot.
Added Neighbor Suppression CLI.
2. Tested on vs except for L3VXLAN+NeighSuppress+remote mac
1. Moved out vxlan related config and show utilities to a separate file. 2. Moved some utility functions to utilitie_common
changes to vxlan config utils
retest this please |
retest this please |
show vxlan tunnel_cfg (which reads from cfg db) which was originally show vxlan tunnel has been reverted back to show vxlan tunnel.
retest vs please |
1 similar comment
retest vs please |
utilities_common/cli.py
Outdated
# | ||
# Use this method to validate unicast IPv4 address | ||
# | ||
def is_ip4_addr_valid(addr, display): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest use existing is_ipaddress
function in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is not used currently. Removing it.
|
||
return True | ||
|
||
def is_vni_vrf_mapped(db, vni): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is specific to vxlan case right? Suggest moving to vxlan.py?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a common API will be needed in both vxlan and vrf. So better to keep it in common.
@click.argument('vrfname', metavar='<vrf-name>', required=True, type=str) | ||
@click.argument('vni', metavar='<vni>', required=True) | ||
@click.pass_context | ||
def add_vrf_vni_map(ctx, vrfname, vni): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you have some test coverage for this case as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok will do.
retest this please |
output = '\tNVO Name : ' + vtepname + ', VTEP : ' + vxlan_table[key]['source_vtep'] | ||
click.echo(output) | ||
|
||
vxlan_keys = config_db.keys('CONFIG_DB', "LOOPBACK_INTERFACE|*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any description to show that only loopback interface can be set to vtep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the show command output there is no other assumption in SONiC components. The VTEP IP should be reachable. Loopback interface is typically used.
config/main.py
Outdated
break | ||
|
||
if (found == 0): | ||
ctx.fail(" VLAN VNI not mapped. Please create VLAN VNI map entry first ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this leading space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Updated code.
@@ -1522,6 +1522,5 @@ def ztp(status, verbose): | |||
cmd = cmd + " --verbose" | |||
run_command(cmd, display_cmd=verbose) | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't look obligatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will handle this in the next PR.
from tabulate import tabulate | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't look obligatory.
@@ -13,7 +13,6 @@ def vxlan(): | |||
"""Show vxlan related information""" | |||
pass | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with this
@@ -41,7 +40,6 @@ def name(vxlan_name): | |||
|
|||
click.echo(tabulate(table, header)) | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with this
Removed space as per review comment.
Added support for VXLAN config and commands as described in the PR sonic-net/SONiC#437 config vxlan add/del and config vxlan evpn_nvo add/del config vxlan map/map_range add show vxlan remote vni/show vxlan remote mac show vxlan tunnel Co-authored-by: Tapash Das <[email protected]> Co-authored-by: Karthikeyan Ananthakrishnan <[email protected]> Co-authored-by: Tapash Das <[email protected]>
- What I did
Added support for VXLAN config and commands as described in the PR sonic-net/SONiC#437
- How I did it
Modified the config and show main.py scripts for vxlan support.
- How to verify it
config vxlan add/del and config vxlan evpn_nvo add/del can be verified by using the
"show vxlan interface" command.
config vxlan map/map_range add can be verified by using the "show vxlan vlanvnimap"
command.
show vxlan remote vni/show vxlan remote mac can be verified by populating the
VXLAN_REMOTE_VNI and VXLAN_FDB tables in APP DB.
show vxlan tunnel can be verified by populating the VXLAN_TUNNEL_TABLE in STATE DB.
- Previous command output (if the output of a command-line utility has changed)
- New command output (if the output of a command-line utility has changed)
Please refer to the HLD for outputs.