-
Notifications
You must be signed in to change notification settings - Fork 745
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
Add Geneve link support #567
Conversation
See comments in PR related to testing of previous PR: #565 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor change here. Otherwise the patch looks good.
Heavily based on the existing Gretap support
Sorry for the ping @vishvananda, but does this PR look good to you? Looking forward to begin consuming these changes. |
Also, @shassard I've been testing this locally and I think there might be a small bug in the marshaling and unmarshling of the VNI. I can't get Geneve interface creation to work properly when the VNI is sufficiently large. For example, replacing the With my changes, L2444 of link_linux.go becomes:
Similarly, L2465 of link_linux.go becomes:
Another thing I've noticed is that after fixing these issues, the Geneve tunnel that's generated doesn't seem to have the remote target IP address set properly. After writing a small program to call these new Geneve operations, I found that the resulting interface has no remote configured (verified via |
Update: found the reason why the remote addr isn't being set correctly. On L2437 of link_linux.go, we actually need to feed |
The Linux kernel geneve VNI is limited to 23bits from what I’ve observed. Does that work better than maxing out at 24bits?
… On Oct 5, 2020, at 5:39 PM, Derek Zhang ***@***.***> wrote:
Also, @shassard I've been testing this locally and I think there might be a small bug in the marshaling and unmarshling of the VNI. I can't get Geneve interface creation to work properly when the VNI is sufficiently large.
For example, replacing the 0x1000 test case with something larger like 0x8eecf6 causes the test to fail. Digging into this, I managed to fix it by copying the VNI marshaling/unmarshaling logic from the VXLAN implementation, which also happens to be a 24-bit identifier.
With my changes, L2444 of link_linux.go becomes:
data.AddRtAttr(nl.IFLA_GENEVE_ID, nl.Uint32Attr(geneve.ID))
Similarly, L2465 of link_linux.go becomes:
geneve.ID = native.Uint32(datum.Value[0:4])
Another thing I've noticed is that after fixing these issues, the Geneve tunnel that's generated doesn't seem to have the remote target IP address set properly. After writing a small program to call these new Geneve operations, I found that the resulting interface has no remote configured (verified via ip -d link show <geneve-interface-name>). There is currently no logic to unmarshal the remote IP address from the netlink response and no tests to assert against the result, so that might be handy here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
I don't believe we're doing anything right now to restrict the input to 24 bits or less, but the parsing of the netlink response is only reading 24 bits. The issue I was encountering was when creating Geneve tunnels with VNIs that are quite a bit smaller than the max 24-bit value. |
Based on the Geneve implementation pull request by @shassard: vishvananda#567
Based on the Geneve implementation pull request by @shassard: vishvananda#567
LGTM |
Heavily based on the existing Gretap support