Skip to content
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

Don't allow mixing "multiple-egress-IP HA" with "auto-allocated-egress-IP HA" #20971

Merged

Conversation

danwinship
Copy link
Contributor

We now have two different ways to do egress IP HA:

  1. Assign multiple egress IPs to a NetNamespace, so that if the node hosting the first one goes down, we switch to using the second one.
  2. Use fully-automatic allocation with egressCIDRs, and let the master move the egress IP between nodes if one node goes down.

So clearly someone's going to say "oh boy, I'll use both and then get twice as much high availability!" This is a terrible idea because what will actually happen is:

  1. Everyone will be happily using egress IP 1 on node 1
  2. Node 1 goes down
  3. Nodes notice that egress IP 1 has stopped responding and switch to using egress IP 2
  4. The master notices that node 1 has gone down and moves egress IP 1 to a different node
  5. Nodes notice that egress IP 1 has started responding again and switch back

So you get twice as much disruption as you would have gotten if you were only using one of the two forms of HA. So this fixes it so that we don't do auto-allocation for egress IPs that are being used as part of multiple-egress-IP HA.

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 13, 2018
@danwinship danwinship requested review from dcbw and squeed and removed request for smarterclayton September 21, 2018 18:14
@dcbw
Copy link
Contributor

dcbw commented Sep 21, 2018

/lgtm

@dcbw
Copy link
Contributor

dcbw commented Sep 21, 2018

/retest

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, dcbw

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit b9cf1ab into openshift:master Sep 22, 2018
@danwinship danwinship deleted the egress-ip-ha-vs-ha branch January 31, 2019 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/networking lgtm Indicates that a PR is ready to be merged. sig/networking size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants