From 1b41c5ff66761b79c74197207359988bbe867ec9 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sun, 23 May 2021 09:12:41 -0400 Subject: [PATCH] daemon: Add socket activation via /run/rpm-ostreed.socket For historical reasons, the daemon ends up doing a lot of initialization before even claiming the DBus name. For example, it calls `ostree_sysroot_load()`. We also end up scanning the RPM database, and actually parse all the GPG keys in `/etc/pki/rpm-gpg` etc. This causes two related problems: - By doing all this work before claiming the bus name, we race against the (pretty low) DBus service timeout of 25s. - If something hard fails at initialization, the client can't easily see the error because it just appears in the systemd journal. The client will just see a service timeout. The solution to this is to adopt systemd socket activation, which drops out DBus as an intermediary. On daemon startup, we now do the process-global initialization (like ostree sysroot) and if that fails, the daemon just sticks around (but without claiming the bus name), ready to return the error message to each client. After this patch: ``` $ systemctl stop rpm-ostreed $ umount /boot $ rpm-ostree status error: Couldn't start daemon: Error setting up sysroot: loading sysroot: Unexpected state: /run/ostree-booted found, but no /boot/loader directory ``` --- Makefile-daemon.am | 7 +- rust/src/daemon.rs | 109 ++++++++++++++++++++- rust/src/lib.rs | 8 ++ src/app/libmain.cxx | 2 + src/app/rpmostree-builtin-start-daemon.cxx | 53 +++++++--- src/daemon/rpm-ostreed.socket | 9 ++ src/daemon/rpmostreed-daemon.hpp | 28 ++++++ 7 files changed, 194 insertions(+), 22 deletions(-) create mode 100644 src/daemon/rpm-ostreed.socket create mode 100644 src/daemon/rpmostreed-daemon.hpp diff --git a/Makefile-daemon.am b/Makefile-daemon.am index 6e6cb3a3d5..6bcbf4f81a 100644 --- a/Makefile-daemon.am +++ b/Makefile-daemon.am @@ -63,14 +63,15 @@ systemdunit_service_in_files = \ $(NULL) systemdunit_service_files = $(systemdunit_service_in_files:.service.in=.service) -systemdunit_timer_files = \ +systemdunit_other_files = \ + $(srcdir)/src/daemon/rpm-ostreed.socket \ $(srcdir)/src/daemon/rpm-ostreed-automatic.timer \ $(srcdir)/src/daemon/rpm-ostree-countme.timer \ $(NULL) systemdunit_DATA = \ $(systemdunit_service_files) \ - $(systemdunit_timer_files) \ + $(systemdunit_other_files) \ $(NULL) systemdunitdir = $(prefix)/lib/systemd/system/ @@ -110,7 +111,7 @@ EXTRA_DIST += \ $(sysconf_DATA) \ $(service_in_files) \ $(systemdunit_service_in_files) \ - $(systemdunit_timer_files) \ + $(systemdunit_other_files) \ $(NULL) CLEANFILES += \ diff --git a/rust/src/daemon.rs b/rust/src/daemon.rs index 41e4984151..242b825d88 100644 --- a/rust/src/daemon.rs +++ b/rust/src/daemon.rs @@ -8,18 +8,19 @@ use crate::cxxrsutil::*; use crate::ffi::{ OverrideReplacementSource, OverrideReplacementType, ParsedRevision, ParsedRevisionKind, }; -use anyhow::{anyhow, format_err, Result}; +use anyhow::{anyhow, format_err, Result, Context}; use cap_std::fs::Dir; use cap_std_ext::dirext::CapStdExtDirExt; use cap_std_ext::{cap_std, rustix}; use fn_error_context::context; use glib::prelude::*; use ostree_ext::{gio, glib, ostree}; -use rustix::fd::BorrowedFd; +use rustix::fd::{BorrowedFd, FromRawFd}; use rustix::fs::MetadataExt; use std::collections::{BTreeMap, BTreeSet}; -use std::io::Read; +use std::io::{Read, Write}; use std::os::unix::fs::PermissionsExt; +use std::os::unix::net::{UnixStream, UnixListener}; use std::path::Path; const RPM_OSTREED_COMMIT_VERIFICATION_CACHE: &str = "rpm-ostree/gpgcheck-cache"; @@ -131,6 +132,108 @@ fn deployment_populate_variant_origin( Ok(()) } +/// Connect to the client socket and ensure the daemon is initialized; +/// this avoids DBus and ensures that we get any early startup errors +/// returned cleanly. +pub(crate) fn start_daemon_via_socket() -> CxxResult<()> { + let address = "/run/rpm-ostree/client.sock"; + let s = UnixStream::connect(address) + .with_context(|| anyhow!("Failed to connect to {}", address))?; + let mut s = std::io::BufReader::new(s); + let mut r = String::new(); + s.read_to_string(&mut r) + .context("Reading from client socket")?; + if r.is_empty() { + Ok(()) + } else { + Err(anyhow!("{r}").into()) + } +} + +fn send_init_result_to_client(client: &UnixStream, err: &Result<()>) { + let mut client = std::io::BufWriter::new(client); + match err { + Ok(_) => { + // On successwe close the stream without writing anything, + // which acknowledges successful startup to the client. + } + Err(e) => { + let msg = e.to_string(); + match client + .write_all(msg.as_bytes()) + .and_then(|_| client.flush()) + { + Ok(_) => {} + Err(inner_err) => { + eprintln!( + "Failed to write error message to client socket (original error: {}): {}", + e, inner_err + ); + } + } + } + } +} + +fn process_clients(listener: UnixListener, res: &Result<()>) { + for stream in listener.incoming() { + match stream { + Ok(stream) => send_init_result_to_client(&stream, res), + Err(e) => { + // This shouldn't be fatal, we continue to start up. + eprintln!("Failed to listen for client stream: {}", e); + } + } + if res.is_err() { + break; + } + } +} + +/// Perform initialization steps required by systemd service activation. +/// +/// This ensures that the system is running under systemd, then receives the +/// socket-FD for main IPC logic, and notifies systemd about ready-state. +pub(crate) fn daemon_main(debug: bool) -> Result<()> { + if !systemd::daemon::booted()? { + return Err(anyhow!("not running as a systemd service")); + } + + let init_res: Result<()> = crate::ffi::daemon_init_inner(debug).map_err(|e| e.into()); + + let mut fds = systemd::daemon::listen_fds(false)?.iter(); + let listener = match fds.next() { + None => { + // If started directly via `systemctl start` or DBus activation, we + // directly propagate the error back to our exit code. + init_res?; + UnixListener::bind("/run/rpmostreed.socket")? + } + Some(fd) => { + if fds.next().is_some() { + return Err(anyhow!("Expected exactly 1 fd from systemd activation")); + } + let listener = unsafe { UnixListener::from_raw_fd(fd) }; + match init_res { + Ok(_) => listener, + Err(e) => { + let err_copy = Err(anyhow!("{}", e)); + process_clients(listener, &err_copy); + return Err(e); + } + } + } + }; + + // On success, we spawn a helper thread that just responds with + // sucess to clients that connect via the socket. In the future, + // perhaps we'll expose an API here. + std::thread::spawn(move || process_clients(listener, &Ok(()))); + + // And now, enter the main loop. + Ok(crate::ffi::daemon_main_inner()?) +} + /// Serialize information about the given deployment into the `dict`; /// this will be exposed via DBus and is hence public API. pub(crate) fn deployment_populate_variant( diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 3e8b564c56..ca3d046d6f 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -299,6 +299,8 @@ pub mod ffi { // daemon.rs extern "Rust" { + fn daemon_main(debug: bool) -> Result<()>; + fn start_daemon_via_socket() -> Result<()>; fn daemon_sanitycheck_environment(sysroot: &OstreeSysroot) -> Result<()>; fn deployment_generate_id(deployment: &OstreeDeployment) -> String; fn deployment_populate_variant( @@ -762,6 +764,12 @@ pub mod ffi { fn c_unit_tests() -> Result<()>; } + unsafe extern "C++" { + include!("rpmostreed-daemon.hpp"); + fn daemon_init_inner(debug: bool) -> Result<()>; + fn daemon_main_inner() -> Result<()>; + } + unsafe extern "C++" { include!("rpmostree-clientlib.h"); fn client_require_root() -> Result<()>; diff --git a/src/app/libmain.cxx b/src/app/libmain.cxx index 66e6ed2de1..676a11bdfd 100644 --- a/src/app/libmain.cxx +++ b/src/app/libmain.cxx @@ -281,6 +281,8 @@ rpmostree_option_context_parse (GOptionContext *context, const GOptionEntry *mai return rpmostreecxx::client_throw_non_ostree_host_error (error); } + rpmostreecxx::start_daemon_via_socket(); + /* root never needs to auth */ if (getuid () != 0) /* ignore errors; we print out a warning if we fail to spawn pkttyagent */ diff --git a/src/app/rpmostree-builtin-start-daemon.cxx b/src/app/rpmostree-builtin-start-daemon.cxx index dcc5382361..0150437b13 100644 --- a/src/app/rpmostree-builtin-start-daemon.cxx +++ b/src/app/rpmostree-builtin-start-daemon.cxx @@ -34,6 +34,7 @@ #include "rpmostree-builtins.h" #include "rpmostree-libbuiltin.h" #include "rpmostree-util.h" +#include "rpmostreed-utils.h" #include "rpmostreed-daemon.h" typedef enum @@ -53,6 +54,7 @@ static GOptionEntry opt_entries[] = { { "debug", 'd', 0, G_OPTION_ARG_NONE, &opt "Use system root SYSROOT (default: /)", "SYSROOT" }, { NULL } }; +static GDBusConnection *bus = NULL; static RpmostreedDaemon *rpm_ostree_daemon = NULL; static void @@ -211,17 +213,14 @@ on_log_handler (const gchar *log_domain, GLogLevelFlags log_level, const gchar * sd_journal_print (priority, "%s", message); } -gboolean -rpmostree_builtin_start_daemon (int argc, char **argv, RpmOstreeCommandInvocation *invocation, - GCancellable *cancellable, GError **error) +namespace rpmostreecxx { +// This function is always called from the Rust side. Hopefully +// soon we'll move more of this code into daemon.rs. +void +daemon_init_inner (bool debug) { - g_autoptr (GOptionContext) opt_context = g_option_context_new (" - start the daemon process"); - g_option_context_add_main_entries (opt_context, opt_entries, NULL); - - if (!g_option_context_parse (opt_context, &argc, &argv, error)) - return FALSE; - - if (opt_debug) + g_autoptr(GError) local_error = NULL; + if (debug) { g_autoptr (GIOChannel) channel = NULL; g_log_set_handler (G_LOG_DOMAIN, (GLogLevelFlags)(G_LOG_LEVEL_DEBUG | G_LOG_LEVEL_INFO), @@ -243,19 +242,24 @@ rpmostree_builtin_start_daemon (int argc, char **argv, RpmOstreeCommandInvocatio g_unix_signal_add (SIGTERM, on_sigint, NULL); /* Get an explicit ref to the bus so we can use it later */ - g_autoptr (GDBusConnection) bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, error); + g_autoptr (GDBusConnection) bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, &local_error); if (!bus) - return FALSE; - if (!start_daemon (bus, error)) + util::throw_gerror(local_error); + if (!start_daemon (bus, &local_error)) { - if (*error) - sd_notifyf (0, "STATUS=error: %s", (*error)->message); - return FALSE; + sd_notifyf (0, "STATUS=error: %s", local_error->message); + util::throw_gerror(local_error); } +} +// Called from rust side to enter mainloop. +void +daemon_main_inner () +{ state_transition (APPSTATE_RUNNING); g_debug ("Entering main event loop"); + g_assert (rpm_ostree_daemon); rpmostreed_daemon_run_until_idle_exit (rpm_ostree_daemon); if (bus) @@ -285,6 +289,23 @@ rpmostree_builtin_start_daemon (int argc, char **argv, RpmOstreeCommandInvocatio g_autoptr (GMainContext) mainctx = g_main_context_default (); while (appstate == APPSTATE_FLUSHING) g_main_context_iteration (mainctx, TRUE); +} +} /* namespace */ + +gboolean +rpmostree_builtin_start_daemon (int argc, + char **argv, + RpmOstreeCommandInvocation *invocation, + GCancellable *cancellable, + GError **error) +{ + g_autoptr(GOptionContext) opt_context = g_option_context_new (" - start the daemon process"); + g_option_context_add_main_entries (opt_context, opt_entries, NULL); + + if (!g_option_context_parse (opt_context, &argc, &argv, error)) + return FALSE; + + rpmostreecxx::daemon_main (opt_debug); return TRUE; } diff --git a/src/daemon/rpm-ostreed.socket b/src/daemon/rpm-ostreed.socket new file mode 100644 index 0000000000..020c640360 --- /dev/null +++ b/src/daemon/rpm-ostreed.socket @@ -0,0 +1,9 @@ +[Unit] +ConditionKernelCommandLine=ostree + +[Socket] +ListenStream=/run/rpm-ostree/client.sock +SocketMode=0600 + +[Install] +WantedBy=sockets.target diff --git a/src/daemon/rpmostreed-daemon.hpp b/src/daemon/rpmostreed-daemon.hpp new file mode 100644 index 0000000000..4c788f0960 --- /dev/null +++ b/src/daemon/rpmostreed-daemon.hpp @@ -0,0 +1,28 @@ +/* + * Copyright (C) 2022 Red Hat, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#pragma once + +#include "rust/cxx.h" + +namespace rpmostreecxx +{ +/* Note: Currently actually defined in rpmostree-builtin-start-daemon.cxx for historical reasons */ +void daemon_init_inner (bool debug); +void daemon_main_inner (); +} \ No newline at end of file