Tweak VRID calculation to avoid 0 #23
Conversation
This change modifies how we calculate the fletcher8 checksum to match the online calculators (for example, [0]). This has two important effects: 1) The maximum value it can return is now 0xEE rather than 0xFF. This is a documented limitation of the fletcher algorithm because it uses modulo operations and 0xF % 0xF is 0, not 0xF, so it's impossible to get a 0xF byte. Unfortunately, this does reduce the id space available. 2) Because the algorithm will not return 255, we can safely add one to the resulting value to ensure we don't end up with a VRID of 0. 0 is not a valid VRID for keepalived, as reported in openshift#21. 0: https://gchq.github.io/CyberChef/#recipe=Fletcher-8_Checksum()&input=bDN2dnFxbmktZjIxY2EtZG5z
openshift-ci-robot
added
the
size/S
|
This is one possible approach. Another is to just clamp the id to > 0, but that way I think we'd double the chance of collisions at 1 because two hashes would be resolving to the same id. This approach slightly increases the chance of collisions across the board. |
|
/assign @celebdor |
| ckA = (ckA + inp[i]) & 0xf | ||
| ckB = (ckB + ckA) & 0xf | ||
| ckA = (ckA + inp[i]) % 0xf | ||
| ckB = (ckB + ckA) % 0xf |
There was a problem hiding this comment.
One other option would be to switch just this line back to &, which would allow us to get to a range of 0 to 254, which is exactly what we want. It wouldn't technically be fletcher 8 then, but I'm not sure how much we care about that.
|
Thanks @cybertron for the patch, this is a nice way to take care of both issues reported in #21 and #22. @celebdor any chance we can get this patch in? We're seeing the issue in CI occasionally. |
|
/lgtm |
openshift-ci-robot
added
lgtm
|
/retest Please review the full test history for this PR and help us cut down flakes. |
8 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. |
|
/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. |
|
/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. |
Because the fletcher8 algorithm only has a total of 255 hashes available, it is highly likely that at some point we will have a collision between our different VRIDs. This change adds checks for that situation and increments the ids until they no longer collide. This should be consistent across all nodes, so they will all end up using the same ids for the same VIPs.
cybertron
force-pushed
the
no-zero-vrid
branch
from
6d9bab6
to
fdf8346
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: celebdor, cybertron 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 |
|
We should backport this to We've seen it happen again in https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/logs/release-openshift-ocp-installer-e2e-openstack-serial-4.2/228 |
|
/backport release-4.2 (wonder if this works) |
|
/cherry-pick release-4.2 |
|
@iamemilio: new pull request created: #35 In response to this:
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. |
This change modifies how we calculate the fletcher8 checksum to
match the online calculators (for example, [0]). This has two
important effects:
The maximum value it can return is now 0xEE rather than 0xFF.
This is a documented limitation of the fletcher algorithm because
it uses modulo operations and 0xF % 0xF is 0, not 0xF, so it's
impossible to get a 0xF byte. Unfortunately, this does reduce the
id space available.
Because the algorithm will not return 255, we can safely add one
to the resulting value to ensure we don't end up with a VRID of
0. 0 is not a valid VRID for keepalived, as reported in FletcherChecksum8() may generate invalid VRID #21.
0: https://gchq.github.io/CyberChef/#recipe=Fletcher-8_Checksum()&input=bDN2dnFxbmktZjIxY2EtZG5z