Skip to content

fix(spurd): stop warning when no spur.conf exists at the default path - #631

Open
nikhilsk wants to merge 1 commit into
ROCm:mainfrom
nikhilsk:fix/spurd-conf-warning
Open

fix(spurd): stop warning when no spur.conf exists at the default path#631
nikhilsk wants to merge 1 commit into
ROCm:mainfrom
nikhilsk:fix/spurd-conf-warning

Conversation

@nikhilsk

Copy link
Copy Markdown

Summary

spurd reads spur.conf best-effort — it supplies local agent settings ([hooks],
[devices], rlimits.memlock, [cluster], [mpi]) and the agent runs on defaults
without it. An agent configured entirely by flags therefore has no such file, yet every
startup logged:

WARN spurd: failed to load spur.conf, using default config path=/etc/spur/spur.conf
  error=failed to read config file: No such file or directory (os error 2)

A warning that fires when nothing is wrong is how operators learn to skip warnings, which
costs them the ones that matter.

Fixes #586.

Approach

Failures are now classified rather than reported uniformly:

--config failure level
default path file absent info — expected shape, the fix
default path malformed, invalid, or unreadable warn
named explicitly any failure warn

spurctld already draws this line (info!("no config file found, using defaults")), so
the two daemons now agree on what an absent config means.

Design notes

A malformed file at the default path still warns. The issue asks for a narrower rule —
"only an explicitly-passed --config path that fails to load warrants a warning" — but
read literally that also silences a broken file at the default path, which is worse than
the noise being removed: the file exists, its settings are being ignored, and nothing says
so. A missing file is a deployment shape; a malformed, invalid, or unreadable one is a
misconfiguration. Flagging this as a deliberate deviation from the issue text.

Classify the error, don't stat the path. Path::exists() would be the shorter test but
cannot separate "absent" from "present and unreadable", so a permission-denied config would
take the quiet path. Matching io::ErrorKind::NotFound on the error load_from_file
already returns distinguishes the two exactly and costs no extra syscall.

Explicitness is read as "not the default value", not "came from the command line". The
field has no env, so ValueSource::CommandLine would work today, but it would silently
misclassify an env-supplied path as a default if one were ever added — the surrounding code
already treats EnvVariable as user intent (spur-cli/src/sbatch.rs). Testing against
DefaultValue stays correct through that change.

ValueSource over config: Option<PathBuf>. Dropping default_value to detect an
explicit path is less code, but --help would lose its [default: /etc/spur/spur.conf]
line. Reading the value's source leaves the Args declaration and the help output
untouched; a test in the smoke table below pins that.

The predicate is a free function so the rule is unit-tested directly rather than through
main().

Compatibility

No change to persisted state, the Raft log, proto, the config schema, or any CLI flag,
default, or help text. The load remains best-effort: spurd still starts on defaults in
every failure case.

The one visible change is the log line itself — anything grepping spurd output for
failed to load spur.conf on a host with no config file will stop matching, which is the
point of the fix.

Testing

cargo test --locked2853 passed, 0 failed, 25 ignored (ignored tests need
PostgreSQL). Five new unit tests cover the matrix above, including a validation failure and
a permission-denied read, and build their ConfigError values through
SlurmConfig::load_from_str rather than hand-rolling stand-ins.

Gates green: cargo fmt --all --check; cargo clippy --workspace --exclude spur-ffi --all-targets --locked with RUSTFLAGS="-D warnings"; cargo test --locked;
cargo deny check (advisories ok, bans ok, licenses ok, sources ok).

Verified against a built binary on a host with no /etc/spur/spur.conf:

Invocation Result
spurd -D (no config present) INFO no spur.conf found, using default config — no warning
spurd -D --config <absent> WARN failed to load spur.conf … No such file or directory
spurd -D -f <absent> (short form) WARN … — short and long forms agree
spurd -D --config <invalid TOML> WARN … failed to parse TOML
spurd -D --config <valid> INFO loaded spur.conf
spurd --help -f, --config <CONFIG> … [default: /etc/spur/spur.conf] intact

Note for reviewers

main() is not unit-testable, so the wiring — that each branch reaches the intended macro
— rests on the smoke runs above rather than on a test. Keeping the rule in a pure predicate
confines that gap to a single match.

spurd loads spur.conf best-effort for local agent settings, so an agent
configured entirely by flags legitimately has no such file. Reporting that
absence at WARN on every startup tells operators something is wrong when
nothing is, which is how people learn to ignore warnings.

Report an absent default path at INFO instead, and keep WARN for every case
where settings the operator intended are being ignored: a --config path they
named themselves that cannot be loaded, or a file that is present but
malformed, invalid, or unreadable. A missing file is an expected deployment
shape; a broken one is a misconfiguration worth surfacing.

The decision is a pure predicate rather than logic inside main(), so it is
covered by unit tests. Whether --config was named explicitly comes from the
argument's ValueSource, which leaves the Args declaration and the documented
default in --help untouched.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 79.31034% with 6 lines in your changes missing coverage. Please review.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #631      +/-   ##
==========================================
+ Coverage   77.68%   77.72%   +0.04%     
==========================================
  Files         172      172              
  Lines       72396    72612     +216     
==========================================
+ Hits        56238    56437     +199     
- Misses      16158    16175      +17     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nikhilsk
nikhilsk marked this pull request as ready for review August 14, 2026 09:19
Copilot AI lite review requested due to automatic review settings August 14, 2026 09:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adjusts spurd’s best-effort config loading so that an absent /etc/spur/spur.conf at the default path is treated as an expected deployment shape (logged at info), while preserving warn visibility for malformed/unreadable configs or explicitly provided --config paths. This improves operational signal-to-noise by eliminating a routine startup warning when nothing is wrong.

Changes:

  • Determine whether --config was explicitly set using Clap ValueSource (non-default sources are treated as operator intent).
  • Classify config-load failures so “default path + not found” logs at info, while all other failures remain warn.
  • Add unit tests covering the config-path/source + failure-kind matrix.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/spurd/src/main.rs
///
/// Anything else — an explicitly requested path, or a file that is present but malformed,
/// invalid, or unreadable — means settings the operator intended are being ignored, and
/// has to stay visible.
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.

spurd logs a warning on every boot when the default spur.conf is absent

3 participants