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
[release-4.9] Bug 2044503: ovs-configuration: use lower than NM default ethernet route metric #2928
[release-4.9] Bug 2044503: ovs-configuration: use lower than NM default ethernet route metric #2928
Conversation
Setting the default NM route metric for ovs-if-br-ex is problematic in the presence of other Ethernet ports. In that case, ovs-if-br-ex as well as the Ethernet ports will have the same route metric, and the winner is undefined. Lower ovs-if-br-ex route metric to 49 to avoid ambiguous situations. Conflicts: templates/common/_base/files/configure-ovs-network.yaml Signed-off-by: Andreas Karis <ak.karis@gmail.com> (cherry picked from commit e6a673d)
@andreaskaris: This pull request references Bugzilla bug 2044503, 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. |
PTAL |
@andreaskaris: This pull request references Bugzilla bug 2044503, 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. |
4 similar comments
@andreaskaris: This pull request references Bugzilla bug 2044503, 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. |
@andreaskaris: This pull request references Bugzilla bug 2044503, 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. |
@andreaskaris: This pull request references Bugzilla bug 2044503, 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. |
@andreaskaris: This pull request references Bugzilla bug 2044503, 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. |
/label backport-risk-assessed Seems like a reasonable change to backport. Holding off on lgtm to give Tim a chance to weigh in if he wants, and we have to wait for the original bug to be verified anyway. |
@andreaskaris: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
/bugzilla refresh |
@andreaskaris: This pull request references Bugzilla bug 2044503, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 6 validation(s) were run on this bug
Requesting review from QA contact: 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. |
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
/lgtm |
Thanks all, this seems good to go /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreaskaris, flavio-fernandes, kikisdeliveryservice, trozet 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-required Please review the full test history for this PR and help us cut down flakes. |
20 similar comments
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/label cherry-pick-approved |
@andreaskaris: All pull requests linked via external trackers have merged: Bugzilla bug 2044503 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. |
/cherrypick |
/cherrypick 4.8 |
@andreaskaris: cannot checkout 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. |
/cherrypick release-4.8 |
@andreaskaris: #2928 failed to apply on top of branch "release-4.8":
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. |
Setting the default NM route metric for ovs-if-br-ex is problematic
in the presence of other Ethernet ports. In that case, ovs-if-br-ex
as well as the Ethernet ports will have the same route metric, and
the winner is undefined. Lower ovs-if-br-ex route metric to 49 to
avoid ambiguous situations.
Conflicts:
templates/common/_base/files/configure-ovs-network.yaml
Signed-off-by: Andreas Karis ak.karis@gmail.com
(cherry picked from commit e6a673d)
Cause:
When more than one ethernet interface has the default route on startup, then NMs default behavior will assign metrics 100, 101, 102..
and so forth to the Ethernet interfaces to avoid ambiguity. NM will make sure that interfaces of the same type will not
have the same metric: https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/configuring_and_managing_networking/managing-the-default-gateway-setting_configuring-and-managing-networking
When ovs-configure.sh OVNKubernetes is run, it sets a metric of 100 to OVS port ovs-if-br-ex (which normally would have metric 800).
The port is no longer an Ethernet type port and the remaining Ethernet interfaces will receive new metrics, assigned in ascending order, starting with 100. This will lead to 2 default routes having metric 100, one of the Ethernet interfaces, and ovs-if-br-ex. It is no longer guaranteed that br-ex will be used as the default interface.
Consequence:
Cluster traffic will not function on clusters with multiple default route ethernet interfaces, such as OpenStack clusters with additional networks.
Fix:
Enforce that the metric configured on the OVN-Kubernetes interface (br-ex and ovs-if-br-ex) is set to 49.
Result:
The default route via br-ex should always be the one with the highest priority (= lowest metric).
After deployment, routes via br-ex should have metric 49, e.g.:
Setting the default NM route metric for ovs-if-br-ex is problematic
in the presence of other Ethernet ports. In that case, ovs-if-br-ex
as well as the Ethernet ports will have the same route metric, and
the winner is undefined. Lower ovs-if-br-ex route metric to 49 to
avoid ambiguous situations.