Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .azure-pipelines/pr_test_scripts.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ t0:
- platform_tests/mellanox/test_psu_power_threshold.py
- platform_tests/mellanox/test_reboot_cause.py
- platform_tests/sfp/test_sfpshow.py
- platform_tests/test_kdump.py
- platform_tests/test_advanced_reboot.py::test_warm_reboot
- platform_tests/test_auto_negotiation.py
# Temporarily remove for blocking builimage and take too much time to complete on KVM
Expand Down
45 changes: 12 additions & 33 deletions tests/common/plugins/pdu_controller/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,39 +32,18 @@ def get_pdu_visible_vars(inventories, pdu_hostnames):

def _get_pdu_controller(duthost, conn_graph_facts):
hostname = duthost.hostname
device_pdu_links = conn_graph_facts['device_pdu_links']
device_pdu_info = conn_graph_facts['device_pdu_info']
if hostname not in device_pdu_links or hostname not in device_pdu_info:
# fall back to using inventory
inv_mgr = duthost.host.options["inventory_manager"]
pdu_host = inv_mgr.get_host(duthost.hostname).get_vars().get("pdu_host")
hosts = inv_mgr.get_host_list('all', pdu_host)
pdu_links = {}
pdu_info = {}
pdu_vars = {}
index = 1
for ph in pdu_host.split(','):
if ph in hosts:
host_vars = hosts[ph]
pdu_links['PSU{}'.format(index)] = {
'N/A': {
'peerdevice': ph,
'peerport': 'probing',
'feed': 'N/A',
}
}
pdu_info[ph] = {
'Hostname': ph,
'Protocol': host_vars['protocol'],
'ManagementIp': host_vars['ansible_host'],
'Type': 'Pdu',
}
pdu_vars[ph] = host_vars
index = index + 1
else:
pdu_links = device_pdu_links[hostname]
pdu_info = device_pdu_info[hostname]
pdu_vars = get_pdu_visible_vars(duthost.host.options["inventory_manager"]._sources, pdu_info.keys())
# To adapt to the kvm testbed, conn_graph_facts is None for kvm.
# Unfortunately, for most DUTs
# we will get None because there is no key pdu_host under most of the hosts in iventory.
# And although we can get the pdu hosts list of a DUT from inventory
# we can not get the hwsku and os of pdu host from inventory.
# So we give the default value `{}` to kvm.
device_pdu_links = conn_graph_facts.get('device_pdu_links', {})
device_pdu_info = conn_graph_facts.get('device_pdu_info', {})

pdu_links = device_pdu_links.get(hostname, {})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if hostnames are not there for device_pdu_links and device_pdu_info, why are we changing it them to empty?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just saw your previous comments.
could you add a comment here to explain why you change to return empty hostnames if they are not found in csv files.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change aims to adapt to the kvm testbed. For kvm, conn_graph_facts is None, so we use function get to avoid KeyError and give it default value {} if the key doesn't exist.

I think here we don't return empty hostnames, the pdu info is empty here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, could you add this to code as comment as well? It'll help a lot

unfortunately, for most DUTs, we will get None because there is no key pdu_host under most of the hosts. And although we can get the pdu hosts list of a DUT from inventory, we can not get the hwsku and os of pdu host from inventory.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, added.

pdu_info = device_pdu_info.get(hostname, {})
pdu_vars = get_pdu_visible_vars(duthost.host.options["inventory_manager"]._sources, pdu_info.keys())

return pdu_manager_factory(duthost.hostname, pdu_links, pdu_info, pdu_vars)

Expand Down