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

Possible issue with multipath when used in bridge mode #2226

Closed
joseph-henry opened this issue Feb 14, 2024 · 4 comments
Closed

Possible issue with multipath when used in bridge mode #2226

joseph-henry opened this issue Feb 14, 2024 · 4 comments
Assignees
Milestone

Comments

@joseph-henry
Copy link
Contributor

Ticket created for tracking purposes

@joseph-henry joseph-henry added this to the 1.14.0 milestone Feb 14, 2024
@joseph-henry joseph-henry self-assigned this Feb 14, 2024
@andrejbinder
Copy link

I re-read through all the code and I believe I have found what is causing the problem. When receiving a packet we set the packet flowId to ZT_QOS_NO_FLOW and then only if we hit a packet that is of the VERB_FRAME type we look into its guts and set the flowId based on the actual protocol source/destination port numbers.

The catch is that packets that have been bridged are not VERB_FRAME but they are VERB_EXT_FRAME which contains no code to set the flowId correctly. This is why in-flow assignments do not work for packets from bridged hosts.

Out-flow assignments work perfectly for those because the flowId calculation is done already in Switch::onLocalEthernet of switch.cpp regardless of whether this is a VERB_FRAME or VERB_EXT_FRAME packet.

@andrejbinder
Copy link

I have also found what looks to me like a typo in the header length discriminator for VERB_FRAME type packets (it was populating flowId only when the packet was big enough to contain a VERB_EXT_FRAME header and at least some payload. I think this was meant to match against VERB_FRAME header size instead. This could make small (VERB_FRAME) packets not to have flowId populated.

@andrejbinder
Copy link

I put together a small patch to fix both of the issues above:

ab.patch

Quick testing seems to show that this has fixed the issue and in-flows now work correctly even for bridged packets. Please review the patch and consider upstreaming.

@joseph-henry
Copy link
Contributor Author

This looks good and seems to work. I've made a PR out of it (#2306) but if you'd rather make your own PR that's fine too. Just let us know what you'd prefer and we can upstream it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants