[conn graph] add support for multiple graph files#2194
[conn graph] add support for multiple graph files#2194yxieca merged 5 commits intosonic-net:masterfrom
Conversation
Put all graph files into a yaml list. When a testbed is requesting graph, parse all files in the list and find the graph file contains all duts of the testbed. Signed-off-by: Ying Xie <ying.xie@microsoft.com>
|
This pull request fixes 1 alert when merging 4c4b5e5 into 7d99e36 - view on LGTM.com fixed alerts:
|
When getting whole graph, the previous way was to not specifying the dut. However, with multiple graph files, we need to provide dut as anchor to match the graph file. Signed-off-by: Ying Xie <ying.xie@microsoft.com>
|
This pull request fixes 1 alert when merging 16a2b02 into 7d99e36 - view on LGTM.com fixed alerts:
|
|
retest vsimage please |
1 similar comment
|
retest vsimage please |
|
This pull request fixes 1 alert when merging fbc5e2a into 7d99e36 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 00db93d into 7d99e36 - view on LGTM.com fixed alerts:
|
|
Would this file also need to be change to reflect the unified graph? https://github.com/Azure/sonic-mgmt/blob/7d99e36a9439571e9ed51cb4634a5b42cb0ff17c/ansible/group_vars/all/inv_mapping.yml#L3 |
I don't think so. That map is still correct. The official lab graph is lab_connection_graph.xml. The new file I added is a truly empty graph file. |
Lawrence, I looked at the pytest conn_graph code again and I think your question makes lots of sense. I think we could potentially ditch the inventory graph map file and use this lookup method for all. But I think we could come back with another PR for that change. |
| Find a graph file contains all devices in testbed. | ||
| """ | ||
| def find_graph(hostnames, anchor): | ||
| filename = LAB_GRAPHFILE_PATH + LAB_CONNECTION_GRAPH_FILE |
There was a problem hiding this comment.
Sure. But improve with another PR?
| def find_graph(hostnames, anchor): | ||
| filename = LAB_GRAPHFILE_PATH + LAB_CONNECTION_GRAPH_FILE | ||
| with open(filename) as fd: | ||
| file_list = yaml.load(fd) |
There was a problem hiding this comment.
I think pyyaml will complain about this when we flip to python3, did you try using safe_load instead?
There was a problem hiding this comment.
I didn't try. I prefer to get this merged as-is since it is tested. Then improve.
| lab_graph.parse_graph() | ||
| if hostnames and lab_graph.contains_hosts(hostnames): | ||
| return lab_graph | ||
| if anchor and lab_graph.contains_hosts(anchor): |
There was a problem hiding this comment.
Maybe this is explained somewhere but what is anchor?
There was a problem hiding this comment.
Explained in the PR comments.
There was a problem hiding this comment.
The naming is still a little confusing to me, but this can be iterated on.
Update sonic-swss submodule pointer to include the following: [BFD]Clean up state_db BFD entries on swss restart (sonic-net#2434) Fix the Fec Mode Setting of gbsyncd (sonic-net#2430) [neighsyncd] Enabling ipv4 link local entries for non-dualtor (sonic-net#2427) tlm_teamd: Filter portchannel subinterface events from STATE_DB LAG_TABLE (sonic-net#2408) PFCWD recovery changes using DLR_INIT (sonic-net#2316) Dynamic port configuration - add port buffer cfg to the port ref counter (sonic-net#2194) Signed-off-by: dprital <drorp@nvidia.com>
3 participants
Summary:
Fixes # (issue)
Type of change
Approach
What is the motivation for this PR?
Before this change add-topo/remove-topo can only work with one inventory / graph file hard coded in conn_graph_facts.py. It is very inconvenient when deploying from a different inventory is needed.
How did you do it?
Put all graph files into a yaml list. When a testbed is requesting graph, parse all files in the list and find the graph file contains all duts of the testbed.
When getting the whole graph, provide dut list as anchor to match the right graph file.
For KVM test: if none of the graph file matches, fall back to an empty graph file so that the test won't fail at deployment stage. KVM test generally doesn't need graph otherwise. And for physical DUT, this empty graph file will cause other failure to stop deployment.
Signed-off-by: Ying Xie ying.xie@microsoft.com
How did you verify/test it?
add-topo/remove-topo from different inventories.