Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[voq] refine eos fanout function to check fanout link oper status #9128

Merged
merged 19 commits into from
Jul 27, 2023
Merged
54 changes: 53 additions & 1 deletion tests/common/devices/eos.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,61 @@ def no_shutdown_multiple(self, interfaces):
return self.no_shutdown(intf_str)

def check_intf_link_state(self, interface_name):
"""
This function returns link oper as well as admin status
e.g. cable not connected:
Ethernet1/1 is down, line protocol is notpresent (notconnect)
link is admin down(cable not present):
Ethernet1/1 is administratively down, line protocol is notpresent (disabled)
link is admin down(cable present):
Ethernet2/1 is administratively down, line protocol is down (disabled)
link is admin up&oper up:
Ethernet2/1 is up, line protocol is up (connected)
link is admin up&oper down:
Ethernet2/1 is down, line protocol is down (notconnect)
In conclusion, if 'up' found in output line, link is oper up&admin up.
Link could not be admin down&oper up.
This function could not tell if it's admin up&oper down
"""
show_int_result = self.eos_command(
commands=['show interface %s' % interface_name])
return 'Up' in show_int_result['stdout_lines'][0]
return 'up' in show_int_result['stdout_lines'][0].lower()

def check_intf_link_oper_state(self, interface_name):
"""
This function returns oper state for eos fanout
e.g. Et1/1 is admin up, oper down
Et2/1 is admin down
Et3/1 is oper up
Et1/1 str2-7804-lc7-1-Ethernet0 notconnect 1102 full 100G 100GBASE-CR4
Et2/1 str2-7804-lc7-1-Ethernet4 disabled 1103 full 100G 100GBASE-CR4
Et3/1 str2-7804-lc7-1-Ethernet8 connected 1104 full 100G 100GBASE-CR4
"""
show_int_result = self.eos_command(commands=['show interface status'])
for output_line in show_int_result['stdout_lines'][0]:
fields = output_line.split(' ')
output_port = fields[0].replace('Et', 'Ethernet')
if interface_name == output_port:
# if link is not oper up, we consider it as oper down, it could be admin down as well
if 'connected' not in fields:
logging.info("Interface {} is oper down on {}".format(output_port, self.hostname))
return False
return True

def check_intf_link_admin_state(self, interface_name):
"""
This function returns admin state for eos fanout
"""
show_int_result = self.eos_command(commands=['show interface status'])
for output_line in show_int_result['stdout_lines'][0]:
fields = output_line.split(' ')
output_port = fields[0].replace('Et', 'Ethernet')
if interface_name == output_port:
# if link is not admin down, we consider it as admin up, however, it could be oper down(notconnect)
if 'disabled' in fields:
logging.info("Interface {} is admin down on {}".format(output_port, self.hostname))
return False
return True
Copy link

@slianganet slianganet Jul 26, 2023

Choose a reason for hiding this comment

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

I would caution against adding new code that tries to interpret or directly parse text output from eos commands. The approach in #9127 would be much preferred as the JSON output gives a well-defined model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you in the json way.
but I think it's necessary to avoid the confusion in the function name as well. if we check for 'connected' then it's checking link oper status only, this is another thing I would want to clarify in this PR

Choose a reason for hiding this comment

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

Sounds good, I'll let others comment on the preferred naming here. Just wanted to give my 2c on using JSON output.


def links_status_down(self, ports):
show_int_result = self.eos_command(commands=['show interface status'])
Expand Down
4 changes: 2 additions & 2 deletions tests/common/devices/fanout.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ def no_shutdown(self, interface_name):

return self.host.no_shutdown(interface_name)

def check_intf_link_state(self, interface_name):
return self.host.check_intf_link_state(interface_name)
def check_intf_link_oper_state(self, interface_name):
return self.host.check_intf_link_oper_state(interface_name)

def __str__(self):
return "{ os: '%s', hostname: '%s', device_type: '%s' }" % (self.os, self.hostname, self.type)
Expand Down
2 changes: 1 addition & 1 deletion tests/common/devices/sonic.py
Original file line number Diff line number Diff line change
Expand Up @@ -1273,7 +1273,7 @@ def check_default_route(self, ipv4=True, ipv6=True):

return True

def check_intf_link_state(self, interface_name):
def check_intf_link_oper_state(self, interface_name):
intf_status = self.show_interface(command="status", interfaces=[interface_name])["ansible_facts"]['int_status']
return intf_status[interface_name]['oper_state'] == 'up'

Expand Down
5 changes: 4 additions & 1 deletion tests/pc/test_lag_2.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,10 @@ def __check_shell_output(self, host, command):
return out['stdout'] == 'True'

def __check_intf_state(self, vm_host, intf, expect):
return vm_host.check_intf_link_state(vm_host, intf) == expect
"""
check neighbor fanout oper status, if nbr is admin shut, oper status will be down too
"""
return vm_host.check_intf_link_oper_state(vm_host, intf) == expect

def __verify_lag_lacp_timing(self, lacp_timer, exp_iface):
if exp_iface is None:
Expand Down
8 changes: 4 additions & 4 deletions tests/voq/test_voq_nbr.py
Original file line number Diff line number Diff line change
Expand Up @@ -839,8 +839,8 @@ def check_intf_status(self, dut, dut_intf, exp_status):
logging.info("status: %s", status)
return status[dut_intf]['oper_state'] == exp_status

def check_fanout_link_state(self, fanout, fanout_port):
return fanout.check_intf_link_state(fanout_port)
def check_fanout_link_oper_state(self, fanout, fanout_port):
return fanout.check_intf_link_oper_state(fanout_port)

def linkflap_down(self, fanout, fanport, dut, dut_intf):
"""
Expand Down Expand Up @@ -884,7 +884,7 @@ def linkflap_up(self, fanout, fanport, dut, dut_intf):
sleep_time = 90
pytest_assert(wait_until(sleep_time, 1, 0, self.check_intf_status, dut, dut_intf, 'up'),
"dut port {} didn't go up as expected".format(dut_intf))
pytest_assert(wait_until(30, 1, 0, self.check_fanout_link_state, fanout, fanport),
pytest_assert(wait_until(30, 1, 0, self.check_fanout_link_oper_state, fanout, fanport),
"fanout port {} on {} didn't go up as expected".format(fanport, fanout.hostname))

def localport_admindown(self, dut, asic, dut_intf):
Expand Down Expand Up @@ -925,7 +925,7 @@ def localport_adminup(self, dut, asic, dut_intf, fanouthosts):
if "portchannel" not in dut_intf.lower():
# Wait for fanout port to be operationally up as well.
fanout, fanport = fanout_switch_port_lookup(fanouthosts, dut.hostname, dut_intf)
pytest_assert(wait_until(30, 1, 0, self.check_fanout_link_state, fanout, fanport),
pytest_assert(wait_until(30, 1, 0, self.check_fanout_link_oper_state, fanout, fanport),
"fanout port {} on {} didn't go up as expected".format(fanport, fanout.hostname))

time.sleep(2)
Expand Down