| Message ID | 9efeb6665651db4c5c42717913d13b1c439db90c.1502202677.git.tredaelli@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Russell Bryant |
| Headers | show |
On 8 August 2017 at 07:42, Timothy Redaelli <tredaelli@redhat.com> wrote: > The reload procedure will trigger a script that saves the flows and tlv > maps then it restarts ovsdb-server, it stops ovs-vswitchd, it sets > other_config:flow-restore-wait=true (to wait till flow restore is > finished), it starts ovs-vswitchd, it restore the backupped flows/tlv > maps and it removes other_config:flow-restore-wait=true (logic mostly > ripped > from ovs-ctl). > > It uses systemctl with --job-mode=ignore-dependencies to restart > ovsdb-server > and stop and start ovs-vswitchd in order to avoid systemd to restart the > other components due to dependencies (as explained in > rhel/README.RHEL.rst). > > It also uses --bundle, when available, in order to minimize the downtime. > The core script can probably be added to ovs-ctl, reuse some functions and then you can call it? This way, other platforms can benefit too. > > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com> > --- > rhel/automake.mk | 1 + > rhel/openvswitch-fedora.spec.in | 5 ++ > rhel/usr_lib_systemd_system_openvswitch.service | 2 +- > rhel/usr_lib_systemd_system_ovsdb-server.service | 1 - > rhel/usr_share_openvswitch_scripts_ovs-reload | 73 > ++++++++++++++++++++++++ > 5 files changed, 80 insertions(+), 2 deletions(-) > create mode 100755 rhel/usr_share_openvswitch_scripts_ovs-reload > > diff --git a/rhel/automake.mk b/rhel/automake.mk > index 1265fa747..93dbeac0c 100644 > --- a/rhel/automake.mk > +++ b/rhel/automake.mk > @@ -23,6 +23,7 @@ EXTRA_DIST += \ > rhel/openvswitch.spec.in \ > rhel/openvswitch-fedora.spec \ > rhel/openvswitch-fedora.spec.in \ > + rhel/usr_share_openvswitch_scripts_ovs-reload \ > rhel/usr_share_openvswitch_scripts_sysconfig.template \ > rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template \ > rhel/usr_lib_systemd_system_openvswitch.service \ > diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora. > spec.in > index 1cad9406e..dc92ab3ca 100644 > --- a/rhel/openvswitch-fedora.spec.in > +++ b/rhel/openvswitch-fedora.spec.in > @@ -289,6 +289,10 @@ install -d -m 0755 $RPM_BUILD_ROOT%{_prefix}/lib/ > ocf/resource.d/ovn > ln -s %{_datadir}/openvswitch/scripts/ovndb-servers.ocf \ > $RPM_BUILD_ROOT%{_prefix}/lib/ocf/resource.d/ovn/ovndb-servers > > +install -p -D -m 0755 \ > + rhel/usr_share_openvswitch_scripts_ovs-reload \ > + $RPM_BUILD_ROOT%{_datadir}/openvswitch/scripts/ovs-reload > + > # remove unpackaged files > rm -f $RPM_BUILD_ROOT%{_bindir}/ovs-parse-backtrace \ > $RPM_BUILD_ROOT%{_sbindir}/ovs-vlan-bug-workaround \ > @@ -493,6 +497,7 @@ fi > %{_datadir}/openvswitch/scripts/ovs-save > %{_datadir}/openvswitch/scripts/ovs-vtep > %{_datadir}/openvswitch/scripts/ovs-ctl > +%{_datadir}/openvswitch/scripts/ovs-reload > %config %{_datadir}/openvswitch/vswitch.ovsschema > %config %{_datadir}/openvswitch/vtep.ovsschema > %{_bindir}/ovs-appctl > diff --git a/rhel/usr_lib_systemd_system_openvswitch.service > b/rhel/usr_lib_systemd_system_openvswitch.service > index faca44b54..2cf29f0e9 100644 > --- a/rhel/usr_lib_systemd_system_openvswitch.service > +++ b/rhel/usr_lib_systemd_system_openvswitch.service > @@ -9,7 +9,7 @@ Requires=ovs-vswitchd.service > [Service] > Type=oneshot > ExecStart=/bin/true > -ExecReload=/bin/true > +ExecReload=/usr/share/openvswitch/scripts/ovs-reload > ExecStop=/bin/true > RemainAfterExit=yes > > diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service > b/rhel/usr_lib_systemd_system_ovsdb-server.service > index 68deace7c..b9814bae1 100644 > --- a/rhel/usr_lib_systemd_system_ovsdb-server.service > +++ b/rhel/usr_lib_systemd_system_ovsdb-server.service > @@ -2,7 +2,6 @@ > Description=Open vSwitch Database Unit > After=syslog.target network-pre.target > Before=network.target network.service > -ReloadPropagatedFrom=openvswitch.service > PartOf=openvswitch.service > > [Service] > diff --git a/rhel/usr_share_openvswitch_scripts_ovs-reload > b/rhel/usr_share_openvswitch_scripts_ovs-reload > new file mode 100755 > index 000000000..793257390 > --- /dev/null > +++ b/rhel/usr_share_openvswitch_scripts_ovs-reload > @@ -0,0 +1,73 @@ > +#! /bin/sh > + > +# Copyright (c) 2017 Red Hat, Inc. > +# > +# Licensed under the Apache License, Version 2.0 (the "License"); > +# you may not use this file except in compliance with the License. > +# You may obtain a copy of the License at: > +# > +# http://www.apache.org/licenses/LICENSE-2.0 > +# > +# Unless required by applicable law or agreed to in writing, software > +# distributed under the License is distributed on an "AS IS" BASIS, > +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > +# See the License for the specific language governing permissions and > +# limitations under the License. > + > +# Highest OpenFlow version enabled by default > +DEFAULT_OFP_VERSION=OpenFlow14 > + > +get_highest_ofp_version() { > + ovs-vsctl get bridge "$1" protocols | \ > + awk -v default_ofp_version="$DEFAULT_OFP_VERSION" \ > + -F '"' '{ print (NF>1)? $(NF-1) : default_ofp_version }' > +} > + > +save_flows () { > + for bridge; do > + # Get the highest enabled OpenFlow version > + ofp_version=$(get_highest_ofp_version "$bridge") > + > + printf "ovs-ofctl -O $ofp_version add-tlv-map %s '" "$bridge" > + ovs-ofctl -O $ofp_version dump-tlv-map $bridge | \ > + awk '/^ 0x/ {if (cnt != 0) printf ","; \ > + cnt++;printf "{class="$1",type="$2",len="$3"}->"$4}' > + echo "'" > + > + printf "%s" "ovs-ofctl -O $ofp_version add-flows $bridge \ > + \"$workdir/$bridge.flows.dump\"" > + > + # If possible, use OpenFlow 1.4 atomic bundle transaction to add > flows > + [ ${ofp_version#OpenFlow} -ge 14 ] && echo " --bundle" > + > + ovs-ofctl -O $ofp_version dump-flows --no-names --no-stats > "$bridge" | \ > + sed -e '/NXST_FLOW/d' \ > + -e '/OFPST_FLOW/d' \ > + -e 's/\(idle\|hard\)_age=[^,]*,//g' \ > + > "$workdir/$bridge.flows.dump" > + done > +} > + > +workdir=$(mktemp -d) > +trap 'rm -rf "$workdir"' EXIT > + > +# Save flows > +bridges=$(ovs-vsctl -- --real list-br) > +flows=$(save_flows $bridges) > + > +# Restart the database first, since a large database may take a > +# while to load, and we want to minimize forwarding disruption. > +systemctl --job-mode=ignore-dependencies restart ovsdb-server > + > +# Stop ovs-vswitchd. > +systemctl --job-mode=ignore-dependencies stop ovs-vswitchd > + > +# Start vswitchd by asking it to wait till flow restore is finished. > +ovs-vsctl --no-wait set open_vswitch . other_config:flow-restore- > wait=true > +systemctl --job-mode=ignore-dependencies start ovs-vswitchd > + > +# Restore saved flows and inform vswitchd that we are done. > +eval "$flows" > +ovs-vsctl --if-exists remove open_vswitch . other_config > flow-restore-wait=true > + > +exit 0 > -- > 2.13.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Aug 8, 2017 at 1:32 PM, Guru Shetty <guru@ovn.org> wrote: > On 8 August 2017 at 07:42, Timothy Redaelli <tredaelli@redhat.com> wrote: > >> The reload procedure will trigger a script that saves the flows and tlv >> maps then it restarts ovsdb-server, it stops ovs-vswitchd, it sets >> other_config:flow-restore-wait=true (to wait till flow restore is >> finished), it starts ovs-vswitchd, it restore the backupped flows/tlv >> maps and it removes other_config:flow-restore-wait=true (logic mostly >> ripped >> from ovs-ctl). >> >> It uses systemctl with --job-mode=ignore-dependencies to restart >> ovsdb-server >> and stop and start ovs-vswitchd in order to avoid systemd to restart the >> other components due to dependencies (as explained in >> rhel/README.RHEL.rst). >> >> It also uses --bundle, when available, in order to minimize the downtime. >> > > The core script can probably be added to ovs-ctl, reuse some functions and > then you can call it? This way, other platforms can benefit too. Sounds like a good suggestion. What would you think about updating the stop/start behavior to do this, as well? It looks like this makes "reload" actually just an "intelligent restart". systemctl has a "restart" command too, which looks like it just does stop/start, and it'd be nice to make that work better too.
On 8 August 2017 at 11:05, Russell Bryant <russell@ovn.org> wrote: > On Tue, Aug 8, 2017 at 1:32 PM, Guru Shetty <guru@ovn.org> wrote: > > On 8 August 2017 at 07:42, Timothy Redaelli <tredaelli@redhat.com> > wrote: > > > >> The reload procedure will trigger a script that saves the flows and tlv > >> maps then it restarts ovsdb-server, it stops ovs-vswitchd, it sets > >> other_config:flow-restore-wait=true (to wait till flow restore is > >> finished), it starts ovs-vswitchd, it restore the backupped flows/tlv > >> maps and it removes other_config:flow-restore-wait=true (logic mostly > >> ripped > >> from ovs-ctl). > >> > >> It uses systemctl with --job-mode=ignore-dependencies to restart > >> ovsdb-server > >> and stop and start ovs-vswitchd in order to avoid systemd to restart the > >> other components due to dependencies (as explained in > >> rhel/README.RHEL.rst). > >> > >> It also uses --bundle, when available, in order to minimize the > downtime. > >> > > > > The core script can probably be added to ovs-ctl, reuse some functions > and > > then you can call it? This way, other platforms can benefit too. > > Sounds like a good suggestion. > > What would you think about updating the stop/start behavior to do > this, as well? It looks like this makes "reload" actually just an > "intelligent restart". systemctl has a "restart" command too, which > looks like it just does stop/start, and it'd be nice to make that work > better too. > We had this discussion when we added intelligent restart to rhel/etc_init.d_openvswitch and debian/openvswitch-switch.init Currently both rhel and debian will take a "--save-flows=yes" option to restart and it will do the same things as the script proposed here does (except for --bundle) via ovs-ctl's restart function . We wanted "--save-flows=yes" to be the default behavior instead of explicitly calling it. But a long term user of OVS said that they use "restart" for a fresh start. That is, they bake in the assumption that a "restart" means the added flows disappear. What this patch does in addition to the "restart" function in ovs-ctl is that it uses --bundle. "restart" function in ovs-ctl calls functions in "ovs-save". So it may make sense for this patch to add the "--bundle" feature to ovs-save and just re-use it? > > -- > Russell Bryant >
On 08/08/2017 08:24 PM, Guru Shetty wrote: > > > On 8 August 2017 at 11:05, Russell Bryant <russell@ovn.org > <mailto:russell@ovn.org>> wrote: > > On Tue, Aug 8, 2017 at 1:32 PM, Guru Shetty <guru@ovn.org > <mailto:guru@ovn.org>> wrote: > > On 8 August 2017 at 07:42, Timothy Redaelli <tredaelli@redhat.com <mailto:tredaelli@redhat.com>> wrote: > > > >> The reload procedure will trigger a script that saves the flows and tlv > >> maps then it restarts ovsdb-server, it stops ovs-vswitchd, it sets > >> other_config:flow-restore-wait=true (to wait till flow restore is > >> finished), it starts ovs-vswitchd, it restore the backupped flows/tlv > >> maps and it removes other_config:flow-restore-wait=true (logic mostly > >> ripped > >> from ovs-ctl). > >> > >> It uses systemctl with --job-mode=ignore-dependencies to restart > >> ovsdb-server > >> and stop and start ovs-vswitchd in order to avoid systemd to restart the > >> other components due to dependencies (as explained in > >> rhel/README.RHEL.rst). > >> > >> It also uses --bundle, when available, in order to minimize the downtime. > >> > > > > The core script can probably be added to ovs-ctl, reuse some functions and > > then you can call it? This way, other platforms can benefit too. > > Sounds like a good suggestion. The script is actually "specialized" to how Fedora/RHEL starts openvswitch since, I think, only Fedora/RHEL have ovsdb-server and ovs-vswitchd as splitted systemd unit files. Ubuntu uses only one .service file (openvswitch-nonetwork.service) that starts both ovsdb-server and ovs-vswitch at the same time, so this approach can't work for them. (It seems) Debian only has /etc/init.d/openvswitch-switch and so this doesn't apply either. > What would you think about updating the stop/start behavior to do > this, as well? It looks like this makes "reload" actually just an > "intelligent restart". systemctl has a "restart" command too, which > looks like it just does stop/start, and it'd be nice to make that work > better too. You can't change the behavior of "systemctl restart", systemd will always call ExecStop and ExecStart, that's the reason I add the new script as "reload" target, but that makes sense since it's not a "standard" restart (it keeps the flows). Moreover I'd like to keep user to have the choice if reloading or restarting the daemon. > We had this discussion when we added intelligent restart > to rhel/etc_init.d_openvswitch and debian/openvswitch-switch.init > > Currently both rhel and debian will take a "--save-flows=yes" option to > restart and it will do the same things as the script proposed here does > (except for --bundle) via ovs-ctl's restart function . We wanted > "--save-flows=yes" to be the default behavior instead of explicitly > calling it. But a long term user of OVS said that they use "restart" for > a fresh start. That is, they bake in the assumption that a "restart" > means the added flows disappear. My implementation doesn't change this assumption, the only assumption is that if you use "reload" you want to keep all the flows (that's the same assumption you have when you use "systemctl reload" on nginx or apache) > What this patch does in addition to the "restart" function in ovs-ctl is > that it uses --bundle. "restart" function in ovs-ctl calls functions in > "ovs-save". So it may make sense for this patch to add the "--bundle" > feature to ovs-save and just re-use it? Sounds like a good suggestion. I'll do another patchset with a commit that adds the --bundle option + get_highest_ofp_version in ovs-save and another commit that changes ovs-reload script to use it. get_highest_ofp_version is used to find if we can use --bundle and to make ovs-ofctl work when you have a bridge with only OpenFlow15+ protocols enabled. Any other suggestions?
On 08/08/2017 11:01 PM, Timothy M. Redaelli wrote: > > The script is actually "specialized" to how Fedora/RHEL starts > openvswitch since, I think, only Fedora/RHEL have ovsdb-server and > ovs-vswitchd as splitted systemd unit files. For the record, SUSE uses the same approach > [...] > > Sounds like a good suggestion. > > I'll do another patchset with a commit that adds the --bundle option + > get_highest_ofp_version in ovs-save and another commit that changes > ovs-reload script to use it. > > get_highest_ofp_version is used to find if we can use --bundle and to > make ovs-ofctl work when you have a bridge with only OpenFlow15+ > protocols enabled. > > Any other suggestions? The rest looks good to me. Thank you
diff --git a/rhel/automake.mk b/rhel/automake.mk index 1265fa747..93dbeac0c 100644 --- a/rhel/automake.mk +++ b/rhel/automake.mk @@ -23,6 +23,7 @@ EXTRA_DIST += \ rhel/openvswitch.spec.in \ rhel/openvswitch-fedora.spec \ rhel/openvswitch-fedora.spec.in \ + rhel/usr_share_openvswitch_scripts_ovs-reload \ rhel/usr_share_openvswitch_scripts_sysconfig.template \ rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template \ rhel/usr_lib_systemd_system_openvswitch.service \ diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in index 1cad9406e..dc92ab3ca 100644 --- a/rhel/openvswitch-fedora.spec.in +++ b/rhel/openvswitch-fedora.spec.in @@ -289,6 +289,10 @@ install -d -m 0755 $RPM_BUILD_ROOT%{_prefix}/lib/ocf/resource.d/ovn ln -s %{_datadir}/openvswitch/scripts/ovndb-servers.ocf \ $RPM_BUILD_ROOT%{_prefix}/lib/ocf/resource.d/ovn/ovndb-servers +install -p -D -m 0755 \ + rhel/usr_share_openvswitch_scripts_ovs-reload \ + $RPM_BUILD_ROOT%{_datadir}/openvswitch/scripts/ovs-reload + # remove unpackaged files rm -f $RPM_BUILD_ROOT%{_bindir}/ovs-parse-backtrace \ $RPM_BUILD_ROOT%{_sbindir}/ovs-vlan-bug-workaround \ @@ -493,6 +497,7 @@ fi %{_datadir}/openvswitch/scripts/ovs-save %{_datadir}/openvswitch/scripts/ovs-vtep %{_datadir}/openvswitch/scripts/ovs-ctl +%{_datadir}/openvswitch/scripts/ovs-reload %config %{_datadir}/openvswitch/vswitch.ovsschema %config %{_datadir}/openvswitch/vtep.ovsschema %{_bindir}/ovs-appctl diff --git a/rhel/usr_lib_systemd_system_openvswitch.service b/rhel/usr_lib_systemd_system_openvswitch.service index faca44b54..2cf29f0e9 100644 --- a/rhel/usr_lib_systemd_system_openvswitch.service +++ b/rhel/usr_lib_systemd_system_openvswitch.service @@ -9,7 +9,7 @@ Requires=ovs-vswitchd.service [Service] Type=oneshot ExecStart=/bin/true -ExecReload=/bin/true +ExecReload=/usr/share/openvswitch/scripts/ovs-reload ExecStop=/bin/true RemainAfterExit=yes diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service b/rhel/usr_lib_systemd_system_ovsdb-server.service index 68deace7c..b9814bae1 100644 --- a/rhel/usr_lib_systemd_system_ovsdb-server.service +++ b/rhel/usr_lib_systemd_system_ovsdb-server.service @@ -2,7 +2,6 @@ Description=Open vSwitch Database Unit After=syslog.target network-pre.target Before=network.target network.service -ReloadPropagatedFrom=openvswitch.service PartOf=openvswitch.service [Service] diff --git a/rhel/usr_share_openvswitch_scripts_ovs-reload b/rhel/usr_share_openvswitch_scripts_ovs-reload new file mode 100755 index 000000000..793257390 --- /dev/null +++ b/rhel/usr_share_openvswitch_scripts_ovs-reload @@ -0,0 +1,73 @@ +#! /bin/sh + +# Copyright (c) 2017 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at: +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Highest OpenFlow version enabled by default +DEFAULT_OFP_VERSION=OpenFlow14 + +get_highest_ofp_version() { + ovs-vsctl get bridge "$1" protocols | \ + awk -v default_ofp_version="$DEFAULT_OFP_VERSION" \ + -F '"' '{ print (NF>1)? $(NF-1) : default_ofp_version }' +} + +save_flows () { + for bridge; do + # Get the highest enabled OpenFlow version + ofp_version=$(get_highest_ofp_version "$bridge") + + printf "ovs-ofctl -O $ofp_version add-tlv-map %s '" "$bridge" + ovs-ofctl -O $ofp_version dump-tlv-map $bridge | \ + awk '/^ 0x/ {if (cnt != 0) printf ","; \ + cnt++;printf "{class="$1",type="$2",len="$3"}->"$4}' + echo "'" + + printf "%s" "ovs-ofctl -O $ofp_version add-flows $bridge \ + \"$workdir/$bridge.flows.dump\"" + + # If possible, use OpenFlow 1.4 atomic bundle transaction to add flows + [ ${ofp_version#OpenFlow} -ge 14 ] && echo " --bundle" + + ovs-ofctl -O $ofp_version dump-flows --no-names --no-stats "$bridge" | \ + sed -e '/NXST_FLOW/d' \ + -e '/OFPST_FLOW/d' \ + -e 's/\(idle\|hard\)_age=[^,]*,//g' \ + > "$workdir/$bridge.flows.dump" + done +} + +workdir=$(mktemp -d) +trap 'rm -rf "$workdir"' EXIT + +# Save flows +bridges=$(ovs-vsctl -- --real list-br) +flows=$(save_flows $bridges) + +# Restart the database first, since a large database may take a +# while to load, and we want to minimize forwarding disruption. +systemctl --job-mode=ignore-dependencies restart ovsdb-server + +# Stop ovs-vswitchd. +systemctl --job-mode=ignore-dependencies stop ovs-vswitchd + +# Start vswitchd by asking it to wait till flow restore is finished. +ovs-vsctl --no-wait set open_vswitch . other_config:flow-restore-wait=true +systemctl --job-mode=ignore-dependencies start ovs-vswitchd + +# Restore saved flows and inform vswitchd that we are done. +eval "$flows" +ovs-vsctl --if-exists remove open_vswitch . other_config flow-restore-wait=true + +exit 0
The reload procedure will trigger a script that saves the flows and tlv maps then it restarts ovsdb-server, it stops ovs-vswitchd, it sets other_config:flow-restore-wait=true (to wait till flow restore is finished), it starts ovs-vswitchd, it restore the backupped flows/tlv maps and it removes other_config:flow-restore-wait=true (logic mostly ripped from ovs-ctl). It uses systemctl with --job-mode=ignore-dependencies to restart ovsdb-server and stop and start ovs-vswitchd in order to avoid systemd to restart the other components due to dependencies (as explained in rhel/README.RHEL.rst). It also uses --bundle, when available, in order to minimize the downtime. Signed-off-by: Timothy Redaelli <tredaelli@redhat.com> --- rhel/automake.mk | 1 + rhel/openvswitch-fedora.spec.in | 5 ++ rhel/usr_lib_systemd_system_openvswitch.service | 2 +- rhel/usr_lib_systemd_system_ovsdb-server.service | 1 - rhel/usr_share_openvswitch_scripts_ovs-reload | 73 ++++++++++++++++++++++++ 5 files changed, 80 insertions(+), 2 deletions(-) create mode 100755 rhel/usr_share_openvswitch_scripts_ovs-reload