From 73e3ccc401829025a9151fddff29d67a0ac2321d Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 16 Jun 2021 09:26:24 -0400 Subject: [PATCH] Use generator to enable ostree-remount.service and ostree-finalize-staged.path We struggled for a long time with enablement of our "internal units", trying to follow the philosophy that units should only be enabled by explicit preset. See https://bugzilla.redhat.com/show_bug.cgi?id=1451458 and https://github.com/coreos/rpm-ostree/pull/1482 etc. And I just saw chat (RH internal on a proprietary system sadly) where someone hit `ostree-remount.service` not being enabled in CentOS8. Thinking about this more, I realized we've shipped a systemd generator for a long time and while its only role until now was to generate `var.mount`, but by using it to force on our internal units, we don't require people to deal with presets anymore. Basically we're inverting things so that "if ostree= is on the kernel cmdline, then enable our units" and not "enable our units, but have them use ConditionKernelCmdline=ostree to skip". Drop the weird gyrations we were doing around `ostree-finalize-staged.path` too; forking `systemctl start` is just asking for bugs. So after this, hopefully we won't ever again have to think about distribution presets and our units. --- configure.ac | 1 + src/libostree/ostree-impl-system-generator.c | 56 +++++++++++++++++--- src/libostree/ostree-sysroot-deploy.c | 22 -------- src/libostree/ostree-sysroot-private.h | 3 +- src/libostree/ostree-sysroot.c | 1 - tests/kolainst/destructive/staged-deploy.sh | 8 +-- 6 files changed, 56 insertions(+), 35 deletions(-) diff --git a/configure.ac b/configure.ac index f37b9cb3..82b9cf32 100644 --- a/configure.ac +++ b/configure.ac @@ -499,6 +499,7 @@ AS_IF([test "x$with_libsystemd" = "xyes"], [ [with_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd)]) AS_IF([test "x$with_systemdsystemunitdir" != "xno"], [ AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir]) + AC_DEFINE_UNQUOTED([SYSTEM_DATA_UNIT_PATH], ["$with_systemdsystemunitdir"], ["unit path"]) ]) AC_ARG_WITH([systemdsystemgeneratordir], AS_HELP_STRING([--with-systemdsystemgeneratordir=DIR], [Directory for systemd generators]), diff --git a/src/libostree/ostree-impl-system-generator.c b/src/libostree/ostree-impl-system-generator.c index cb9170b3..73da882d 100644 --- a/src/libostree/ostree-impl-system-generator.c +++ b/src/libostree/ostree-impl-system-generator.c @@ -113,13 +113,39 @@ stateroot_from_ostree_cmdline (const char *ostree_cmdline, } #endif -/* Implementation of ostree-system-generator */ -gboolean -_ostree_impl_system_generator (const char *ostree_cmdline, - const char *normal_dir, - const char *early_dir, - const char *late_dir, - GError **error) +/* Forcibly enable our internal units, since we detected ostree= on the kernel cmdline */ +static gboolean +require_internal_units (const char *normal_dir, + const char *early_dir, + const char *late_dir, + GError **error) +{ + GCancellable *cancellable = NULL; + + glnx_autofd int normal_dir_dfd = -1; + if (!glnx_opendirat (AT_FDCWD, normal_dir, TRUE, &normal_dir_dfd, error)) + return FALSE; + + if (!glnx_shutil_mkdir_p_at (normal_dir_dfd, "local-fs.target.requires", 0755, cancellable, error)) + return FALSE; + if (symlinkat (SYSTEM_DATA_UNIT_PATH "/ostree-remount.service", normal_dir_dfd, "local-fs.target.requires/ostree-remount.service") < 0) + return glnx_throw_errno_prefix (error, "symlinkat"); + + if (!glnx_shutil_mkdir_p_at (normal_dir_dfd, "multi-user.target.wants", 0755, cancellable, error)) + return FALSE; + if (symlinkat (SYSTEM_DATA_UNIT_PATH "/ostree-finalize-staged.path", normal_dir_dfd, "multi-user.target.wants/ostree-finalize-staged.path") < 0) + return glnx_throw_errno_prefix (error, "symlinkat"); + + return TRUE; +} + +/* Generate var.mount */ +static gboolean +fstab_generator (const char *ostree_cmdline, + const char *normal_dir, + const char *early_dir, + const char *late_dir, + GError **error) { #ifdef HAVE_LIBMOUNT /* Not currently cancellable, but define a var in case we care later */ @@ -225,3 +251,19 @@ _ostree_impl_system_generator (const char *ostree_cmdline, return glnx_throw (error, "Not implemented"); #endif } + +/* Implementation of ostree-system-generator */ +gboolean +_ostree_impl_system_generator (const char *ostree_cmdline, + const char *normal_dir, + const char *early_dir, + const char *late_dir, + GError **error) +{ + if (!require_internal_units (normal_dir, early_dir, late_dir, error)) + return FALSE; + if (!fstab_generator (ostree_cmdline, normal_dir, early_dir, late_dir, error)) + return FALSE; + + return TRUE; +} diff --git a/src/libostree/ostree-sysroot-deploy.c b/src/libostree/ostree-sysroot-deploy.c index 840775d4..6a13a41b 100644 --- a/src/libostree/ostree-sysroot-deploy.c +++ b/src/libostree/ostree-sysroot-deploy.c @@ -3106,28 +3106,6 @@ ostree_sysroot_stage_tree_with_options (OstreeSysroot *self, if (booted_deployment == NULL) return glnx_prefix_error (error, "Cannot stage deployment"); - /* This is used by the testsuite to exercise the path unit, until it becomes the default - * (which is pending on the preset making it everywhere). */ - if ((self->debug_flags & OSTREE_SYSROOT_DEBUG_TEST_STAGED_PATH) == 0) - { - /* This is a bit of a hack. When adding a new service we have to end up getting - * into the presets for downstream distros; see e.g. https://src.fedoraproject.org/rpms/ostree/pull-request/7 - * - * Then again, it's perhaps a bit nicer to only start the service on-demand anyways. - */ - const char *const systemctl_argv[] = {"systemctl", "start", "ostree-finalize-staged.service", NULL}; - int estatus; - if (!g_spawn_sync (NULL, (char**)systemctl_argv, NULL, G_SPAWN_SEARCH_PATH, - NULL, NULL, NULL, NULL, &estatus, error)) - return FALSE; - if (!g_spawn_check_exit_status (estatus, error)) - return FALSE; - } - else - { - g_print ("test-staged-path: Not running `systemctl start`\n"); - } /* OSTREE_SYSROOT_DEBUG_TEST_STAGED_PATH */ - g_autoptr(OstreeDeployment) deployment = NULL; if (!sysroot_initialize_deployment (self, osname, revision, origin, opts, &deployment, cancellable, error)) diff --git a/src/libostree/ostree-sysroot-private.h b/src/libostree/ostree-sysroot-private.h index 641f0533..bf3ecf32 100644 --- a/src/libostree/ostree-sysroot-private.h +++ b/src/libostree/ostree-sysroot-private.h @@ -37,8 +37,7 @@ typedef enum { OSTREE_SYSROOT_DEBUG_TEST_FIFREEZE = 1 << 2, /* This is a temporary flag until we fully drop the explicit `systemctl start * ostree-finalize-staged.service` so that tests can exercise the new path unit. */ - OSTREE_SYSROOT_DEBUG_TEST_STAGED_PATH = 1 << 3, - OSTREE_SYSROOT_DEBUG_TEST_NO_DTB = 1 << 4, /* https://github.com/ostreedev/ostree/issues/2154 */ + OSTREE_SYSROOT_DEBUG_TEST_NO_DTB = 1 << 3, /* https://github.com/ostreedev/ostree/issues/2154 */ } OstreeSysrootDebugFlags; typedef enum { diff --git a/src/libostree/ostree-sysroot.c b/src/libostree/ostree-sysroot.c index b0ae66cf..9d4d64c9 100644 --- a/src/libostree/ostree-sysroot.c +++ b/src/libostree/ostree-sysroot.c @@ -190,7 +190,6 @@ ostree_sysroot_init (OstreeSysroot *self) { "mutable-deployments", OSTREE_SYSROOT_DEBUG_MUTABLE_DEPLOYMENTS }, { "test-fifreeze", OSTREE_SYSROOT_DEBUG_TEST_FIFREEZE }, { "no-xattrs", OSTREE_SYSROOT_DEBUG_NO_XATTRS }, - { "test-staged-path", OSTREE_SYSROOT_DEBUG_TEST_STAGED_PATH }, { "no-dtb", OSTREE_SYSROOT_DEBUG_TEST_NO_DTB }, }; diff --git a/tests/kolainst/destructive/staged-deploy.sh b/tests/kolainst/destructive/staged-deploy.sh index 0068ed56..b5d0b319 100755 --- a/tests/kolainst/destructive/staged-deploy.sh +++ b/tests/kolainst/destructive/staged-deploy.sh @@ -8,6 +8,10 @@ prepare_tmpdir case "${AUTOPKGTEST_REBOOT_MARK:-}" in "") + # Test our generator + test -f /run/systemd/generator/multi-user.target.wants/ostree-finalize-staged.path + test -f /run/systemd/generator/local-fs.target.requires/ostree-remount.service + # Initial cleanup to handle the cosa fast-build case ## TODO remove workaround for https://github.com/coreos/rpm-ostree/pull/2021 mkdir -p /var/lib/rpm-ostree/history @@ -19,7 +23,6 @@ case "${AUTOPKGTEST_REBOOT_MARK:-}" in # Start up path unit systemctl enable --now ostree-finalize-staged.path # Write staged-deploy commit - export OSTREE_SYSROOT_DEBUG="test-staged-path" cd /ostree/repo/tmp # https://github.com/ostreedev/ostree/issues/1569 ostree checkout -H ${commit} t @@ -29,8 +32,7 @@ case "${AUTOPKGTEST_REBOOT_MARK:-}" in systemctl show -p SubState ostree-finalize-staged.path | grep -q waiting systemctl show -p ActiveState ostree-finalize-staged.service | grep -q inactive systemctl show -p TriggeredBy ostree-finalize-staged.service | grep -q path - ostree admin deploy --stage staged-deploy | tee out.txt - assert_file_has_content out.txt 'test-staged-path: Not running' + ostree admin deploy --stage staged-deploy systemctl show -p SubState ostree-finalize-staged.path | grep running systemctl show -p ActiveState ostree-finalize-staged.service | grep active new_mtime=$(stat -c '%.Y' /sysroot/ostree/deploy)