Skip to content

Commit

Permalink
daemon: Add socket activation via /run/rpm-ostreed.socket
Browse files Browse the repository at this point in the history
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.
There's a new `rpm-ostreed.socket` and the daemon can be activated
that way.

The client (when run as root, the socket is only accessible to root
right now) connects, which will activate the daemon and attempt
initialization - without claiming the DBus name yet.

If something goes wrong here, the daemon will reply to the client
that activated it with the error, and then also exit with failure.

On success, everything continues as normal, including claiming
the DBus name.

Note that this also logically replaces the code that does explicit
`systemctl start rpm-ostreed` invocations.

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
```

Co-authored-by: Colin Walters <[email protected]>
  • Loading branch information
RishabhSaini and cgwalters committed Aug 23, 2023
1 parent 1d4e1f5 commit c589d98
Show file tree
Hide file tree
Showing 15 changed files with 387 additions and 60 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ cap-std-ext = "2.0"
cap-std = { version = "1.0.3", features = ["fs_utf8"] }
containers-image-proxy = "0.5.1"
# Explicitly force on libc
rustix = { version = "0.37", features = ["use-libc"] }
rustix = { version = "0.37", features = ["use-libc", "net"] }
cap-primitives = "1.0.3"
chrono = { version = "0.4.26", features = ["serde"] }
clap = { version = "4.3", features = ["derive"] }
Expand Down
7 changes: 4 additions & 3 deletions Makefile-daemon.am
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,15 @@ systemdunit_service_file_names = \
$(NULL)

systemdunit_service_files = $(addprefix $(srcdir)/src/daemon/,$(systemdunit_service_file_names))
systemdunit_timer_files = \
systemdunit_other_files = \
$(srcdir)/src/daemon/rpm-ostreed-automatic.timer \
$(srcdir)/src/daemon/rpm-ostree-countme.timer \
$(srcdir)/src/daemon/rpm-ostreed.socket \
$(NULL)

systemdunit_DATA = \
$(systemdunit_service_files) \
$(systemdunit_timer_files) \
$(systemdunit_other_files) \
$(NULL)

systemdunitdir = $(prefix)/lib/systemd/system/
Expand Down Expand Up @@ -103,7 +104,7 @@ EXTRA_DIST += \
$(sysconf_DATA) \
$(service_in_files) \
$(systemdunit_service_in_files) \
$(systemdunit_timer_files) \
$(systemdunit_other_files) \
$(NULL)

CLEANFILES += \
Expand Down
49 changes: 49 additions & 0 deletions rpmostree-cxxrs.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "rpmostree-package-variants.h"
#include "rpmostree-rpm-util.h"
#include "rpmostree-util.h"
#include "rpmostreed-daemon.hpp"
#include "rpmostreemain.h"
#include "src/libpriv/rpmostree-cxxrs-prelude.h"
#include <algorithm>
Expand Down Expand Up @@ -2195,6 +2196,10 @@ extern "C"
::rust::Str opt_deploy_id, ::rust::Str opt_os_name,
::rpmostreecxx::OstreeDeployment **return$) noexcept;

::rust::repr::PtrLen rpmostreecxx$cxxbridge1$daemon_main (bool debug) noexcept;

void rpmostreecxx$cxxbridge1$daemon_terminate () noexcept;

::rust::repr::PtrLen rpmostreecxx$cxxbridge1$daemon_sanitycheck_environment (
const ::rpmostreecxx::OstreeSysroot &sysroot) noexcept;

Expand Down Expand Up @@ -2970,6 +2975,34 @@ extern "C"
return throw$;
}

::rust::repr::PtrLen
rpmostreecxx$cxxbridge1$daemon_init_inner (bool debug) noexcept
{
void (*daemon_init_inner$) (bool) = ::rpmostreecxx::daemon_init_inner;
::rust::repr::PtrLen throw$;
::rust::behavior::trycatch (
[&] {
daemon_init_inner$ (debug);
throw$.ptr = nullptr;
},
::rust::detail::Fail (throw$));
return throw$;
}

::rust::repr::PtrLen
rpmostreecxx$cxxbridge1$daemon_main_inner () noexcept
{
void (*daemon_main_inner$) () = ::rpmostreecxx::daemon_main_inner;
::rust::repr::PtrLen throw$;
::rust::behavior::trycatch (
[&] {
daemon_main_inner$ ();
throw$.ptr = nullptr;
},
::rust::detail::Fail (throw$));
return throw$;
}

::rust::repr::PtrLen
rpmostreecxx$cxxbridge1$client_require_root () noexcept
{
Expand Down Expand Up @@ -4038,6 +4071,22 @@ deployment_get_base (::rpmostreecxx::OstreeSysroot &sysroot, ::rust::Str opt_dep
return ::std::move (return$.value);
}

void
daemon_main (bool debug)
{
::rust::repr::PtrLen error$ = rpmostreecxx$cxxbridge1$daemon_main (debug);
if (error$.ptr)
{
throw ::rust::impl< ::rust::Error>::error (error$);
}
}

void
daemon_terminate () noexcept
{
rpmostreecxx$cxxbridge1$daemon_terminate ();
}

void
daemon_sanitycheck_environment (const ::rpmostreecxx::OstreeSysroot &sysroot)
{
Expand Down
5 changes: 5 additions & 0 deletions rpmostree-cxxrs.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "rpmostree-package-variants.h"
#include "rpmostree-rpm-util.h"
#include "rpmostree-util.h"
#include "rpmostreed-daemon.hpp"
#include "rpmostreemain.h"
#include "src/libpriv/rpmostree-cxxrs-prelude.h"
#include <algorithm>
Expand Down Expand Up @@ -1843,6 +1844,10 @@ ::rpmostreecxx::OstreeDeployment *deployment_get_base (::rpmostreecxx::OstreeSys
::rust::Str opt_deploy_id,
::rust::Str opt_os_name);

void daemon_main (bool debug);

void daemon_terminate () noexcept;

void daemon_sanitycheck_environment (const ::rpmostreecxx::OstreeSysroot &sysroot);

::rust::String deployment_generate_id (const ::rpmostreecxx::OstreeDeployment &deployment) noexcept;
Expand Down
113 changes: 76 additions & 37 deletions rust/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ use crate::core::OSTREE_BOOTED;
use crate::cxxrsutil::*;
use crate::ffi::SystemHostType;
use crate::utils;
use anyhow::{anyhow, Result};
use anyhow::{anyhow, Context, Result};
use camino::Utf8Path;
use fn_error_context::context;
use gio::prelude::*;
use ostree_ext::{gio, glib};
Expand All @@ -20,6 +21,7 @@ const BUS_NAME: &str = "org.projectatomic.rpmostree1";
const SYSROOT_PATH: &str = "/org/projectatomic/rpmostree1/Sysroot";
const OS_INTERFACE: &str = "org.projectatomic.rpmostree1.OS";
const OS_EX_INTERFACE: &str = "org.projectatomic.rpmostree1.OSExperimental";
const CLIENT_SOCKET_PATH: &str = "/run/rpm-ostree/client.sock";

/// A unique DBus connection to the rpm-ostree daemon.
/// This currently wraps a C++ client connection.
Expand Down Expand Up @@ -49,7 +51,8 @@ impl ClientConnection {
SYSROOT_PATH,
"org.projectatomic.rpmostree1.Sysroot",
gio::Cancellable::NONE,
)?;
)
.context("Initializing sysroot proxy")?;
// Today the daemon mode requires running inside a booted deployment.
let booted = sysroot_proxy
.cached_property("Booted")
Expand Down Expand Up @@ -156,46 +159,82 @@ pub(crate) fn client_handle_fd_argument(
}
}

/// Explicitly ensure the daemon is started via systemd, if possible.
///
/// This works around bugs from DBus activation, see
/// https://github.com/coreos/rpm-ostree/pull/2932
///
/// Basically we load too much data before claiming the bus name,
/// and dbus doesn't give us a useful error. Instead, let's talk
/// to systemd directly and use its client tools to scrape errors.
///
/// What we really should do probably is use native socket activation.
pub(crate) fn client_start_daemon() -> CxxResult<()> {
let service = "rpm-ostreed.service";
// Assume non-root can't use systemd right now.
if rustix::process::getuid().as_raw() != 0 {
return Ok(());
// If the client socket doesn't exist, try to ask systemd to start it.
// This can happen even when the socket unit is installed because
// presets may not enable it.
fn check_and_start_daemon_socket() -> Result<bool> {
if Utf8Path::new(CLIENT_SOCKET_PATH).exists() {
return Ok(true);
}
if !rustix::process::getuid().is_root() {
return Ok(false);
}
// Unfortunately, RHEL8 systemd will count "systemctl start"
// invocations against the restart limit, so query the status
// first.
let activeres = Command::new("systemctl")
.args(&["is-active", "rpm-ostreed"])
let socket_unit = "rpm-ostreed.socket";
tracing::debug!("{CLIENT_SOCKET_PATH} does not exist, explicitly starting socket unit");
let start_result = Command::new("systemctl")
.args(&["--no-ask-password", "start", socket_unit])
.output()?;
// Explicitly don't check the error return value, we don't want to
// hard fail on it.
if String::from_utf8_lossy(&activeres.stdout).starts_with("active") {
// It's active, we're done. Note that while this is a race
// condition, that's fine because it will be handled by DBus
// activation.
if !start_result.status.success() {
let err = String::from_utf8_lossy(&start_result.stderr);
tracing::warn!("Failed to start {socket_unit}: {err}");
}
Ok(true)
}

/// 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.
#[context("Starting daemon via socket")]
fn start_daemon_via_socket() -> Result<()> {
use cap_std::io_lifetimes::IntoSocketlike;

let capable = check_and_start_daemon_socket()?;
if !capable {
tracing::debug!("Falling back to DBus activation");
return Ok(());
}
let res = Command::new("systemctl")
.args(&["--no-ask-password", "start", service])
.status()?;
if !res.success() {
let _ = Command::new("systemctl")
.args(&["--no-pager", "status", service])
.status();
return Err(anyhow!("{}", res).into());

let address = sockaddr()?;
let socket = rustix::net::socket(
rustix::net::AddressFamily::UNIX,
rustix::net::SocketType::STREAM,
rustix::net::Protocol::from_raw(0),
)?;
let addr = crate::client::sockaddr()?;
tracing::debug!("Starting daemon via {address:?}");
rustix::net::connect_unix(&socket, &addr)
.with_context(|| anyhow!("Failed to connect to {address:?}"))?;
let socket = socket.into_socketlike();
crate::daemon::write_message(
&socket,
crate::daemon::SocketMessage::ClientHello {
selfid: crate::core::self_id()?,
},
)
.context("Writing ClientHello")?;
rustix::net::shutdown(&socket, rustix::net::Shutdown::Write).expect("shutdown");
let resp = crate::daemon::recv_message(&socket)?;
match resp {
crate::daemon::SocketMessage::ServerOk => Ok(()),
crate::daemon::SocketMessage::ServerError { msg } => {
Err(anyhow!("server error: {msg}").into())
}
o => Err(anyhow!("unexpected message: {o:?}").into()),
}
Ok(())
}

/// Returns the address of the client socket.
pub(crate) fn sockaddr() -> Result<rustix::net::SocketAddrUnix> {
rustix::net::SocketAddrUnix::new("/run/rpm-ostree/client.sock").map_err(anyhow::Error::msg)
}

pub(crate) fn client_start_daemon() -> CxxResult<()> {
// systemctl and socket paths only work for root right now; in the future
// the socket may be opened up.
if rustix::process::getuid().as_raw() != 0 {
return Ok(());
}
return start_daemon_via_socket().map_err(Into::into);
}

/// Convert the GVariant parameters from the DownloadProgress DBus API to a human-readable English string.
Expand Down
8 changes: 8 additions & 0 deletions rust/src/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,14 @@ fn stage_container_rpm_files(rpms: Vec<File>) -> CxxResult<Vec<String>> {
Ok(r)
}

/// Return an opaque identifier for the current executing binary. This can
/// be passed via IPC to verify that client and server are running the same code.
pub(crate) fn self_id() -> Result<String> {
use std::os::unix::fs::MetadataExt;
let metadata = std::fs::metadata("/proc/self/exe").context("Failed to read /proc/self/exe")?;
Ok(format!("dev={};inode={}", metadata.dev(), metadata.ino()))
}

#[cfg(test)]
mod test {
use super::*;
Expand Down
Loading

0 comments on commit c589d98

Please sign in to comment.