From aa8d7fb0ceaabfaf10252180e2ddee049d07aae3 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 5 Jan 2023 09:02:50 -0500 Subject: [PATCH] Add a `try_fail_point!` macro and use it in more places When I was debugging https://issues.redhat.com/browse/OCPBUGS-2866 it would have been quite convenient to have a failpoint set up in the transaction init path, because I could have forcibly injected a sleep there. Add one there, and in a few other places I semi-randomly chose by skimming through the code. The cost of this is very low, but if we find other bugs in the future it will likely be useful. Further, we can and should start using failpoints to forcibly inject errors in our CI tests. Since movement on review of https://github.com/tikv/fail-rs/pull/68 has been slow, we just copy the code into our repo for now. --- rust/src/cxxrsutil.rs | 6 +++++ rust/src/failpoint_bridge.rs | 16 ------------- rust/src/failpoints.rs | 34 +++++++++++++++++++++++++++ rust/src/lib.rs | 8 +++---- rust/src/main.rs | 2 +- rust/src/sysroot_upgrade.rs | 3 +++ src/daemon/rpmostreed-transaction.cxx | 14 ++++++++++- src/libpriv/rpmostree-core.cxx | 6 +++++ src/libpriv/rpmostree-importer.cxx | 2 ++ 9 files changed, 69 insertions(+), 22 deletions(-) delete mode 100644 rust/src/failpoint_bridge.rs create mode 100644 rust/src/failpoints.rs diff --git a/rust/src/cxxrsutil.rs b/rust/src/cxxrsutil.rs index bf6c7698cd..b985185812 100644 --- a/rust/src/cxxrsutil.rs +++ b/rust/src/cxxrsutil.rs @@ -150,6 +150,12 @@ mod err { } } + impl From> for CxxError { + fn from(e: Box) -> Self { + Self(e.to_string()) + } + } + impl From for CxxError { fn from(v: cxx::Exception) -> Self { Self(format!("{:#}", v)) diff --git a/rust/src/failpoint_bridge.rs b/rust/src/failpoint_bridge.rs deleted file mode 100644 index 26519410b3..0000000000 --- a/rust/src/failpoint_bridge.rs +++ /dev/null @@ -1,16 +0,0 @@ -//! C++ bindings for the failpoint crate. -// SPDX-License-Identifier: Apache-2.0 OR MIT - -use anyhow::Result; - -/// Expose the `fail::fail_point` macro to C++. -pub fn failpoint(p: &str) -> Result<()> { - ostree_ext::glib::g_debug!("rpm-ostree", "{}", p); - fail::fail_point!(p, |r| { - Err(match r { - Some(ref msg) => anyhow::anyhow!("{}", msg), - None => anyhow::anyhow!("failpoint {}", p), - }) - }); - Ok(()) -} diff --git a/rust/src/failpoints.rs b/rust/src/failpoints.rs new file mode 100644 index 0000000000..0a96c386f2 --- /dev/null +++ b/rust/src/failpoints.rs @@ -0,0 +1,34 @@ +//! Wrappers and utilities on top of the `fail` crate. +// SPDX-License-Identifier: Apache-2.0 OR MIT + +use anyhow::Result; + +/// TODO: Use https://github.com/tikv/fail-rs/pull/68 once it merges +#[macro_export] +macro_rules! try_fail_point { + ($name:expr) => {{ + if let Some(e) = fail::eval($name, |msg| { + let msg = msg.unwrap_or_else(|| "synthetic failpoint".to_string()); + anyhow::Error::msg(msg) + }) { + return Err(From::from(e)); + } + }}; + ($name:expr, $cond:expr) => {{ + if $cond { + $crate::try_fail_point!($name); + } + }}; +} + +/// Expose the `fail::fail_point` macro to C++. +pub fn failpoint(p: &str) -> Result<()> { + ostree_ext::glib::g_debug!("rpm-ostree", "{}", p); + fail::fail_point!(p, |r| { + Err(match r { + Some(ref msg) => anyhow::anyhow!("{}", msg), + None => anyhow::anyhow!("failpoint {}", p), + }) + }); + Ok(()) +} diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 838bd469e4..915ce05282 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -360,7 +360,7 @@ pub mod ffi { fn generate_object_path(base: &str, next_segment: &str) -> Result; } - // failpoint_bridge.rs + // failpoints.rs extern "Rust" { fn failpoint(p: &str) -> Result<()>; } @@ -914,8 +914,8 @@ pub(crate) use daemon::*; mod deployment_utils; pub(crate) use deployment_utils::*; mod dirdiff; -pub mod failpoint_bridge; -use failpoint_bridge::*; +pub mod failpoints; +use failpoints::*; mod extensions; pub(crate) use extensions::*; #[cfg(feature = "fedora-integration")] @@ -956,6 +956,6 @@ mod testutils; pub(crate) use self::testutils::*; mod treefile; pub use self::treefile::*; -mod utils; +pub(crate) mod utils; pub use self::utils::*; mod variant_utils; diff --git a/rust/src/main.rs b/rust/src/main.rs index 85d7ffafe9..4139920ae4 100644 --- a/rust/src/main.rs +++ b/rust/src/main.rs @@ -123,7 +123,7 @@ fn inner_main() -> Result { } // Initialize failpoints let _scenario = fail::FailScenario::setup(); - fail::fail_point!("main"); + rpmostree_rust::try_fail_point!("main"); // Call this early on; it invokes e.g. setenv() so must be done // before we create threads. rpmostree_rust::ffi::early_main(); diff --git a/rust/src/sysroot_upgrade.rs b/rust/src/sysroot_upgrade.rs index b62040e28f..c8489e3fe0 100644 --- a/rust/src/sysroot_upgrade.rs +++ b/rust/src/sysroot_upgrade.rs @@ -123,6 +123,8 @@ pub(crate) fn pull_container( let cancellable = cancellable.glib_reborrow(); let imgref = &OstreeImageReference::try_from(imgref)?; + crate::try_fail_point!("sysroot-upgrade::container-pull"); + let r = Handle::current().block_on(async { crate::utils::run_with_cancellable( async { pull_container_async(repo, imgref).await }, @@ -141,6 +143,7 @@ pub(crate) fn layer_prune( c.set_error_if_cancelled()?; } tracing::debug!("pruning image layers"); + crate::try_fail_point!("sysroot-upgrade::layer-prune"); let n_pruned = ostree_ext::container::store::gc_image_layers(repo)?; systemd::journal::print(6, &format!("Pruned container image layers: {n_pruned}")); Ok(()) diff --git a/src/daemon/rpmostreed-transaction.cxx b/src/daemon/rpmostreed-transaction.cxx index e8e3bca334..6d184f9cab 100644 --- a/src/daemon/rpmostreed-transaction.cxx +++ b/src/daemon/rpmostreed-transaction.cxx @@ -329,7 +329,17 @@ transaction_execute_thread (GTask *task, gpointer source_object, gpointer task_d // Further, we join the main Tokio async runtime. auto guard = rpmostreecxx::rpmostreed_daemon_tokio_enter (rpmostreed_daemon_get ()); - if (clazz->execute != NULL) + try + { + rpmostreecxx::failpoint ("transaction::execute"); + } + catch (std::exception &e) + { + success = FALSE; + glnx_throw (&local_error, "%s", e.what ()); + } + + if (success && clazz->execute != NULL) { // This try/catch shouldn't be needed; every CXX call should be wrapped with the CXX macro. // But in case we regress on this, it's better to error out than crash. @@ -605,6 +615,8 @@ transaction_initable_init (GInitable *initable, GCancellable *cancellable, GErro return FALSE; sd_journal_print (LOG_INFO, "Loaded sysroot"); + CXX_TRY (rpmostreecxx::failpoint ("transaction::lock"), error); + if (!ostree_sysroot_try_lock (priv->sysroot, &lock_acquired, error)) return FALSE; diff --git a/src/libpriv/rpmostree-core.cxx b/src/libpriv/rpmostree-core.cxx index ebb9b07497..232003f27a 100644 --- a/src/libpriv/rpmostree-core.cxx +++ b/src/libpriv/rpmostree-core.cxx @@ -1416,6 +1416,7 @@ check_goal_solution (RpmOstreeContext *self, GPtrArray *removed_pkgnames, GHashTable *replaced_pkgnames, GError **error) { HyGoal goal = dnf_context_get_goal (self->dnfctx); + CXX_TRY (rpmostreecxx::failpoint ("core::check-goal-solution"), error); /* check that we're not removing anything we didn't expect */ { @@ -4057,6 +4058,8 @@ rpmostree_context_assemble (RpmOstreeContext *self, GCancellable *cancellable, G return FALSE; int tmprootfs_dfd = self->tmprootfs_dfd; /* Alias to avoid bigger diff */ + CXX_TRY (rpmostreecxx::failpoint ("core::assemble"), error); + /* In e.g. removing a package we walk librpm which doesn't have canonical * /usr, so we need to build up a mapping. */ @@ -4503,6 +4506,8 @@ rpmostree_context_assemble (RpmOstreeContext *self, GCancellable *cancellable, G gboolean rpmostree_context_assemble_end (RpmOstreeContext *self, GCancellable *cancellable, GError **error) { + CXX_TRY (rpmostreecxx::failpoint ("core::assemble-end"), error); + if (!ensure_tmprootfs_dfd (self, error)) return FALSE; if (self->treefile_rs->get_cliwrap ()) @@ -4534,6 +4539,7 @@ rpmostree_context_commit (RpmOstreeContext *self, const char *parent, g_autofree char *ret_commit_checksum = NULL; auto task = rpmostreecxx::progress_begin_task ("Writing OSTree commit"); + CXX_TRY (rpmostreecxx::failpoint ("core::commit"), error); g_auto (RpmOstreeRepoAutoTransaction) txn = { 0, diff --git a/src/libpriv/rpmostree-importer.cxx b/src/libpriv/rpmostree-importer.cxx index 2b6f445afc..7885ccc4dc 100644 --- a/src/libpriv/rpmostree-importer.cxx +++ b/src/libpriv/rpmostree-importer.cxx @@ -677,6 +677,8 @@ rpmostree_importer_run (RpmOstreeImporter *self, char **out_csum, char **out_met if (g_cancellable_set_error_if_cancelled (cancellable, error)) return FALSE; + CXX_TRY (rpmostreecxx::failpoint ("rpm-importer::run"), error); + g_autofree char *metadata_sha256 = NULL; g_autofree char *csum = NULL; if (!import_rpm_to_repo (self, &csum, &metadata_sha256, cancellable, error))