| Message ID | 1467408383-5703-4-git-send-email-aconole@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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 > 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. > > Signed-off-by: Aaron Conole <aconole@redhat.com> This is probably obvious, but I'm going to defer to Flavio and Russell on this one too.
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 :-) > 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. > Signed-off-by: Aaron Conole <aconole@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. > [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. > + > +[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. > +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. > -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
Flavio Leitner <fbl@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 <aconole@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
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 [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 +ConditionPathIsReadWrite=/var/run/openvswitch/db.sock + +[Service] +Type=forking +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 -ExecStop=/usr/share/openvswitch/scripts/ovs-ctl stop +ExecStop=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd stop RuntimeDirectory=openvswitch RuntimeDirectoryMode=0755
This commit builds upon some of the recent ovs-ctl changes to build a more integrated systemd setup. A new service (openvswitch-network) is 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. Signed-off-by: Aaron Conole <aconole@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