Exposing ServerConfig options

[augustuswm]

I wanted to start a discussion on how we might go about exposing some additional server settings to the end user, namely the pagination default values. This PR is the most straightforward take and does not consider backwards compatibility. The main changes are:

1. Wherever ServerConfig was used ConfigDropshot is now used
2. Instead of passing a reference to a ConfigDropshot during construction, the value is passed.
3. There are two new fields on ConfigDropshot. page_max_nitems and page_default_nitems.

This causes two breaking changes:

1. We no longer accept a reference during construction.
2. Callers not using the Default implementation will have to add the new fields.

The are a couple options we could also consider here.

1. ConfigDropshot already derives Clone. We could continue accepting a reference and clone the users config. Not as ideal, but one less breaking change.
2. As we have move fields on the ConfigDropshot struct, we could consider a builder for it. I'm not sure how helpful this is at the moment given the Default impl.

Either way the primary goal here is to come up with a pattern to expose some of these configurations to end users at the server level (specifically ignoring the ability to control these settings per endpoint).

[davepacheco]

Thanks for this! I'm glad to finally resolve that TODO about merging ServerConfig and ConfigDropshot.

I would eliminate the first breaking change by having HttpServerStarter continue to accept &ConfigDropshot and just clone it. In retrospect maybe it would have been better to take an owned value here but I don't think avoiding one clone during server startup is worth the API breakage. (I think this will also significantly reduce the size of the PR.)

Could you also update CHANGELOG.adoc, adding a new "Breaking Changes" section under "Unreleased changes" that points to this issue and explains the extra default values?

[augustuswm]

Thanks for the review! I updated it back to taking a config by reference and just cloned before passing to the inner server. (Now a much simple PR)

Breaking change note has been added. Let me know if the tone of it makes sense.

[ahl]

do we want to add #[non_exhaustive] to the struct? Would that force consumers to use ..Default::default() when constructing this? i.e. such that adding more fields would not be a breaking change?

[davepacheco]

I wondered the same thing. Consumers can already do that if they want. It's kind of nice that the way it is now you can have your code break if we add some new config option so that you get a chance to look yourself at whether our default works for you. I think I've used this in the past.

[davepacheco]

We don't have to fix this here but I think I disagree with past-me here. If you want to do that, just check the changelog. We shouldn't need to bump a major to add new things.

A builder interface would be nice, too.

(I don't propose that we do anything in this PR.)

[davepacheco]

Thanks -- this is looking good.

I just noticed that ServerConfig was sort of exposed via rqctx.server.config. Would you mind doing a test build of Omicron with this dropshot to make sure we're not using something there that's hard to fix up?

[augustuswm]

Sorry for letting this languish. Picking it back up now. I've got omicron mostly building (just a few dependency conflicts to resolve), and then I'll get a test run done.

[augustuswm]

Omicron test suite passed with one test that failed due to exceeding a 60 second limit when running all of them. That test succeeded when run individually, so maybe that is my machine not providing enough resources to it.

[davepacheco]

Omicron test suite passed with one test that failed due to exceeding a 60 second limit when running all of them. That test succeeded when run individually, so maybe that is my machine not providing enough resources to it.

Do you have more information about the test failure? It does seem unlikely to be related but I like to be pretty sure. And if it's a flake, we should file an Omicron ticket.

[davepacheco]

Also @augustuswm do you have a link to the changes you had to make to Omicron to build with this? Thanks!

[davepacheco]

We don't have to fix this here but I think I disagree with past-me here. If you want to do that, just check the changelog. We shouldn't need to bump a major to add new things.

A builder interface would be nice, too.

(I don't propose that we do anything in this PR.)

[augustuswm]

This is a bit behind main now at this point but here is a commit (with patched deps targeting my local versions): oxidecomputer/omicron@a646a8f

Two notable bits:

• Update to syn
• Update to propolis to make the propolis-mock-server crate work

I'll try some test runs here again to try and pick up the actual test that failed, I should have noted it down before.

[davepacheco]

Cool. Those diffs look good. We've gone back and forth on putting the ..Default::default() in there vs. adding the new fields but I think this makes more sense now. We should get that into a PR for Omicron after this lands. That doesn't have to be on you but I do want to make sure it gets done.