Add proposal for multi-DUT and multi-ASIC testing support by wangxin · Pull Request #2347 · sonic-net/sonic-mgmt

wangxin · 2020-10-15T09:53:26Z

Description of PR

Summary:
Fixes # (issue)

This is a documentation proposing changes to test infrastructure for multi-DUT and multi-ASIC testing support.

Type of change

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

Approach

What is the motivation for this PR?

SONiC is evolving to support multiple DUT and multiple ASIC systems. The test infrastructure also needs to be updated to support that new architecture.

How did you do it?

Added a document proposing changes to test infrastructure. This document is mainly based on work from Arvindsrinivasan Lakshminarasimhan and Sandeep Malhotra.

How did you verify/test it?

Any platform specific information?

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

Documentation

wangxin · 2020-10-15T10:16:25Z

@sanmalho-git Could you please help review as well?

lguohan · 2020-10-15T11:42:24Z

@wangxin, sonic-mgmt pr not running now?

sanmalho-git · 2020-10-15T15:26:07Z

I am assuming that the 'duthost' fixture going to be updated to return 'MultiAsicSonicHost'. If so, can we add the changed code for that fixture.

Assuming 'duthost' is a 'MultiAsicSonicHost', then with the approach above, we are giving the flexibility to enhance a test for multi-asic by using the 'duthost' fixture. If the test needs to be backward compatible to both single-asic DUT and multi-asic DUT, the call in the test would be

duthost.foo()

Now for single-asic DUT the return is a dictionary. But for multi-asic DUT, this is a list of dictionaries. We now have to deal with the two different data structures depending on whether we are multi-asic or single asic DUT.

daall · 2020-10-15T18:05:26Z

retest this please

wangxin · 2020-10-16T00:34:21Z

Yes, the duthost fixture will automatically be a MultiAsicSonicHost instance after the duthosts fixture is update to a list of MultiAsicSonicHost. It's because the duthost fixture is implemented as returning an item from the duthosts list according to dut_index in pytest request.session object. If dut_index is not available, always return the first item from duthosts as duthost.

The new duthost fixture can be used for both single and multi-ASIC DUT. For multi-ASIC DUT, for example, if we call

duthost.bgp_facts()

Without asic_index argument, the call will be delegated to SonicHost. Eventually, self.sonichost.bgp_facts() will be called. self.sonichost is an attribute of MultiAsicSonicHost. It refers to instance of the SonicHost class. The returned result will be just calling bgp_facts module without instance_id argument. It will be a dictionary same as calling bgp_facts without instance_id on single-ASIC DUT.

If we call

duthost.bgp_facts(asic_index=1)

With asic_index=1, method bgp_facts of SonicAsic class will be called for object self.asics[1]. self.asics is an attribute of MultiAsicSonicHost. It's a list of SonicAsic instances. The method bgp_facts of SonicAsic will eventually call self.sonichost.bgp_facts(instance_id=1). Here the original asic_index=1 is translated to instance_id=1 in SonicAsic.bgp_facts method. This self.sonichost is an attribute of SonicAsic. It is a reference to instance of the SonicHost class. The returned result will be calling bgp_facts module with instance_id=1 argument. It will be a dictionary same as calling bgp_facts with instance_id=1 on single-ASIC DUT.

If we call

duthost.bgp_facts(asic_index="all")

With asic_index="all", the bgp_facts method of object in self.asics list will be called. The returned result will be a list.

So, the key design here is that if asic_index argument is "all", then call the module for all ASICs and return the result in a list.
Without asic_index, the module is just called for the host (or global/default namespace).

sanmalho-git · 2020-10-16T21:42:43Z

Let's assume we have this call in the test case to ansible module 'foo' that is multi-asic aware.

duthost.foo(asic_index='all')

The key is that the same call should work for a single-asic, and multi-asic duthost.

If duthost represents a single asic pizza box, then this will eventually hit MultiAsicSonicHost._run_on_asics. With this, we will hit SonicAsic.foo (which has self.asic_index=0) with asic_index='all'. So, asic_index=0 is passed into 'foo' ansible module. At this time, the ansible module doesn't have the knowledge whether this asic_index 0 is to represent the 'global' namespace, or actually 'asic0'.

When prototyping this for bgp_facts ansible module, I see it trying to get bgp_facts on 'bgp0' instance - which doesn't exist for single asic pizza box, and thus we fail.

One way to solve this it to add a check for num_asic in the asic_index='all' case in MultiAsicSonicHost._run_on_asics method. If num_asic == 1, then call the module on self.sonichost, else call on SonicAsic:

    def _run_on_asics(self, *module_args, **complex_args):
        if "asic_index" not in complex_args:
            # Default ASIC/namespace
            return getattr(self.sonichost, self.attr)(*module_args, **complex_args)
        else:
            asic_index = complex_args.pop("asic_index")
            if type(asic_index) == int:
                # Specific ASIC/namespace
                return getattr(self.asics[asic_index], self.attr)(*module_args, **complex_args)
            elif type(asic_index) == str and asic_index.lower() == "all":
                # All ASICs/namespaces
                if self.sonichost.facts['num_asic'] == 1:
                    return [getattr(self.sonichost, self.attr)(*module_args, **complex_args)]
                return [getattr(asic, self.attr)(*module_args, **complex_args) for asic in self.asics]
            else:
                raise ValueError("Argument 'asic_index' must be an int or string 'all'.")

wangxin · 2020-10-19T07:09:39Z

@sanmalho-git Good catch! Thank you! I'll update the document.

lguohan · 2020-10-21T08:35:37Z