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

VXLAN config and show utilities #870

Merged
merged 20 commits into from
Dec 19, 2020
Merged

VXLAN config and show utilities #870

merged 20 commits into from
Dec 19, 2020

Conversation

srj102
Copy link
Contributor

@srj102 srj102 commented Apr 8, 2020

- 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

  1. config vxlan add/del and config vxlan evpn_nvo add/del can be verified by using the
    "show vxlan interface" command.

  2. config vxlan map/map_range add can be verified by using the "show vxlan vlanvnimap"
    command.

  3. show vxlan remote vni/show vxlan remote mac can be verified by populating the
    VXLAN_REMOTE_VNI and VXLAN_FDB tables in APP DB.

  4. 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.

@srj102 srj102 changed the title Evpn vxlan VXLAN config and show utilities Apr 8, 2020
@lgtm-com
Copy link

lgtm-com bot commented Apr 8, 2020

This pull request introduces 2 alerts when merging 9165184b710f39d5da4beae4854d991f66a88ddd into a0f3b93 - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass
  • 1 for Unused local variable

@lguohan
Copy link
Contributor

lguohan commented Apr 8, 2020

@Sj102, can you fix the lgtm warnings

@prsunny prsunny self-assigned this Apr 8, 2020
@lgtm-com
Copy link

lgtm-com bot commented Apr 17, 2020

This pull request introduces 3 alerts when merging 831931bddefdb2fa6d712dc5c6efa3ce6d113ec0 into eccefa4 - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass
  • 1 for Unused local variable
  • 1 for Variable defined multiple times

@lgtm-com
Copy link

lgtm-com bot commented May 29, 2020

This pull request introduces 1 alert when merging 819b757481fb2692e3259c9be1385f438f57d50f into fab196e - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass

config/main.py Outdated
"""Del VXLAN"""
db = ctx.obj['db']

vxlan_keys = db.keys('CONFIG_DB', "EVPN_NVO|*")
Copy link
Contributor

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

Copy link
Contributor Author

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|*")
Copy link
Contributor

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

Copy link
Contributor Author

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

@lgtm-com
Copy link

lgtm-com bot commented Jun 18, 2020

This pull request introduces 2 alerts and fixes 5 when merging 0541c729f6c5d4b312b4b7c6199b315aecebf217 into fd7781b - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass
  • 1 for Syntax error

fixed alerts:

  • 4 for Module-level cyclic import
  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Jun 18, 2020

This pull request introduces 1 alert when merging 316171402fcf3d136471f56d885bff48a2654493 into fd7781b - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass

@srj102 srj102 marked this pull request as ready for review June 19, 2020 04:43
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated
break

if (found == 1):
print "VNI {} mapped to Vrf {}, Please remove VRF VNI mapping".format(vni, vrf_key)

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().

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 is a utility function which is used by other functions and hence returning True or False.

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.

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

Copy link
Contributor Author

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.

config/main.py Outdated Show resolved Hide resolved
Copy link

@dev-aviz dev-aviz left a comment

Choose a reason for hiding this comment

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

review comments provided

Copy link
Contributor

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

  1. Suggest moving this functionality to separate file, like "vxlan.py" as there is lots of code being added.
  2. Also can you provide some sample config/show output in the description so that can be used as reference?

config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
show/main.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Nov 11, 2020

This pull request introduces 2 alerts when merging 9095051bd556093ff36d76390e6eced9861b781b into d5eb2f8 - view on LGTM.com

new alerts:

  • 2 for Unused import

@prsunny
Copy link
Contributor

prsunny commented Nov 13, 2020

Unit tests required

srj102 and others added 10 commits December 11, 2020 00:50
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
@prsunny
Copy link
Contributor

prsunny commented Dec 11, 2020

retest this please

@srj102
Copy link
Contributor Author

srj102 commented Dec 14, 2020

retest this please

srj102 and others added 3 commits December 16, 2020 09:48
show vxlan tunnel_cfg (which reads from cfg db) which was originally show vxlan tunnel has been
reverted back to show vxlan tunnel.
@srj102
Copy link
Contributor Author

srj102 commented Dec 16, 2020

retest vs please

1 similar comment
@srj102
Copy link
Contributor Author

srj102 commented Dec 17, 2020

retest vs please

#
# Use this method to validate unicast IPv4 address
#
def is_ip4_addr_valid(addr, display):
Copy link
Contributor

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

Copy link
Contributor

@tapashdas tapashdas Dec 17, 2020

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):
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 this is specific to vxlan case right? Suggest moving to vxlan.py?

Copy link
Contributor

@tapashdas tapashdas Dec 17, 2020

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):
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok will do.

@prsunny
Copy link
Contributor

prsunny commented Dec 17, 2020

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|*")
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/Azure/SONiC/blob/8645a6a3dd1467153b45ee4f9a08cbf7fd9183d0/doc/vxlan/EVPN/EVPN_VXLAN_HLD.md#512-show-commands

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.

msreddypaluru
msreddypaluru previously approved these changes Dec 18, 2020
config/main.py Outdated
break

if (found == 0):
ctx.fail(" VLAN VNI not mapped. Please create VLAN VNI map entry first ")

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?

Copy link
Contributor

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)


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.

Copy link
Contributor Author

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


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


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))


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.
@prsunny prsunny merged commit aad2c38 into sonic-net:master Dec 19, 2020
anand-kumar-subramanian pushed a commit to anand-kumar-subramanian/sonic-utilities that referenced this pull request Mar 2, 2021
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 <tapash.das@broadcom.com>
Co-authored-by: Karthikeyan Ananthakrishnan <karthikeyan.ananthakrishnan@broadcom.com>
Co-authored-by: Tapash Das <48195098+tapashdas@users.noreply.github.com>
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.

9 participants