OCPBUGS-54682: Fix - NetworkManager restart or crash renders br-ex unusable #5304
Conversation
openshift-ci-robot
added
jira/severity-critical
|
@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.
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.
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.
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.
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.
That's fair.
- If we had no
PORT_CONNECTION_UUIDpreviously, we can't reach this code here. - If we had correct
PORT_CONNECTION_UUIDpreviously, we recalculate it here but we don't need to. - If we had wrong
PORT_CONNECTION_UUIDpreviously, 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.
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.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: emy, mkowalski The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
…on NMS managed br-ex
|
New changes are detected. LGTM label has been removed. |
|
@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. |
| # 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.
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
- What I did
Restarting Network Manager on a node will lead to the node losing connection if the nodes
br-exinterface is being managed by nmstate. This is happening because the ofport dispatcher script does not take into account that thebr-exbridge ID isbr-ex-brinstead ofbr-ex. This PR is adding a check to fall back to check for nmstate managedbr-exif 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.