[sled-agent] Improve Error Messages!

[smklein]

@jgallagher brought up a good point last week, that our errors in Omicron (often making use of thiserror, to merely propagate errors) lose a fair bit of context.

This issue tracks auditing error messages for the following criteria:

• If an error is propagated, it it clear where it came from?
• If an error is propagated, is it clear what caused it to fail?

Often, merely attaching an auxiliary "message string" is sufficient to uncover this context.

To be audited (this is serving as a checklist for me as I work through):

• ./src/illumos/dladm.rs:pub enum Error {
• ./src/illumos/zone.rs:pub enum Error {
• ./src/illumos/zpool.rs:pub enum ParseError {
• ./src/illumos/zpool.rs:pub enum Error {
• ./src/illumos/addrobj.rs:pub enum Error {
• ./src/illumos/running_zone.rs:pub enum Error {
• ./src/illumos/mod.rs:pub enum ExecutionError {
• ./src/illumos/zfs.rs:pub enum Error {
• ./src/bootstrap/agent.rs:pub enum BootstrapError {
• ./src/bootstrap/trust_quorum/error.rs:pub enum TrustQuorumError {
• ./src/bootstrap/spdm/error.rs:pub enum SpdmError {
  • Ignoring intentionally; SPDM is planned to be deprecated
• ./src/bootstrap/params.rs:pub enum SubnetError {
• ./src/rack_setup/service.rs:pub enum SetupServiceError {
• ./src/storage_manager.rs:pub enum Error {
• ./src/instance.rs:pub enum Error {
• ./src/updates.rs:pub enum Error {
• ./src/services.rs:pub enum Error {
• ./src/instance_manager.rs:pub enum Error {
• ./src/config.rs:pub enum ConfigError {
• ./src/sled_agent.rs:pub enum Error {

[smklein]

So I realized something kinda interesting while sorting through these errors - "who is responsible for reporting context" has been fairly unclear.

To be more specific: If module A calls an error-returning function in module B with arguments args, and something goes wrong, is it the responsibility of B to report the context (including any info from args) in an error, or it is up to A to attach this context as things go up the error chain?

This seems like a classic "caller vs callee" saving convention. For my in-progress PR addressing errors, I am opting to put this responsibility on the callee. In doing so, errors can be more liberally propagated upwards, as their error context should be fairly "self-contained".

This choice was partially motivated out of frustration with errors like std::io::Error, where calls such as:

let f = File::open("foo.txt")?;

Can propagate information without including arguments, which makes debugging a pain. In this case, an error could be seen as "OS error: not found" without actually indicating what file was not found.

[davepacheco]

To be more specific: If module A calls an error-returning function in module B with arguments args, and something goes wrong, is it the responsibility of B to report the context (including any info from args) in an error, or it is up to A to attach this context as things go up the error chain?

This seems like a classic "caller vs callee" saving convention. For my in-progress PR addressing errors, I am opting to put this responsibility on the callee. In doing so, errors can be more liberally propagated upwards, as their error context should be fairly "self-contained".

For what it's worth, I like this pattern and I've been trying to do that as well. There's a DRY argument for it too: why have every caller construct that context (in different ways, most likely) when the callee has the information to do it?

I'm also a big fan of each layer of the call stack adding its own context. I too wish File::open included the filename in the message. Then if you're in a wrapper function that's loading a config file specifically, it could add context "loading config", knowing the full error will say loading config: open "foo.txt": no such file or directory. If you're doing this while initializing a server, you could add "initializing server", knowing the lower layers will say what they're doing and the higher layers will wrap with more information.

anyhow's context() is really useful for this, but obviously only works for the generic anyhow errors. I've wondered about having something similar to anyhow::Context for our public Error type that prepends the string to the internal message of whatever error you've got. So you could do something like:

datastore
    .vpc_delete(opctx, &authz_vpc)
    .internal_context("deleting VPC because we're deleting the Project")?

With the idea being that whatever kind of error vpc_delete() returns, the internal message is prepended with that string, similar to what anyhow::Context does. What do you think?

[smklein]

With the idea being that whatever kind of error vpc_delete() returns, the internal message is prepended with that string, similar to what anyhow::Context does. What do you think?

I think the big question to me would be: "What's the new type of the error?" - this seems like the most prominent difficulty to me. anyhow::Context is a sealed trait, which only supports conversion to the anyhow::Error concrete type. So, in a limited capacity: If we have commonly used errors (like in Nexus) this would be easy to add, but this seems more challenging to use in cases where we have fine-grained errors.

TL;DR: I really like that "extra context" API, but I also like using fine-grained error types, and providing a trait to add context that is workable across many different error types seems both handy + somewhat tricky to do.

[david-crespo]

This feels related to #347 — especially the idea of each caller adding its own context as you move up the stack. On the Nexus side, the fact that datastore is returning external API error structs feels wrong. It looks from #967 that sled agent is doing it better.

[davepacheco]

@smklein wrote:

With the idea being that whatever kind of error vpc_delete() returns, the internal message is prepended with that string, similar to what anyhow::Context does. What do you think?

I think the big question to me would be: "What's the new type of the error?" - this seems like the most prominent difficulty to me. anyhow::Context is a sealed trait, which only supports conversion to the anyhow::Error concrete type. So, in a limited capacity: If we have commonly used errors (like in Nexus) this would be easy to add, but this seems more challenging to use in cases where we have fine-grained errors.

Yeah, I think that's true. I was envisioning it would return the exact same type (and variant) of error. That's critical -- in order for people to be able to add this kind of context freely, doing so needs to preserve the semantics and type signature of their function. I think you're right that doing this is more challenging with more fine-grained error types and I think that's a cost to be considered in deciding whether to use more fine-grained error types.

@david-crespo wrote:

This feels related to #347 — especially the idea of each caller adding its own context as you move up the stack. On the Nexus side, the fact that datastore is returning external API error structs feels wrong. It looks from #967 that sled agent is doing it better.

(This is starting to get pretty far away from just producing better error messages -- sorry for starting this here.)

Yeah, there's some ugliness with using a big enum for a bunch of different interfaces, some of which are pretty unrelated to each other. I've heard the advice of creating specialized error types for different interfaces, which can lead to more robust code because the type actually expresses what can really happen. But I've always seen this presented in pretty simple examples, with a specific function or two with a couple of failure modes each. When we have that situation, I agree that we should use more fine-grained error types (e.g., authn::Error).

I'm less clear on how to apply this idea to a system like Nexus with lots of functions in different subsystems that can all produce many of the same errors that are essential to handle correctly (at least: authn/authz failures, the "internal server error" case (i.e., "my invariants were violated by something outside my control") and "service unavailable" (i.e., "I can't do this right now because something I depend on isn't working")). I'm a little worried that with fine-grained error types, people will tend to bucket these into a generic SomethingThatDoesntHaveToDoWithMe variant that doesn't handle all of these properly. If you have some ideas for how this could look, that'd be really helpful.

Moreover, the benefit of using fine-grained error types is that the type accurately describes what can happen, which is useful to a degree. But outside of the simple cases, there's almost always an explicit list of handled errors and everything else is propagated, retried, or otherwise handled in a generic way. What concrete problem would a more fine-grained type solve there?

There's also value in being able to talk about errors uniformly, in terms of semantics (e.g., "this was the caller's fault" vs. "this was the server's fault"), metrics (e.g., "how many 503-like errors did we see, regardless of whether they were from API requests"), the context item I mentioned above, etc. This doesn't necessitate using the exact same type but it's a convenient way to do it.