-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Comments
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. |
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. |
I put together a small patch to fix both of the issues above: 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. |
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. |
Ticket created for tracking purposes
The text was updated successfully, but these errors were encountered: