ipv6: Make IPAddrToHWAddr() aware of IPv6 #101
Conversation
I had a problem in an IPv6 cluster where certain traffic was getting dropped. A closer look revealed that duplicate MACs had been configured in the OVN virtual network topology. In particular, the join bridge between the gateway router and distributed cluster router had the same MAC on both of its ports. The root cause was this function. It assumed IPv4. While working on a fix for this and writing test cases, I discovered that the function would sometimes also break for IPv4. It assumed that the IPv4 address was always in 4-byte form. Sometimes the IP type actually has 16 bytes, even when holding an IPv4 address. The first fix is to ensure that we're always using the 4-byte representation when working with the IPv4 address. The second fix is to make this function aware of IPv6. We have more bytes in the address than we have bytes in a MAC, so it's not technically possible to always guarantee this results in a unique mac. However, the subnets we have hard coded internally in OVN are of a pretty simple format like fd98::N, so using the first two and last two bytes seems good enough. Really it's better to just generate a random unique MAC, but this patch attempts a compromise by covering some cases without re-introducing the performance concern that was addressed when this code was first introduced by commit b453f73. Signed-off-by: Russell Bryant <russell@ovn.org>
This is a follow up after some discussion on PR openshift#1091. Even with this limitation in place, we will still end up with some duplicate MACs in a system. The duplicates won't be on the same logical switch though, so it's not harmful. The cluster subnet doesn't change this situation.
openshift-ci-robot
added
size/M
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, knobunc, pecameron, russellb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
1 similar comment
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@danwinship: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
cherry-pick of ovn-org/ovn-kubernetes#1091 and ovn-org/ovn-kubernetes#1094
/cc @russellb
/assign @pecameron @knobunc