Fix in 'nbrhosts' pytest fixture by yvolynets-mlnx · Pull Request #1594 · sonic-net/sonic-mgmt

yvolynets-mlnx · 2020-04-21T17:24:28Z

Signed-off-by: Yuriy Volynets [email protected]

Description of PR

Summary: Fix "nbrhosts" fixture to use inventory file with defined VMs
Fixes # (issue)

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Approach

How did you do it?

During run tests which uses "nbrhosts" fixture it was observed the following error:

    @pytest.fixture(scope="module")
    def nbrhosts(ansible_adhoc, testbed, creds):
        """
        Shortcut fixture for getting VM host
        """

        vm_base = int(testbed['vm_base'][2:])
        devices = {}
        for k, v in testbed['topo']['properties']['topology']['VMs'].items():
>           devices[k] = EosHost(ansible_adhoc, "VM%04d" % (vm_base + v['vm_offset']), creds['eos_login'], creds['eos_password'])

conftest.py:200:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
common/devices.py:362: in __init__
    AnsibleHostBase.__init__(self, ansible_adhoc, hostname, connection="network_cli")
common/devices.py:36: in __init__
    self.host = ansible_adhoc(become=True, connection=connection)[hostname]
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <pytest_ansible.host_manager.v28.HostManagerV28 object at 0x7fd207ec8410>, item = 'VM0604'

    def __getitem__(self, item):
        """Return a ModuleDispatcher instance described the provided `item`."""
        # Handle slicing
        if isinstance(item, slice):
            new_item = "all["
            if item.start is not None:
                new_item += str(item.start)
            new_item += '-'
            if item.stop is not None:
                new_item += str(item.stop)
            item = new_item + ']'

        if item in self.__dict__:
            return self.__dict__[item]
        else:
            if not self.has_matching_inventory(item):
>               raise KeyError(item)
E               KeyError: 'VM0604'

Description:
For now main inventory file which is used for test cases run is located in "ansible/inventory" file.
But also for now Virtual Machine hosts are defined in separate "ansible/veos" inventory file.
So to have possibility in test cases to simultaneously use devices like DUT/Fanouts and VMs there should be possibility to specify which inventory file to use for specific device type.

Base item what is used for create interface to the devices is pytest-ansible fixture "ansible_adhoc".
It uses inventory file which is specified by "--inventory" option.
For now as main inventory what is passed for test case run is "ansible/inventory", so it was added global variable which points to another inventory with defined VMs.

Later this can be enhanced by combining inventory files or by adding pytest option for passing specific inventory files.

How did you verify/test it?

Tested on the local setup.

Any platform specific information?

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

Documentation

Signed-off-by: Yuriy Volynets <[email protected]>

lgtm-com · 2020-04-21T17:40:17Z

This pull request fixes 1 alert when merging bed8be4 into 2254d9b - view on LGTM.com

fixed alerts:

1 for Unused import

lguohan · 2020-04-22T09:25:52Z

when we run pytest, we specify the inventory option, such as --inventory veos.vtb. are you saying you need hardcode another inventory file? if you need to run the test on both testbed, why not combine two inventory files?

what if the ansible/inventory file is not available on the disk. it will give errors.

lguohan · 2020-04-22T09:26:39Z

tests/conftest.py

                  'common.plugins.psu_controller',
                  'common.plugins.sanity_check')
-
+VEOS_INVENTORY = os.path.normpath(os.path.join(os.path.split(__file__)[0], "../ansible/veos"))


it does not look like a correct approach to hardcode another inventory file.

yvolynets-mlnx · 2020-04-22T10:35:53Z

when we run pytest, we specify the inventory option, such as --inventory veos.vtb. are you saying you need hardcode another inventory file? if you need to run the test on both testbed, why not combine two inventory files?

what if the ansible/inventory file is not available on the disk. it will give errors.

"are you saying you need hardcode another inventory file?"
No. But wee need a way to use info from different inventory files at the same time.

So this is a fix for current implementation, which does not change anything except extends "nbrhosts" fixture.
Currently there is several different inventory files, but at the same time can be used only one of the inventory files. But in the code there is a need to have them both used.
So this was added just for specific pytest fixture which uses inventory with defined VMs.

"if you need to run the test on both testbed, why not combine two inventory files?"
Agree.

So how do you suggest to proceed with this fix?

lguohan · 2020-04-23T08:47:47Z

by design ansible use one inventory file, why not stick to that design guide. another way is to introduce dynamic inventory and then basically introduce another level of abstraction to allow us to define whatever inventory format we want.

lguohan · 2020-04-23T08:48:17Z

@yxieca , @wangxin to chime in.

yxieca · 2020-04-23T15:23:01Z

@yvolynets-mlnx I am with @lguohan , I don't fully understand the need of using more than 1 inventory file. Why VMs were defined in another inventory? Are these VMs serving another test in the second inventory?

If so, are these VMs the same set of VMs supporting another DUT in another inventory?

If these 2 sets of VMs just looks same but they are different instances, they the information can be duplicated into 2 inventories. If they are the same set of VMs, then I think we should merge the 2 inventory files. There must be reason that you didn't take merging inventory approach. Can you elaborate the difficulty of doing so?

wangxin · 2020-04-24T07:01:49Z

I agree that hard code another inventory file in conftest.py is not a good design.

by design ansible use one inventory file, why not stick to that design guide. another way is to introduce dynamic inventory and then basically introduce another level of abstraction to allow us to define whatever inventory format we want.

But regarding the VMs definition, it resides in two example inventory files:

ansible/veos
ansible/veos.vtb

And in ansible/inventory, it only contains SONiC switch, PTF and PDU definitions. I think we are following these examples.

So, I have couple of questions:

It looks like ansible/veos and ansible/veos.vtb are duplicated? We should at least remove one of them?
Shall the ansible/inventory file be removed as well and put all its content in ansible/veos.vtb or ansible/veos to merge them?

yvolynets-mlnx · 2020-04-24T09:21:10Z

@yxieca
@yvolynets-mlnx I am with @lguohan , I don't fully understand the need of using more than 1 inventory file. Why VMs were defined in another inventory? - I agree with you and have the same questions :)

Are these VMs serving another test in the second inventory? - yes, because DUT defined in one inventory but VMs in another.

There must be reason that you didn't take merging inventory approach. Can you elaborate the difficulty of doing so? - I didn't know why there is several inventory files with separated definition of VMs and DUTs when pytest-ansible by design use one inventory file. So to not break existed approach I decided to fix it in such way.

@yxieca @lguohan @wangxin
So what approach to fix this issue should be used?
Create one combined inventory file?

yxieca · 2020-04-24T16:05:37Z

@yvolynets-mlnx without investigating further. I suspect following:

inventory 'inventory' defined some VM information overlapping with the other inventory test, I think in this case veos.vtb. I don't think merging would be a good idea.

The reason for the 2 inventories to exist, is that 'inventory' is intended for physical DUTs. 'veos.vtb' is intended for VS tests. In the past, I noticed that veos.vtb defined duplicate VM names collides into another inventory file we used internally. after addressing the issue of reading proper inventory file. this issue was addressed.

I think what you are facing here, is that veos.vtb is lack of certain definition (e.g. VM), and these definition happen to look the same as what was in 'inventory' but they are different instances.

I suggest you add whatever you need into the veos.vtb and not changing 'inventory'. We do need to find a non-intrusive way to rename veos.vtb resources to not collide with physical testbeds in the future.

Please let me know if my guess is different from reality.

Blueve · 2020-04-29T01:54:42Z

The inventory file was designed for supporting a specific test, support dynamic inventory file injection seems not much help here.
I am guessing the one reason why the vmhosts and dut related hosts are separated is to reuse a part of inventory file for different tests? Just like this:

TestA: inventoryA + veos
TestB: inventoryB + veos

In other hand, the playbook version tests are not using ansible inventory file to access vmhost, instead they use mgmtip and user/pwd to access directly, that's why no same failure happens on original tests.

If so, we can have a script to build a complete inventory file from template, and make a pre-test script to generate the full inventory file before do testing instead of combine them in test runtime.

In my mind, the inventory file is a kind of "input" rather than code. It is make sense to get a false result when you put false input. I vote for fixing the inventory file.

wangxin · 2020-04-30T13:45:43Z

I got another idea of fixing the issue of DUT host and VM host info in different inventory files.

Ansible supports accepting multiple inventory files. But pytest-ansible always passes a single string to argument sources of ansible.inventory.manager.InventoryManager. Indeed the sources argument can be a list. The approach to fix this is to add an auto-use fixture in sonic-mgmt/tests/conftest.py. The fixture can be like below:

@pytest.fixture(scope="session", autouse=True)
def enhance_inventory(request):
    inv = request.config.getoption("ansible_inventory")
    einv = inv.split(",")
    try:
        setattr(request.config.option, "ansible_inventory", einv)
    except AttributeError:
        logger.error("Failed to set enhanced 'ansible_inventory' to request.config.option")

With this fixture, we can pass comma separated inventory files to --inventory CLI argument. For example:

py.test --inventory ../ansible/inventory,../ansible/veos  --host-pattern all --testbed vms-kvm-t0 --testbed_file vtestbed.csv --test_example.py

The enhance_inventory fixture can split single string ../ansible/inventory,../ansible/veos to a list. Then the list will be passed to ansible.inventory.manager.InventoryManager.

If we use this approach, fixture conn_graph_facts in tests/common/fixtures/conn_graph_facts.py need to be refactored. Because this fixture assumes single inventory file is used, and uses that inventory file name to look up corresponding lab_conn_graph.xml file from ansible/group_vars/all/inv_mapping.json.

@yxieca @Blueve What do you think?

yxieca · 2020-04-30T15:42:41Z

I got another idea of fixing the issue of DUT host and VM host info in different inventory files.

Ansible supports accepting multiple inventory files. But pytest-ansible always passes a single string to argument sources of ansible.inventory.manager.InventoryManager. Indeed the sources argument can be a list. The approach to fix this is to add an auto-use fixture in sonic-mgmt/tests/conftest.py. The fixture can be like below:
@pytest.fixture(scope="session", autouse=True)
def enhance_inventory(request):
    inv = request.config.getoption("ansible_inventory")
    einv = inv.split(",")
    try:
        setattr(request.config.option, "ansible_inventory", einv)
    except AttributeError:
        logger.error("Failed to set enhanced 'ansible_inventory' to request.config.option")
With this fixture, we can pass comma separated inventory files to --inventory CLI argument. For example:
py.test --inventory ../ansible/inventory,../ansible/veos  --host-pattern all --testbed vms-kvm-t0 --testbed_file vtestbed.csv --test_example.py
The enhance_inventory fixture can split single string ../ansible/inventory,../ansible/veos to a list. Then the list will be passed to ansible.inventory.manager.InventoryManager.

If we use this approach, fixture conn_graph_facts in tests/common/fixtures/conn_graph_facts.py need to be refactored. Because this fixture assumes single inventory file is used, and uses that inventory file name to look up corresponding lab_conn_graph.xml file from ansible/group_vars/all/inv_mapping.json.

@yxieca @Blueve What do you think?

Nice!

yvolynets-mlnx · 2020-05-05T14:20:50Z

I got another idea of fixing the issue of DUT host and VM host info in different inventory files.
Ansible supports accepting multiple inventory files. But pytest-ansible always passes a single string to argument sources of ansible.inventory.manager.InventoryManager. Indeed the sources argument can be a list. The approach to fix this is to add an auto-use fixture in sonic-mgmt/tests/conftest.py. The fixture can be like below:
@pytest.fixture(scope="session", autouse=True)
def enhance_inventory(request):
    inv = request.config.getoption("ansible_inventory")
    einv = inv.split(",")
    try:
        setattr(request.config.option, "ansible_inventory", einv)
    except AttributeError:
        logger.error("Failed to set enhanced 'ansible_inventory' to request.config.option")
With this fixture, we can pass comma separated inventory files to --inventory CLI argument. For example:
py.test --inventory ../ansible/inventory,../ansible/veos  --host-pattern all --testbed vms-kvm-t0 --testbed_file vtestbed.csv --test_example.py
The enhance_inventory fixture can split single string ../ansible/inventory,../ansible/veos to a list. Then the list will be passed to ansible.inventory.manager.InventoryManager.
If we use this approach, fixture conn_graph_facts in tests/common/fixtures/conn_graph_facts.py need to be refactored. Because this fixture assumes single inventory file is used, and uses that inventory file name to look up corresponding lab_conn_graph.xml file from ansible/group_vars/all/inv_mapping.json.
@yxieca @Blueve What do you think?
Nice!

Idea is good. But in such case there is a need to some how distinguish which of the provided inventory files should be used for: SonicHost, EosHost and another possible hosts?
It can be distinguished by inventory name, like: "veos" or "inventory", however it does not look reliable.

@yxieca @Blueve @wangxin
Thoughts?

wangxin · 2020-05-06T06:31:18Z

If use my proposed approach, the InventoryManager object created by pytest-ansible will accept multiple inventory files as sources. Then it should be able to "merge" the inventory files. When we create SonicHost, EosHost or other hosts from ansible_adhoc, we do not need to care about in which inventory file the host is defined.

yvolynets-mlnx · 2020-05-06T11:33:52Z

If use my proposed approach, the InventoryManager object created by pytest-ansible will accept multiple inventory files as sources. Then it should be able to "merge" the inventory files. When we create SonicHost, EosHost or other hosts from ansible_adhoc, we do not need to care about in which inventory file the host is defined.

In such case it is good approach, I will update code with this new approach.
Thank you.

wangxin · 2020-05-06T14:02:58Z

@yvolynets-mlnx Actually, I have already created a PR using the approach: #1645
Could you please help review?

yxieca · 2020-05-07T20:07:53Z

Do we still need this change?

yvolynets-mlnx · 2020-05-08T15:48:58Z

Do we still need this change?

Let me check that this #1645 PR fixed this issue, then I will close it.

yvolynets-mlnx · 2020-05-15T10:31:34Z

Do we still need this change?

Let me check that this #1645 PR fixed this issue, then I will close it.

Verified, now its fixed by #1645 PR.

Including below commits: 36f7332 2021-01-14 | modified ERR log to NOTICE log for FDB notification failure after VLAN delete (sonic-net#1595) [madhanmellanox] c21c883 2021-01-12 | [ci]: download artifacts from master branch build (sonic-net#1597) [lguohan] a1d03a4 2021-01-12 | [fgnhgorch] Match mode changes for Fine Grained ECMP (sonic-net#1565) [anish-n] 1b65f3d 2021-01-12 | [ci]: use sonicbld pool (sonic-net#1594) [lguohan] 48ae866 2021-01-12 | [pfcwd] Update PFC storm detection logic for Mellanox platforms (sonic-net#1586) [Volodymyr Samotiy] 850001f 2021-01-12 | [FPMSYNCD] Evpn/Vxlan related changes to support FRR7.5(sonic-net#1585) [KISHORE KUNAL] 64ca9bb 2021-01-12 | [ci]: only copy artifacts when build is successful (sonic-net#1590) [lguohan] 17d0dae 2021-01-11 | [Fdborch] Fix for arm compilation (sonic-net#1592) [Prince Sunny] 693a02c 2021-01-08 | [gearbox] Add support for "hwinfo" field (sonic-net#1547) [Baptiste Covolato] 7e3b2c6 2021-01-09 | [Evpn Warmreboot] Added Dependancy check logic in VrfMgr (sonic-net#1466) [nkelapur] a960e2e 2021-01-09 | [Orchagent]: FdbOrch changes for EVPN VXLAN (sonic-net#1275) [Pankaj Jain] 097cfda 2021-01-08 | [swss test] update setup guide for swss tests (sonic-net#1582) [Ying Xie] b42253a 2021-01-05 | Fix for armhf build (sonic-net#1580) [Qi Luo] d8c1465 2021-01-05 | [dvs] Update/disable DVS tests with new FRR 7.5 behavior (sonic-net#1579) [Danny Allen] f6c7422 2021-01-05 | ASIC internal temperature sensors support (sonic-net#1517) [Santhosh Kumar T] 0aa9ef2 2021-01-01 | Simply by auto iterator type, because we will tune the return types of library functions (sonic-net#1577) [Qi Luo] 773238b 2020-12-31 | [build]: Fix format string for size_t (sonic-net#1576) [Qi Luo] 7ba4e43 2020-12-30 | [fgnhgorch] add warm reboot support for fgnhg (sonic-net#1538) [weixchen1215] 4cf6617 2020-12-30 | [ci]: add build for arm64 and armhf (sonic-net#1572) [lguohan] 6ebc0ed 2020-12-29 | [ci]: add azure-pipeline for amd64 (sonic-net#1571) [lguohan] e32b9d0 2020-12-29 | [FDBSYNCD] Added pytest for fdbsyncd (sonic-net#1560) [KISHORE KUNAL] a43f6be 2020-12-30 | [crm] Add support for snat, dnat and ipmc crm resources (sonic-net#1511) [Prabhu Sreenivasan] 7fc3888 2020-12-29 | PY Test script for EVPN L3 VxLAN (sonic-net#1330) [Tapash Das] 6eb36d9 2020-12-27 | vlanmgr changes related to EVPN VxLan warmboot (sonic-net#1460) [anilkpan] Signed-off-by: Guohan Lu <[email protected]>

…nic-net#7563) * [202012][swss/swss-common/utilities/platform-daemons] Update submodule sonic-swss - [flex-counters] Delay flex counters stats init for faster boot time [202012] (sonic-net#1736) sonic-swss-common - [swig] allow threads (sonic-net#477) sonic-utilities - [sfpshow] Gracefully handle improper 'specification_compliance' field (sonic-net#1594) sonic-platform-daemons - [xcvrd] Change the y_cable presence logic to use "mux_cable" table as identifier from Config DB (sonic-net#176) - [xcvrd] Enhance Media Settings (sonic-net#177) Signed-off-by: Danny Allen <[email protected]>

97d971372fac773b98d46bb8f800df7b845e518e (HEAD -> 201911, origin/201911) [sfpshow] Gracefully handle improper 'specification_compliance' field (sonic-net#1594) (sonic-net#1729) 2099c73cea81ff4524e680b6f9335c0b0f13b94e [CLI] Implement null_route_helper script (sonic-net#1740) b56659175986fe0e5b82c6bd6b3dde163164777b [minigraph][port_config] Consume port_config.json while reloading minigraph (sonic-net#1725) e840c42da2a40db2bf993672271f6b75c51c426f Change the method name to align with master, reduce diverge issue (sonic-net#1703) Signed-off-by: Abhishek Dosi <[email protected]>

yvolynets-mlnx added 2 commits April 21, 2020 18:22

Fixed usage of inventory in 'nbrhosts' fixture

f457b06

Signed-off-by: Yuriy Volynets <[email protected]>

Make path independent from directory run

bed8be4

Signed-off-by: Yuriy Volynets <[email protected]>

yvolynets-mlnx changed the title ~~Fix test nbr health~~ Fix in 'nbrhosts' pytest fixture Apr 21, 2020

lguohan reviewed Apr 22, 2020

View reviewed changes

yvolynets-mlnx mentioned this pull request Apr 29, 2020

[pytest/nbrhost]: add neighbor config info in nbrhost class #1621

Merged

3 tasks

wangxin mentioned this pull request May 6, 2020

Add support of specifying multiple inventory files #1645

Merged

3 tasks

wangxin requested review from Blueve and yxieca May 6, 2020 09:38

yvolynets-mlnx closed this May 15, 2020

Conversation

yvolynets-mlnx commented Apr 21, 2020 • edited by lguohan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Type of change

Approach

How did you do it?

How did you verify/test it?

Any platform specific information?

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

Documentation

Uh oh!

lgtm-com bot commented Apr 21, 2020

Uh oh!

lguohan commented Apr 22, 2020

Uh oh!

lguohan Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

yvolynets-mlnx commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lguohan commented Apr 23, 2020

Uh oh!

lguohan commented Apr 23, 2020

Uh oh!

yxieca commented Apr 23, 2020

Uh oh!

wangxin commented Apr 24, 2020

Uh oh!

yvolynets-mlnx commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yxieca commented Apr 24, 2020

Uh oh!

Blueve commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wangxin commented Apr 30, 2020

Uh oh!

yxieca commented Apr 30, 2020

Uh oh!

yvolynets-mlnx commented May 5, 2020

Uh oh!

wangxin commented May 6, 2020

Uh oh!

yvolynets-mlnx commented May 6, 2020

Uh oh!

wangxin commented May 6, 2020

Uh oh!

yxieca commented May 7, 2020

Uh oh!

yvolynets-mlnx commented May 8, 2020

Uh oh!

yvolynets-mlnx commented May 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yvolynets-mlnx commented Apr 21, 2020 •

edited by lguohan

Loading

yvolynets-mlnx commented Apr 22, 2020 •

edited

Loading

yvolynets-mlnx commented Apr 24, 2020 •

edited

Loading

Blueve commented Apr 29, 2020 •

edited

Loading