Bug 1898178: [release-4.6] Handle egress IP assignment for node IPs #349
Conversation
When an egress IP is specified equaling a node's IP address, we can end up in the situation where the assignment procedure assigns the egress IP to another cluster node, and thus missing the node actually hosting that IP address. This patch thus fixes this by invoking an error during the assignment procedure and triggering an event notifying him/her of this unsupported request. Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>
Previously we only logged an error when the specified egress IP was invalid. Having worked on the previous commit I realized that it is more coherent to return an error to the user notifying him/her of this incorrect definition. The risk is that if a user specified multiple egress IPs and one is incorrect and we trigger an event, that this event goes unoticied but causes confusion later on. Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>
The test "should skip populating egress node data for nodes that have incorrect IP address" explicitly defines bad IP addresses for nodes as to make sure we don't use such nodes for egress assignment, however: addEgressNode did not take into account that the node in question might not exist in the map due to the previous condition. The test flakes as the checks it performs do not change state and thus the Eventually calls might pass faster than the code path is executed, i.e: it all depends on the CPU Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>
PR: ovn-org/ovn-kubernetes#1668 re-worked the way egress IP assignment is done, taking into account that users do not accidentally assign a node IP as egress IP and screws up cluster networking. However, it introduced a bug, namely: we now have a problem with reconciliation of nodes for egress, ultimately skipping any UPDATE event for nodes. Thus, if ovnkube-node does not label a node in question with k8s.ovn.org/node-primary-ifaddr before ovnkube-master received the ADD for that node, then we never proceed to update our internal data with that node when we receive the UPDATE. This commit fixes that Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>
openshift-ci-robot
added
bugzilla/severity-high
|
@alexanderConstantinescu: This pull request references Bugzilla bug 1898178, which is invalid:
Comment 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexanderConstantinescu, 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 |
openshift-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 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. |
|
/bugzilla refresh Recalculating validity in case the underlying Bugzilla bug has changed. |
|
@openshift-bot: This pull request references Bugzilla bug 1898178, which is invalid:
Comment 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. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
4 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. |
|
/bugzilla refresh |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@alexanderConstantinescu: This pull request references Bugzilla bug 1898178, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 6 validation(s) were run on this bug
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. |
openshift-ci-robot
removed
the
bugzilla/invalid-bug
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/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. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Going to go ahead and clear cherry-pick-approved on the new failures. Lets make sure we understand why these are failing and pick back up on Wednesday. |
sdodson
removed
the
cherry-pick-approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
7 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. |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
/retest |
|
@sdodson could you remove you /hold? All CI jobs on 4.6 have started passing, so I see no point in holding this anymore. |
|
/retest Sure, just in general I'm fine with you clearing the hold as long as you've reviewed the persistent failures and have reason to believe they've been resolved, so don't block on me. Holds can be cleared by anyone. |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
(patch manager) looks like quite a large and potentially risky change, but it's a clean backport from 4.7, and it prevents what sounds like a nasty impact from a misconfiguration |
markmc
added
the
cherry-pick-approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
6 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. |
|
@alexanderConstantinescu: All pull requests linked via external trackers have merged: Bugzilla bug 1898178 has been moved to the MODIFIED state. 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. |
- What this PR does and why is it needed
This back-ports the essential bits for the egress IP assignment procedure to guard against node IP assignment (as outlined in ovn-org/ovn-kubernetes#1668). This PR requires a couple of additional commits that came after ovn-org/ovn-kubernetes#1668, as that PR had some flaws that needed to be fixed afterwards.
/assign @danwinship
- Special notes for reviewers
- How to verify it
- Description for the changelog