[ovs-dev] [PATCH 2/2] fedora package: fix systemd order and dependencies

Gurucharan Shetty shettyg at nicira.com
Wed Dec 4 21:04:38 UTC 2013


On Wed, Dec 4, 2013 at 12:12 PM, Flavio Leitner <fbl at redhat.com> wrote:
> On Wed, Dec 04, 2013 at 10:27:10AM -0800, Gurucharan Shetty wrote:
>> On Mon, Dec 2, 2013 at 5:13 PM, Flavio Leitner <fbl at redhat.com> wrote:
>> > There is a chicken and egg issue where common OVS
>> > configuration uses a controller which is only accessible
>> > via the network. So starting OVS before network.target
>> > would break those configurations.
>> >
>> > However, the network doesn't come up after boot because
>> > OVS isn't started until after the network scripts tries
>> > to configure the ovs.
>> >
>> > This is partially fixed by commits:
>> >    commit: 602453000e28ec1076c0482ce13c284765a84409
>> >    rhel: Automatically start openvswitch service before bringing an ovs interfa
>> >
>> >    commit: 3214851c31538e8690e31f95702f8927a8c0838b
>> >    rhel: Add OVSREQUIRES to automatically bring up OpenFlow interface dependencies
>> >
>> > But still there is the dependency issue between network.target
>> > and openvswitch which this patch fixes it.  It provides two systemd
>> > service units. One to run at any time (openvswitch-nonetwork.service)
>> > which runs 'ovs-ctl start' and the other one (openvswith.service) to
>> > run after network.target which works as a frontend to the admin.
>> >
>> > The openvswitch-nonetwork.service is used internally by the
>> > 'ifup-ovs/ifdown-ovs' scripts when adding or removing ports to
>> > the bridge or when the openvswitch.service is enabled by the admin.
>> >
>> > Signed-off-by: Flavio Leitner <fbl at redhat.com>
>> > ---
>> >  rhel/automake.mk                                          |  3 ++-
>> >  rhel/etc_sysconfig_network-scripts_ifdown-ovs             | 10 +++++++++-
>> >  rhel/etc_sysconfig_network-scripts_ifup-ovs               | 12 ++++++++++--
>> >  rhel/openvswitch-fedora.spec.in                           |  3 +++
>> >  rhel/usr_lib_systemd_system_openvswitch-nonetwork.service | 14 ++++++++++++++
>> >  rhel/usr_lib_systemd_system_openvswitch.service           | 11 +++++++----
>> >  6 files changed, 45 insertions(+), 8 deletions(-)
>> >  create mode 100644 rhel/usr_lib_systemd_system_openvswitch-nonetwork.service
>> >
>> > diff --git a/rhel/automake.mk b/rhel/automake.mk
>> > index 2911e71..cd06823 100644
>> > --- a/rhel/automake.mk
>> > +++ b/rhel/automake.mk
>> > @@ -22,7 +22,8 @@ EXTRA_DIST += \
>> >         rhel/openvswitch-fedora.spec \
>> >         rhel/openvswitch-fedora.spec.in \
>> >         rhel/usr_share_openvswitch_scripts_sysconfig.template \
>> > -       rhel/usr_lib_systemd_system_openvswitch.service
>> > +       rhel/usr_lib_systemd_system_openvswitch.service \
>> > +       rhel/usr_lib_systemd_system_openvswitch-nonetwork.service
>> >
>> >  update_rhel_spec = \
>> >    ($(ro_shell) && sed -e 's,[@]VERSION[@],$(VERSION),g') \
>> > diff --git a/rhel/etc_sysconfig_network-scripts_ifdown-ovs b/rhel/etc_sysconfig_network-scripts_ifdown-ovs
>> > index 8e768c8..2a3d8d2 100755
>> > --- a/rhel/etc_sysconfig_network-scripts_ifdown-ovs
>> > +++ b/rhel/etc_sysconfig_network-scripts_ifdown-ovs
>> > @@ -34,7 +34,15 @@ if [ ! -x ${OTHERSCRIPT} ]; then
>> >      OTHERSCRIPT="/etc/sysconfig/network-scripts/ifdown-eth"
>> >  fi
>> >
>> > -[ -f /var/lock/subsys/openvswitch ] || /sbin/service openvswitch start
>> > +if [ -x /usr/bin/systemctl ]; then
>> > +       if ! systemctl --quiet is-active openvswitch-nonetwork.service; then
>> I think this is considered non-portable.
>> https://www.sourceware.org/autobook/autobook/autobook_216.html
>
> That construction form is used quite frequently in rpm distros. There
> are some examples in /etc/sysconfig/network-scripts/network-functions
> and this is specific to rhel/.
>
> What about using the return value? Something like:
As Ben mentioned, since this is only for RHEL distros, the current
code should be okay.

>
> /usr/bin/systemctl --quiet is-active openvswitch-nonetwork.service
> if [ $? -ne 0 ]; then
>       /usr/bin/systemctl start openvswitch-nonetwork.service
> fi

Also,
> +if [ -x /usr/bin/systemctl ]; then
Should we also check whether the platform is fedora? Otherwise, it may
break centos openvwitch rpms in the future if it includes systemctl?

>
>
>> > +               /usr/bin/systemctl start openvswitch-nonetwork.service
>> > +       fi
>> > +else
>> > +       if [ ! -f /var/lock/subsys/openvswitch ]; then
>> > +               /sbin/service openvswitch start
>> > +       fi
>> > +fi
>> >
>> >  case "$TYPE" in
>> >         OVSBridge)
>> > diff --git a/rhel/etc_sysconfig_network-scripts_ifup-ovs b/rhel/etc_sysconfig_network-scripts_ifup-ovs
>> > index 017346d..817f93f 100755
>> > --- a/rhel/etc_sysconfig_network-scripts_ifup-ovs
>> > +++ b/rhel/etc_sysconfig_network-scripts_ifup-ovs
>> > @@ -60,8 +60,16 @@ fi
>> >         fi
>> >  done
>> >
>> > -[ -f /var/lock/subsys/openvswitch ] || /sbin/service openvswitch start
>> > -
>> > +if [ -x /usr/bin/systemctl ]; then
>> > +       if ! systemctl --quiet is-active openvswitch-nonetwork.service; then
>> > +               /usr/bin/systemctl start openvswitch-nonetwork.service
>> > +       fi
>> > +else
>> > +       if [ ! -f /var/lock/subsys/openvswitch ]; then
>> > +               /sbin/service openvswitch start
>> > +       fi
>> > +fi
>> > +
>> >  case "$TYPE" in
>> >         OVSBridge)
>> >                 # If bridge already exists and is up, it has been configured through
>> > diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
>> > index 5384c32..e6b40a5 100644
>> > --- a/rhel/openvswitch-fedora.spec.in
>> > +++ b/rhel/openvswitch-fedora.spec.in
>> > @@ -45,6 +45,8 @@ install -d -m 755 $RPM_BUILD_ROOT/etc
>> >  install -d -m 755 $RPM_BUILD_ROOT/etc/openvswitch
>> >  install -p -D -m 0644 rhel/usr_lib_systemd_system_openvswitch.service \
>> >          $RPM_BUILD_ROOT%{_unitdir}/openvswitch.service
>> > +install -p -D -m 0644 rhel/usr_lib_systemd_system_openvswitch-nonetwork.service \
>> > +        $RPM_BUILD_ROOT%{_unitdir}/openvswitch-nonetwork.service
>> >  install -m 755 rhel/etc_init.d_openvswitch \
>> >          $RPM_BUILD_ROOT%{_datadir}/openvswitch/scripts/openvswitch.init
>> >  install -d -m 755 $RPM_BUILD_ROOT/etc/sysconfig
>> > @@ -101,6 +103,7 @@ systemctl start openvswitch.service
>> >  %config /etc/sysconfig/openvswitch
>> >  %config /etc/logrotate.d/openvswitch
>> >  %{_unitdir}/openvswitch.service
>> > +%{_unitdir}/openvswitch-nonetwork.service
>> >  %{_datadir}/openvswitch/scripts/openvswitch.init
>> >  %{_sysconfdir}/sysconfig/network-scripts/ifup-ovs
>> >  %{_sysconfdir}/sysconfig/network-scripts/ifdown-ovs
>> > diff --git a/rhel/usr_lib_systemd_system_openvswitch-nonetwork.service b/rhel/usr_lib_systemd_system_openvswitch-nonetwork.service
>> > new file mode 100644
>> > index 0000000..f9fc83d
>> > --- /dev/null
>> > +++ b/rhel/usr_lib_systemd_system_openvswitch-nonetwork.service
>> > @@ -0,0 +1,14 @@
>> > +[Unit]
>> > +Description=Open vSwitch Internal Unit
>> > +After=syslog.target
>> > +PartOf=openvswitch.service
>> > +Wants=openvswitch.service
>> > +
>> > +[Service]
>> > +Type=oneshot
>> > +EnvironmentFile=-/etc/sysconfig/openvswitch
>> > +RemainAfterExit=yes
>> > +ExecStart=/usr/share/openvswitch/scripts/ovs-ctl start --system-id=random \
>> > +       $FORCE_COREFILES $OVSDB_SERVER_PRIORITY $VSWITCHD_PRIORITY \
>> > +       $VSWITCHD_MLOCKALL $BRCOMPAT $OVS_CTL_OPTS
>> I think this will not work well.
>> If you see the rhel startup script, you will see code like this:
>> set ovs_ctl ${1-start}
>> if test X"$OVSDB_SERVER_PRIORITY" != X; then
>>     set "$@" --ovsdb-server-priority="$OVSDB_SERVER_PRIORITY"
>> fi
>> "$@"
>
> I forgot to include the /etc/sysconfig/openvswitch in the patch.
> It looks like this:
Okay. Is there any reason not to call
"/usr/share/openvswitch/scripts/openvswitch.init start" instead? It
will ensure that we have the same type of template file for all
platforms.
I saw a "-" in "EnvironmentFile=-/etc/sysconfig/openvswitch". Does it
have any significance?


>
>    ### Configuration options for openvswitch
>
>    ## FORCE_COREFILES: If 'yes' then core files will be enabled.
>    #FORCE_COREFILES="--force-corefiles=yes"
>
>    ## OVSDB_SERVER_PRIORITY: "nice" priority at which to run ovsdb-server.
>    #OVSDB_SERVER_PRIORITY="--ovsdb-server-priority=-10"
>
>    ## VSWITCHD_PRIORITY: "nice" priority at which to run ovs-vswitchd.
>    #VSWITCHD_PRIORITY="--ovs-vswitchd-priority=-10"
>
>    ## VSWITCHD_MLOCKALL: Whether to pass ovs-vswitchd the --mlockall option.
>    #     This option should be set to "yes" or "no".  The default is "yes".
>    #     Enabling this option can avoid networking interruptions due to
>    #     system memory pressure in extraordinary situations, such as
>    #     multiple
>    #     concurrent VM import operations.
>    #VSWITCHD_MLOCKALL="--mlockall=yes"
>
>    ## BRCOMPAT: If 'yes' compatibility mode will be enabled.
>    #BRCOMPAT=--brcompat
>
>    ## OVS_CTL_OPTS: Extra options to pass to ovs-ctl.  This is, for example,
>    # a suitable place to specify --ovs-vswitchd-wrapper=valgrind.
>    #OVS_CTL_OPTS="--ovs-vswitchd-wrapper=valgrind"
>
>
> By the way, it is hard coded --systemd-id=random in
> rhel/etc_init.d_openvswitch. I think it should be in
> /etc/sysconfig/openvswitch too, right?
I don't have any strong feelings about it (Others may). Feel free to
submit a patch.
"random" has a special meaning (you can look at ovs-ctl man page) and
should be the default. So the startup scripts will have to change to
handle that.


>
> Thanks for reviewing it.
> fbl
>
>
>> > +ExecStop=/usr/share/openvswitch/scripts/ovs-ctl stop
>> > diff --git a/rhel/usr_lib_systemd_system_openvswitch.service b/rhel/usr_lib_systemd_system_openvswitch.service
>> > index f39d7e6..6e08a9a 100644
>> > --- a/rhel/usr_lib_systemd_system_openvswitch.service
>> > +++ b/rhel/usr_lib_systemd_system_openvswitch.service
>> > @@ -1,12 +1,15 @@
>> >  [Unit]
>> > -Description=Open vSwitch
>> > -After=syslog.target network.target
>> > +Description=Open vSwitch Unit
>> > +After=syslog.target
>> > +After=network.target
>> > +After=openvswitch-nonetwork.service
>> > +Requires=openvswitch-nonetwork.service
>> >
>> >  [Service]
>> >  Type=oneshot
>> > -ExecStart=/usr/share/openvswitch/scripts/openvswitch.init start
>> > -ExecStop=/usr/share/openvswitch/scripts/openvswitch.init stop
>> >  RemainAfterExit=yes
>> > +ExecStart=/bin/true
>> > +ExecStop=/bin/true
>>
>>
>> >
>> >  [Install]
>> >  WantedBy=multi-user.target
>> > --
>> > 1.8.3.1
>> >
>> > _______________________________________________
>> > dev mailing list
>> > dev at openvswitch.org
>> > http://openvswitch.org/mailman/listinfo/dev
>>



More information about the dev mailing list