io: add fault injection helpers to persistence layer

[dotnwat]

io: add fault injection helpers to persistence layer

Backports Required

• none - not a bug fix
• none - this is a backport
• none - issue does not exist in previous branches
• none - papercut/not impactful enough to backport
• v23.3.x
• v23.2.x
• v23.1.x

Release Notes

• none

[rockwotj]

LGTM, a suggestions to make tests a little more readable.

[rockwotj]

nit: there is a fair amount of noise with the std::make_exception_ptr(fault_injection()). Can we either:

Consolidate these into a helper function or create a helper method to call make_exeception_ptr like seastar does in a couple of places? Ex: https://github.com/scylladb/seastar/blob/4b01caf666187ebae8344b8a0b6b73974b0876d9/include/seastar/core/abort_source.hh#L172-L175

[rockwotj]

What happened to coro4lyfe?

[dotnwat]

haha. given this is the bottom of the stack, every I/O will incur the performance overhead of coroutine (see Travis' talk a couple months ago).

[nvartolomei]

Quickly skimmed the code and wondering if you have considered adding a wrapper over persistence which does the failure injecting instead of modifying the actual persistence implementations? Something like

class fij_persistence : persistence {
  fij_persistence(persistence p);
}

auto p = fij_persistence{disk_persistence{}};

You wouldn't have to duplicate the failure injection logic then in every single persistence implementation (we don't expect many though)

[dotnwat]

@nvartolomei

Quickly skimmed the code and wondering if you have considered adding a wrapper over persistence which does the failure injecting instead of modifying the actual persistence implementations? Something like

Great idea. I did this originally and it worked ok but I am still on the fence if one is better than the other. The downside of this is that since the infrastructure I'm building up still operated on the super class persistence* i had this annoying need to dynamic_cast into the fault-injection wrapper type so I could inject the failures.

Then I thought, well, is this just for testing? If so, then that's fine. But ultimately it seems like it might be beneficial to simply have fault injection be built-in so it's easy to use rather than needing to swap out implementations etc...

Any thoughts on how to do that better?

[rockwotj]

More complex, but you can always decouple injecting from the persistence wrapper. Example usage:

auto [failable_persistence, failure_injector] =
        wrap_in_failure_injector(std::move(persistence));
failure_injector.fail_next_open();
EXPECT_THROWS(co_await failable_persistence.open());

[rockwotj]

LGTM, happy to review a switch to decoupling failure injecting from persistence itself.

[dotnwat]

LGTM, happy to review a switch to decoupling failure injecting from persistence itself.

Thanks. I'm definitely on the same page with your and Nicoale that there are a few ways to approach fault injection. There is totally some aspect of fault injection related to the interfaces for injection and wiring things up into a higher level (a few cases exist in the tree). However at the lowest level I'm not sure there is a benefit to a wrappers/inheritance approach if we have the goal of being able to inject faults into a release build and injecting faults outside of a unit test context--swapping out an implementation at that lowest level seems problematic?

[rockwotj]

swapping out an implementation at that lowest level seems problematic?

I don't think so if the interface is well defined? What problems do you foresee?

benefit to a wrappers/inheritance approach

I am ambivalent about the decorator pattern here especially because we're only repeating the error injecting twice (my DRY alarm is set to 3), more of a benefit in my mind is decoupling fault injection, because now it's impossible at a typesystem level for us to need to pass the persistence layer to a component that doesn't need it (ie. admin api to inject failures or something) and now lower-level bits in the io subsystem can't have access to injecting failures (structurally) without explicitly passing in the injector object.

Anyways, I don't feel strongly about this (I did approve after all), but I kind of like the decoupling for the reasons I mention.