-
Notifications
You must be signed in to change notification settings - Fork 109
Delete cluster name state file whenever slurm accounting is configured or updated #2994
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2994 +/- ##
========================================
Coverage 75.50% 75.50%
========================================
Files 23 23
Lines 2356 2356
========================================
Hits 1779 1779
Misses 577 577
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Do we have an integ test capturing the same scenario when we execute the cluster update? |
cookbooks/aws-parallelcluster-slurm/recipes/config/config_slurm_accounting.rb
Outdated
Show resolved
Hide resolved
cookbooks/aws-parallelcluster-slurm/recipes/update/clear_slurm_accounting.rb
Outdated
Show resolved
Hide resolved
| code <<-CLUSTERSTATE | ||
| rm /var/spool/slurm.state/clustername | ||
| CLUSTERSTATE | ||
| only_if { ::File.exist?('/var/spool/slurm.state/clustername') } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to use bash, why using an only_if rather than having a rm -f?
Also, why using bash and not using the file resource with action delete?
| code <<-CLUSTERSTATE | ||
| rm /var/spool/slurm.state/clustername | ||
| CLUSTERSTATE | ||
| only_if { ::File.exist?('/var/spool/slurm.state/clustername') } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
With this change we are fixing a bug. Can we surface it in the changelog? |
| action %i(disable stop) | ||
| end | ||
|
|
||
| bash "Remove existing cluster name state file" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is duplicated. What about reducing code duplication by defining this logic into a function and call that function or use the file resources to delete the file?
CHANGELOG.md
Outdated
| **CHANGES** | ||
| - Ubuntu 20.04 is no longer supported. | ||
| - Upgrade Slurm to version 24.11.5. | ||
| - Addressed cluster id mismatch known issue by deleting the file `/var/spool/slurm.state/clustername` before configuring slurm accounting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: extra indent, and Slurm must be capitalized
Description of changes
/var/spool/slurm.state/clusternamedoes not match the cluster id that slurm dbd hasclear_slurm_accountingandconfig_slurm_accountingconfig_slurm_accountingand have the cluster id mismatchclear_slurm_accountingand have the cluster id mismatchTests
test_slurm_accountingandtest_slurm integtests./var/spool/slurm.state/clustername, once I restart slurmctld, the slurm state remains the same.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.