Skip to content

Commit

Permalink
Add a try_fail_point! macro and use it in more places
Browse files Browse the repository at this point in the history
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 tikv/fail-rs#68
has been slow, we just copy the code into our repo for now.
  • Loading branch information
cgwalters committed Jan 12, 2023
1 parent 384c1ad commit aa8d7fb
Show file tree
Hide file tree
Showing 9 changed files with 69 additions and 22 deletions.
6 changes: 6 additions & 0 deletions rust/src/cxxrsutil.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,12 @@ mod err {
}
}

impl From<Box<dyn std::error::Error + Send + Sync>> for CxxError {
fn from(e: Box<dyn std::error::Error + Send + Sync>) -> Self {
Self(e.to_string())
}
}

impl From<cxx::Exception> for CxxError {
fn from(v: cxx::Exception) -> Self {
Self(format!("{:#}", v))
Expand Down
16 changes: 0 additions & 16 deletions rust/src/failpoint_bridge.rs

This file was deleted.

34 changes: 34 additions & 0 deletions rust/src/failpoints.rs
Original file line number Diff line number Diff line change
@@ -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(())
}
8 changes: 4 additions & 4 deletions rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ pub mod ffi {
fn generate_object_path(base: &str, next_segment: &str) -> Result<String>;
}

// failpoint_bridge.rs
// failpoints.rs
extern "Rust" {
fn failpoint(p: &str) -> Result<()>;
}
Expand Down Expand Up @@ -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")]
Expand Down Expand Up @@ -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;
2 changes: 1 addition & 1 deletion rust/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ fn inner_main() -> Result<i32> {
}
// 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();
Expand Down
3 changes: 3 additions & 0 deletions rust/src/sysroot_upgrade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand All @@ -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(())
Expand Down
14 changes: 13 additions & 1 deletion src/daemon/rpmostreed-transaction.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;

Expand Down
6 changes: 6 additions & 0 deletions src/libpriv/rpmostree-core.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
{
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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 ())
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions src/libpriv/rpmostree-importer.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down

0 comments on commit aa8d7fb

Please sign in to comment.