| Message ID | f2f5a0d3e733422bdb62c7933164d0a53385c51f.1773245898.git.tredaelli@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | aaron conole |
| Headers | show |
| Series | Add systemd socket activation for ovsdb-server | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Timothy Redaelli via dev <ovs-dev@openvswitch.org> writes: > When systemd socket-activates ovsdb-server, it sets LISTEN_FDNAMES > to the socket unit name and passes the listening socket as fd 3. > Detect this in do_start_ovsdb() and use --remote=pfd:3 instead of > --remote=punix:$DB_SOCK. > > Validate LISTEN_PID against the current shell's PID, as required by > sd_listen_fds(3), to ensure the variables were set for this process > and not inherited from a parent. > > Unset LISTEN_FDS, LISTEN_FDNAMES, and LISTEN_PID after consuming > them to prevent propagation to child processes. Which child processes gets started when we are under systemd? I thought we don't use the monitor process when run under systemd, so I'm not sure why we would need to clear these? Is it just precaution? > Co-authored-by: Lubomir Rintel <lkundrak@v3.sk> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com> > --- > utilities/ovs-ctl.in | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in > index c65c76812..dff416f08 100644 > --- a/utilities/ovs-ctl.in > +++ b/utilities/ovs-ctl.in > @@ -149,7 +149,13 @@ do_start_ovsdb () { > set "$@" --no-self-confinement > fi > set "$@" -vconsole:emer -vsyslog:err -vfile:info > - set "$@" --remote=punix:"$DB_SOCK" > + if test X"$LISTEN_PID" = X"$$" && \ > + test X"$LISTEN_FDNAMES" = X"ovsdb-server.socket"; then > + unset LISTEN_FDS LISTEN_FDNAMES LISTEN_PID > + set "$@" --remote=pfd:3 I think also LISTEN_PIDFDID should be cleared if we're clearing these, right? > + else > + set "$@" --remote=punix:"$DB_SOCK" > + fi > set "$@" --private-key=db:Open_vSwitch,SSL,private_key > set "$@" --certificate=db:Open_vSwitch,SSL,certificate > set "$@" --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert
On Tue, 24 Mar 2026 16:26:03 -0400 Aaron Conole <aconole@redhat.com> wrote: > Timothy Redaelli via dev <ovs-dev@openvswitch.org> writes: > > > When systemd socket-activates ovsdb-server, it sets LISTEN_FDNAMES > > to the socket unit name and passes the listening socket as fd 3. > > Detect this in do_start_ovsdb() and use --remote=pfd:3 instead of > > --remote=punix:$DB_SOCK. > > > > Validate LISTEN_PID against the current shell's PID, as required by > > sd_listen_fds(3), to ensure the variables were set for this process > > and not inherited from a parent. > > > > Unset LISTEN_FDS, LISTEN_FDNAMES, and LISTEN_PID after consuming > > them to prevent propagation to child processes. > > Which child processes gets started when we are under systemd? I thought > we don't use the monitor process when run under systemd, so I'm not sure > why we would need to clear these? Is it just precaution? you are right that ovsdb-server runs with --no-monitor under systemd, so no monitor process is forked. The clearing follows sd_listen_fds(3) which documents that these variables should be unset so they are "no longer inherited by child processes" [1]. Even without --monitor, the daemon --detach forks and the environment would be inherited. It is cheap insurance against any child misinterpreting stale values. > > Co-authored-by: Lubomir Rintel <lkundrak@v3.sk> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com> > > --- > > utilities/ovs-ctl.in | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in > > index c65c76812..dff416f08 100644 > > --- a/utilities/ovs-ctl.in > > +++ b/utilities/ovs-ctl.in > > @@ -149,7 +149,13 @@ do_start_ovsdb () { > > set "$@" --no-self-confinement > > fi > > set "$@" -vconsole:emer -vsyslog:err -vfile:info > > - set "$@" --remote=punix:"$DB_SOCK" > > + if test X"$LISTEN_PID" = X"$$" && \ > > + test X"$LISTEN_FDNAMES" = X"ovsdb-server.socket"; then > > + unset LISTEN_FDS LISTEN_FDNAMES LISTEN_PID > > + set "$@" --remote=pfd:3 > > I think also LISTEN_PIDFDID should be cleared if we're clearing these, > right? good catch, v1 was only clearing 3 of the 4 variables documented in sd_listen_fds(3). v2 now unsets all four: LISTEN_FDS, LISTEN_FDNAMES, LISTEN_PID, and LISTEN_PIDFDID. > > + else > > + set "$@" --remote=punix:"$DB_SOCK" > > + fi > > set "$@" --private-key=db:Open_vSwitch,SSL,private_key > > set "$@" --certificate=db:Open_vSwitch,SSL,certificate > > set "$@" --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert > [1] sd_listen_fds(3): "If the unset_environment parameter is non-zero, sd_listen_fds() will unset the $LISTEN_FDS, $LISTEN_PID, $LISTEN_PIDFDID, and $LISTEN_FDNAMES environment variables before returning [...] the variables are no longer inherited by child processes."
diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in index c65c76812..dff416f08 100644 --- a/utilities/ovs-ctl.in +++ b/utilities/ovs-ctl.in @@ -149,7 +149,13 @@ do_start_ovsdb () { set "$@" --no-self-confinement fi set "$@" -vconsole:emer -vsyslog:err -vfile:info - set "$@" --remote=punix:"$DB_SOCK" + if test X"$LISTEN_PID" = X"$$" && \ + test X"$LISTEN_FDNAMES" = X"ovsdb-server.socket"; then + unset LISTEN_FDS LISTEN_FDNAMES LISTEN_PID + set "$@" --remote=pfd:3 + else + set "$@" --remote=punix:"$DB_SOCK" + fi set "$@" --private-key=db:Open_vSwitch,SSL,private_key set "$@" --certificate=db:Open_vSwitch,SSL,certificate set "$@" --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert