[fm] Toggle to disable fm_analysis background task#10360
Merged
Conversation
Alternative to #10358 (the comment-out approach). Adds an opt-in `fm.analysis_enabled` flag (default false, via #[serde(default)]) to FmTasksConfig, threaded through FmAnalysis::new and short-circuited in activate() with a 'disabled' marker on the status — matching the existing convention used by support_bundle_collector, instance_updater, sp_ereport_ingester, and friends. Customer-shipped smf/nexus/{single-sled,multi-sled}/config-partial.toml inherit the default (off); test/example configs explicitly set true so omdb golden output and dev/CI behavior are unchanged. No #[allow(dead_code)] warts and no golden-file churn. Re-enable in production: flip analysis_enabled = true in the config-partial.toml files (or change the default). Closes #10348 once the analysis subsystem is ready.
CI flagged five FmAnalysis::new() calls in fm_analysis.rs's tests
module that hadn't been updated for the new analysis_enabled
parameter. cargo check (without --tests) and cargo nextest -p
nexus-config didn't catch them; cargo clippy --all-targets did.
Threads a const ANALYSIS_ENABLED = true through each call site so the
intent ('these tests exercise the analysis path') is named at the
call site rather than a bare `true`.
hawkw
approved these changes
May 4, 2026
Member
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
Great, I think this is the nicest solution for now, and I agree that the default value makes this safe to do in a patch release.
|
|
||
| /// Default for [`FmTasksConfig::analysis_enabled`]. | ||
| fn default_fm_analysis_enabled() -> bool { | ||
| // TODO(10349): Flip to true |
Member
There was a problem hiding this comment.
turbo nitpick: maybe this should be
Suggested change
| // TODO(10349): Flip to true | |
| // TODO(#10349): Flip to true |
iliana
pushed a commit
that referenced
this pull request
May 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alternative to #10358
Adds an opt-in
fm.analysis_enabledflag which defaults to false.Yet another possible fix for #10348
To re-enable: toggle
default_fm_analysis_enabledto returntrueinstead offalse.