On 7/20/26 11:02 AM, Dumitru Ceara wrote: > 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? >
Sorry, I should've replied to v3 instead: https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ > 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
