Skip to content

[multi asic] Change the fib test to support multi asic platform#3214

Merged
arlakshm merged 6 commits intosonic-net:masterfrom
arlakshm:masic_fib
Apr 12, 2021
Merged

[multi asic] Change the fib test to support multi asic platform#3214
arlakshm merged 6 commits intosonic-net:masterfrom
arlakshm:masic_fib

Conversation

@arlakshm
Copy link
Contributor

Description of PR

Summary:
Fixes # (issue)

Type of change

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

Approach

What is the motivation for this PR?

The PR has changes to run the tests in test_fib.py on multi asic platforms

How did you do it?

The PR has following changes.

  • Update the fixtures in the test_fib.py for multi asic platforms
  • Add a new options ignore_ttl to the ptf fib tests.

How did you verify/test it?

Verify on multi asic

arlakshm@b65425d3c0c1:/data/repos/public/sonic-mgmt/tests$ ./run_tests.sh -d multi_asic_1 -n vms13-t1-lag-2 -c fib/test_fib.py -i /data/repos/files/veos_vtb -f /data/repos/files/vtestbed.csv -l WARNING -k debug -u -m individual -t t1,any -e '--deep_clean --disable_loganalyzer --skip_sanity '
=== Running tests individually ===
/usr/local/lib/python2.7/dist-packages/ansible/parsing/vault/__init__.py:44: CryptographyDeprecationWarning: Python 2 is no longer supported by the Python core team. Support for it is now deprecated in cryptography, and will be removed in a future release.
  from cryptography.exceptions import InvalidSignature
================================================================================= test session starts ==================================================================================
platform linux2 -- Python 2.7.17, pytest-4.6.5, py-1.9.0, pluggy-0.13.1
ansible: 2.8.12
rootdir: /data/repos/public/sonic-mgmt/tests, inifile: pytest.ini
plugins: forked-1.3.0, xdist-1.28.0, html-1.22.1, metadata-1.10.0, repeat-0.9.1, ansible-2.2.2
collected 3 items

fib/test_fib.py::test_basic_fib[True-True-1514] PASSED                                                                                                                           [ 33%]
fib/test_fib.py::test_hash[ipv4] PASSED                                                                                                                                          [ 66%]
fib/test_fib.py::test_hash[ipv6] PASSED                                                                                                                                          [100%]

---------------------------------------------------- generated xml file: /data/repos/public/sonic-mgmt/tests/logs/fib/test_fib.xml -----------------------------------------------------
============================================================================= 3 passed in 1818.33 seconds ==============================================================================
+ exit 0
arlakshm@b65425d3c0c1:/data/repos/public/sonic-mgmt/tests$

Any platform specific information?

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

Documentation

@arlakshm arlakshm requested a review from a team as a code owner March 25, 2021 18:56
@arlakshm arlakshm requested review from smaheshm and wangxin March 25, 2021 18:57
Signed-off-by: Arvindsrinivasan Lakshminarasimhan <[email protected]>

asic = duthost.asic_instance(asic_index)

asic.shell("{} redis-dump -d 0 -k 'ROUTE*' -y > /tmp/fib.{}.txt".format(asic._ns_arg, timestamp))
Copy link
Contributor

@smaheshm smaheshm Mar 29, 2021

Choose a reason for hiding this comment

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

why not use "asic.command()". No need to use "asic._ns_arg" which looks like a private attribute as per convention.

duthost.asic_instance(asic_index).command("redis-dump -d 0 -k 'ROUTE*' -y > /tmp/fib.{}.txt")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smaheshm the ansible command module doesnt work with >
https://github.com/ansible/ansible/blob/f9f839fa08eee46ad7a86d6cbc7519541a50c7ef/lib/ansible/modules/command.py#L22. shell module is recommended for such cases

Change the ns_arg to public variable

complex_args['namespace'] = self.namespace
return self.sonichost.config_facts(*module_args, **complex_args)


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remote extra space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

# ignore the prefix, if the prefix nexthop is not a frontend port
if 'members' in po[ifname]:
if 'role' in ports[po[ifname]['members'][0]] and ports[po[ifname]['members'][0]]['role'] == 'Int':
skip = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we checking if prefixes from T0 ASICs are learned on T1 ASICS, and vice versa? We are removing all the prefixes that have next hop as the backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smaheshm for this test purpose there is no difference if the route is learnt from T0 or T2. We need to build the fib table with all the external nexthops for the ptf tests to validate the packet forwarding

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. We should add test case to verify all ASICs have the same prefixes in the FIB if not already there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use -64-lag topology for multi asic platform testing. In this topology some ASICs T2 neighbors and some have T0 neighbors. This covers the case of more than one ASIC having the same prefix.

Signed-off-by: Arvindsrinivasan Lakshminarasimhan <[email protected]>
@arlakshm
Copy link
Contributor Author

arlakshm commented Apr 1, 2021

@smaheshm , @wangxin can you review this PR and approve if not more comments

Copy link
Contributor

@smaheshm smaheshm left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Collaborator

@wangxin wangxin left a comment

Choose a reason for hiding this comment

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

LGTM, @arlakshm, can you address the merge conflicts?

@arlakshm arlakshm merged commit c1060f4 into sonic-net:master Apr 12, 2021
saravanansv pushed a commit to saravanansv/sonic-mgmt that referenced this pull request May 6, 2021
…c-net#3214)

The PR has following changes.
- Update the fixtures in the test_fib.py for multi asic platforms
- Add a new options ignore_ttl to the ptf fib tests.

Signed-off-by: Arvindsrinivasan Lakshminarasimhan <[email protected]>
vmittal-msft pushed a commit to vmittal-msft/sonic-mgmt that referenced this pull request Sep 28, 2021
…c-net#3214)

The PR has following changes.
- Update the fixtures in the test_fib.py for multi asic platforms
- Add a new options ignore_ttl to the ptf fib tests.

Signed-off-by: Arvindsrinivasan Lakshminarasimhan <[email protected]>
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.

3 participants