-
Notifications
You must be signed in to change notification settings - Fork 4.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
Auto egress IP code reorg in prep for future features #19603
Auto egress IP code reorg in prep for future features #19603
Conversation
Instead of trying to do it entirely in terms of "egress IPs whose status has changed", track "egress IPs whose status has changed" and "namespaces whose status has changed". This simplifies the handling of namespaces that are blocked due to duplicate Egress IPs, and will later make it easier to handle activating the next Egress IP for a namespace when the current one stops working.
In preparation for later fully supporting multiple NetNamespace EgressIPs, begin sanity-checking all of them (so that if namespace A has EgressIPs ["1.2.3.4", "1.2.3.6"] and namespace B has EgressIPs ["1.2.3.5", "1.2.3.6"], that's considered an error even though "1.2.3.6" would never get used by the current code).
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship 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 |
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.
/lgtm
func (eip *egressIPWatcher) egressIPChanged(eg *egressIPInfo) { | ||
eip.changedEgressIPs = append(eip.changedEgressIPs, eg) | ||
for _, ns := range eg.namespaces { | ||
eip.changedNamespaces = append(eip.changedNamespaces, ns) |
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.
Might the same namespace would be added to the list multiple times? If so do we care?
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.
Might the same namespace would be added to the list multiple times? If so do we care?
It gets converted to a map (and thus de-duped) later one. (Why isn't it a map to start with? Um... It probably made sense in some earlier version of the code)
After talking with @knobunc , here's the first half of #19578 as a separate PR... that way we can merge this sooner and if there are any bugs in the reorg part we'll catch them early.
Mostly this is just code reorganization; the only user-visible change is:
@openshift/sig-networking PTAL