Flavio Leitner <f...@sysclose.org> writes:
> On Fri, Jul 01, 2016 at 05:26:23PM -0400, Aaron Conole wrote:
>> This commit builds upon some of the recent ovs-ctl changes to build a
>> more integrated systemd setup. A new service (openvswitch-network) is
>
> I think you renamed to 'ovs-vswitchd' after we talked offline :-)
d'oh! needed to update the commit message.. will do.
>> added to track the ovs-vswitchd, and openvswitch-nonetwork is reserved
>> for the ovsdb-server daemon. The systemd scripts still use ovs-ctl to
>> actually initialize the daemons.
>
> That's cool.
As always, thanks so much for the review, Flavio!
>> Signed-off-by: Aaron Conole <acon...@redhat.com>
>> ---
>> rhel/automake.mk | 1 +
>> rhel/openvswitch-fedora.spec.in | 3 ++-
>> rhel/usr_lib_systemd_system_openvswitch.service | 5 +++--
>> rhel/usr_lib_systemd_system_ovs-vswitchd.service | 21 +++++++++++++++++++++
>> rhel/usr_lib_systemd_system_ovsdb-server.service | 11 ++++++-----
>> 5 files changed, 33 insertions(+), 8 deletions(-)
>> create mode 100644 rhel/usr_lib_systemd_system_ovs-vswitchd.service
>>
>> diff --git a/rhel/automake.mk b/rhel/automake.mk
>> index 7907a87..a3c180c 100644
>> --- a/rhel/automake.mk
>> +++ b/rhel/automake.mk
>> @@ -27,6 +27,7 @@ EXTRA_DIST += \
>> rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template \
>> rhel/usr_lib_systemd_system_openvswitch.service \
>> rhel/usr_lib_systemd_system_ovsdb-server.service \
>> + rhel/usr_lib_systemd_system_ovs-vswitchd.service \
>> rhel/usr_lib_systemd_system_ovn-controller.service \
>> rhel/usr_lib_systemd_system_ovn-controller-vtep.service \
>> rhel/usr_lib_systemd_system_ovn-northd.service
>> diff --git a/rhel/openvswitch-fedora.spec.in
>> b/rhel/openvswitch-fedora.spec.in
>> index ed7b3c4..09756ec 100644
>> --- a/rhel/openvswitch-fedora.spec.in
>> +++ b/rhel/openvswitch-fedora.spec.in
>> @@ -189,7 +189,7 @@ install -d -m 0755
>> $RPM_BUILD_ROOT%{_sysconfdir}/openvswitch
>> install -p -D -m 0644 \
>> rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template \
>> $RPM_BUILD_ROOT/%{_sysconfdir}/sysconfig/openvswitch
>> -for service in openvswitch ovsdb-server \
>> +for service in openvswitch ovsdb-server ovs-vswitchd \
>> ovn-controller ovn-controller-vtep ovn-northd; do
>> install -p -D -m 0644 \
>> rhel/usr_lib_systemd_system_${service}.service \
>> @@ -417,6 +417,7 @@ fi
>> %config(noreplace) %{_sysconfdir}/logrotate.d/openvswitch
>> %{_unitdir}/openvswitch.service
>> %{_unitdir}/ovsdb-server.service
>> +%{_unitdir}/ovs-vswitchd.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.service
>> b/rhel/usr_lib_systemd_system_openvswitch.service
>> index 96c697b..205cba7 100644
>> --- a/rhel/usr_lib_systemd_system_openvswitch.service
>> +++ b/rhel/usr_lib_systemd_system_openvswitch.service
>> @@ -2,12 +2,13 @@
>> Description=Open vSwitch
>> After=syslog.target network.target ovsdb-server.service
>> Requires=ovsdb-server.service
>> +Requires=ovs-vswitchd.service
>>
>> [Service]
>> Type=oneshot
>> -ExecStart=/bin/true
>> -ExecStop=/bin/true
>> RemainAfterExit=yes
>> +ExecStart=/bin/true
>> +ExecReload=/bin/true
>
> You're adding ExecReload to avoid signaling any process and since the
> reload signal is propagated, that is good. But I don't see why we
> don't need ExecStop anymore. I was quite sure they all fall into the
> same problem.
I'll have to get back to you on this. I don't remember why that was
dropped, and reading the manual, I'm convinced that ExecStart and
ExecReload should also be dropped, but here I've put them back in. Let
me get back to you on this.
>> [Install]
>> WantedBy=multi-user.target
>> diff --git a/rhel/usr_lib_systemd_system_ovs-vswitchd.service
>> b/rhel/usr_lib_systemd_system_ovs-vswitchd.service
>> new file mode 100644
>> index 0000000..18546d7
>> --- /dev/null
>> +++ b/rhel/usr_lib_systemd_system_ovs-vswitchd.service
>> @@ -0,0 +1,21 @@
>> +[Unit]
>> +Description=Open vSwitch Forwarding Unit
>> +After=ovsdb-server.target
>> +PartOf=openvswitch.service
>> +Wants=openvswitch.service
>> +Requires=ovsdb-server.service
>> +ReloadPropagatedFrom=ovsdb-server.service
>
> If I am reading right: openvswitch propagates to ovsdb-server
> which propagates to ovs-vswitchd.
> Looks good.
>
>> +ConditionPathIsReadWrite=/var/run/openvswitch/db.sock
>
> Interesting, so we start ovs-vswitchd only when db.sock is ready.
>
> By default, the prefix is /usr/local and so the db.sock location
> would be /usr/local/var/run/openvswitch/db.sock. That can be
> fixed moving the file to .in and add to the makefile as OVS does
> for spec files, for instance.
I can make it depend on rundir, but there were other hardcoded paths, so
I thought one more wouldn't make a difference. I could fix all the
hardcoded paths, though. Should I do that?
>
>> +
>> +[Service]
>> +Type=forking
>
> I think systemd monitors the thread after the first fork as
> the main process which in this case it would be the execution
> of the daemon itself, which forks again. Shouldn't we provide
> PIDFile then? I honestly don't know what systemd will do in
> this case.
That's what the --no-monitoring (well, renaming it to --no-monitor) call
does. 'Type=forking' is a hint to systemd to watch for the last process
remaining instead of reasoning that the first process started is the
process to track. It appears (at least on fedora and RHEL7) to track
the ovs daemons properly.
systemd won't poll the PID at the PIDFile. It's really just a hint to
systemd that the process now exists. There is no detection of failure
or any kind of other fancy systemd integrations. I tried using that
instead of writing the monitor patch.
>> +EnvironmentFile=-/etc/sysconfig/openvswitch
>> +ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
>> + --no-ovsdb-server --no-monitoring start \
>> + --system-id=random $OPTIONS
>> +ExecStop=/usr/share/openvswitch/scripts/ovs-ctl --no-ovsdb-server stop
>> +ExecReload=/usr/share/openvswitch/scripts/ovs-ctl --no-ovsdb-server \
>> + --no-monitoring restart \
>> + --system-id=random
>> +RuntimeDirectory=openvswitch
>> +RuntimeDirectoryMode=0755
>> diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service
>> b/rhel/usr_lib_systemd_system_ovsdb-server.service
>> index e4c2a66..1fe09e5 100644
>> --- a/rhel/usr_lib_systemd_system_ovsdb-server.service
>> +++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
>> @@ -1,15 +1,16 @@
>> [Unit]
>> -Description=Open vSwitch Internal Unit
>> +Description=Open vSwitch Database Unit
>> After=syslog.target
>> PartOf=openvswitch.service
>> Wants=openvswitch.service
>> +ReloadPropagatedFrom=openvswitch.service
>>
>> [Service]
>> -Type=oneshot
>> -RemainAfterExit=yes
>> +Type=forking
>> EnvironmentFile=-/etc/sysconfig/openvswitch
>> -ExecStart=/usr/share/openvswitch/scripts/ovs-ctl start \
>> +ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
>> + --no-ovs-vswitchd --no-monitoring start \
>> --system-id=random $OPTIONS
>
>
> We now use $OPTIONS from /etc/sysconfig/openvswitch for both daemons.
> Does that work? I think ovs-ctl will parse the options and only
> apply the ones that make sense for each daemon, so we would be OK.
Well, it was working for me, but I didn't do too much testing on that
front. If you would prefer, I can change it to be per-daemon options
(so $OVS_VSWITCHD_OPTIONS and $OVSDB_SERVER_OPTIONS) if that would suit
better.
>> -ExecStop=/usr/share/openvswitch/scripts/ovs-ctl stop
>> +ExecStop=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd stop
>> RuntimeDirectory=openvswitch
>> RuntimeDirectoryMode=0755
>> --
>> 2.5.5
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
