From 2fd73948f9d0101b8bf3eaf32778baf4f4143e34 Mon Sep 17 00:00:00 2001 From: sanmalho Date: Mon, 21 Dec 2020 17:23:01 -0500 Subject: [PATCH 1/3] Added fixtures for selecting nodes from multi-duts based on card type/hwsku. Added the following fixtures that can be used for parameterizing tests for chassis: - rand_one_frontend_dut_hostname - returns a random dut that is a frontend node. - enum_supervisor_dut_hostname - returns a dut that is a supervisor node. For pizza box (single dut) this will the pizza box itself. - A supervisor node is defined as having 'type' variable in the inventory as 'supervisor' - For pizza box (single dut) the single dut itself will be the supervisor node. - enum_frontend_dut_hostname - returns all duts that are frontend nodes - A frontend node is defined as a node that is not a supervisor. - enum_rand_one_per_hwsku_frontend_hostname - returns 1 random frontend dut per hwsksu amongst all the frontend nodes. - hwsku for a node is derived from the 'hwsku' or 'sonic_hwsku' variable in the inventory for the node. To achieve the above: - Added dut_utils under common/helpers to: - get_inventory_variable - get the value of a variable in the inventory for a given node. - is_supervisor_node - check if 'type' variable is defined in the inventory for the node and is 'supervisor' - is_frontend_node - check if node is not a supervisor node. - Modified SonicHost.is_supervisor_node to use dut_utils.is_supervisor_node. --- tests/common/devices.py | 9 ++-- tests/common/helpers/dut_utils.py | 32 ++++++++++++ tests/common/utilities.py | 14 +++++ tests/conftest.py | 87 ++++++++++++++++++++++++++++++- 4 files changed, 136 insertions(+), 6 deletions(-) create mode 100644 tests/common/helpers/dut_utils.py diff --git a/tests/common/devices.py b/tests/common/devices.py index 460d08b96f0..b284a65d136 100644 --- a/tests/common/devices.py +++ b/tests/common/devices.py @@ -24,6 +24,7 @@ from errors import RunAnsibleModuleFail from errors import UnsupportedAnsibleModule from tests.common.helpers.constants import DEFAULT_ASIC_ID, DEFAULT_NAMESPACE, NAMESPACE_PREFIX +from tests.common.helpers.dut_utils import is_supervisor_node # HACK: This is a hack for issue https://github.com/Azure/sonic-mgmt/issues/1941 and issue # https://github.com/ansible/pytest-ansible/issues/47 @@ -354,11 +355,9 @@ def is_supervisor_node(self): the inventory, and it is 'supervisor', then return True, else return False. In future, we can change this logic if possible to derive it from the DUT. """ - if 'type' in self.host.options["inventory_manager"].get_host(self.hostname).get_vars(): - node_type = self.host.options["inventory_manager"].get_host(self.hostname).get_vars()["type"] - if node_type is not None and node_type == 'supervisor': - return True - return False + im = self.host.options['inventory_manager'] + inv_files = im._sources + return is_supervisor_node(inv_files, self.hostname) def is_frontend_node(self): """Check if the current node is a frontend node in case of multi-DUT. diff --git a/tests/common/helpers/dut_utils.py b/tests/common/helpers/dut_utils.py new file mode 100644 index 00000000000..c54e7aede34 --- /dev/null +++ b/tests/common/helpers/dut_utils.py @@ -0,0 +1,32 @@ +from tests.common.utilities import get_host_visible_vars + + +def is_supervisor_node(inv_files, hostname): + """Check if the current node is a supervisor node in case of multi-DUT. + @param inv_files: List of inventory file paths, In tests, + you can be get it from get_inventory_files in tests.common.utilities + @param hostname: hostname as defined in the inventory + Returns: + Currently, we are using 'type' in the inventory to make the decision. If 'type' for the node is defined in + the inventory, and it is 'supervisor', then return True, else return False. In future, we can change this + logic if possible to derive it from the DUT. + """ + node_type = get_host_visible_vars(inv_files, hostname, variable='type') + if node_type and node_type == 'supervisor': + return True + return False + + +def is_frontend_node(inv_files, hostname): + """Check if the current node is a frontend node in case of multi-DUT. + @param inv_files: List of inventory file paths, In tests, + you can be get it from get_inventory_files in tests.common.utilities + @param hostname: hostname as defined in the inventory + Returns: + True if it is not any other type of node. Currently, the only other type of node supported is 'supervisor' + node. If we add more types of nodes, then we need to exclude them from this method as well. + """ + return not is_supervisor_node(inv_files, hostname) + + + diff --git a/tests/common/utilities.py b/tests/common/utilities.py index 2a3f0dc0b09..0679a3ef5b8 100644 --- a/tests/common/utilities.py +++ b/tests/common/utilities.py @@ -144,6 +144,20 @@ def get_variable_manager(inv_files): return VariableManager(loader=DataLoader(), inventory=get_inventory_manager(inv_files)) +def get_inventory_files(request): + """Use request.config.getoption('ansible_inventory') to the get list of inventory files. + The 'ansible_inventory' option could have already been converted to a list by #enchance_inventory fixture. + Args: + request: request paramater for pytest. + """ + if isinstance(request.config.getoption("ansible_inventory"), list): + # enhance_inventory fixture changes ansible_inventory to a list. + inv_files = request.config.getoption("ansible_inventory") + else: + inv_files = [inv_file.strip() for inv_file in request.config.getoption("ansible_inventory").split(",")] + return inv_files + + def get_host_vars(inv_files, hostname, variable=None): """Use ansible's InventoryManager to get value of variables defined for the specified host in the specified inventory files. diff --git a/tests/conftest.py b/tests/conftest.py index 79a096486ca..dd30f13e9ed 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -20,6 +20,9 @@ from tests.common.helpers.dut_ports import encode_dut_port_name from tests.common.devices import DutHosts from tests.common.testbed import TestbedInfo +from tests.common.utilities import get_inventory_files, get_host_visible_vars +from tests.common.helpers.dut_utils import is_supervisor_node, is_frontend_node + from tests.common.connections import ConsoleHost @@ -192,6 +195,16 @@ def rand_one_dut_hostname(request): dut_hostnames = random.sample(dut_hostnames, 1) return dut_hostnames[0] +@pytest.fixture(scope="module") +def rand_one_frontend_dut_hostname(request): + """ + """ + frontend_dut_hostnames = generate_params_frontend_hostname(request) + if len(frontend_dut_hostnames) > 1: + dut_hostnames = random.sample(frontend_dut_hostnames, 1) + return dut_hostnames[0] + + @pytest.fixture(scope="module", autouse=True) def reset_critical_services_list(duthosts): """ @@ -561,7 +574,7 @@ def get_host_data(request, dut): This function parses multple inventory files and returns the dut information present in the inventory ''' inv_data = None - inv_files = [inv_file.strip() for inv_file in request.config.getoption("ansible_inventory").split(",")] + inv_files = get_inventory_files(request) for inv_file in inv_files: inv_mgr = InventoryManager(loader=DataLoader(), sources=inv_file) if dut in inv_mgr.hosts: @@ -569,6 +582,68 @@ def get_host_data(request, dut): return inv_data +def generate_params_frontend_hostname(request): + 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) + frontend_duts = [] + inv_files = get_inventory_files(request) + for dut in tbinfo.testbed_topo[tbname]["duts"]: + if is_frontend_node(inv_files, dut): + frontend_duts.append(dut) + + return frontend_duts + + +def generate_params_frontend_hostname_rand_per_hwsku(request): + frontend_hosts = generate_params_frontend_hostname(request) + inv_files = get_inventory_files(request) + # Create a list of hosts per hwsku + host_hwskus = {} + for a_host in frontend_hosts: + a_host_hwsku = get_host_visible_vars(inv_files, a_host, variable='hwsku') + if not a_host_hwsku: + # Lets try 'sonic_hwsku' as well + a_host_hwsku = get_host_visible_vars(inv_files, a_host, variable='sonic_hwsku') + if a_host_hwsku: + if a_host_hwsku not in host_hwskus: + host_hwskus[a_host_hwsku] = [a_host] + else: + host_hwskus[a_host_hwsku].append(a_host) + else: + pytest.fail("Test selected require a node per hwsku, but 'hwsku' for '{}' not defined in the inventory".format(a_host)) + + frontend_hosts_per_hwsku = [] + for hosts in host_hwskus.values(): + if len(hosts) == 1: + frontend_hosts_per_hwsku.append(hosts[0]) + else: + frontend_hosts_per_hwsku.extend(random.sample(hosts, 1)) + + return frontend_hosts_per_hwsku + + +def generate_params_supervisor_hostname(request): + 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) + + if len(tbinfo.testbed_topo[tbname]["duts"]) == 1: + # We have a single node - dealing with pizza box, return it + return [tbinfo.testbed_topo[tbname]["duts"][0]] + inv_files = get_inventory_files(request) + for dut in tbinfo.testbed_topo[tbname]["duts"]: + if is_supervisor_node(inv_files, dut): + return [dut] + pytest.fail("Test selected require a supervisor node, none of the DUTs '{}' are a supervisor node".format(tbinfo.testbed_topo[tbname]["duts"])) + + def generate_param_asic_index(request, dut_indices, param_type): logging.info("generating {} asic indicies for DUT [{}] in ".format(param_type, dut_indices)) @@ -730,6 +805,16 @@ def pytest_generate_tests(metafunc): elif "enum_dut_hostname" in metafunc.fixturenames: dut_hostnames = generate_params_dut_hostname(metafunc) metafunc.parametrize("enum_dut_hostname", dut_hostnames) + elif "enum_supervisor_dut_hostname" in metafunc.fixturenames: + supervisor_hosts = generate_params_supervisor_hostname(metafunc) + metafunc.parametrize("enum_supervisor_dut_hostname", supervisor_hosts) + elif "enum_frontend_dut_hostname" in metafunc.fixturenames: + frontend_hosts = generate_params_frontend_hostname(metafunc) + metafunc.parametrize("enum_frontend_dut_hostname", frontend_hosts) + elif "enum_rand_one_per_hwsku_frontend_hostname" in metafunc.fixturenames: + frontend_hosts_per_hwsku = generate_params_frontend_hostname_rand_per_hwsku(metafunc) + metafunc.parametrize("enum_rand_one_per_hwsku_frontend_hostname", frontend_hosts_per_hwsku) + if "enum_asic_index" in metafunc.fixturenames: metafunc.parametrize("enum_asic_index",generate_param_asic_index(metafunc, dut_indices, ASIC_PARAM_TYPE_ALL)) From 0abe36ef712870dbd1cace7aa31bbac659baef0b Mon Sep 17 00:00:00 2001 From: sanmalho Date: Wed, 6 Jan 2021 13:02:54 -0500 Subject: [PATCH 2/3] Code optimization; review comments; updates based on converting tests - Pulling common code of getting duts in testbed into a function. - There was code to create TestbedInfo by parsing the testbed-file and getting the testbed info for a testbed. This code was repeated in tbinfo fixture and all the parameterization helper methods. Moved this into another function 'get_tbinfo' and made other functions/fixtures call this function. - Review comments: - When selecting a frontend node, if we find that none of the DUTs in the testbed are frontend nodes, then we should fail. - scope of parameterized fixtures - By default, all the 'enum_dut*' fixtures in pytest_generate_tests were created with scope 'function'. This made these fixtures not usable in other fixtures that have 'module' scope. - Added scope to be 'module' to all the 'enum_dut*' fixtures in 'pytest_generate_tests'. --- tests/conftest.py | 128 +++++++++++++++++++++------------------------- 1 file changed, 57 insertions(+), 71 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index dd30f13e9ed..0cc916ad745 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -142,10 +142,9 @@ def config_logging(request): dataplane_logger.setLevel(logging.ERROR) -@pytest.fixture(scope="session") -def tbinfo(request): +def get_tbinfo(request): """ - Create and return testbed information + Helper function to create and return testbed information """ tbname = request.config.getoption("--testbed") tbfile = request.config.getoption("--testbed_file") @@ -153,7 +152,15 @@ def tbinfo(request): raise ValueError("testbed and testbed_file are required!") testbedinfo = TestbedInfo(tbfile) - return testbedinfo.testbed_topo.get(tbname, {}) + return tbname, testbedinfo.testbed_topo.get(tbname, {}) + +@pytest.fixture(scope="session") +def tbinfo(request): + """ + Create and return testbed information + """ + _, testbedinfo = get_tbinfo(request) + return testbedinfo @pytest.fixture(name="duthosts", scope="session") @@ -190,7 +197,9 @@ def duthost(duthosts, request): def rand_one_dut_hostname(request): """ """ - dut_hostnames = generate_params_dut_hostname(request) + tbname, testbedinfo = get_tbinfo(request) + duts_in_testbed = testbedinfo["duts"] + dut_hostnames = generate_params_dut_hostname(duts_in_testbed, tbname) if len(dut_hostnames) > 1: dut_hostnames = random.sample(dut_hostnames, 1) return dut_hostnames[0] @@ -199,9 +208,10 @@ def rand_one_dut_hostname(request): def rand_one_frontend_dut_hostname(request): """ """ - frontend_dut_hostnames = generate_params_frontend_hostname(request) - if len(frontend_dut_hostnames) > 1: - dut_hostnames = random.sample(frontend_dut_hostnames, 1) + tbname, testbedinfo = get_tbinfo(request) + duts_in_testbed = testbedinfo["duts"] + frontend_dut_hostnames = generate_params_frontend_hostname(request, duts_in_testbed, tbname) + dut_hostnames = random.sample(frontend_dut_hostnames, 1) return dut_hostnames[0] @@ -582,24 +592,20 @@ def get_host_data(request, dut): return inv_data -def generate_params_frontend_hostname(request): - 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) +def generate_params_frontend_hostname(request, duts_in_testbed, tbname): frontend_duts = [] inv_files = get_inventory_files(request) - for dut in tbinfo.testbed_topo[tbname]["duts"]: + for dut in duts_in_testbed: if is_frontend_node(inv_files, dut): frontend_duts.append(dut) - + assert len(frontend_duts) > 0, \ + "Test selected require at-least one frontend node, " \ + "none of the DUTs '{}' in testbed '{}' are a supervisor node".format(duts_in_testbed, tbname) return frontend_duts -def generate_params_frontend_hostname_rand_per_hwsku(request): - frontend_hosts = generate_params_frontend_hostname(request) +def generate_params_frontend_hostname_rand_per_hwsku(request, duts_in_testbed, tbname): + frontend_hosts = generate_params_frontend_hostname(request, duts_in_testbed, tbname) inv_files = get_inventory_files(request) # Create a list of hosts per hwsku host_hwskus = {} @@ -626,39 +632,26 @@ def generate_params_frontend_hostname_rand_per_hwsku(request): return frontend_hosts_per_hwsku -def generate_params_supervisor_hostname(request): - 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) - - if len(tbinfo.testbed_topo[tbname]["duts"]) == 1: +def generate_params_supervisor_hostname(request, duts_in_testbed, tbname): + if len(duts_in_testbed) == 1: # We have a single node - dealing with pizza box, return it - return [tbinfo.testbed_topo[tbname]["duts"][0]] + return [duts_in_testbed[0]] inv_files = get_inventory_files(request) - for dut in tbinfo.testbed_topo[tbname]["duts"]: + for dut in duts_in_testbed: + # Expecting only a single supervisor node if is_supervisor_node(inv_files, dut): return [dut] - pytest.fail("Test selected require a supervisor node, none of the DUTs '{}' are a supervisor node".format(tbinfo.testbed_topo[tbname]["duts"])) + pytest.fail("Test selected require a supervisor node, " + + "none of the DUTs '{}' in testbed '{}' are a supervisor node".format(duts_in_testbed, tbname)) -def generate_param_asic_index(request, dut_indices, param_type): +def generate_param_asic_index(request, duts_in_testbed, 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) - #if the params are not present treat the device as a single asic device asic_index_params = [DEFAULT_ASIC_ID] for dut_id in dut_indices: - dut = tbinfo.testbed_topo[tbname]["duts"][dut_id] + dut = duts_in_testbed[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: @@ -669,26 +662,16 @@ def generate_param_asic_index(request, dut_indices, param_type): dut_id, dut, asic_index_params)) return asic_index_params -def generate_params_dut_index(request): - 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) - num_duts = len(tbinfo.testbed_topo[tbname]["duts"]) - logging.info("Num of duts in testbed topology {}".format(num_duts)) + +def generate_params_dut_index(duts_in_testbeds, tbname): + num_duts = len(duts_in_testbeds) + logging.info("Num of duts in testbed '{}' is {}".format(tbname, num_duts)) return range(num_duts) -def generate_params_dut_hostname(request): - 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) - duts = tbinfo.testbed_topo[tbname]["duts"] - logging.info("DUTs in testbed topology: {}".format(str(duts))) - return duts +def generate_params_dut_hostname(duts_in_testbed, tbname): + logging.info("DUTs in testbed '{}' are: {}".format(tbname, str(duts_in_testbed))) + return duts_in_testbed def generate_port_lists(request, port_scope): @@ -797,29 +780,32 @@ def generate_priority_lists(request, prio_scope): def pytest_generate_tests(metafunc): # The topology always has atleast 1 dut dut_indices = [0] - + tbname, testbedinfo = get_tbinfo(metafunc) + duts_in_testbed = testbedinfo["duts"] # Enumerators ("enum_dut_index", "enum_dut_hostname", "rand_one_dut_hostname") are mutually exclusive if "enum_dut_index" in metafunc.fixturenames: - dut_indices = generate_params_dut_index(metafunc) - metafunc.parametrize("enum_dut_index",dut_indices) + dut_indices = generate_params_dut_index(duts_in_testbed, tbname) + metafunc.parametrize("enum_dut_index", dut_indices, scope="module") elif "enum_dut_hostname" in metafunc.fixturenames: - dut_hostnames = generate_params_dut_hostname(metafunc) - metafunc.parametrize("enum_dut_hostname", dut_hostnames) + dut_hostnames = generate_params_dut_hostname(duts_in_testbed, tbname) + metafunc.parametrize("enum_dut_hostname", dut_hostnames, scope="module") elif "enum_supervisor_dut_hostname" in metafunc.fixturenames: - supervisor_hosts = generate_params_supervisor_hostname(metafunc) - metafunc.parametrize("enum_supervisor_dut_hostname", supervisor_hosts) + supervisor_hosts = generate_params_supervisor_hostname(metafunc, duts_in_testbed, tbname) + metafunc.parametrize("enum_supervisor_dut_hostname", supervisor_hosts, scope="module") elif "enum_frontend_dut_hostname" in metafunc.fixturenames: - frontend_hosts = generate_params_frontend_hostname(metafunc) - metafunc.parametrize("enum_frontend_dut_hostname", frontend_hosts) + frontend_hosts = generate_params_frontend_hostname(metafunc, duts_in_testbed, tbname) + metafunc.parametrize("enum_frontend_dut_hostname", frontend_hosts, scope="module") elif "enum_rand_one_per_hwsku_frontend_hostname" in metafunc.fixturenames: - frontend_hosts_per_hwsku = generate_params_frontend_hostname_rand_per_hwsku(metafunc) - metafunc.parametrize("enum_rand_one_per_hwsku_frontend_hostname", frontend_hosts_per_hwsku) + frontend_hosts_per_hwsku = generate_params_frontend_hostname_rand_per_hwsku(metafunc, duts_in_testbed, tbname) + metafunc.parametrize("enum_rand_one_per_hwsku_frontend_hostname", frontend_hosts_per_hwsku, scope="module") if "enum_asic_index" in metafunc.fixturenames: - metafunc.parametrize("enum_asic_index",generate_param_asic_index(metafunc, dut_indices, ASIC_PARAM_TYPE_ALL)) + metafunc.parametrize("enum_asic_index", + generate_param_asic_index(metafunc, duts_in_testbed, tbname, dut_indices, ASIC_PARAM_TYPE_ALL)) if "enum_frontend_asic_index" in metafunc.fixturenames: - metafunc.parametrize("enum_frontend_asic_index",generate_param_asic_index(metafunc, dut_indices, ASIC_PARAM_TYPE_FRONTEND)) + metafunc.parametrize("enum_frontend_asic_index", + generate_param_asic_index(metafunc, duts_in_testbed, tbname, dut_indices, ASIC_PARAM_TYPE_FRONTEND)) if "enum_dut_portname" in metafunc.fixturenames: metafunc.parametrize("enum_dut_portname", generate_port_lists(metafunc, "all_ports")) From 1a7effcd2b72c6ca895109e1a854d21ade522d01 Mon Sep 17 00:00:00 2001 From: sanmalho Date: Wed, 6 Jan 2021 13:48:37 -0500 Subject: [PATCH 3/3] Fixed lgtm warnings --- tests/conftest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 0cc916ad745..cb693deebba 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -802,10 +802,10 @@ def pytest_generate_tests(metafunc): if "enum_asic_index" in metafunc.fixturenames: metafunc.parametrize("enum_asic_index", - generate_param_asic_index(metafunc, duts_in_testbed, tbname, dut_indices, ASIC_PARAM_TYPE_ALL)) + generate_param_asic_index(metafunc, duts_in_testbed, dut_indices, ASIC_PARAM_TYPE_ALL)) if "enum_frontend_asic_index" in metafunc.fixturenames: metafunc.parametrize("enum_frontend_asic_index", - generate_param_asic_index(metafunc, duts_in_testbed, tbname, dut_indices, ASIC_PARAM_TYPE_FRONTEND)) + generate_param_asic_index(metafunc, duts_in_testbed, dut_indices, ASIC_PARAM_TYPE_FRONTEND)) if "enum_dut_portname" in metafunc.fixturenames: metafunc.parametrize("enum_dut_portname", generate_port_lists(metafunc, "all_ports"))