frr icon indicating copy to clipboard operation
frr copied to clipboard

srv6: Introduce IPv6 address in segment-list used by TE Policy

Open dmytroshytyi-6WIND opened this issue 1 year ago • 30 comments

These series of patches introduce new feature: user may configure ipv6-address in segment list.

Format:

index INDEX ipv6-address X:X::X:X

Example:

segment-routing
 traffic-eng
  segment-list SL1
   index 0 ipv6-address 2001:db8::1

Signed-off-by: Dmytro Shytyi [email protected]

dmytroshytyi-6WIND avatar Dec 21 '23 17:12 dmytroshytyi-6WIND

Fix warnings by checkpatch.

dmytroshytyi-6WIND avatar Dec 26 '23 12:12 dmytroshytyi-6WIND

2 small fixes for isis_sr_te uploaded in the recent push.

dmytroshytyi-6WIND avatar Dec 26 '23 15:12 dmytroshytyi-6WIND

Thank you very much for your reviews, @ton31337, @riw777, @donaldsharp.

I was on PTO, thus the response delay. I expect to look at this PR in coming time.

@cscarpitta, WDYT about the cli format for ipv6 address? Does it look good?

dmytroshytyi-6WIND avatar Jan 18 '24 12:01 dmytroshytyi-6WIND

ci:rerun

dmytroshytyi-6WIND avatar Feb 16 '24 10:02 dmytroshytyi-6WIND

Dear @ton31337, @riw777, @donaldsharp. Thank you for reviews. I have responded on all comments and provided a new code version that addresses the comments. It passes all basics sanity checks and topotests. Could you please have a look at this PR again?

dmytroshytyi-6WIND avatar Feb 16 '24 14:02 dmytroshytyi-6WIND

ci:rerun

dmytroshytyi-6WIND avatar Mar 01 '24 05:03 dmytroshytyi-6WIND

CI:rerun libyang version may have changed during execution which caused the failure

mwinter-osr avatar Mar 28 '24 12:03 mwinter-osr

@donaldsharp, you raised the next question:

What is supposed to happen when we have > 1 million routes? What is a reasonable number of items that are going to be sent. Are we going to run out of packet space? Are we going to just cpuhog like you would not believe?

There is no more a loop over all routes and the max segs is verified and limited if exceeds the defined in header value. Thanks.

dmytroshytyi-6WIND avatar Mar 29 '24 15:03 dmytroshytyi-6WIND

ci:rerun

dmytroshytyi-6WIND avatar Mar 29 '24 18:03 dmytroshytyi-6WIND

Hello, @donaldsharp @ton31337 @riw777. The PR passes the CI tests and comments are addressed. WDYT?

dmytroshytyi-6WIND avatar Apr 02 '24 08:04 dmytroshytyi-6WIND

looks good to me ... waiting on @donaldsharp, I think?

riw777 avatar Apr 16 '24 11:04 riw777

looks good to me ... waiting on @donaldsharp, I think?

Seems like the blocker (changes requested) was introduced by @donaldsharp. It would be perfect to get his reply based on the latest status of PR.

dmytroshytyi-6WIND avatar Apr 23 '24 15:04 dmytroshytyi-6WIND

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar May 02 '24 08:05 github-actions[bot]

Rebase was performed to fix emerging conflicts.

dmytroshytyi-6WIND avatar May 02 '24 13:05 dmytroshytyi-6WIND

ci:rerun

dmytroshytyi-6WIND avatar May 02 '24 14:05 dmytroshytyi-6WIND

ci:rerun 1 unrelated failure

dmytroshytyi-6WIND avatar May 03 '24 22:05 dmytroshytyi-6WIND

ci:rerun

dmytroshytyi-6WIND avatar May 06 '24 12:05 dmytroshytyi-6WIND

unrelated failures...

dmytroshytyi-6WIND avatar May 06 '24 18:05 dmytroshytyi-6WIND

Hello @ton31337 , @donaldsharp, @riw777 Thank you very much for these reviews. Is there anything you would like to add as a comments for this PR?

dmytroshytyi-6WIND avatar May 06 '24 18:05 dmytroshytyi-6WIND

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Jun 18 '24 16:06 github-actions[bot]

ci:rerun

dmytroshytyi-6WIND avatar Aug 01 '24 15:08 dmytroshytyi-6WIND

Hello @donaldsharp,

Basic tests are OK.

Could you please take a look at this PR?

Thanks in advance.

dmytroshytyi-6WIND avatar Aug 01 '24 19:08 dmytroshytyi-6WIND

Hello @riw777 @ton31337 @donaldsharp

  1. basic tests are OK
  2. There is old "changes requested" that blocks the merging. All comments already were addressed some time ago.

@donaldsharp, no more new comments from reviewers for some time... If you don't have any comments left, please change the status of your review. Hopefully we will be able to unblock and merge it.

dmytroshytyi-6WIND avatar Aug 09 '24 18:08 dmytroshytyi-6WIND

I think the only thing holding this is @donaldsharp clearing his change request block ...

riw777 avatar Aug 14 '24 22:08 riw777

When I run the new test under any type of load it fails badly:

================================================================== short test summary info ==================================================================
FAILED ospf6_ecmp_inter_area/test_h.py::test_ecmp_inter_area - AssertionError: 'r1' wrong number of route nexthops
FAILED ospf6_ecmp_inter_area/test_e.py::test_ecmp_inter_area - AssertionError: 'r1' wrong number of route nexthops
FAILED isis_srv6_te_topo1/test_zy.py::test_rib_ipv6_step1 - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zs.py::test_srv6_te_policy_additional_route - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zd.py::test_rib_ipv6_step1 - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_ze.py::test_srv6_te_policy_activated - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zz.py::test_srv6_te_policy_removed - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zr.py::test_srv6_te_policy_removed - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zi.py::test_srv6_te_policy_additional_route - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zc.py::test_srv6_locator_step1 - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zh.py::test_rib_ipv6_step1 - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zv.py::test_srv6_te_policy_additional_route - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_isis_srv6_te_topo1.py::test_srv6_te_policy_removed - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zt.py::test_srv6_te_policy_activated - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zn.py::test_srv6_te_policy_activated - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zm.py::test_srv6_locator_step1 - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zk.py::test_srv6_te_policy_removed - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zo.py::test_srv6_te_policy_removed - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zw.py::test_srv6_locator_step1 - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zj.py::test_srv6_te_policy_activated - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zb.py::test_rib_ipv6_step1 - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zu.py::test_srv6_locator_step1 - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_za.py::test_rib_ipv6_step1 - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zp.py::test_srv6_te_policy_additional_route - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zf.py::test_srv6_locator_step1 - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zl.py::test_srv6_te_policy_activated - AssertionError: "rt1" JSON output mismatches the expected result
FAILED isis_srv6_te_topo1/test_zq.py::test_srv6_locator_step1 - AssertionError: "rt1" JSON output mismatches the expected result
FAILED ospf6_ecmp_inter_area/test_b.py::test_ecmp_inter_area - AssertionError: 'r1' wrong number of route nexthops
FAILED isis_srv6_te_topo1/test_zx.py::test_srv6_te_policy_additional_route - AssertionError: "rt1" JSON output mismatches the expected result
================================================= 29 failed, 1732 passed, 279 skipped in 819.59s (0:13:39) =============================```

Please spend some time running your new test under different loads and identifying and fixing the problems found. 

donaldsharp avatar Aug 15 '24 13:08 donaldsharp

as a note when I run the test by itself or concurrently with itself it works fine. It's only when scale/load is placed on the system that causes it to fail badly. This means, to me, that the test is starting to look for data before some other event has occurred and we get this output. See the hundreds of fixes in the topotest directory as we fix this sort of problem in the test for guidance on how to approach the problem. #16586, #16583, #16523, #16510 and #16485 as recent examples of this sort of problem.

donaldsharp avatar Aug 15 '24 13:08 donaldsharp