[ovs-dev] rhel: Add support for "systemctl reload openvswitch"

The reload procedure will trigger a script that saves the flows and tlv
maps (using ovs-save) 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).

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    | 36 ++++++++++++++++++++++++
 5 files changed, 43 insertions(+), 2 deletions(-)
 create mode 100755 rhel/usr_share_openvswitch_scripts_ovs-reload

On Tue, 31 Oct 2017 12:11:32 +0100
Timothy Redaelli <tredaelli@redhat.com> wrote:

> The reload procedure will trigger a script that saves the flows and tlv
> maps (using ovs-save) 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).
> 
> 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    | 36 ++++++++++++++++++++++++
>  5 files changed, 43 insertions(+), 2 deletions(-)
>  create mode 100755 rhel/usr_share_openvswitch_scripts_ovs-reload
> 
> diff --git a/rhel/automake.mk b/rhel/automake.mk
> index 9336f0912..0955dceed 100644
> --- a/rhel/automake.mk
> +++ b/rhel/automake.mk
> @@ -24,6 +24,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_udev_rules.d_91-vfio.rules \
> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
> index fb7d918c6..87bec39c9 100644
> --- a/rhel/openvswitch-fedora.spec.in
> +++ b/rhel/openvswitch-fedora.spec.in
> @@ -314,6 +314,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 \
> @@ -539,6 +543,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 5baac822d..234d39355 100644
> --- a/rhel/usr_lib_systemd_system_ovsdb-server.service
> +++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
> @@ -3,7 +3,6 @@ Description=Open vSwitch Database Unit
>  After=syslog.target network-pre.target
>  Before=network.target network.service
>  Wants=ovs-delete-transient-ports.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..3ac1a46c6
> --- /dev/null
> +++ b/rhel/usr_share_openvswitch_scripts_ovs-reload
> @@ -0,0 +1,36 @@
> +#! /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.
> +
> +# Save flows
> +bridges=$(ovs-vsctl -- --real list-br)
> +flows=$(/usr/share/openvswitch/scripts/ovs-save 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


This looks good to me, my only comment is that the script is in rhel/
and uses systemd so maybe we should reflect that in the script's name?
Because OVS itself might provide an ovs-reload too and then we will
have issues.

What do you think?
Thanks,

On Fri, Nov 03, 2017 at 01:55:00PM -0200, Flavio Leitner wrote:
> On Tue, 31 Oct 2017 12:11:32 +0100
> Timothy Redaelli <tredaelli@redhat.com> wrote:
> 
> > The reload procedure will trigger a script that saves the flows and tlv
> > maps (using ovs-save) 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).
> > 
> > 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    | 36 ++++++++++++++++++++++++
> >  5 files changed, 43 insertions(+), 2 deletions(-)
> >  create mode 100755 rhel/usr_share_openvswitch_scripts_ovs-reload
> > 
> > diff --git a/rhel/automake.mk b/rhel/automake.mk
> > index 9336f0912..0955dceed 100644
> > --- a/rhel/automake.mk
> > +++ b/rhel/automake.mk
> > @@ -24,6 +24,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_udev_rules.d_91-vfio.rules \
> > diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
> > index fb7d918c6..87bec39c9 100644
> > --- a/rhel/openvswitch-fedora.spec.in
> > +++ b/rhel/openvswitch-fedora.spec.in
> > @@ -314,6 +314,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 \
> > @@ -539,6 +543,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 5baac822d..234d39355 100644
> > --- a/rhel/usr_lib_systemd_system_ovsdb-server.service
> > +++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
> > @@ -3,7 +3,6 @@ Description=Open vSwitch Database Unit
> >  After=syslog.target network-pre.target
> >  Before=network.target network.service
> >  Wants=ovs-delete-transient-ports.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..3ac1a46c6
> > --- /dev/null
> > +++ b/rhel/usr_share_openvswitch_scripts_ovs-reload
> > @@ -0,0 +1,36 @@
> > +#! /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.
> > +
> > +# Save flows
> > +bridges=$(ovs-vsctl -- --real list-br)
> > +flows=$(/usr/share/openvswitch/scripts/ovs-save 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
> 
> 
> This looks good to me, my only comment is that the script is in rhel/
> and uses systemd so maybe we should reflect that in the script's name?
> Because OVS itself might provide an ovs-reload too and then we will
> have issues.
> 
> What do you think?

Is this script reasonably generic, as opposed to RHEL-specific?  If so
then we could move it into utilities or make it a command under ovs-ctl.

On 11/03/2017 07:20 PM, Ben Pfaff wrote:
[...]
>>
>> This looks good to me, my only comment is that the script is in rhel/
>> and uses systemd so maybe we should reflect that in the script's name?
>> Because OVS itself might provide an ovs-reload too and then we will
>> have issues.
>>
>> What do you think?
> 
> Is this script reasonably generic, as opposed to RHEL-specific?  If so
> then we could move it into utilities or make it a command under ovs-ctl.

Unlucky this is specific for RHEL (and derivates) since Debian/Ubuntu
uses only one service file for both ovsdb-server and ovs-vswitchd and so
this script can't work, but I think they can use "ovs-ctl restart" that
have the same behavior when you don't have multiple systemd service files.

On Fri, Nov 03, 2017 at 07:26:05PM +0100, Timothy M. Redaelli wrote:
> On 11/03/2017 07:20 PM, Ben Pfaff wrote:
> [...]
> >>
> >> This looks good to me, my only comment is that the script is in rhel/
> >> and uses systemd so maybe we should reflect that in the script's name?
> >> Because OVS itself might provide an ovs-reload too and then we will
> >> have issues.
> >>
> >> What do you think?
> > 
> > Is this script reasonably generic, as opposed to RHEL-specific?  If so
> > then we could move it into utilities or make it a command under ovs-ctl.
> 
> Unlucky this is specific for RHEL (and derivates) since Debian/Ubuntu
> uses only one service file for both ovsdb-server and ovs-vswitchd and so
> this script can't work, but I think they can use "ovs-ctl restart" that
> have the same behavior when you don't have multiple systemd service files.

OK.  If Debian/Ubuntu changes someday, we can reconsider.