[dualtor] fixing test_downstream_ecmp_nexthops#8906
[dualtor] fixing test_downstream_ecmp_nexthops#8906prsunny merged 4 commits intosonic-net:masterfrom
Conversation
What I did: changed test_downstream_ecmp_nexthops to check if packets being sent are funneled towards a single mux neighbor or portchannel Why I did it: test_downstream_ecmp_nexthops fails due to swss change which programs a single active mux nexthop or a single tunnel route for a given route How to test: run sonic-mgmt dualtor/test_orchagent_active_tor_downstream.py test on t0 topo. Signed-off-by: Nikola Dancejic <ndancejic@microsoft.com>
|
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
|
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
Signed-off-by: Nikola Dancejic <ndancejic@microsoft.com>
|
@prsunny for vis/review |
|
|
||
| if count > 0 and count != expect_packet_num: | ||
| all_pkts = False | ||
| pt_assert(all_pkts, "Packets not sent up single standby port {}".format( |
There was a problem hiding this comment.
Are we only counting the packet or validating the IP-IP packet?
There was a problem hiding this comment.
currently only counting packets, should I add IP-IP validation?
| all_pkts = False | ||
| pt_assert(all_pkts, "Packets not sent down single active port {}".format(downlink_int)) | ||
|
|
||
| if len(downlink_ints) == 0: |
There was a problem hiding this comment.
Is this condition ever satisfied? In test_downstream_ecmp_nexthops, the list nexthop_interfaces doesn't look like it's ever modified which means this would never get hit.
There was a problem hiding this comment.
I think you're right, I'll copy nexthop_interfaces and remove standby interfaces so that downlink_ints only reflects active neighbors
| expected_uplink_ports = list() | ||
| expected_uplink_portchannels = list() |
There was a problem hiding this comment.
Looks like these lists never get used
There was a problem hiding this comment.
yes I think you're right, I removed that section
|
|
||
|
|
||
| # verify nexthops are only sent to single active or standby mux | ||
| def check_nexthops_single_downlink(rand_selected_dut, ptfadapter, dst_server_addr, |
There was a problem hiding this comment.
I think this name is a little misleading since we're also checking the case where we expect all the packets to be sent to a single uplink. On that note, would it make more sense to break that case out into it's own helper function check_nexthops_single_uplink?
| all_pkts = False | ||
| pt_assert(all_pkts, "Packets not sent down single active port {}".format(downlink_int)) |
There was a problem hiding this comment.
what's the purpose of using all_pkts here? Would be cleaner to call pytest.fail
| all_pkts = False | ||
| pt_assert(all_pkts, "Packets not sent up single standby port {}".format( |
There was a problem hiding this comment.
same as above, prefer pytest.fail
| for send_packet, exp_pkt, exp_tunnel_pkt in packets_to_send: | ||
| testutils.send(ptfadapter, int(ptf_t1_intf.strip("eth")), send_packet, count=1) | ||
| # expect multi-mux nexthops to focus packets to one downlink | ||
| all_allowed_ports = expected_downlink_ports |
There was a problem hiding this comment.
Does this mean we never match/count any packets on uplink ports?
There was a problem hiding this comment.
we do later, but if we have expected_downlink_ports, then all traffic should be routed towards those. In the case where there aren't any, we then check the uplink portchannels to make sure traffic is going towards a single uplink
theasianpianist
left a comment
There was a problem hiding this comment.
Suggest breaking logic for checking uplinks and downlinks into separate functions to make the test flow a bit clearer.
Signed-off-by: Nikola Dancejic <ndancejic@microsoft.com>
|
@Ndancejic PR conflicts with 202012 branch |
* [dualtor] fixing test_downstream_ecmp_nexthops
|
Cherry-pick PR to 202305: #10020 |
|
@Ndancejic PR conflicts with 202205 branch |
* [dualtor] fixing test_downstream_ecmp_nexthops
|
Hello |
|
Hi @Ndancejic, we need this fix for 202205, could you please open a new PR to resolve the conflict? |
|
posted PR for 202205 branch: #11001 |
* [dualtor] fixing test_downstream_ecmp_nexthops
What I did:
changed test_downstream_ecmp_nexthops to check if packets being sent are funneled towards a single mux neighbor or portchannel
Why I did it:
test_downstream_ecmp_nexthops fails due to swss change which programs a single active mux nexthop or a single tunnel route for a given route
How to test:
run sonic-mgmt dualtor/test_orchagent_active_tor_downstream.py test on t0 topo.
Description of PR
fixes active_tor_downstream test to work with multiple mux routes programmed
Summary:
ADO: 18206088
Type of change
Back port request
Approach
What is the motivation for this PR?
test_downstream_ecmp_nexthops fails due to swss change which programs a single active mux nexthop or a single tunnel route for a given route
How did you do it?
changed test_downstream_ecmp_nexthops to check if packets being sent are funneled towards a single mux neighbor or portchannel
How did you verify/test it?
run sonic-mgmt dualtor/test_orchagent_active_tor_downstream.py test on t0 topo.