Skip to content

Disable sacct metrics if sacct exists but accounting is disabled#535

Merged
aditigaur4 merged 2 commits into
masterfrom
azreenzaman/exporter-sacct-fix
Jul 15, 2026
Merged

Disable sacct metrics if sacct exists but accounting is disabled#535
aditigaur4 merged 2 commits into
masterfrom
azreenzaman/exporter-sacct-fix

Conversation

@azreenz

@azreenz azreenz commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Check AccountStorageType in scontrol show config during intialize step to check for slurm accounting. If accounting is disabled we disable the collector in the exporter. If scontrol show config fails to run during intialize step ie we don't know accounting is explictly disabled we allow the collector to run.

When sacct exisits but accounting is disabled we get the following warning in the log:

2026-06-30 15:09:13 INFO: Starting Azure SLURM Prometheus Exporter v0.2.0
2026-06-30 15:09:13 WARNING: Slurm accounting storage is disabled
2026-06-30 15:09:13 WARNING: Accounting is disabled, disabling sacct metrics
2026-06-30 15:09:13 INFO: Prometheus exporter serving on http://0.0.0.0:9101/metrics
2026-06-30 15:09:13 DEBUG: Command: /usr/bin/squeue -h -o %i|%j|%D|%N|%P|%T|%V|%S|%u, Exit code: 0, Time Elapsed: 0.016286
2026-06-30 15:09:13 DEBUG: Command: /usr/bin/sinfo -h -o %N|%D|%R|%E|%T, Exit code: 0, Time Elapsed: 0.015528
2026-06-30 15:09:13 DEBUG: Command: /usr/bin/sdiag --json, Exit code: 0, Time Elapsed: 0.014899
2026-06-30 15:09:14 DEBUG: Command: /opt/cycle/jetpack/bin/jetpack config azure.metadata.compute.location None, Exit code: 0, Time Elapsed: 0.341106

Copilot AI review requested due to automatic review settings June 30, 2026 15:12

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

This PR updates the Slurm sacct collector initialization to synchronously “preflight” sacct so the exporter can disable sacct metrics when Slurm accounting storage is disabled, and adjusts unit tests to mock the new subprocess call.

Changes:

  • Add an sacct preflight call during Sacct.initialize() to detect the “Slurm accounting storage is disabled” message and raise SacctNotAvailException.
  • Add/adjust tests to mock subprocess.run and validate the accounting-disabled behavior.
Criterion Max Points Points Awarded Notes
PR Description Accuracy 20 20 Matches the changes (preflight to detect accounting disabled and disable collector).
PR Atomicity 20 20 Single cohesive change (collector behavior + tests).
Logical Implementation 10 10 Straightforward and localized.
Regression Risk 10 0 Disabling the collector on init can change global Prometheus output (see comment about disable_created_metrics() ordering).
Exception Handling 10 10 Preflight failure is handled without breaking init flow.
Code Comments 10 10 No problematic commentary introduced.
Repetitive Code 10 10 No significant duplication introduced.
Spelling 5 5 No spelling issues found in the changed code.
Logging Quality 5 5 Logging is generally clear for preflight failures/disablement.

FINAL SCORE: 90/100

RECOMMENDATION: MERGE
RATIONALE: The change aligns with the PR’s goal and test coverage was updated accordingly. Consider addressing the noted Prometheus global-metric side effect to avoid unexpected output changes when accounting is disabled.
BLOCKERS: None

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
azure-slurm-exporter/exporter/sacct.py Adds sacct preflight during initialization to detect disabled accounting and disable the collector.
azure-slurm-exporter/test/test_sacct.py Mocks the new subprocess preflight call and adds a test for the accounting-disabled case.

Comment thread azure-slurm-exporter/exporter/sacct.py Outdated
Comment thread azure-slurm-exporter/exporter/sacct.py Outdated
bwatrous
bwatrous previously approved these changes Jul 7, 2026

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

Comment thread azure-slurm-exporter/exporter/sacct.py Outdated
Comment thread azure-slurm-exporter/exporter/sacct.py Outdated
Comment thread azure-slurm-exporter/test/test_sacct.py Outdated
Comment thread azure-slurm-exporter/test/test_sacct.py Outdated
Comment thread azure-slurm-exporter/test/test_sacct.py Outdated
Comment thread azure-slurm-exporter/test/test_sacct.py Outdated
Comment thread azure-slurm-exporter/test/test_sacct.py Outdated
@azreenz
azreenz force-pushed the azreenzaman/exporter-sacct-fix branch from f77f636 to 79c781d Compare July 15, 2026 16:54
@azreenz
azreenz requested a review from Copilot July 15, 2026 16:54

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread azure-slurm-exporter/exporter/util.py
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@azreenz
azreenz requested a review from aditigaur4 July 15, 2026 20:04
@aditigaur4 aditigaur4 changed the title Disable sacct metrics if sacct exisits but accounting is disabled Disable sacct metrics if sacct exists but accounting is disabled Jul 15, 2026
@aditigaur4
aditigaur4 merged commit 049346f into master Jul 15, 2026
4 checks passed
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.

4 participants