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
```
  • Loading branch information
cgwalters committed Aug 4, 2022
1 parent 1d5f32f commit 2d70a00
Show file tree
Hide file tree
Showing 15 changed files with 418 additions and 28 deletions.
4 changes: 3 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ camino = "1.0.9"
cap-std-ext = "0.26"
cap-std = { version = "0.25", features = ["fs_utf8"] }
# Explicitly force on libc
rustix = { version = "0.35", features = ["use-libc"] }
rustix = { version = "0.35", features = ["use-libc", "net"] }
cap-primitives = "0.25.2"
cap-tempfile = "0.25.2"
chrono = { version = "0.4.19", features = ["serde"] }
Expand Down Expand Up @@ -123,6 +123,8 @@ lto = "thin"
# Note: If you add a feature here, you also probably want to update utils.rs:get_features()
fedora-integration = []
rhsm = ["libdnf-sys/rhsm"]
# Enable hard requirement on `rpm-ostreed.socket`; requires https://bugzilla.redhat.com/show_bug.cgi?id=2110012
client-socket = []
bin-unit-tests = []
# ASAN+UBSAN
sanitizers = []
Expand Down
10 changes: 7 additions & 3 deletions Makefile-daemon.am
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,18 @@ 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-automatic.timer \
$(srcdir)/src/daemon/rpm-ostree-countme.timer \
$(NULL)

if CLIENT_SOCKET
systemdunit_other_files += $(srcdir)/src/daemon/rpm-ostreed.socket
endif

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

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

CLEANFILES += \
Expand Down
8 changes: 8 additions & 0 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,14 @@ AC_ARG_ENABLE(featuresrs,
AS_HELP_STRING([--enable-featuresrs],
[Rust features, see Cargo.toml for more information]),,
[enable_featuresrs=])

AC_ARG_ENABLE(client-socket,
AS_HELP_STRING([--enable-client-socket],
[(default: no)]),,
[enable_client_socket=no])
AS_IF([test x$enable_client_socket = xyes], [enable_featuresrs="$enable_featuresrs client-socket"])
AM_CONDITIONAL(CLIENT_SOCKET, [echo $enable_featuresrs | grep -q 'client-socket'])

AC_SUBST([RUST_FEATURES], $enable_featuresrs)

# Initialize libtool
Expand Down
9 changes: 8 additions & 1 deletion packaging/rpm-ostree.spec.in
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ BuildRequires: rust
%bcond_with rhsm
%endif

# https://bugzilla.redhat.com/show_bug.cgi?id=2110012
%if 0%{?fedora} >= 37
%bcond_without client_socket
%else
%bcond_with client_socket
%endif

# RHEL (8,9) doesn't ship zchunk today. Keep this in sync
# with libdnf: https://gitlab.com/redhat/centos-stream/rpms/libdnf/-/blob/762f631e36d1e42c63a794882269d26c156b68c1/libdnf.spec#L45
%if 0%{?rhel}
Expand Down Expand Up @@ -179,7 +186,7 @@ env NOCONFIGURE=1 ./autogen.sh
export RUSTFLAGS="%{build_rustflags}"
%endif
%configure --disable-silent-rules --enable-gtk-doc %{?rpmdb_default} %{?with_sanitizers:--enable-sanitizers} %{?with_bin_unit_tests:--enable-bin-unit-tests} \
%{?with_rhsm:--enable-featuresrs=rhsm}
%{?with_rhsm:--enable-featuresrs=rhsm} %{?with_client_socket:--enable-client-socket}

%make_build

Expand Down
55 changes: 55 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 @@ -2164,6 +2165,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 @@ -2930,6 +2935,40 @@ 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;
},
[&] (const char *catch$) noexcept {
throw$.len = ::std::strlen (catch$);
throw$.ptr = const_cast<char *> (::cxxbridge1$exception (catch$, throw$.len));
});
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;
},
[&] (const char *catch$) noexcept {
throw$.len = ::std::strlen (catch$);
throw$.ptr = const_cast<char *> (::cxxbridge1$exception (catch$, throw$.len));
});
return throw$;
}

::rust::repr::PtrLen
rpmostreecxx$cxxbridge1$client_require_root () noexcept
{
Expand Down Expand Up @@ -3946,6 +3985,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 @@ -1821,6 +1822,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
63 changes: 59 additions & 4 deletions rust/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,12 @@ 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 cap_std_ext::rustix;
use gio::prelude::*;
use ostree_ext::{gio, glib};
use std::io::{BufRead, Write};
use std::os::unix::io::IntoRawFd;
use std::process::Command;

/// The well-known bus name.
const BUS_NAME: &str = "org.projectatomic.rpmostree1";
Expand Down Expand Up @@ -49,7 +48,8 @@ impl ClientConnection {
SYSROOT_PATH,
"org.projectatomic.rpmostree1.Sysroot",
gio::NONE_CANCELLABLE,
)?;
)
.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 @@ -155,6 +155,45 @@ pub(crate) fn client_handle_fd_argument(
}
}

/// 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.
#[cfg(feature = "client-socket")]
fn start_daemon_via_socket() -> Result<()> {
use cap_std::io_lifetimes::IntoSocketlike;

let address = sockaddr()?;
let socket = rustix::net::socket(
rustix::net::AddressFamily::UNIX,
rustix::net::SocketType::SEQPACKET,
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()?,
},
)?;
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()),
}
}

/// 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)
}

/// Explicitly ensure the daemon is started via systemd, if possible.
///
/// This works around bugs from DBus activation, see
Expand All @@ -165,12 +204,16 @@ pub(crate) fn client_handle_fd_argument(
/// 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<()> {
#[cfg(not(feature = "client-socket"))]
fn start_daemon_via_systemctl() -> Result<()> {
use std::process::Command;

let service = "rpm-ostreed.service";
// Assume non-root can't use systemd right now.
if rustix::process::getuid().as_raw() != 0 {
return Ok(());
}

// Unfortunately, RHEL8 systemd will count "systemctl start"
// invocations against the restart limit, so query the status
// first.
Expand All @@ -197,6 +240,18 @@ pub(crate) fn client_start_daemon() -> CxxResult<()> {
Ok(())
}

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(());
}
#[cfg(feature = "client-socket")]
return start_daemon_via_socket().map_err(Into::into);
#[cfg(not(feature = "client-socket"))]
return start_daemon_via_systemctl().map_err(Into::into);
}

/// Convert the GVariant parameters from the DownloadProgress DBus API to a human-readable English string.
pub(crate) fn client_render_download_progress(progress: &crate::ffi::GVariant) -> String {
let progress = progress
Expand Down
8 changes: 8 additions & 0 deletions rust/src/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,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 2d70a00

Please sign in to comment.