Fix a couple issues in change_lag_lacp_timer
#15778
Conversation
Updated to work with single-asic LCs Updated to work on all 3 dst_port_ids Also made testQosSaiPfcXonLimit use fixture change_lag_lacp_timer
tests/qos/qos_sai_base.py
Outdated
| for port_ch, port_intf in dst_mgfacts['minigraph_portchannels'].items(): | ||
| for member in port_intf['members']: | ||
| if member in dst_interfaces: | ||
| lag_names.append( port_ch ) |
There was a problem hiding this comment.
why do we get lag_names on all dst device, regardless of single-asic/multi-asic?
There was a problem hiding this comment.
Some QOS tests send packets to multiple destinations dst_port_id, dst_port_2_id, dst_port_3_id
We disable TX on all 3 of these ports:
https://github.com/sonic-net/sonic-mgmt/blob/master/tests/saitests/py3/sai_qos_tests.py#L2543
And any of these 3 ports could be a LAG so therefor needs to have the LACP multiplier increased on the neighboring device.
|
@arista-nwolfe , could you paste the error/failures without this PR? |
The failure will likely manifest as something like: This is because one of the destination port-channels went down too early causing the packets that we enqueued to be dropped instead of stored in the destination port's buffer. |
Regarding the |
|
@XuChen-MSFT Can you please help review? |
tests/qos/qos_sai_base.py
Outdated
| for port_ch, port_intf in dst_mgfacts['minigraph_portchannels'].items(): | ||
| for member in port_intf['members']: | ||
| if member in dst_interfaces: | ||
| lag_names.append( port_ch ) |
There was a problem hiding this comment.
Is it possible for any two dst ports to be members of the same port channel? Then it would be better to use a set rather than a list as the lag_names variable type.
oh make sense, I ignored the dnx check, so it will only impact T2 layer. |
The only part that will impact a fully multi-asic system is the increasing of LACP multiplier for |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
please resolve precommit check tests/qos/qos_sai_base.py:2604:13: F841 local variable 'src_dut' is assigned to but never used |
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@ansrajpu-git would you please also review this? thanks. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/Azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@arlakshm @arista-nwolfe , adding the same fixture for few more qos tests. Please review PR # #16224 |
#14469 was introduced to increase the LACP timeout of EOS neighbors during QOS tests.
This was required because the QOS tests would disable TX credits on the test's dest ports, without TX credits LACP packets won't egress and eventually the LAG will be brought down by the EOS neighbor due to LACP timeout.
This change adds a couple improvements to that existing PR:
-Updated to work with single-asic LCs
-Updated to change the LACP timeout multiplier for all 3 dst_port_id neighbors.
Also made testQosSaiPfcXonLimit use fixture change_lag_lacp_timer
Type of change
Back port request