On 7/2/26 4:32 AM, Zhang Hua via dev wrote: > ovn-ctl and ovn-lib currently chown the OVN log directory > recursively when services start. That can undo ownership set by > distribution tooling such as tmpfiles.d and logrotate. In particular, > Debian and Ubuntu need ovn-controller.log to remain root:adm so rsyslog > can read it, but a later OVN service restart may change it back to the > OVN daemon user/group. > > Add an OVN tmpfiles.d template for the log directory and > ovn-controller.log, with configurable log user, log group and tmpfiles.d > installation directory. Debian configures the log owner as root:adm, > while Fedora/RHEL keeps openvswitch:openvswitch. > > Stop recursively changing the OVN log directory ownership from ovn-ctl > and ovn-lib at service startup. Runtime, database and configuration > paths are still chowned according to --ovn-user, preserving the existing > privilege-drop behavior for non-log state. > > Reported-at: https://github.com/ovn-org/ovn/issues/310 > Signed-off-by: Zhang Hua <[email protected]> > ---
Hi Zhang Hua, Thanks for the patch! Hi Frode, Would you have some time to look at this patch from an Ubuntu/Debian perspective? Thanks, Dumitru > Submitted-at: https://github.com/ovn-org/ovn/pull/311 > > v2: > - Added the trailing dot required by checkpatch to the subject. > > Testing: > - Ran utilities/checkpatch.py -1. > - Ran make check TESTSUITEFLAGS="-j$(nproc)". > > Makefile.am | 4 ++++ > configure.ac | 3 +++ > debian/ovn-common.install | 1 + > debian/rules | 2 +- > m4/ovn.m4 | 31 +++++++++++++++++++++++++++++++ > rhel/ovn-fedora.spec.in | 3 +++ > utilities/automake.mk | 4 ++++ > utilities/ovn-ctl | 6 ++---- > utilities/ovn-lib.in | 1 - > utilities/ovn-tmpfiles.conf.in | 2 ++ > 10 files changed, 51 insertions(+), 6 deletions(-) > create mode 100644 utilities/ovn-tmpfiles.conf.in > > diff --git a/Makefile.am b/Makefile.am > index 0f2389b25..79929ee6c 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -131,6 +131,7 @@ noinst_PROGRAMS = > noinst_SCRIPTS = > OVSIDL_BUILT = > pkgdata_DATA = > +tmpfiles_DATA = > sbin_SCRIPTS = > scripts_SCRIPTS = > completion_SCRIPTS = > @@ -148,6 +149,7 @@ endif > scriptsdir = $(pkgdatadir)/scripts > completiondir = $(sysconfdir)/bash_completion.d > pkgconfigdir = $(libdir)/pkgconfig > +tmpfilesdir = @TMPFILESDIR@ > > # This ensures that files added to EXTRA_DIST are always distributed, > # even if they are inside an Automake if...endif conditional block that is > @@ -166,6 +168,8 @@ SUFFIXES += .in > sed \ > -e 's,[@]PKIDIR[@],$(PKIDIR),g' \ > -e 's,[@]LOGDIR[@],$(LOGDIR),g' \ > + -e 's,[@]LOGUSER[@],$(LOGUSER),g' \ > + -e 's,[@]LOGGROUP[@],$(LOGGROUP),g' \ > -e 's,[@]DBDIR[@],$(DBDIR),g' \ > -e 's,[@]PYTHON3[@],$(PYTHON3),g' \ > -e 's,[@]OVN_RUNDIR[@],$(OVN_RUNDIR),g' \ > diff --git a/configure.ac b/configure.ac > index cfa4cc386..d19e26612 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -88,6 +88,9 @@ OVS_CHECK_NETLINK > OVS_CHECK_LINUX_NETLINK > OVS_CHECK_OPENSSL > OVN_CHECK_LOGDIR > +OVN_CHECK_LOGUSER > +OVN_CHECK_LOGGROUP > +OVN_CHECK_TMPFILESDIR > OVN_CHECK_PYTHON3 > OVN_CHECK_FLAKE8 > OVN_CHECK_SPHINX > diff --git a/debian/ovn-common.install b/debian/ovn-common.install > index fc48f07e4..b890c6797 100644 > --- a/debian/ovn-common.install > +++ b/debian/ovn-common.install > @@ -12,4 +12,5 @@ usr/share/ovn/scripts/ovn-lib > usr/share/ovn/scripts/ovn-bugtool-nbctl-show > usr/share/ovn/scripts/ovn-bugtool-sbctl-lflow-list > usr/share/ovn/scripts/ovn-bugtool-sbctl-show > +usr/lib/tmpfiles.d/ovn-tmpfiles.conf > usr/lib/*/libovn*.so.* > diff --git a/debian/rules b/debian/rules > index b25a0b48e..0d5da5b26 100755 > --- a/debian/rules > +++ b/debian/rules > @@ -30,7 +30,7 @@ override_dh_autoreconf: > dh_autoreconf $(DH_AS_NEEDED) > > override_dh_auto_configure: > - dh_auto_configure -- --enable-ssl --enable-shared > --with-ovs-source=${OVSDIR} $(EXTRA_CONFIGURE_OPTS) > + dh_auto_configure -- --enable-ssl --enable-shared > --with-ovs-source=${OVSDIR} --with-log-user=root --with-log-group=adm > $(EXTRA_CONFIGURE_OPTS) > > override_dh_auto_test: > ifeq (,$(filter nocheck,$(DEB_BUILD_OPTIONS))) > diff --git a/m4/ovn.m4 b/m4/ovn.m4 > index 22ad1a27f..f022a0152 100644 > --- a/m4/ovn.m4 > +++ b/m4/ovn.m4 > @@ -127,6 +127,37 @@ AC_DEFUN([OVN_CHECK_LOGDIR], > [LOGDIR='${localstatedir}/log/${PACKAGE}']) > AC_SUBST([LOGDIR])]) > > +dnl Checks for the user that should own log files. > +AC_DEFUN([OVN_CHECK_LOGUSER], > + [AC_ARG_WITH( > + [log-user], > + AS_HELP_STRING([--with-log-user=USER], > + [user used for log files [[root]]]), > + [LOGUSER=$withval], > + [LOGUSER=root]) > + AC_SUBST([LOGUSER])]) > + > +dnl Checks for the group that should own log files. > +AC_DEFUN([OVN_CHECK_LOGGROUP], > + [AC_ARG_WITH( > + [log-group], > + AS_HELP_STRING([--with-log-group=GROUP], > + [group used for log files [[root]]]), > + [LOGGROUP=$withval], > + [LOGGROUP=root]) > + AC_SUBST([LOGGROUP])]) > + > +dnl Checks for the directory in which to install tmpfiles.d configuration. > +AC_DEFUN([OVN_CHECK_TMPFILESDIR], > + [AC_ARG_WITH( > + [tmpfilesdir], > + AS_HELP_STRING([--with-tmpfilesdir=DIR], > + [directory used for tmpfiles.d configuration > + [[PREFIX/lib/tmpfiles.d]]]), > + [TMPFILESDIR=$withval], > + [TMPFILESDIR='${prefix}/lib/tmpfiles.d']) > + AC_SUBST([TMPFILESDIR])]) > + > dnl Checks for the directory in which to store the OVN database. > AC_DEFUN([OVN_CHECK_DBDIR], > [AC_ARG_WITH( > diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in > index 131b3eaab..1fdaad706 100644 > --- a/rhel/ovn-fedora.spec.in > +++ b/rhel/ovn-fedora.spec.in > @@ -159,6 +159,8 @@ cd - > --with-ovs-source=$PWD/openvswitch-%{ovsver} \ > %if %{with libcapng} > --enable-libcapng \ > + --with-log-user=openvswitch \ > + --with-log-group=openvswitch \ > %else > --disable-libcapng \ > %endif > @@ -531,6 +533,7 @@ fi > %{_mandir}/man8/ovn-debug.8* > %{_prefix}/lib/ocf/resource.d/ovn/ovndb-servers > %config(noreplace) %{_sysconfdir}/logrotate.d/ovn > +%{_tmpfilesdir}/ovn-tmpfiles.conf > %{_unitdir}/[email protected] > > %files docker > diff --git a/utilities/automake.mk b/utilities/automake.mk > index b620038d0..c22b5d8c3 100644 > --- a/utilities/automake.mk > +++ b/utilities/automake.mk > @@ -26,6 +26,7 @@ EXTRA_DIST += \ > utilities/ovn-ctl \ > utilities/ovn-lib.in \ > utilities/ovn-ctl.8.xml \ > + utilities/ovn-tmpfiles.conf.in \ > utilities/ovn-docker-overlay-driver.in \ > utilities/ovn-docker-underlay-driver.in \ > utilities/ovn-nbctl.8.xml \ > @@ -48,6 +49,7 @@ EXTRA_DIST += \ > CLEANFILES += \ > utilities/ovn-ctl.8 \ > utilities/ovn-lib \ > + utilities/ovn-tmpfiles.conf \ > utilities/ovn-docker-overlay-driver \ > utilities/ovn-docker-underlay-driver \ > utilities/ovn-nbctl.8 \ > @@ -66,7 +68,9 @@ CLEANFILES += \ > EXTRA_DIST += utilities/ovn-sim.in > noinst_SCRIPTS += utilities/ovn-sim > > +tmpfiles_DATA += utilities/ovn-tmpfiles.conf > utilities/ovn-lib: $(top_builddir)/config.status > +utilities/ovn-tmpfiles.conf: $(top_builddir)/config.status > > # ovn-nbctl > bin_PROGRAMS += utilities/ovn-nbctl > diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl > index 3b62ca9b7..58effd5e5 100755 > --- a/utilities/ovn-ctl > +++ b/utilities/ovn-ctl > @@ -276,9 +276,8 @@ $cluster_remote_port > upgrade_db "$file" "$schema" > fi > > - # Set the owner of the ovn_dbdir (with -R option) to OVN_USER if set. > - # This is required because the ovndbs are created with root permission > - # if not present when create_cluster/upgrade_db is called. > + # Database files may be created as root before ovsdb-server drops > + # privileges, so keep ownership aligned with OVN_USER when configured. > INSTALL_USER="$(id -un)" > INSTALL_GROUP="$(id -gn)" > [ "$OVN_USER" != "" ] && INSTALL_USER="${OVN_USER%:*}" > @@ -286,7 +285,6 @@ $cluster_remote_port > > chown -R $INSTALL_USER:$INSTALL_GROUP $ovn_dbdir > chown -R $INSTALL_USER:$INSTALL_GROUP $OVN_RUNDIR > - chown -R $INSTALL_USER:$INSTALL_GROUP $ovn_logdir > chown -R $INSTALL_USER:$INSTALL_GROUP $ovn_etcdir > > set ovsdb-server > diff --git a/utilities/ovn-lib.in b/utilities/ovn-lib.in > index 5a0766816..0b19149da 100644 > --- a/utilities/ovn-lib.in > +++ b/utilities/ovn-lib.in > @@ -133,7 +133,6 @@ start_ovn_daemon () { > set "$@" --detach > test X"$MONITOR" = Xno || set "$@" --monitor > > - chown -R $INSTALL_USER:$INSTALL_GROUP $ovn_logdir > chown -R $INSTALL_USER:$INSTALL_GROUP $ovn_rundir > > start_wrapped_daemon "$wrapper" $daemon "$priority" "$@" > diff --git a/utilities/ovn-tmpfiles.conf.in b/utilities/ovn-tmpfiles.conf.in > new file mode 100644 > index 000000000..d37391f88 > --- /dev/null > +++ b/utilities/ovn-tmpfiles.conf.in > @@ -0,0 +1,2 @@ > +d @LOGDIR@ 0750 @LOGUSER@ @LOGGROUP@ - > +f @LOGDIR@/ovn-controller.log 0640 @LOGUSER@ @LOGGROUP@ - > \ No newline at end of file _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
