Make Ansible Dir Configurable - #26868
Conversation
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Makes the Ansible directory configurable for test execution and transceiver inventory loading.
Changes:
- Adds an Ansible parent-directory override to
run_tests.sh. - Introduces a shared
ansible_rootfixture. - Updates topology, conditional-mark, and transceiver paths.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/run_tests.sh |
Configures Ansible paths through an environment override. |
tests/conftest.py |
Adds the session-scoped Ansible-root fixture. |
tests/common/testbed.py |
Resolves topology files relative to the testbed location. |
tests/common/plugins/conditional_mark/__init__.py |
Uses configured Ansible paths for facts and variables. |
tests/transceiver/conftest.py |
Loads transceiver data from the configured Ansible root. |
tests/transceiver/attribute_parser/paths.py |
Makes inventory paths Ansible-root-relative. |
f7b4e24 to
3f0113d
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
tests/common/testbed.py:388
- For
nut-*topologies this now searches the regularvarsdirectory, so files such asansible/vars/nut_topos/nut-2tiers.ymlare no longer found andTestbedInforaisesFileNotFoundError. Use the NUT-specific path constant in this branch.
topo_dir = os.path.join(os.path.dirname(self.testbed_filename), self.TOPOLOGY_FILEPATH)
tests/transceiver/attribute_parser/paths.py:19
- This shared path is now relative to the ansible root, but
tests/transceiver/cdb_firmware_upgrade/conftest.py:49-51still passesget_repo_root()toTransceiverFirmwareInfoParser. The parser consequently looks under<repo>/files/transceiver/..., finds no manifests, and skips the entire CDB firmware-upgrade suite. Update that fixture to use the newansible_rootfixture (and rename the parser argument if appropriate).
# Root directory (relative to ansible root)
REL_TRANSCEIVER_INV_DIR = os.path.join('files', 'transceiver', 'inventory')
tests/common/plugins/conditional_mark/init.py:176
- Only the
dut_basic_factsinventory was switched to the configured ansible root. The same unconditional fact-loading flow still builds-i ../ansible/{inv_name}inload_minigraph_facts,load_config_facts, andload_switch_capabilities_facts(lines 262, 298, and 341), so an override can mix facts from the custom tree with the checkout's inventory or fail when that tree is external. Route every inventory argument throughANSIBLE_CONFIG_PATHbefore claiming the ansible location is configurable.
inv_full_path = ANSIBLE_CONFIG_PATH.joinpath(inv_name)
ansible_cmd = (
'ansible -M {} -m dut_basic_facts -i {} {} -o'
.format(ANSIBLE_LIBRARY_PATH, inv_full_path, dut_name))
tests/common/plugins/conditional_mark/init.py:28
- When
ANSIBLE_CONFIGis unset, this fallback is resolved relative to the pytest working directory rather than this module. A normal invocation fromtests/therefore points several directories outside the checkout, whereas the previous implementation resolved from__file__; ASIC lookup and inventory loading silently fail. Keep the fallback anchored to the repository while allowing the environment override.
ANSIBLE_CONFIG_PATH = pathlib.Path(os.getenv('ANSIBLE_CONFIG', '../../../../ansible'))
3f0113d to
4493b3c
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
4493b3c to
28819a9
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
28819a9 to
9c67987
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Signed-off-by: Ariz Zubair <arizzubair@microsoft.com>
9c67987 to
e4cf105
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/conftest.py:4296
ANSIBLE_PARENT_DIR_OVERRIDEexports the selected tree throughANSIBLE_CONFIG, but this fixture ignores it and instead derives the inventory root from the independently configurable testbed-file path. For example,run_tests.sh -f vtestbed.yamlmakes this return., so transceiver lookups targettests/files/transceiver/...and can silently skip for missing data. PreferANSIBLE_CONFIG, retaining the current derivation only as a fallback.
tbfile = request.config.getoption("testbed_file")
return pathlib.Path(tbfile).parent
tests/common/plugins/conditional_mark/init.py:28
- When
ANSIBLE_CONFIGis unset, this relative fallback is resolved against pytest's working directory rather than this plugin's directory. A normal invocation fromtests/therefore resolves it outside the checkout instead of to<repo>/ansible, causing ASIC variables, inventories, libraries, and proxy files to be missed. Keep the fallback anchored to__file__as before.
ANSIBLE_CONFIG_PATH = pathlib.Path(os.getenv('ANSIBLE_CONFIG', '../../../../ansible'))
Signed-off-by: Ariz Zubair <arizzubair@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (7)
tests/common/sai_adhoc.py:27
- The no-environment fallback resolves to
tests/ansible, not the repository'sansibledirectory (sai_adhoc.pyis undertests/common). This breaks standalone use of this helper wheneverANSIBLE_CONFIGis unset; retain the original repository-relative location by going up two levels.
ansible_config_path = pathlib.Path(os.getenv("ANSIBLE_CONFIG", pathlib.Path(__file__).resolve().parent.joinpath("../ansible")))
tests/common/plugins/conditional_mark/init.py:29
- This fallback only climbs to
tests/before appendingansible, so conditional-mark collection looks under the nonexistenttests/ansibletree whenANSIBLE_CONFIGis unset. The plugin previously resolved the repository-levelansibledirectory, which requires four parent traversals from this directory.
ANSIBLE_CONFIG_PATH = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../../ansible")))
tests/common/helpers/pfc_storm.py:13
- From
tests/common/helpers,../../ansibleresolves totests/ansible; therefore PFC storm setup cannot find the playbooks and inventory whenANSIBLE_CONFIGis not exported. The repository-level default needs one additional...
ANSIBLE_ROOT = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../ansible")))
tests/common/helpers/dut_utils.py:35
- The fallback path now points at
tests/ansible/files, which does not exist, rather than the existing repository-levelansible/files. Calls such asconn_graph_facts(..., filepath=LAB_CONNECTION_GRAPH_PATH)consequently fail withoutANSIBLE_CONFIG; climb one more directory here.
LAB_CONNECTION_GRAPH_PATH = pathlib.Path(
os.getenv("ANSIBLE_CONFIG", pathlib.Path(__file__).resolve().parent.joinpath("../../ansible"))).joinpath("files")
tests/common/fixtures/pfc_asym.py:19
- This default resolves to
tests/ansiblefromtests/common/fixtures, so all role/template paths built fromANSIBLE_ROOTbecome invalid for direct pytest runs that do not setANSIBLE_CONFIG. Preserve the repository-level fallback with another parent traversal.
ANSIBLE_ROOT = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../ansible")))
tests/common/fixtures/conn_graph_facts.py:80
- When
ANSIBLE_CONFIGis absent, this computestests/ansible/filesrather than the repository'sansible/files, so graph group and connection graph loading fails.conn_graph_facts.pyis three levels below the repository root, not two.
ansible_config_path = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../ansible")))
lab_conn_graph_path = os.path.join(ansible_config_path, "files/")
tests/common/devices/aos.py:36
- The fallback from
tests/common/deviceslands intests/ansible, so AOS playbook/template execution fails whenever the environment variable is not set. The previous default targeted the repository-levelansibletree, which requires../../../ansiblehere.
ansible_root = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../ansible")))
Signed-off-by: Ariz Zubair <arizzubair@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Suppressed comments (11)
tests/common/helpers/pfc_storm.py:13
- This configurable root is not applied to the Mellanox generator source at line 198, which remains
../ansible/roles/.... With an external ansible tree,_deploy_pfc_genstill reads from the checkout and fails, so the new configuration is only effective for playbook execution. Build that source path fromANSIBLE_ROOTas well.
ANSIBLE_ROOT = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../ansible")))
tests/common/sai_adhoc.py:29
- The configured path is used only to open
testbed.yaml;run_command,run_copy, andrun_fetchstill construct inventory paths as./ansible/{inv_file}(lines 69-70, 95-96, and 125-126). Thus the DUT/PTF can be discovered from the external tree, but every subsequent ad-hoc operation targets the checkout instead. Share this root with those commands.
ansible_config_path = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../ansible")))
with open(ansible_config_path.joinpath("testbed.yaml")) as f:
tests/common/plugins/conditional_mark/init.py:31
ANSIBLE_CONFIG_PATHis not actually the single inventory root for this plugin:load_minigraph_facts,load_config_facts,load_switch_capabilities_facts, andload_console_factsstill pass../ansible/{inv_name}at lines 260, 296, 339, and 371. Conditional-mark collection with an external ansible tree will therefore return incomplete facts and can apply marks incorrectly. Derive every inventory argument from this path.
ANSIBLE_CONFIG_PATH = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../../ansible")))
ASIC_NAME_PATH = ANSIBLE_CONFIG_PATH.joinpath("group_vars/sonic/variables")
ANSIBLE_LIBRARY_PATH = ANSIBLE_CONFIG_PATH.joinpath("library")
tests/common/helpers/pfc_storm.py:13
- When
ANSIBLE_CONFIGis unset, this fallback resolves totests/ansible, because the file is undertests/common/helpers. That directory does not exist, so PFC storm playbook execution loses the previous default behavior; the fallback needs one more..to reach the repository root.
This issue also appears on line 12 of the same file.
ANSIBLE_ROOT = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../ansible")))
tests/common/fixtures/pfc_asym.py:19
- This fallback is one directory too shallow: from
tests/common/fixturesit producestests/ansiblerather than the repository'sansibledirectory. Runs that rely on the documented fallback will therefore fail to find the PFC helper files and templates.
ANSIBLE_ROOT = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../ansible")))
tests/common/fixtures/conn_graph_facts.py:80
- The no-environment fallback resolves from
tests/common/fixturestotests/ansible, not<repo>/ansible. Consequently connection-graph loading fails whenever callers do not exportANSIBLE_CONFIG, even though this branch is intended to preserve the existing checkout layout.
ansible_config_path = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../ansible")))
lab_conn_graph_path = os.path.join(ansible_config_path, "files/")
tests/common/plugins/conditional_mark/init.py:31
- This fallback lands at
tests/ansible(conditional_markneeds four parent traversals to reach the repository root). WithoutANSIBLE_CONFIG, ASIC variables, inventory, modules, and proxy files are all looked up in a nonexistent directory.
This issue also appears on line 28 of the same file.
ANSIBLE_CONFIG_PATH = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../../ansible")))
ASIC_NAME_PATH = ANSIBLE_CONFIG_PATH.joinpath("group_vars/sonic/variables")
ANSIBLE_LIBRARY_PATH = ANSIBLE_CONFIG_PATH.joinpath("library")
tests/common/sai_adhoc.py:29
- The fallback is resolved relative to
tests/common, so../ansiblepoints totests/ansiblerather than<repo>/ansible. Direct invocation withoutANSIBLE_CONFIGcan no longer read the defaulttestbed.yaml.
This issue also appears on line 27 of the same file.
ansible_config_path = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../ansible")))
with open(ansible_config_path.joinpath("testbed.yaml")) as f:
tests/common/helpers/dut_utils.py:35
- This changes
LAB_CONNECTION_GRAPH_PATHfrom a string toPath.AnsibleHostBase._runJSON-serializes module kwargs (tests/common/devices/base.py:196-197,222-223), and its encoder does not supportPath, so bothconn_graph_facts(..., filepath=LAB_CONNECTION_GRAPH_PATH)calls fail before invoking Ansible. Returning a string also allows correcting the fallback, which currently resolves totests/ansible.
LAB_CONNECTION_GRAPH_PATH = pathlib.Path(
os.getenv("ANSIBLE_CONFIG", pathlib.Path(__file__).resolve().parent.joinpath("../../ansible"))).joinpath("files")
tests/common/testbed.py:29
- These constants still insert a literal
../ansibleafter the testbed file's directory. For an externally located root such as/opt/inventory/testbed.yaml, topology lookup becomes/opt/inventory/../ansible/varsinstead of/opt/inventory/vars, so arbitrary external ansible directory names remain unsupported. Since lookup is now based on the testbed file location, use its adjacentvarsdirectory directly.
TOPOLOGY_FILEPATH = "../ansible/vars/"
NUT_TOPOLOGY_FILEPATH = "../ansible/vars/nut_topos"
tests/common/helpers/dut_utils.py:35
- Only the connection-graph path is made configurable here.
creds_on_dutstill resets its root toBASI_PATH/../../../ansibleat line 686, so external inventories silently load credentials from the checkout (or find none). Introduce/reuse an ansible-root value for both paths.
LAB_CONNECTION_GRAPH_PATH = pathlib.Path(
os.getenv("ANSIBLE_CONFIG", pathlib.Path(__file__).resolve().parent.joinpath("../../ansible"))).joinpath("files")
| ansible_root = pathlib.Path(os.getenv("ANSIBLE_CONFIG", | ||
| pathlib.Path(__file__).resolve().parent.joinpath("../../ansible"))) |
Signed-off-by: Ariz Zubair <arizzubair@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (9)
tests/common/testbed.py:29
- These paths are now joined to the directory containing
testbed.yaml, which is already the ansible root. Keeping../ansibleonly works when that directory happens to be namedansible; for an external tree such as/inventory/testbed.yaml, topology loading incorrectly targets/ansible/varsinstead of/inventory/vars. Make the paths relative to the testbed directory itself.
TOPOLOGY_FILEPATH = "../ansible/vars/"
NUT_TOPOLOGY_FILEPATH = "../ansible/vars/nut_topos"
tests/common/sai_adhoc.py:28
- The no-environment fallback resolves to
tests/ansible, not the repository'sansibledirectory: this file's parent istests/common, so only one..is insufficient. This breaks the advertised default behavior wheneverANSIBLE_CONFIGis unset.
ansible_config_path = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../ansible")))
tests/common/plugins/conditional_mark/init.py:29
- The fallback is one directory too shallow. From
tests/common/plugins/conditional_mark,../../../ansibleresolves totests/ansible, so ASIC facts, inventory modules, and proxy files are not found whenANSIBLE_CONFIGis absent.
ANSIBLE_CONFIG_PATH = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../../ansible")))
tests/common/helpers/pfc_storm.py:13
- From
tests/common/helpers, this fallback resolves totests/ansible; the previous default resolved to the repository-levelansibledirectory. PFC storm execution therefore fails without an explicitly setANSIBLE_CONFIG.
ANSIBLE_ROOT = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../ansible")))
tests/common/helpers/dut_utils.py:35
- The fallback lost one
..compared with the old path. It now producestests/ansible/files, sois_mellanox_fanoutcannot load connection-graph facts in the default no-environment case.
LAB_CONNECTION_GRAPH_PATH = pathlib.Path(
os.getenv("ANSIBLE_CONFIG", pathlib.Path(__file__).resolve().parent.joinpath("../../ansible"))).joinpath("files")
tests/common/fixtures/pfc_asym.py:19
- This fallback resolves to
tests/ansible, while all playbooks and templates used below live in the repository-levelansibletree. WithANSIBLE_CONFIGunset, PFC asymmetry setup will use nonexistent paths.
ANSIBLE_ROOT = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../ansible")))
tests/common/fixtures/conn_graph_facts.py:79
- The fallback is one level too shallow: from
tests/common/fixtures,../../ansiblepoints attests/ansible. As a result, graph group and connection graph files are missed unless callers setANSIBLE_CONFIG.
ansible_config_path = pathlib.Path(os.getenv("ANSIBLE_CONFIG",
pathlib.Path(__file__).resolve().parent.joinpath("../../ansible")))
tests/common/devices/aos.py:36
- The fallback appends
../../../ansibleto the file path rather than its parent, so normalization yieldstests/ansibleinstead of the repository-levelansibledirectory. AOS template execution therefore regresses wheneverANSIBLE_CONFIGis unset.
ansible_root = str(pathlib.Path(os.getenv(
"ANSIBLE_CONFIG", pathlib.Path(__file__).joinpath("../../../ansible"))).resolve()) + "/"
tests/conftest.py:4291
- The new root is not yet a single source of truth, so the stated external-ansible layout remains unsupported for many normal suites. For example,
creds_on_dutstill reads../../../ansible(tests/common/helpers/dut_utils.py:686), route/stress tests still passpath="../ansible/"toannounce_routes, andload_config_factsstill uses../ansible/{inventory}(tests/common/plugins/conditional_mark/__init__.py:296). Those paths bypass this fixture even whenANSIBLE_CONFIGis set; migrate the remaining consumers (or a shared resolver they all use) before claiming the framework is configurable.
@pytest.fixture(scope="session")
def ansible_root(request):
Description of PR
Summary:
Makes the location of the
ansibledirectory configurable instead of assuming it always lives at a fixed relative path (../ansible) from thetestsdirectory in the repo checkout.Test code across
tests/hard-coded relative paths such as../ansible/...,../../../ansible/files, oros.path.dirname(__file__) + '/../../../../ansible/...'. This prevents running the test framework when the ansible inventory/config tree lives outside thesonic-mgmtcheckout (e.g. a separate inventory repo or a packaged/installed layout).This PR introduces a single source of truth for the ansible root:
ansible_rootpytest fixture intests/conftest.py, resolved from theANSIBLE_CONFIGenvironment variable, falling back to the directory of the--testbed_fileoption.os.getenv("ANSIBLE_CONFIG", <path relative to __file__>)usingpathlib.tests/run_tests.shnow derives all ansible paths fromANSIBLE_PARENT_DIR, overridable with the newANSIBLE_PARENT_DIR_OVERRIDEenvironment variable (defaults to the repo root, preserving current behavior).Fixes # (issue)
It fixes the challenge of running the sonic-mgmt tests with an outside ansible directory.
Type of change
Back port request
Tracking issue/work item for backport/cherry-pick request (GitHub issue or Microsoft ADO):
Failure type:
Tested branch
Test result
Approach
What is the motivation for this PR?
The
ansibledirectory location was hard-coded as a relative path in many places undertests/, tying the test framework to thesonic-mgmtrepo layout. Making it configurable allows the ansible inventory/config tree to live outside the checkout and removes fragile../../../..-style path arithmetic.How did you do it?
ansible_rootfixture intests/conftest.py(ANSIBLE_CONFIGenv var, else the parent dir of the testbed file) and consumed it inpdu,k8smasters,topo_bgp_routes,tests/bgp/test_bgp_vnet.py::setup_vnet,tests/transceiver/conftest.py::port_attributes_dict, andtests/transceiver/cdb_firmware_upgrade/conftest.py::transceiver_firmware_info_parser.ANSIBLE_CONFIG-basedpathlibresolution in:tests/common/devices/aos.pytests/common/fixtures/conn_graph_facts.pytests/common/fixtures/pfc_asym.pytests/common/helpers/dut_utils.pytests/common/helpers/pfc_storm.pytests/common/plugins/conditional_mark/__init__.py(singleANSIBLE_CONFIG_PATHnow derivesASIC_NAME_PATH,ANSIBLE_LIBRARY_PATH, inventory path, and proxy env files)tests/common/sai_adhoc.pytests/common/testbed.pynow resolves topology dirs relative to the testbed file location rather than__file__.tests/transceiver/attribute_parser/paths.pypaths are now relative to the ansible root instead of the repo root.tests/run_tests.sh: addedANSIBLE_PARENT_DIR(overridable viaANSIBLE_PARENT_DIR_OVERRIDE), used forINVENTORY,TESTBED_FILE, and all exportedANSIBLE_*variables; documented in help and debug output.How did you verify/test it?
Any platform specific information?
None. Changes are test-framework/path-resolution only; default behavior is unchanged when the env vars are not set.
Supported testbed topology if it's a new test case?
N/A
Documentation
The new
ANSIBLE_PARENT_DIR_OVERRIDEvariable is documented inrun_tests.sh -houtput.