Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tunable parameters for Nexus config #1086

Merged
merged 2 commits into from
May 18, 2022
Merged

Conversation

bnaecker
Copy link
Collaborator

  • Add the [tunables] section to the nexus config
  • Includes a key max_vpc_ipv4_subnet_prefix. This takes the place of
    the compile-time constant
    omicron_nexus::defaults::MAX_VPC_IPV4_SUBNET_PREFIX. The proximal
    reason for this is to allow a small subnet when testing, to keep the
    subnet allocation integration test short.

- Add the `[tunables]` section to the nexus config
- Includes a key `max_vpc_ipv4_subnet_prefix`. This takes the place of
  the compile-time constant
  `omicron_nexus::defaults::MAX_VPC_IPV4_SUBNET_PREFIX`. The proximal
  reason for this is to allow a small subnet when testing, to keep the
  subnet allocation integration test short.
@bnaecker bnaecker requested a review from davepacheco May 18, 2022 20:32
@bnaecker
Copy link
Collaborator Author

For the record:

On main:

bnaecker@shale : ~/omicron $ time cargo t -- integration_tests::subnet_allocation
    Finished test [unoptimized + debuginfo] target(s) in 0.37s
     Running unittests src/lib.rs (target/debug/deps/authz_macros-174b819b1c918d21)
...

real	1m23.797s
user	1m22.112s
sys	0m10.396s

On this branch:

bnaecker@shale : ~/omicron $ time cargo t -- integration_tests::subnet_allocation
    Finished test [unoptimized + debuginfo] target(s) in 0.37s
     Running unittests src/lib.rs (target/debug/deps/authz_macros-174b819b1c918d21)
...
real	0m26.130s
user	0m17.913s
sys	0m6.578s
bnaecker@shale : ~/omicron $

@david-crespo
Copy link
Contributor

Remember there are a couple of other configs to update.

https://github.com/oxidecomputer/omicron/blob/main/nexus/examples/config.toml
https://github.com/oxidecomputer/omicron/blob/main/nexus/examples/config-file.toml

I wonder if it's even worth keeping these around, though. Is anyone using them? If so maybe we should have a test to make sure they're kept up to date (just musing, that would be a separate PR).

Also: there's some other stuff in the config that arguably could belong under tunables — is that worth moving around? Meh.

request_body_max_bytes = 1048576

@bnaecker
Copy link
Collaborator Author

Yeah, moving the maximum request body might be a good idea. I specifically didn't add the tunables to the other configuration files, though, since I want those to use the default.

I'm not sure anything uses the config-file.toml, but we're definitely still using config.toml.

@david-crespo
Copy link
Contributor

Ah, missed the #[serde(default)] bit. That wasn't being used by anything last time I had occasion to look at config.rs. 👍

Copy link
Collaborator

@davepacheco davepacheco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! That's a nice improvement.

Re: the config files: I definitely use config.toml. I think it's nice to have an example of a file-based logger, but that could also be commented-out in config.toml instead. For the same reason, I'd kind of like to see the tunables in here. To me, the example config file is documentation + example as much as something for people to use directly.

I also think it'd be nice to have a test that at least parses all the config files. (Agreed: not part of this PR.)

- More documentation
- Add tunables to nexus/examples/config.toml
- Improve test
@bnaecker bnaecker enabled auto-merge (squash) May 18, 2022 22:19
@bnaecker bnaecker merged commit 9a5a187 into main May 18, 2022
@bnaecker bnaecker deleted the tunable-configuration-params branch May 18, 2022 23:12
leftwo pushed a commit that referenced this pull request Jan 28, 2024
Crucible changes
Remove a superfluous copy during write serialization (#1087)
Update to progenitor v0.5.0, pull in required Omicron updates (#1115)
Update usdt to v0.5.0 (#1116)
Do not panic on reinitialize of a downstairs client. (#1114)
Bump (tracing-)opentelemetry(-jaeger) (#1113)
Make the Guest -> Upstairs queue fully async (#1086)
Switch to per-block ownership (#1107)
Handle timeout in the client IO task (#1109)
Enforce buffer alignment (#1106)
Block size buffers (#1105)
New dtrace probes and a counter struct in the Upstairs. (#1104)
Implement read decryption offloading (#1089)
Remove Arc + Mutex from Buffer (#1094)
Comment cleanup and rename of DsState::Repair -> Reconcile (#1102)
do not panic the dynamometer for OOB writes (#1101)
Allow dsc to start the downstairs in read-only mode. (#1098)
Use the omicron-zone-package methods for topo sorting (#1099)
Package with topological sorting (#1097)
Fix clippy lints in dsc (#1095)

Propolis changes:
PHD: demote artifact store logs to DEBUG, enable DEBUG on CI (#626)
PHD: fix missing newlines in serial.log (#622)
PHD: fix run_shell_command with multiline commands (#621)
PHD: fix `--artifact-directory` not doing anything (#618)
Update h2 dependency
Update Crucible (and Omicron) dependencies
PHD: refactor guest serial console handling (#615)
phd: add basic "migration-from-base" tests + machinery (#609)
phd: Ensure min disk size fits read-only parents (#611)
phd: automatically fetch `crucible-downstairs` from Buildomat (#604)
Mitigate behavior from illumos#16183
PHD: add guest adapter for WS2022 (#607)
phd: include error cause chain in failure output (#606)
add QEMU pvpanic ISA device (#596)
Add crucible-mem backend
Make crucible opt parsing more terse in standalone
leftwo added a commit that referenced this pull request Jan 29, 2024
Crucible changes
Remove a superfluous copy during write serialization (#1087) Update to
progenitor v0.5.0, pull in required Omicron updates (#1115) Update usdt
to v0.5.0 (#1116)
Do not panic on reinitialize of a downstairs client. (#1114) Bump
(tracing-)opentelemetry(-jaeger) (#1113)
Make the Guest -> Upstairs queue fully async (#1086) Switch to per-block
ownership (#1107)
Handle timeout in the client IO task (#1109)
Enforce buffer alignment (#1106)
Block size buffers (#1105)
New dtrace probes and a counter struct in the Upstairs. (#1104)
Implement read decryption offloading (#1089)
Remove Arc + Mutex from Buffer (#1094)
Comment cleanup and rename of DsState::Repair -> Reconcile (#1102) do
not panic the dynamometer for OOB writes (#1101) Allow dsc to start the
downstairs in read-only mode. (#1098) Use the omicron-zone-package
methods for topo sorting (#1099) Package with topological sorting
(#1097)
Fix clippy lints in dsc (#1095)

Propolis changes:
PHD: demote artifact store logs to DEBUG, enable DEBUG on CI (#626) 
PHD: fix missing newlines in serial.log (#622)
PHD: fix run_shell_command with multiline commands (#621) 
PHD: fix `--artifact-directory` not doing anything (#618) Update h2
dependency
Update Crucible (and Omicron) dependencies
PHD: refactor guest serial console handling (#615) 
phd: add basic "migration-from-base" tests + machinery (#609) 
phd: Ensure min disk size fits read-only parents (#611) 
phd: automatically fetch `crucible-downstairs` from Buildomat (#604)
Mitigate behavior from illumos#16183
PHD: add guest adapter for WS2022 (#607)
phd: include error cause chain in failure output (#606) add QEMU pvpanic
ISA device (#596)
Add crucible-mem backend
Make crucible opt parsing more terse in standalone

Co-authored-by: Alan Hanson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants