-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
slurm.conf.example: Filter out more Slurm node state flags #469
Conversation
Thanks for the patch. Overall looks good! Could be nice if you try to keep a similar structure for the commit message with a title which starts with the part of code being touched. See e500d9b by example :) |
@@ -18,8 +18,8 @@ reverse: sinfo -h -N -o "%R" -n $NODE | |||
[slurmstate,st] | |||
map: sinfo -h -o "%N" -t $GROUP | |||
all: sinfo -h -o "%N" | |||
list: sinfo -h -o "%T" | tr -d '*~#$@+' | |||
reverse: sinfo -h -N -o "%T" -n $NODE | tr -d '*~#$@+' | |||
list: sinfo -h -o "%T" | tr -d '*~#!%$@+^-' |
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.
I hope Slurm supports UTF-8, as it will eventually exhaust us-ascii special characters... ;)
Wondering if you should not have a different strategy, where you filter only keeping the standard characters and remove all the other ones?
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.
I can't wait for emoji-based node states :)
But more seriously, I think the filtering should ideally not be based on what Slurm states strings could contain. but rather on what is considered acceptable characters in a group name, namely not those:
clustershell/lib/ClusterShell/NodeSet.py
Line 74 in a1875cb
ILLEGAL_GROUP_CHARS = set("@,!&^*") |
But I'm not sure we can get that illegal characters list in a group configuration file, can we?
* filter out the following characters from the node states returned by sinfo: *~#!%$@+^- Slurm 21.08 introduced a new node state flag (^), and we missed a few before. See https://slurm.schedmd.com/sinfo.html#SECTION_NODE-STATE-CODES for a complete list.
Good point about the commit message, fixed. |
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.
Based on the discussion, i'm ok with the current patch
Slurm 21.08 introduced a new node state flag (^), and we missed a few before.
See https://slurm.schedmd.com/sinfo.html#SECTION_NODE-STATE-CODES for a complete list