-
Notifications
You must be signed in to change notification settings - Fork 449
OCPBUGS-54682: Fix - NetworkManager restart or crash renders br-ex unusable #5304
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
Conversation
@emy: This pull request references Jira Issue OCPBUGS-54682, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
cc: @mkowalski |
/jira backport release-4.16,release-4.17,release-4.18,release-4.19,release-4.20 |
@mkowalski: The following backport issues have been created:
Queuing cherrypicks to the requested branches to be created after this PR merges: 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 openshift-eng/jira-lifecycle-plugin repository. |
@openshift-ci-robot: once the present PR merges, I will cherry-pick it on top of 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-sigs/prow repository. |
/lgtm |
BRIDGE_NAME=$(nmcli -t -f connection.interface-name conn show "${BRIDGE_ID}" | awk -F ':' '{print $NF}') || true | ||
if [ "${BRIDGE_NAME}" == "" ]; then | ||
#Check if br-ex is managed by nmstate (br-ex-br) | ||
PORT_CONNECTION_UUID=$(nmcli -t -f device,type,uuid conn | awk -F ':' '{if( $1=="br-ex" && $2~/^ovs-bridge/) print $NF}') |
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.
I'm curious how this works. If the connection uuid was incorrectly detected, do we not exit on line 46? Are we still getting a uuid there but it's not the correct one?
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.
If there is no connection that matches both "br-ex" and "ovs-bridge" then PORT_CONNECTION_UUID
will be empty, thus the next line fails and process exits with code 10 or some other big number.
If there is only one connection that matches both "br-ex" and "ovs-bridge", then PORT_CONNECTION_UUID
will get one value and it's a correct value. This is the scenario when everything works okay.
If there are two or more connections that match both "br-ex" and "ovs-bridge", then PORT_CONNECTION_UUID
will get a random value out of 2 or more possible ones. This scenario is bad, undesired. How can it happen that we end up in such a scenario? We would need to have 2 connections that have the same device
and type
. Can it ever happen?
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.
Actually I realize now that you are asking about L46 which uses output of
PORT_CONNECTION_UUID=$(nmcli -t -f device,type,uuid conn | awk -F ':' '{if( ($1=="'${PORT}'" || $3=="'${PORT}'") && $2~/^ovs*/) print $NF}')
but I do not see there how we can get an incorrect one easily. At least, in more scenarios than we do today. Let's start with
[root@master-0 ~]# nmcli -t -f device,type,uuid conn
br-ex:ovs-interface:2225810d-187f-432f-9c09-67bf9727ae88
br-ex:ovs-bridge:ce9f4f14-e059-4d8e-bf2e-2379fc3fed8a
enp2s0:802-3-ethernet:8d1041e5-9992-450e-954f-8e4f981ae9d2
br-ex:ovs-port:bf2011c7-24f6-4e35-85cd-ca639f4769e5
enp2s0:ovs-port:fb1ae9eb-d718-4a72-9b42-f12c1f2c9942
enp1s0:802-3-ethernet:d4a98e92-8232-40d5-9a2c-c69796bbd40c
enp3s0:802-3-ethernet:d4a98e92-8232-40d5-9a2c-c69796bbd40c
enp4s0:802-3-ethernet:d4a98e92-8232-40d5-9a2c-c69796bbd40c
lo:loopback:ec98b9a0-abe4-409a-86c7-ffc9e3fb3ae0
:802-3-ethernet:e5bf500e-35e8-4888-b4eb-74314c6473e5
From that we get
[root@master-0 ~]# export INTERFACE_NAME=enp2s0
[root@master-0 ~]# INTERFACE_CONNECTION_UUID=$(nmcli -t -f device,type,uuid conn | awk -F ':' '{if($1=="'${INTERFACE_NAME}'" && $2!~/^ovs*/) print $NF}')
[root@master-0 ~]# echo $INTERFACE_CONNECTION_UUID
8d1041e5-9992-450e-954f-8e4f981ae9d2
[root@master-0 ~]# INTERFACE_OVS_SLAVE_TYPE=$(nmcli -t -f connection.slave-type conn show "${INTERFACE_CONNECTION_UUID}" | awk -F ':' '{print $NF}')
[root@master-0 ~]# echo $INTERFACE_OVS_SLAVE_TYPE
ovs-port
[root@master-0 ~]# PORT=$(nmcli -t -f connection.master conn show "${INTERFACE_CONNECTION_UUID}" | awk -F ':' '{print $NF}')
[root@master-0 ~]# echo $PORT
fb1ae9eb-d718-4a72-9b42-f12c1f2c9942
[root@master-0 ~]# PORT_CONNECTION_UUID=$(nmcli -t -f device,type,uuid conn | awk -F ':' '{if( ($1=="'${PORT}'" || $3=="'${PORT}'") && $2~/^ovs*/) print $NF}')
[root@master-0 ~]# echo $PORT_CONNECTION_UUID
fb1ae9eb-d718-4a72-9b42-f12c1f2c9942
So seems like PORT_CONNECTION_UUID
is trying to find type ovs*
with the name that is your interface name (e.g. eth0
). For those it's okay because we will not have multiple ovs*
with such a name.
It could get tricky when you have the run with INTERFACE_NAME=br-ex
, but that one finishes very quickly, i.e.
[root@master-0 ~]# export INTERFACE_NAME=br-ex
[root@master-0 ~]# INTERFACE_CONNECTION_UUID=$(nmcli -t -f device,type,uuid conn | awk -F ':' '{if($1=="'${INTERFACE_NAME}'" && $2!~/^ovs*/) print $NF}')
[root@master-0 ~]# echo $INTERFACE_CONNECTION_UUID
[root@master-0 ~]#
So, do we actually miss something?
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.
I'm not sure. The reason I'm asking is that we're recalculating the port UUID here with a slightly different command than the one above, but if the one above didn't find any UUID then the script would have exited before now. Which leads me to believe that either we get a different UUID from this command for some reason, or we don't need to recalculate it at all.
The latter would make this second call unnecessary, but it would still work fine so I'm mostly making sure I understand the logic correctly.
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.
That's fair.
- If we had no
PORT_CONNECTION_UUID
previously, we can't reach this code here. - If we had correct
PORT_CONNECTION_UUID
previously, we recalculate it here but we don't need to. - If we had wrong
PORT_CONNECTION_UUID
previously, it's actually bad
I have a gut feeling we are in the scenario (3). Look that the previous PORT_CONNECTION_UUID
, it only matches for ovs
and not for ovs-bridge
. Given that for br-ex*
we have more than one entry matching ovs*
, the way of calculating it here is more robust than the way of calculating it the old way.
Maybe we should just move PORT_CONNECTION_UUID=$(
from there up to L44 ? As I read it, this should work for both old and new way of defining br-ex
. L44 works correctly for old method and may(?) race for the new one.
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.
Pretty sure I ended up in scenario (3). I'll check back and I agree that we could/should make this a little more solid for cases where a wrong selection could happen.
/hold |
…on NMS managed br-ex
# Get the port's master. If it doesn't have any, assume it's not our bridge | ||
BRIDGE_ID=$(nmcli -t -f connection.master conn show "${PORT_CONNECTION_UUID}" | awk -F ':' '{print $NF}') | ||
BRIDGE_ID=$(nmcli -t -f general.name conn show "${PORT_CONNECTION_UUID}" | awk -F ':' '{print $NF}') |
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.
This change changes the logic
[root@worker-1 ~]# nmcli -t -f general.name conn show "${PORT_CONNECTION_UUID}" | awk -F ':' '{print $NF}'
ovs-port-phys0
[root@worker-1 ~]# nmcli -t -f connection.master conn show "${PORT_CONNECTION_UUID}" | awk -F ':' '{print $NF}'
br-ex
/lgtm |
/hold cancel |
/jira refresh |
@emy: This pull request references Jira Issue OCPBUGS-54682, which is valid. The bug has been moved to the POST state. 3 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 openshift-eng/jira-lifecycle-plugin repository. |
/retest-required |
@emy: 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-sigs/prow repository. I understand the commands that are listed here. |
/approve Deferring to existing reviews by folks more well-versed in networking than myself. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djoshy, emy, mkowalski 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 |
/verified @mkowalski I have ran this code against a cluster with MC that creates nmstate that creates a bond out of 2 interfaces and plugs it to br-ex. Restarting NM on such a system produces the following log
On the same system without this patch the log when restarting NM looks as follows
|
@mkowalski: The 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 openshift-eng/jira-lifecycle-plugin repository. |
/verified by @mkowalski |
@mkowalski: This PR has been marked as verified by 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 openshift-eng/jira-lifecycle-plugin repository. |
b5688e7
into
openshift:main
@emy: Jira Issue Verification Checks: Jira Issue OCPBUGS-54682 Jira Issue OCPBUGS-54682 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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 openshift-eng/jira-lifecycle-plugin repository. |
@openshift-ci-robot: new pull request created: #5352 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-sigs/prow repository. |
@openshift-ci-robot: new pull request created: #5353 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-sigs/prow repository. |
@openshift-ci-robot: new pull request created: #5354 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-sigs/prow repository. |
@openshift-ci-robot: new pull request created: #5355 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-sigs/prow repository. |
@openshift-ci-robot: new pull request created: #5356 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-sigs/prow repository. |
Fix included in accepted release 4.21.0-0.nightly-2025-10-17-012128 |
- What I did
Restarting Network Manager on a node will lead to the node losing connection if the nodes
br-ex
interface is being managed by nmstate. This is happening because the ofport dispatcher script does not take into account that thebr-ex
bridge ID isbr-ex-br
instead ofbr-ex
. This PR is adding a check to fall back to check for nmstate managedbr-ex
if no bridge ID can be found.- How to verify it
Deploy a nmstate managed br-ex cluster.
Restart NetworkManager using
systemctl restart NetworkManager
.Node will lose connection if fix was unsuccessful.
Node will retain connection if fix was successful.
- Description for the changelog
Nodes with a br-ex interface managed by nmstate will not lose connection anymore if Network Manager is restarted on the node.