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

fix(router): traditional router could error with "invalid order function for sorting" #10514

Merged
merged 1 commit into from
Mar 22, 2023

Conversation

bungle
Copy link
Member

@bungle bungle commented Mar 17, 2023

Summary

The same route may have been added multiple times to routes_by_sources[<ip|port>] or routes_by_destination[<ip|port>] which lead to "invalid order function for sorting" when we tried to sort the routes. There was also sorting issue when multiple routes sorted the same. This fixes the issue.

KAG-918

Issues Resolved

Fix #10446
Close #10470

@bungle
Copy link
Member Author

bungle commented Mar 17, 2023

@sabertobihwy and @zhangzerui20,

What are your thoughts on this alternate implementation?

Perhaps we can check if we have other structures too where we add same route when not needed.

@bungle bungle force-pushed the fix/trad-routed-src-dst-sort branch from c67bd35 to 73128e8 Compare March 17, 2023 14:59
@bungle bungle force-pushed the fix/trad-routed-src-dst-sort branch 2 times, most recently from 8e437d1 to ed0ee53 Compare March 17, 2023 15:03
@bungle bungle requested a review from chronolaw March 17, 2023 15:04
@bungle bungle added pr/do not merge pr/wip A work in progress PR opened to receive feedback and removed pr/please review labels Mar 17, 2023
@bungle bungle force-pushed the fix/trad-routed-src-dst-sort branch from ed0ee53 to 1b27e75 Compare March 17, 2023 18:42
@pull-request-size pull-request-size bot added size/L and removed size/M labels Mar 17, 2023
@chronolaw chronolaw changed the title fix(router) traditional router could error with "invalid order function for sorting" fix(router): traditional router could error with "invalid order function for sorting" Mar 20, 2023
@bungle bungle force-pushed the fix/trad-routed-src-dst-sort branch 4 times, most recently from 6936d38 to 7919b75 Compare March 20, 2023 12:46
@ghost
Copy link

ghost commented Mar 20, 2023

I reviewed this PR and understood the fix and it's great!

@ghost
Copy link

ghost commented Mar 20, 2023

Perhaps we can check if we have other structures too where we add same route when not needed.

I have tried some combinations locally with no problems.

BTW, the idea of tried is to make routes_by_destination appear as a table with duplicate references, e.g.:

["0.0.0.0"] = { <table 2>, <table 1>, <table 1>, <table 1>
    [0] = 4
  }

now this PR works well.

@bungle bungle added pr/please review and removed pr/do not merge pr/wip A work in progress PR opened to receive feedback labels Mar 20, 2023
@bungle bungle force-pushed the fix/trad-routed-src-dst-sort branch from 7919b75 to 11507e8 Compare March 21, 2023 09:19
@bungle
Copy link
Member Author

bungle commented Mar 21, 2023

@chronolaw / @sabertobihwy, feel free to approve if you think it is fine.

@bungle bungle force-pushed the fix/trad-routed-src-dst-sort branch from 11507e8 to 2c3b731 Compare March 21, 2023 13:39
…on for sorting"

### Summary

The same route may have been added multiple times to `routes_by_sources[<ip|port>]`
or `routes_by_destionation[<ip|port>]` which lead to "invalid order function for
sorting" when we tried to sort the routes. This fixes the issue.

KAG-918

### Issues Resolved

Fix #10446
Closes #10470

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
@bungle bungle force-pushed the fix/trad-routed-src-dst-sort branch from 2c3b731 to 04d5f65 Compare March 22, 2023 12:07
@bungle bungle merged commit a2a54a3 into master Mar 22, 2023
@bungle bungle deleted the fix/trad-routed-src-dst-sort branch March 22, 2023 12:29
lhanjian pushed a commit that referenced this pull request Dec 23, 2024
…er_token_param_type (#10514)


Co-authored-by: Michael Martin <flrgh@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when configuring routes with more than three destination rules
2 participants