Add an env var to allow disabling debug backtrace recording in the DB pool claim code

[nickelization]

I ran into some issues running simulated Omicron on macOS because the cost of generating a backtrace was high for the debug build I was using; the whole startup process slowed down enough that we hit the 10 minute timeout and failed on the start_nexus_external step.

I didn't manage to get numbers on exactly how slow each individual backtrace was, but sampling the call stacks with sample showed lots of instances of threads stuck in this function. Since the docs say this can be expensive in certain circumstances (and especially since backtrace generation is guarded by a global mutex) it's not entirely surprising that this could clog things up, even though we haven't run into this before on other platforms. My understanding is this has been measured on Illumos and found to be cheap there, so I don't expect this is something that would manifest in production, but a macOS debug build obviously has plenty of differences from an Illumos release build, so it makes sense to me that I could've stumbled across a slow path somewhere in there.

The odd thing is that although I was able to fix it by locally disabling the call to Backtrace::force_capture, I wasn't able to reproduce the problem anymore after I re-enabled it. I'm not sure what could have possibly changed to make this slow path suddenly go fast on the same code, though I did notice that the build size dropped from 1.9GB to 1.2GB, so something weird is definitely going on with my dev setup and I can't for the life of me figure out what happened.

All that said...@davepacheco mentioned it might be reasonable to have a switch for this code anyway, and since I already wrote the code to add a flag I figured I'd whip up a PR for this while it's fresh on my mind.

[davepacheco]

Sorry about the trouble. I should have heeded the warning about this being expensive on some platforms!

I do think it's reasonable to have a switch to turn this off. I don't love sticking a getenv call right in this hot path. Thinking out loud: in an ideal world it could be an argument to omicron-dev, though that wouldn't work for the test suite. What about having the ControlPlaneTestContext setup (which is used for both omicron-dev run-all and all the tests) check for an env var like this and plumb some configuration through accordingly? It would probably be an annoying amount of plumbing, though it would ensure that this couldn't be set outside of those contexts. Alternatively, the Pool could do this at construction time, but it doesn't seem right for some arbitrary code like this to look at the environment instead of passing configuration in.

I'd also suggest checking the specific value for the variable (like 1). (I hate having to unset a variable rather than just setting it to 0 to turn it off. It also makes it harder to programmatically clear.)

[nickelization]

Ah no worries! Seems to be some kind of an edge case on top of the already-edge-case of running on macOS, and I probably would've done the same. Easy change either way.

I guess I hadn't thought of getenv as being expensive enough to worry about, but that's a fair point, and I like the idea of just reading it once on startup and then plumbing it through via ControlPlaneTestContext. If it's a lot of plumbing that probably means it's a good exercise for me as a new hire too :)

I thought about making it check for a specific value, particularly since I tend to dislike "negative"/"inverse" config flags like this where you enable the flag to disable a feature...but then I worry about it feeling arbitrary that you have to know the magic value to set it to (i.e. 0/1 vs true/false vs on/off, etc.). I may be overthinking this though, and I guess this is an obscure enough flag that having to know to specifically set it to 0 or 1 is probably not so bad, and then I can make it a normal flag instead of an inverted one too, so I'll go ahead and do that.

[nickelization]

@davepacheco Well, not sure if this is exactly what you pictured - I didn't see an obvious way to plumb it through ControlPlaneTestContext since that seems to be created after we've already started up Nexus, so I threaded it through via NexusConfig instead. But getting the env var is now in the test_utils ControlPlaneStarter code instead of inline in the DB pool code, so I think this is a bit cleaner than my first stab, and I confirmed locally that it does work for omicron-dev run-all. Let me know what you think.

[davepacheco]

Alright, sorry in advance! I realize this is kind of complex and maybe not the best documented. Thinking out loud:

• The Pool is constructed in ServerContext::new(). This function accepts the NexusConfig argument as well as other arguments (like the logger and debug_dropbox) that are set up by the caller for their environment. This is all plumbed through from InternalServer::start(), which accepts basically the same arguments.
• This whole path is invoked by basically two callers:
  • run_server(), which is the real nexus binary, which provides the config file from a command-line argument
  • NexusServer::start_internal, which is used by the test suite and omicron-dev. It looks like there are lots of callers here. The most common source config is tests/config.test.toml (in the repo). In fact, I think they might all start with that file, but they get there via different paths and some customize the config.
• People who want to customize how Nexus operates can edit the configuration file they give it.
• People who want to customize how omicron-dev runs can either edit the config file or provide arguments on the command line (and I think that makes sense).
• People who want to customize how the test suite runs can edit the test config file or maybe we can use an environment variable to tweak behavior.

From first principles: it seems confusing to me to add this to the configuration file and set it in the in-memory config (after loading it from disk) based on an environment variable because the consumers that want to set this (a human running omicron-dev or the test suite) can already just change the config file instead. i.e., if a user wants to do this, can they not just edit tests/config.test.toml (and not set any environment variable)?

Stepping back further: I think something else you could do is not have this in the config at all, but keep it as one of the few other parameters that's passed around separately from the Nexus config. (Philosophically, this would be saying: this isn't a runtime property of Nexus, it's something about the environment that consumers of Nexus get to decide.) In that case, you could have a CLI argument to omicron-dev that's plumbed down to omicron_dev_setup_with_config() and presumably setup_with_config_impl(). Then you'd have to figure out how the test suite figures it out. The nexus_test macro uses ControlPlaneBuilder, which could easily have a method for customizing this, and that would pass the right value to setup_with_config_impl() (based on the environment variable).

To summarize, I see 3 options here:

1. Only make it part of the Nexus config. Forget the environment variable. Let folks tweak the config if that's what they want.
2. Don't make it part of the config at all. Instead, have callers of setup_with_config_impl() specify it.

• nexus_test checks the environment variable to figure out what to pass here.
• omicron-dev accepts a command-line argument for this that gets passed down to setup_with_config_impl(). (This flag might be settable via the same environment variable that you use in the test suite so that setting the env var is sufficient to get it everywhere.)

3. Do what you're doing now: make it part of the config, but munge the config in common code used by the test suite and omicron-dev. This is my least favorite because it means there are different ways to provide the same parameter and some code paths implicit override it under some conditions -- it just seems unnecessarily confusing.

There are really two separable things here:

1. How it's determined what to do (Nexus config vs. an explicit config from the call stack)
2. Who determines it

For example, there's a variant of what you have here where you still use the environment variable in the same spot you're checking it now, but instead of munging config, it affects the argument that gets passed to NexusServer::start_internal. I still think I'd prefer options (1) or (2) above but this is also an option.

---

Sorry again -- I realize all of this is way more involved than what we were trying to do here. I'm just trying to avoid adding more ad hoc complexity to an already complex set of code paths. Hopefully this doesn't feel like a total waste of time?

[nickelization]

@davepacheco not a waste of time at all, thanks so much for taking the time to write all that! Even if it seems a little silly to go this deep on a tiny test flag that I meant to add in passing, this kind of nuts and bolts stuff is helpful for me in getting to know Omicron better, and I definitely don't want to introduce any more complexity than necessary either.

I hadn't actually learned much about the toml config files yet. If it's common for folks to edit the local config anyway, then simply adding this as a new parameter and relying on local config edits as a way to set this seems like a much simpler and cleaner solution.

For context, I previously wasn't considering the option of editing local config files, and I had actually applied serde(skip) to the new field to avoid any possible knock-on effects of adding a new field, and was solely using the in-memory config struct as a convenient place to thread that value through the code; this was a big part of what felt hacky to me about my own approach. But, given the prior assumption that we wanted to use an env var to set this, and that we wanted to read the env var in the top level startup code and then pass the value down from there, it seemed less bad than having to add a bunch of function arguments in various places just to pass this around. So anyway, not that this is any better, but the current version of this PR doesn't actually offer two ways to change this flag: it's only ever settable via the env var.

But anyway, yes, I think we can make this simpler by adding the field without the serde(skip) attribute, and then simply reading it from the config file. I'll give these changes a go and see how it looks. It still feels a little ugly to have the extra parameter in Pool::new and having to hardcode it to true in places where we don't have access to a set of Nexus config values, but looking at the set of places where that happens I don't think that'll be a big deal in practice, and if it is then we can revisit it easily enough.

Anyway, thanks again for the detailed review! Should have a new version pushed up later today.

[davepacheco]

Ugh, sorry, I had these comments queued up with my other one but used the wrong GitHub button and they didn't get posted. Sorry.

[davepacheco]

We've been a little inconsistent on whether to provide defaults here, but if we're going to keep this in the config file, I would consider updating all the files in this repo (you could probably find them by searching for one of these other properties). Some of these (like nexus/examples/config.toml) include comments about stuff like this explaining why you'd change these and showing how you'd do that.

[nickelization]

Yeah, good call, I can update existing config files to add this.

[davepacheco]

I'm not sure why we wouldn't test this. I think you do want to add it to the toml input above and make sure it has the right value here, right?

[nickelization]

Right, so the issue before was that it deliberately wasn't being set via the config file (via serde(skip, ...)), and I didn't want to hardcode this to check for true or false since then the test would fail if the env var changed it. Since we've now decided to actually make it a normal entry in the config file though, I agree, this should be changed to actually include it in the input toml and test it that way.

[davepacheco]

Could you make this an enum (see this section of RFD 643))?

Same in the other constructor. I think the config file could stay a boolean.

[nickelization]

Definitely. I almost used that technique in the initial implementation, but thought others might see it as overkill (I've seen folks have mixed opinions about this kind of thing in past jobs). I didn't know about RFD 643 though, and I'm happy to do it that way 👍

[nickelization]

@davepacheco sorry for the prolonged back and forth, but I do have one more question: do you know what the story is behind this attribute line that's present on various fields of DeploymentConfig?

#[schemars(skip)] // TODO we're protected against dropshot changes

I understand this keeps the field out of the generated JSON schema, but I'm having trouble pinning down whether/why we would need to do this in this particular case (rather than just updating the schema to include the new field). It seems like other more recently added fields have copied this attribute, so I'm guessing maybe I should too (and so I did that in my latest commit) but I don't really want to just blindly copy that without understanding the bigger picture here.

I did trace this stuff back to originating with #3715, but didn't find enough context there to answer this. I imagine it may have something to do with maintaining compatibility across versions, but we don't use the deny_unknown_fields serde attribute here so it's not like an older version of the software would fail to deserialize data that included the new field, and as long as we include a reasonable default value then it seems like new versions of the software should still handle older serialized versions of the struct fine, even if they're missing the new field....then again, I'm not really familiar with dropshot yet, so maybe that comment would clue me into something if I had more background knowledge?