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

Add basic MPLS testcases #3483

Merged
merged 38 commits into from
May 5, 2022
Merged

Add basic MPLS testcases #3483

merged 38 commits into from
May 5, 2022

Conversation

SijiJ
Copy link
Contributor

@SijiJ SijiJ commented May 14, 2021

Description of PR

Summary: To add basic MPLS testcases to sonic-mgmt/tests

The PR adds basic label forwarding tests involving push, swap, pop operatios.
Fixes # (issue)

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • [ x] Test case(new)

Approach

What is the motivation for this PR?

As part of qualification of new MPLS feature added.

How did you do it?

Added MPLS feature testcases on t1 Pytest testbed.

How did you verify/test it?

Verified it on t1 testbed. All three MPLS label action ,push, pop and swap.

Any platform specific information?

Verified on Juniper PTX10008, it would work on any platform with MPLS SAI support.

Supported testbed topology if it's a new test case?

T1

Documentation

https://github.com/SijiJ/sonic-mgmt/blob/master/docs/testplan/MPLS-test-plan.md

@SijiJ SijiJ requested a review from a team as a code owner May 14, 2021 13:54
@ghost
Copy link

ghost commented May 14, 2021

CLA assistant check
All CLA requirements met.

@SijiJ SijiJ requested review from wangxin and yxieca as code owners May 17, 2021 14:11
yxieca
yxieca previously approved these changes May 23, 2021
@yxieca
Copy link
Collaborator

yxieca commented May 23, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@smaheshm smaheshm self-requested a review July 6, 2021 18:22
@smaheshm
Copy link
Contributor

One concern is hardcoding values in json file, e.g. Nexthop IP, interface names etc. It will be better if these files can be generated by getting the interface names and nexthops at run time.

@smaheshm
Copy link
Contributor

smaheshm commented Aug 27, 2021

I understand the scope is MPLS basic forwarding. In addition to the tests you already listed, I think more can be added for basic MPLS forwarding tests. For e.g following test cases should be added for label forwarding tests. Feel free to add more.

POP - multi label, only top label should be poppped

NHOP ECMP:

  • Multiple label next hops
  • Multiple label and IP next hops

LABEL ECMP

  • Load balancing for label, (src/dst IP remains same, change label)
  • Mupltiple labels. Verify ECMP, change the outermost label

LABEL ECMP - port flap

  • bring down a port - check load balancing
  • bring up the port - check load balancing

MPLS QoS

  • Verify QoS mapping between IP and MPLS at ingress
  • Verify QoS mapping between IP and MPLS at egress

MAX Labels test

  • pick 4 labels as MAX, push labels and verify forwarding

Negative test case:

  • Send MPLS traffic on i/f with no MPLS enabled - traffic must be dropped

tests/mpls/test_mpls.py Outdated Show resolved Hide resolved
tests/mpls/test_mpls.py Outdated Show resolved Hide resolved
tests/mpls/test_mpls.py Outdated Show resolved Hide resolved
tests/mpls/test_mpls.py Outdated Show resolved Hide resolved
tests/mpls/test_mpls.py Outdated Show resolved Hide resolved
tests/mpls/test_mpls.py Outdated Show resolved Hide resolved
tests/mpls/test_mpls.py Outdated Show resolved Hide resolved
tests/mpls/test_mpls.py Outdated Show resolved Hide resolved
tests/mpls/test_mpls.py Outdated Show resolved Hide resolved
tests/mpls/test_mpls.py Outdated Show resolved Hide resolved
tests/mpls/test_mpls.py Outdated Show resolved Hide resolved
Adding setup in conftest file
Addressing review comments.
Removing unused codes
Removeing unused code
@lgtm-com
Copy link

lgtm-com bot commented Mar 16, 2022

This pull request introduces 1 alert when merging a101ed0 into 5fecae8 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

Correction for lgtm
addressing lgtm review
@zhangyanzhao
Copy link
Contributor

@wangxin would you please help to approve if you are ok with the change? Thanks.

@zhangyanzhao
Copy link
Contributor

@SijiJ would you please to address all the comments to move forward? This is blocking the 202111 final delivery. Thanks.

@SijiJ
Copy link
Contributor Author

SijiJ commented Apr 26, 2022

@SijiJ would you please to address all the comments to move forward? This is blocking the 202111 final delivery. Thanks.

Moved all review comments to resolved.

tests/mpls/test_mpls.py Outdated Show resolved Hide resolved
tests/mpls/test_mpls.py Show resolved Hide resolved
tests/mpls/test_mpls.py Outdated Show resolved Hide resolved
@wangxin
Copy link
Collaborator

wangxin commented May 5, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wangxin wangxin merged commit 3522391 into sonic-net:master May 5, 2022
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.

7 participants