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

[Multi asic]:parameterize asic_index and dut_index #2245

Merged
merged 3 commits into from
Oct 27, 2020

Conversation

arlakshm
Copy link
Contributor

Signed-off-by: Arvindsrinivasan Lakshminarasimhan arlakshm@microsoft.com

Description of PR

Summary:
Fixes # (issue)

Type of change

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

Approach

What is the motivation for this PR?

This is PR is add support for testing multi asic platform.
In case of SONiC on multi asic platforms, each asic is bound to a namespace. The dataplane and control plane containers are replicated per namespace.
The tests which are testing functionality within a namespace, can be repeated on all namespaces by passing asic_index as parameter to the test. This way the testing logic remains. The test will be repeated N times, where the N is the num_asics.

Recently sonic-mgmt introduced the support for multiple duts in the topology. We are proposing to pass dut_index as parameter to the testcase. So, the test will be repeated on each dut in the topology.

If asic_index and dut_index is used in the test, the test will be repeated on each asic of each dut in the topology.

How did you do it?

  • parameterize asic_index
    To pass the asic_index or asic_frontend_index or dut_index as parameters to the test, the py test hook pytest_generate_tests is used. If the fixtures asic_index or asic_frontend_index are present then params are generated based on the _num_asics _ or frontend_asics values present for the DUT in the inventory file
    If these fields are not present then the DUT is treated as single asic device and None will be passed as asic_index or asic_frontend_index
    Example of multi asic DUT info in inventory
        multi_asic_1:
          ansible_host: 10.1.1.100
          ansible_hostv6: fec0::ffff:afa:7
          type: kvm
          hwsku: multi_asic
          ansible_password: password
          ansible_user: admin
          num_asics: 6
          frontend_asics : [0,1,2,3]
  • Add namespace option to the config_fact library

How did you verify/test it?

Logs from running on single ASIC DUT

============================= test session starts ==============================
platform linux2 -- Python 2.7.12, pytest-4.6.5, py-1.9.0, pluggy-0.13.1 -- /usr/bin/python
cachedir: .pytest_cache
metadata: {'Python': '2.7.12', 'Platform': 'Linux-4.10.0-42-generic-x86_64-with-Ubuntu-16.04-xenial', 'Packages': {'py': '1.9.0', 'pytest': '4.6.5', 'pluggy': '0.13.1'}, 'Plugins': {u'repeat': u'0.8.0', u'html': u'1.22.1', u'xdist': u'1.28.0', u'ansible': u'2.2.2', u'forked': u'1.3.0', u'metadata': u'1.10.0'}}
ansible: 2.8.12
rootdir: /data/public/my_repo/sonic-mgmt/tests, inifile: pytest.ini
plugins: ansible-2.2.2, forked-1.3.0, xdist-1.28.0, html-1.22.1, repeat-0.8.0, metadata-1.10.0
collecting ... 
----------------------------- live log collection ------------------------------
01:32:40 INFO conftest.py:generate_params_dut_index:495: Num of duts in testbed topology 1
01:32:40 INFO conftest.py:generate_param_asic_index:461: generating num_asics asic indicies for  DUT [[0]] in 
01:32:40 INFO conftest.py:generate_param_asic_index:485: dut_index 0 dut name str-s6000-acs-8  asics params = [None]
collected 1 item

bgp/test_bgp_fact.py::test_bgp_facts[0-None] 
-------------------------------- live log setup --------------------------------
01:32:40 INFO __init__.py:set_default:49: Completeness level not set during test execution. Setting to default level: CompletenessLevel.basic
01:32:40 INFO __init__.py:check_test_completeness:128: Test has no defined levels. Continue without test completeness checks
01:32:54 INFO conftest.py:disable_container_autorestart:447: Disable container autorestart
01:32:55 INFO conftest.py:creds:350: dut str-s6000-acs-8 belongs to groups [u'sonic', 'fanout']
01:32:55 INFO conftest.py:creds:362: skip empty var file ../ansible/group_vars/all/env.yml
01:32:55 INFO conftest.py:creds:362: skip empty var file ../ansible/group_vars/all/corefile_uploader.yml
01:32:56 INFO __init__.py:sanity_check:45: Start pre-test sanity check
01:32:56 INFO __init__.py:sanity_check:55: Found marker: m.name=topology, m.args=('any',), m.kwargs={}
01:32:56 INFO __init__.py:sanity_check:55: Found marker: m.name=device_type, m.args=('vs',), m.kwargs={}
01:32:56 INFO __init__.py:sanity_check:89: Sanity check settings: skip_sanity=True, check_items=set(['services', 'bgp', 'interfaces', 'processes', 'dbmemory']), allow_recover=False, recover_method=adaptive, post_check=False
01:32:56 INFO __init__.py:sanity_check:92: Skip sanity check according to command line argument or configuration of test script.
01:32:56 INFO __init__.py:loganalyzer:16: Log analyzer is disabled
PASSED                                                                   [100%]
------------------------------ live log teardown -------------------------------
01:33:04 INFO conftest.py:disable_container_autorestart:454: Recover container autorestart

Logs on multi asic DUT

============================= test session starts ==============================
platform linux2 -- Python 2.7.12, pytest-4.6.5, py-1.9.0, pluggy-0.13.1 -- /usr/bin/python
cachedir: .pytest_cache
metadata: {'Python': '2.7.12', 'Platform': 'Linux-4.10.0-42-generic-x86_64-with-Ubuntu-16.04-xenial', 'Packages': {'py': '1.9.0', 'pytest': '4.6.5', 'pluggy': '0.13.1'}, 'Plugins': {u'repeat': u'0.8.0', u'html': u'1.22.1', u'xdist': u'1.28.0', u'ansible': u'2.2.2', u'forked': u'1.3.0', u'metadata': u'1.10.0'}}
ansible: 2.8.12
rootdir: /data/public/my_repo/sonic-mgmt/tests, inifile: pytest.ini
plugins: ansible-2.2.2, forked-1.3.0, xdist-1.28.0, html-1.22.1, repeat-0.8.0, metadata-1.10.0
collecting ... 
----------------------------- live log collection ------------------------------
02:04:14 INFO conftest.py:generate_params_dut_index:495: Num of duts in testbed topology 1
02:04:14 INFO conftest.py:generate_param_asic_index:461: generating num_asics asic indicies for  DUT [[0]] in 
02:04:14 INFO conftest.py:generate_param_asic_index:485: dut_index 0 dut name multi_asic_1  asics params = [0, 1, 2, 3, 4, 5]
collected 6 items

bgp/test_bgp_fact.py::test_bgp_facts[0-0] 
-------------------------------- live log setup --------------------------------
02:04:14 INFO __init__.py:set_default:49: Completeness level not set during test execution. Setting to default level: CompletenessLevel.basic
02:04:14 INFO __init__.py:check_test_completeness:128: Test has no defined levels. Continue without test completeness checks
02:04:18 INFO conftest.py:disable_container_autorestart:447: Disable container autorestart
02:04:21 INFO conftest.py:creds:350: dut multi_asic_1 belongs to groups [u'sonic', 'fanout']
02:04:21 INFO conftest.py:creds:362: skip empty var file ../ansible/group_vars/all/env.yml
02:04:21 INFO conftest.py:creds:362: skip empty var file ../ansible/group_vars/all/corefile_uploader.yml
02:04:22 INFO __init__.py:sanity_check:45: Start pre-test sanity check
02:04:22 INFO __init__.py:sanity_check:55: Found marker: m.name=topology, m.args=('any',), m.kwargs={}
02:04:22 INFO __init__.py:sanity_check:55: Found marker: m.name=device_type, m.args=('vs',), m.kwargs={}
02:04:22 INFO __init__.py:sanity_check:89: Sanity check settings: skip_sanity=True, check_items=set(['services', 'bgp', 'interfaces', 'processes', 'dbmemory']), allow_recover=False, recover_method=adaptive, post_check=False
02:04:22 INFO __init__.py:sanity_check:92: Skip sanity check according to command line argument or configuration of test script.
02:04:22 INFO __init__.py:loganalyzer:16: Log analyzer is disabled
PASSED                                                                   [ 16%]
bgp/test_bgp_fact.py::test_bgp_facts[0-1] 
-------------------------------- live log setup --------------------------------
02:04:24 INFO __init__.py:set_default:49: Completeness level not set during test execution. Setting to default level: CompletenessLevel.basic
02:04:24 INFO __init__.py:check_test_completeness:128: Test has no defined levels. Continue without test completeness checks
02:04:24 INFO __init__.py:loganalyzer:16: Log analyzer is disabled
PASSED                                                                   [ 33%]
bgp/test_bgp_fact.py::test_bgp_facts[0-2] 
-------------------------------- live log setup --------------------------------
02:04:26 INFO __init__.py:set_default:49: Completeness level not set during test execution. Setting to default level: CompletenessLevel.basic
02:04:26 INFO __init__.py:check_test_completeness:128: Test has no defined levels. Continue without test completeness checks
02:04:26 INFO __init__.py:loganalyzer:16: Log analyzer is disabled
PASSED                                                                   [ 50%]
bgp/test_bgp_fact.py::test_bgp_facts[0-3] 
-------------------------------- live log setup --------------------------------
02:04:28 INFO __init__.py:set_default:49: Completeness level not set during test execution. Setting to default level: CompletenessLevel.basic
02:04:28 INFO __init__.py:check_test_completeness:128: Test has no defined levels. Continue without test completeness checks
02:04:28 INFO __init__.py:loganalyzer:16: Log analyzer is disabled
FAILED                                                                   [ 66%]
bgp/test_bgp_fact.py::test_bgp_facts[0-4] 
-------------------------------- live log setup --------------------------------
02:04:30 INFO __init__.py:set_default:49: Completeness level not set during test execution. Setting to default level: CompletenessLevel.basic
02:04:30 INFO __init__.py:check_test_completeness:128: Test has no defined levels. Continue without test completeness checks
02:04:30 INFO __init__.py:loganalyzer:16: Log analyzer is disabled
PASSED                                                                   [ 83%]
bgp/test_bgp_fact.py::test_bgp_facts[0-5] 
-------------------------------- live log setup --------------------------------
02:04:32 INFO __init__.py:set_default:49: Completeness level not set during test execution. Setting to default level: CompletenessLevel.basic
02:04:32 INFO __init__.py:check_test_completeness:128: Test has no defined levels. Continue without test completeness checks
02:04:32 INFO __init__.py:loganalyzer:16: Log analyzer is disabled
PASSED                                                                   [100%]
------------------------------ live log teardown -------------------------------
02:04:34 INFO conftest.py:disable_container_autorestart:454: Recover container autorestart


=================================== FAILURES ===================================
_____________________________ test_bgp_facts[0-3] ______________________________

duthosts = [<tests.common.devices.SonicHost object at 0x7f3aa26120d0>]
dut_index = 0, asic_index = 3

    def test_bgp_facts(duthosts, dut_index, asic_index):
        """compare the bgp facts between observed states and target state"""
    
        duthost = duthosts[dut_index]
        bgp_facts =duthost.bgp_facts(instance_id=asic_index)['ansible_facts']
        namespace = duthost.get_namespace_from_asic_id(asic_index)
        config_facts = duthost.config_facts(host=duthost.hostname, source="running",namespace=namespace)['ansible_facts']
    
        for k, v in bgp_facts['bgp_neighbors'].items():
            # Verify bgp sessions are established
>           assert v['state'] == 'established'
E           AssertionError: assert u'active' == 'established'
E             - active
E             + established

bgp/test_bgp_fact.py:18: AssertionError
=========================== short test summary info ============================
FAILED bgp/test_bgp_fact.py::test_bgp_facts[0-3] - AssertionError: assert u'a...
===================== 1 failed, 5 passed in 22.51 seconds ======================

Any platform specific information?

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

Documentation

Signed-off-by: Arvindsrinivasan Lakshminarasimhan <arlakshm@microsoft.com>
@arlakshm
Copy link
Contributor Author

retest this please

@sanmalho-git
Copy link
Contributor

conftest.py, function generate_param_asic_index:

  • doesn't handle multiple inventory files - eg --inventory "inventory1,inventory2"
  • Within an inventory it is assuming all hosts are defined under "all.children.sonic.hosts".
hosts = inv['all']['children']['sonic']['hosts']

We have different box types, and have grouped our inventory based on the box types. For example:

all:
  children:
    sonic:
      children:
        sonic_type1:
        sonic_type2:
      vars:
        sonic_version: v2

sonic_type1:
  hosts:
    box1:
      ansible_host: xxx.xxx.xxx.xxx
      num_asic: 3
    box2:
      ansible_host: xxx.xxx.xxx.xxx
      num_asic: 3

If we want to add the 'num_asic' data to box1, it won't work.

One possible solution is to define a function that takes care of this situation as below.

def get_host_data(request, host):
    inv_files = [inv_file.strip() for inv_file in request.config.getoption("ansible_inventory").split(",")]
    for a_inv_file in inv_files:
        with open(a_inv_file, 'r') as fh:
            inv = yaml.safe_load(fh)
            if 'hosts' in inv['all']['children']['sonic']:
                hosts = inv['all']['children']['sonic']['hosts']
                if host in hosts:
                    return hosts[host]

            if 'children' in inv['all']['children']['sonic']:
                children = inv['all']['children']['sonic']['children']
                for a_child in children:
                    hosts = inv[a_child]['hosts']
                    if host in hosts:
                        return hosts[host]
    return None

generate_param_asic_index then calls this function with a check if it is None.

def generate_param_asic_index(request, dut_indices, param_type):
    logging.info("generating {} asic indicies for  DUT [{}] in ".format(param_type, dut_indices))
    tbname = request.config.getoption("--testbed")
    tbfile = request.config.getoption("--testbed_file")
    if tbname is None or tbfile is None:
        raise ValueError("testbed and testbed_file are required!")

    tbinfo = TestbedInfo(tbfile)

    asic_index_params = [DEFAULT_ASIC_ID]

    for dut_id in dut_indices:
        dut = tbinfo.testbed_topo[tbname]["duts"][dut_id]
        inv_data = get_host_data(request, dut)
        if inv_data is not None:
            if param_type == ASIC_PARAM_TYPE_ALL and ASIC_PARAM_TYPE_ALL in inv_data:
                asic_index_params = range(int(inv_data[ASIC_PARAM_TYPE_ALL]))
            if param_type == ASIC_PARAM_TYPE_FRONTEND and ASIC_PARAM_TYPE_ALL in inv_data:
                asic_index_params = inv_data[ASIC_PARAM_TYPE_ALL]
            logging.info("dut_index {} dut name {}  asics params = {}".format(
                dut_id, dut, asic_index_params))
    return asic_index_params

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 with minor comments.

This approach is good for tests that just need to check something for each ASIC of each DUT. For more complicated scenarios, the more flexible DutHosts approach may fit better. Since the dut_index and asic_index fixtures are optional, I think both approaches can coexist.

The only problem is that test_bgp_facts.py may be updated by other people to use other approach.

if param_type == ASIC_PARAM_TYPE_ALL and ASIC_PARAM_TYPE_ALL in inv_data:
asic_index_params = range(int(inv_data[ASIC_PARAM_TYPE_ALL]))
if param_type == ASIC_PARAM_TYPE_FRONTEND and ASIC_PARAM_TYPE_ALL in inv_data:
asic_index_params = inv_data[ASIC_PARAM_TYPE_ALL]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be more robust to use ansible.inventory.manager.InventoryManager to load inventory file and get host variables. Example: https://github.com/Azure/sonic-mgmt/blob/master/ansible/library/testbed_vm_info.py#L91
Using yaml.safe_load assumes that the inventory file must be yaml format. But anyway it is OK to use yaml.safe_load for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to use the inventory manager.

inv_data = hosts[dut]
if param_type == ASIC_PARAM_TYPE_ALL and ASIC_PARAM_TYPE_ALL in inv_data:
asic_index_params = range(int(inv_data[ASIC_PARAM_TYPE_ALL]))
if param_type == ASIC_PARAM_TYPE_FRONTEND and ASIC_PARAM_TYPE_ALL in inv_data:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use elif here?

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 in the latest commit

@wangxin
Copy link
Collaborator

wangxin commented Oct 20, 2020

@arlakshm Could you fix the conflicts?

Signed-off-by: Arvindsrinivasan Lakshminarasimhan <arlakshm@microsoft.com>
Signed-off-by: Arvindsrinivasan Lakshminarasimhan <arlakshm@microsoft.com>
@arlakshm
Copy link
Contributor Author

conftest.py, function generate_param_asic_index:

  • doesn't handle multiple inventory files - eg --inventory "inventory1,inventory2"
  • Within an inventory it is assuming all hosts are defined under "all.children.sonic.hosts".
hosts = inv['all']['children']['sonic']['hosts']

We have different box types, and have grouped our inventory based on the box types. For example:

all:
  children:
    sonic:
      children:
        sonic_type1:
        sonic_type2:
      vars:
        sonic_version: v2

sonic_type1:
  hosts:
    box1:
      ansible_host: xxx.xxx.xxx.xxx
      num_asic: 3
    box2:
      ansible_host: xxx.xxx.xxx.xxx
      num_asic: 3

If we want to add the 'num_asic' data to box1, it won't work.

One possible solution is to define a function that takes care of this situation as below.

def get_host_data(request, host):
    inv_files = [inv_file.strip() for inv_file in request.config.getoption("ansible_inventory").split(",")]
    for a_inv_file in inv_files:
        with open(a_inv_file, 'r') as fh:
            inv = yaml.safe_load(fh)
            if 'hosts' in inv['all']['children']['sonic']:
                hosts = inv['all']['children']['sonic']['hosts']
                if host in hosts:
                    return hosts[host]

            if 'children' in inv['all']['children']['sonic']:
                children = inv['all']['children']['sonic']['children']
                for a_child in children:
                    hosts = inv[a_child]['hosts']
                    if host in hosts:
                        return hosts[host]
    return None

generate_param_asic_index then calls this function with a check if it is None.

def generate_param_asic_index(request, dut_indices, param_type):
    logging.info("generating {} asic indicies for  DUT [{}] in ".format(param_type, dut_indices))
    tbname = request.config.getoption("--testbed")
    tbfile = request.config.getoption("--testbed_file")
    if tbname is None or tbfile is None:
        raise ValueError("testbed and testbed_file are required!")

    tbinfo = TestbedInfo(tbfile)

    asic_index_params = [DEFAULT_ASIC_ID]

    for dut_id in dut_indices:
        dut = tbinfo.testbed_topo[tbname]["duts"][dut_id]
        inv_data = get_host_data(request, dut)
        if inv_data is not None:
            if param_type == ASIC_PARAM_TYPE_ALL and ASIC_PARAM_TYPE_ALL in inv_data:
                asic_index_params = range(int(inv_data[ASIC_PARAM_TYPE_ALL]))
            if param_type == ASIC_PARAM_TYPE_FRONTEND and ASIC_PARAM_TYPE_ALL in inv_data:
                asic_index_params = inv_data[ASIC_PARAM_TYPE_ALL]
            logging.info("dut_index {} dut name {}  asics params = {}".format(
                dut_id, dut, asic_index_params))
    return asic_index_params

@sanmalho-git
Add support of multiple Inventory files in the latest commit

@arlakshm
Copy link
Contributor Author

retest this please

@sanmalho-git
Copy link
Contributor

commit

conftest.py, function generate_param_asic_index:

  • doesn't handle multiple inventory files - eg --inventory "inventory1,inventory2"
  • Within an inventory it is assuming all hosts are defined under "all.children.sonic.hosts".
hosts = inv['all']['children']['sonic']['hosts']

We have different box types, and have grouped our inventory based on the box types. For example:

all:
  children:
    sonic:
      children:
        sonic_type1:
        sonic_type2:
      vars:
        sonic_version: v2

sonic_type1:
  hosts:
    box1:
      ansible_host: xxx.xxx.xxx.xxx
      num_asic: 3
    box2:
      ansible_host: xxx.xxx.xxx.xxx
      num_asic: 3

If we want to add the 'num_asic' data to box1, it won't work.
One possible solution is to define a function that takes care of this situation as below.

def get_host_data(request, host):
    inv_files = [inv_file.strip() for inv_file in request.config.getoption("ansible_inventory").split(",")]
    for a_inv_file in inv_files:
        with open(a_inv_file, 'r') as fh:
            inv = yaml.safe_load(fh)
            if 'hosts' in inv['all']['children']['sonic']:
                hosts = inv['all']['children']['sonic']['hosts']
                if host in hosts:
                    return hosts[host]

            if 'children' in inv['all']['children']['sonic']:
                children = inv['all']['children']['sonic']['children']
                for a_child in children:
                    hosts = inv[a_child]['hosts']
                    if host in hosts:
                        return hosts[host]
    return None

generate_param_asic_index then calls this function with a check if it is None.

def generate_param_asic_index(request, dut_indices, param_type):
    logging.info("generating {} asic indicies for  DUT [{}] in ".format(param_type, dut_indices))
    tbname = request.config.getoption("--testbed")
    tbfile = request.config.getoption("--testbed_file")
    if tbname is None or tbfile is None:
        raise ValueError("testbed and testbed_file are required!")

    tbinfo = TestbedInfo(tbfile)

    asic_index_params = [DEFAULT_ASIC_ID]

    for dut_id in dut_indices:
        dut = tbinfo.testbed_topo[tbname]["duts"][dut_id]
        inv_data = get_host_data(request, dut)
        if inv_data is not None:
            if param_type == ASIC_PARAM_TYPE_ALL and ASIC_PARAM_TYPE_ALL in inv_data:
                asic_index_params = range(int(inv_data[ASIC_PARAM_TYPE_ALL]))
            if param_type == ASIC_PARAM_TYPE_FRONTEND and ASIC_PARAM_TYPE_ALL in inv_data:
                asic_index_params = inv_data[ASIC_PARAM_TYPE_ALL]
            logging.info("dut_index {} dut name {}  asics params = {}".format(
                dut_id, dut, asic_index_params))
    return asic_index_params

@sanmalho-git
Add support of multiple Inventory files in the latest commit

Tested out your changes, and they work and look good.

@arlakshm arlakshm merged commit 4703756 into sonic-net:master Oct 27, 2020
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.

3 participants