Basic high availability for auto egress IPs #20620
Conversation
If a namespace has multiple egress IPs, monitor egress traffic and switch to an alternate egress IP if the currently-selected one appears dead.
Most dump-flows calls are part of health checks and don't normally need to be logged about unless they fail.
openshift-ci-robot
added
size/XL
|
/retest |
|
|
||
| nodesByNodeIP map[string]*nodeEgress | ||
| namespacesByVNID map[uint32]*namespaceEgress | ||
| egressIPs map[string]*egressIPInfo | ||
|
|
||
| changedEgressIPs []*egressIPInfo | ||
| changedNamespaces []*namespaceEgress | ||
| changedEgressIPs map[*egressIPInfo]bool |
There was a problem hiding this comment.
@liggitt had asked:
what was the intent of making these map keys pointers? are the same instances of egressIPInfo received on every change notification? if a remove/add is enqueued, how do we ensure we process them in the right order?
The maps are only non-empty during the processing of a single event. Previously, if, say, a namespace with 3 egress IPs was deleted, then we'd end up adding the corresponding *namespaceEgress object to changedNamespaces 3 times, and then at the end in syncEgressIPs we'd have to de-dup that list so we only processed the changed namespace once. This commit is just changing things to use a map instead, with the namespaces/IPs as keys, so that we don't need a separate de-duping step at the end.
There was a problem hiding this comment.
just to be clear, keying a map using a pointer to a struct containing data does not dedupe if you get pointers to two different instances of the struct, even it those two instances contain the same data.
There was a problem hiding this comment.
Right. They're the same pointers. (They're all current elements of either the namespacesByVNID or egressIPs maps above.)
There was a problem hiding this comment.
do the namespacesByVNID and egressIPs maps store the same value under multiple keys?
There was a problem hiding this comment.
the code was already de-duping them by pointer equality before, it was just doing it in a different part of the code
There was a problem hiding this comment.
do the namespacesByVNID and egressIPs maps store the same value under multiple keys?
no
seems like changedEgressIPs and changedNamespaces could maybe store the keys from the egressIPs and namespacesByVNID maps instead. it's too easy to introduce changes that accidentally get a map wrong wrong when using pointers as map keys.
the code was already de-duping them by pointer equality before, it was just doing it in a different part of the code
ok. this lgtm then, but a follow-up to stop using pointers as map keys would be ideal.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, liggitt 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 |
Backport of #19578 to 3.10.z