Bug 1854306: Initialize host ovs differently for Openshift-SDN and Ovn-kubernetes by ovs-configuration.service #2066

pliurh · 2020-09-09T12:43:37Z

- What I did
Fixes: BZ 1854306

Clean NM objects created for ovn-kube shared gateway mode by configure-ovs.sh, when cluster network is openshift-sdn.
Render ovs-configuration.service differently for openshift-sdn and ovn-kube.
Remove unneeded ovs bridges.

- How to verify it
Executing the SDN migration process

- Description for the changelog
Initialize host ovs differently for Openshift-SDN and Ovn-kubernetes

openshift-ci-robot · 2020-09-09T12:43:46Z

@pliurh: This pull request references Bugzilla bug 1854306, 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.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1854306: Create a script to clean NM objects created by configure-ovs.sh

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.

pliurh · 2020-09-09T12:45:24Z

@trozet @cybertron @abhat PTAL

cgwalters · 2020-09-09T14:29:54Z

What would help here I think is to teach NM to have a way to make transient changes to the network setup i.e. don't persist into /etc. Then we'd run configure-ovs.sh at each boot, and wouldn't need a cleanup script.

cybertron

What would help here I think is to teach NM to have a way to make transient changes to the network setup i.e. don't persist into /etc. Then we'd run configure-ovs.sh at each boot, and wouldn't need a cleanup script.

@bcrochet was working on a way to make transient network config changes that might be relevant.

One concern with the keepalived workaround inline. Ideally we would remove that once my fix is in anyway, but if it unblocks things in the meantime I guess it's fine.

cybertron · 2020-09-09T15:51:43Z

templates/common/_base/files/configure-ovs-network.yaml

I think this will fail on platforms that don't use keepalived.

What would help here I think is to teach NM to have a way to make transient changes to the network setup i.e. don't persist into /etc. Then we'd run configure-ovs.sh at each boot, and wouldn't need a cleanup script.

@bcrochet was working on a way to make transient network config changes that might be relevant.

One concern with the keepalived workaround inline. Ideally we would remove that once my fix is in anyway, but if it unblocks things in the meantime I guess it's fine.

Proposal here: openshift/enhancements#399
PR here: #2017

I think this will fail on platforms that don't use keepalived.

fixed.

pliurh · 2020-09-11T08:02:24Z

What would help here I think is to teach NM to have a way to make transient changes to the network setup i.e. don't persist into /etc. Then we'd run configure-ovs.sh at each boot, and wouldn't need a cleanup script.

This is not only about NM objects. In ovs-configuration.sh, we use NM to create ovs bridge and interfaces, which are stored in ovsdb and also need to be removed properly.

pliurh · 2020-09-11T08:10:05Z

PR: #2017 cannot help this case.
As we need to configure the OVS before kubelet is up, we cannot wait for nm-state to do the job.

danwinship · 2020-09-14T18:05:55Z

templates/common/_base/files/clean-ovs-network.yaml

This should not be necessary; the setup side needs to deal with the possibility that the first time it runs, NM might be partway through bringing the network up for the first time, and so we don't know which interface will end up being the default. But in the cleanup case, either br-ex and ovs-port-phys0 and the rest already exist as NetworkManager objects, and you know what needs to be done with them; or else they don't exist, and there's no cleanup to be done.

danwinship · 2020-09-14T18:07:50Z

templates/common/_base/files/clean-ovs-network.yaml

Given how the scripts are closely connected (needing to know the same interface names, etc) it seems like it might be better to put the setup and cleanup both into a single file, and run it as either configure-ovs.sh setup or configure-ovs.sh cleanup depending on which mode you need?

or, if you're going to merge openshift/cluster-network-operator#782 into this, then maybe just configure-ovs.sh {{.NetworkType}}, where configure-ovs.sh OpenShiftSDN would delete br-ex if it existed, and configure-ovs.sh OVNKubernetes would delete br0 if it existed and set up br-ex.

danwinship · 2020-09-14T18:08:49Z

templates/common/_base/files/configure-ovs-network.yaml

Would it be better to make keepalived be After: network-online.target ?

keepalived runs in static pod. So it is After: network-online.target. However, the keepalived-monitor depends on kube-apiserver to generate the correct keepalived.conf. We cannot control the order of the starting of static pods. So when keepalived is up before kube-apiserver without a up-to-date config, it will mistakenly set the VIP to the old interface and break the network connectivity. Then kube-apiserver cannot come up as expected. We end up in a dead lock.

The logic here is just a workaround for this case, so that keepalived can not start before kube-apiserver. Ideally, keepalived side shall be able to handle this situation properly after fixing https://bugzilla.redhat.com/show_bug.cgi?id=1873955. But before that is available, this workaround can work.

danwinship · 2020-09-14T18:11:38Z

templates/common/_base/units/ovs-configuration.service.yaml

according to the template docs, you should be able to say

enabled: {{if eq .NetworkType "OVNKubernetes" "OpenShiftSDN"}}true{{else}}false{{end}}

trozet · 2020-09-14T19:12:17Z

templates/common/_base/files/clean-ovs-network.yaml

is this part necessary? The other connections that are lower priority should automatically become active with iface after ovs-if-phys0 gets deleted.

openshift-ci-robot · 2020-09-15T09:14:48Z

@pliurh: This pull request references Bugzilla bug 1854306, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1854306: Initialize host ovs differently for Openshift-SDN and Ovn-kubernetes by ovs-configuration.service

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.

knobunc · 2020-09-15T14:08:00Z

/retest

knobunc · 2020-09-16T13:22:20Z

/approve

pliurh · 2020-09-17T13:36:34Z

/assign @ashcrow

pliurh · 2020-09-17T13:39:00Z

/hold
wait until #2085 merged

pliurh · 2020-09-21T15:18:25Z

/unhold
The keepalived issue has been fixed by openshift/baremetal-runtimecfg#100

rbbratta · 2020-09-22T00:56:58Z

templates/common/_base/files/configure-ovs-network.yaml

neither cat nor grep are needed

driver=$(awk -F "=" '/DRIVER/ {print $2}' < "/sys/class/net/${intf}/device/uevent")

this isn't new code. He's just moving it into the if statement so this PR isn't really fixing that

rbbratta · 2020-09-22T01:01:09Z

templates/common/_base/files/configure-ovs-network.yaml

Isn't ifconfig deprecated?

ip link set dev "$intf" allmulticast on

rbbratta · 2020-09-22T01:19:44Z

could we run scripts through shellcheck? https://www.shellcheck.net


In configure-ovs-network.sh line 14:
    echo "Driver name is" $driver
                          ^-----^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
    echo "Driver name is" "$driver"


In configure-ovs-network.sh line 45:
    ifaces=$(ovs-vsctl list-ifaces ${iface})
                                   ^------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
    ifaces=$(ovs-vsctl list-ifaces "${iface}")


In configure-ovs-network.sh line 46:
    for intf in $ifaces; do configure_driver_options $intf; done
                                                     ^---^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
    for intf in $ifaces; do configure_driver_options "$intf"; done


In configure-ovs-network.sh line 76:
    nmcli c add type ovs-bridge conn.interface br-ex con-name br-ex 802-3-ethernet.mtu ${iface_mtu} 802-3-ethernet.cloned-mac-address ${iface_mac}
                                                                                                                                      ^----------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
    nmcli c add type ovs-bridge conn.interface br-ex con-name br-ex 802-3-ethernet.mtu ${iface_mtu} 802-3-ethernet.cloned-mac-address "${iface_mac}"


In configure-ovs-network.sh line 80:
  old_conn=$(nmcli --fields UUID,DEVICE conn show --active | grep ${iface} | awk '{print $1}')
                                                                  ^------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
  old_conn=$(nmcli --fields UUID,DEVICE conn show --active | grep "${iface}" | awk '{print $1}')


In configure-ovs-network.sh line 84:
    nmcli c add type ovs-port conn.interface ${iface} master br-ex con-name ovs-port-phys0
                                             ^------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
    nmcli c add type ovs-port conn.interface "${iface}" master br-ex con-name ovs-port-phys0


In configure-ovs-network.sh line 92:
  nmcli device disconnect $iface
                          ^----^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
  nmcli device disconnect "$iface"


In configure-ovs-network.sh line 95:
    nmcli c add type 802-3-ethernet conn.interface ${iface} master ovs-port-phys0 con-name ovs-if-phys0 \
                                                   ^------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
    nmcli c add type 802-3-ethernet conn.interface "${iface}" master ovs-port-phys0 con-name ovs-if-phys0 \


In configure-ovs-network.sh line 102:
    if nmcli --fields ipv4.method,ipv6.method conn show $old_conn | grep manual; then
                                                        ^-------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
    if nmcli --fields ipv4.method,ipv6.method conn show "$old_conn" | grep manual; then


In configure-ovs-network.sh line 105:
      if egrep -l --include=*.nmconnection $old_conn ${NM_CONN_PATH}/*; then
         ^---^ SC2196: egrep is non-standard and deprecated. Use grep -E instead.
                                           ^-------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
      if egrep -l --include=*.nmconnection "$old_conn" ${NM_CONN_PATH}/*; then


In configure-ovs-network.sh line 106:
        old_conn_file=$(egrep -l --include=*.nmconnection $old_conn ${NM_CONN_PATH}/*)
                        ^---^ SC2196: egrep is non-standard and deprecated. Use grep -E instead.
                                                          ^-------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
        old_conn_file=$(egrep -l --include=*.nmconnection "$old_conn" ${NM_CONN_PATH}/*)


In configure-ovs-network.sh line 111:
        nmcli conn clone ${old_conn} ${old_conn}-clone
                         ^---------^ SC2086: Double quote to prevent globbing and word splitting.
                                     ^---------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
        nmcli conn clone "${old_conn}" "${old_conn}"-clone


In configure-ovs-network.sh line 124:
      cp -f ${old_conn_file} ${new_conn_file}
            ^--------------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
      cp -f "${old_conn_file}" ${new_conn_file}


In configure-ovs-network.sh line 126:
        nmcli conn delete ${old_conn}-clone
                          ^---------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
        nmcli conn delete "${old_conn}"-clone


In configure-ovs-network.sh line 127:
        rm -f ${old_conn_file}
              ^--------------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
        rm -f "${old_conn_file}"


In configure-ovs-network.sh line 161:
        ovs-if-br-ex 802-3-ethernet.mtu ${iface_mtu} 802-3-ethernet.cloned-mac-address ${iface_mac}
                                                                                       ^----------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
        ovs-if-br-ex 802-3-ethernet.mtu ${iface_mtu} 802-3-ethernet.cloned-mac-address "${iface_mac}"


In configure-ovs-network.sh line 173:
      configure_driver_options ${iface}
                               ^------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
      configure_driver_options "${iface}"


In configure-ovs-network.sh line 187:
      configure_driver_options ${iface}
                               ^------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
      configure_driver_options "${iface}"


In configure-ovs-network.sh line 201:
  nmcli conn up $old_conn
                ^-------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
  nmcli conn up "$old_conn"

For more information:
https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...
https://www.shellcheck.net/wiki/SC2196 -- egrep is non-standard and depreca...

rbbratta · 2020-09-22T01:26:27Z

templates/common/_base/files/configure-ovs-network.yaml

use guards and always quote args to rm

bad

old_conn_file='-r .*' rm -f ${old_conn_file}

better

old_conn_file='-r .*' rm -f -- "${old_conn_file}"

pliurh · 2020-09-22T02:05:30Z

@rbbratta The diff result of Github is a bit misleading here. You can have more clear diff result with vscode. Actually, this patch didn't modify the lines you comment on. This patch only adds the following logic to this file. I prefer we focus on the new code in this PR and address your comments in a separate one.

if [ "$1" == "OVNKubernetes" ]; then
  <old logic - unchanged>
elif [ "$1" == "OpenShiftSDN" ]; then
  <new logic of removing NM objects>
fi

1. Clean NM objects created for ovn-kube shared gateway mode by configure-ovs.sh, when cluster network is openshift-sdn. 2. Render ovs-configuration.service differently for openshift-sdn and ovn-kube. 3. Remove unneeded ovs bridges.

pliurh · 2020-09-22T03:53:02Z

/test e2e-metal-ipi

pliurh · 2020-09-22T11:06:24Z

/test okd-e2e-aws

knobunc · 2020-09-22T20:15:24Z

/retest

pliurh · 2020-09-23T01:52:59Z

/test okd-e2e-aws

trozet

/lgtm

trozet · 2020-09-23T03:11:30Z

@kikisdeliveryservice or @ashcrow can you please approve?

yuqi-zhang · 2020-09-23T13:52:25Z

/approve

cgwalters

I really hope after 4.7 starts we can approach this in a much more structured fashion that involves more testing, much more observability/debuggability and much less shell script.

cgwalters · 2020-09-23T13:52:52Z

templates/common/_base/units/ovs-configuration.service.yaml

@@ -1,5 +1,5 @@
 name: ovs-configuration.service
-enabled: {{if eq .NetworkType "OVNKubernetes"}}true{{else}}false{{end}}
+enabled: {{if eq .NetworkType "OVNKubernetes" "OpenShiftSDN"}}true{{else}}false{{end}}


Is this ever false now?

CNO also supports kuryr and other 3-party CNIs.

openshift-ci-robot · 2020-09-23T13:54:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, knobunc, pliurh, trozet, yuqi-zhang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,yuqi-zhang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-09-23T14:37:22Z

/retest

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

openshift-bot · 2020-09-23T14:50:15Z

/retest

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

openshift-ci-robot · 2020-09-23T15:41:37Z

@pliurh: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp-upgrade	8a4d6e00b96c76cd7cab34910fed3b3a6a78d69c	link	`/test e2e-gcp-upgrade`

Full PR test history. Your PR dashboard.

Details

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.

openshift-bot · 2020-09-23T15:41:45Z

/retest

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

trozet · 2020-09-23T16:25:01Z

templates/common/_base/files/configure-ovs-network.yaml

+      fi
+
+      # remove bridges created by ovn-kubernetes, try to delete br-ex again in case NM fail to talk to ovsdb
+      ovs-vsctl --timeout=30 --if-exists del-br br-int -- --if-exists del-br br-local -- --if-exists del-br br-ex


@pliurh one thing I just thought of is that, if you are using openshift-sdn and you reboot a node, this will run again and delete these bridges. Does openshift-sdn also use br-int bridge? Would this cause problems for openshift-sdn to delete the bridge on each reboot?

@danwinship ^ ?

openshift-sdn does not use br-int, it only uses br0. And it will end up destroying and recreating br0 if OVS brings it up after a reboot anyway. It doesn't want OVS preserving its state across reboots.

openshift-ci-robot · 2020-09-23T17:42:38Z

@pliurh: Some pull requests linked via external trackers have merged:

openshift/machine-config-operator#2066

The following pull requests linked via external trackers have not merged:

openshift/cluster-network-operator#793 is open

These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state.

Bugzilla bug 1854306 has not been moved to the MODIFIED state.

Details

In response to this:

Bug 1854306: Initialize host ovs differently for Openshift-SDN and Ovn-kubernetes by ovs-configuration.service

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 added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Sep 9, 2020

openshift-ci-robot requested review from cgwalters and runcom September 9, 2020 12:43

cybertron suggested changes Sep 9, 2020

View reviewed changes

pliurh force-pushed the revert-ovs-configuration branch from ae3d40b to 8a4d6e0 Compare September 10, 2020 05:39

pliurh mentioned this pull request Sep 14, 2020

Bug 1854306: Remove ovs bridges when sdn/ovnkube pod is terminated openshift/cluster-network-operator#782

Closed

danwinship reviewed Sep 14, 2020

View reviewed changes

trozet reviewed Sep 14, 2020

View reviewed changes

pliurh force-pushed the revert-ovs-configuration branch from 8a4d6e0 to 81ee8af Compare September 15, 2020 09:12

pliurh changed the title ~~Bug 1854306: Create a script to clean NM objects created by configure-ovs.sh~~ Bug 1854306: Initialize host ovs differently for Openshift-SDN and Ovn-kubernetes by ovs-configuration.service Sep 15, 2020

pliurh force-pushed the revert-ovs-configuration branch from 81ee8af to bfd0417 Compare September 15, 2020 14:53

pliurh mentioned this pull request Sep 16, 2020

Bug 1854306: Run ovn in shared gw mode when configure-ovs.sh run with parameter OVNKubernetes openshift/cluster-network-operator#793

Merged

pliurh force-pushed the revert-ovs-configuration branch from bfd0417 to 72c9d33 Compare September 17, 2020 13:35

openshift-ci-robot assigned ashcrow Sep 17, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 17, 2020

ashcrow requested a review from yuqi-zhang September 17, 2020 17:57

trozet mentioned this pull request Sep 19, 2020

Bug 1874696: Add ovs-notification.service #2102

Closed

pliurh force-pushed the revert-ovs-configuration branch from 72c9d33 to 734fbe0 Compare September 21, 2020 09:44

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2020

juanluisvaladas mentioned this pull request Sep 21, 2020

Bug 1874696: Remove systemctl calls openshift/cluster-network-operator#785

Merged

rbbratta reviewed Sep 22, 2020

View reviewed changes

pliurh force-pushed the revert-ovs-configuration branch from 734fbe0 to 7c71f5f Compare September 22, 2020 01:50

pliurh force-pushed the revert-ovs-configuration branch from 7c71f5f to 6323fe4 Compare September 22, 2020 07:58

trozet approved these changes Sep 23, 2020

View reviewed changes

openshift-ci-robot assigned trozet Sep 23, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2020

cgwalters approved these changes Sep 23, 2020

View reviewed changes

trozet reviewed Sep 23, 2020

View reviewed changes

openshift-merge-robot merged commit 36f8ce3 into openshift:master Sep 23, 2020

openshift-ci-robot mentioned this pull request Sep 25, 2020

Bug 1854306: Set a 30s timeout for kubectl command in ovnkube-node openshift/cluster-network-operator#803

Merged

Bug 1854306: Initialize host ovs differently for Openshift-SDN and Ovn-kubernetes by ovs-configuration.service #2066

Bug 1854306: Initialize host ovs differently for Openshift-SDN and Ovn-kubernetes by ovs-configuration.service #2066

Uh oh!

Conversation

pliurh commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 9, 2020

Uh oh!

pliurh commented Sep 9, 2020

Uh oh!

cgwalters commented Sep 9, 2020

Uh oh!

cybertron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pliurh commented Sep 11, 2020

Uh oh!

pliurh commented Sep 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Sep 15, 2020

Uh oh!

knobunc commented Sep 15, 2020

Uh oh!

knobunc commented Sep 16, 2020

Uh oh!

pliurh commented Sep 17, 2020

Uh oh!

pliurh commented Sep 17, 2020

Uh oh!

pliurh commented Sep 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbbratta commented Sep 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pliurh commented Sep 22, 2020

Uh oh!

pliurh commented Sep 22, 2020

Uh oh!

pliurh commented Sep 22, 2020

Uh oh!

knobunc commented Sep 22, 2020

Uh oh!

pliurh commented Sep 23, 2020

Uh oh!

pliurh commented Sep 9, 2020 •

edited

Loading

openshift-ci-robot commented Sep 23, 2020 •

edited

Loading