Skip to content
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

Update Slurm bindings #362

Merged
merged 3 commits into from Oct 12, 2017
Merged

Update Slurm bindings #362

merged 3 commits into from Oct 12, 2017

Conversation

kcgthb
Copy link
Contributor

@kcgthb kcgthb commented Oct 11, 2017

  • shortcuts for group names
  • better filtering of states and partition names with '*'
  • added job bindings

  * shortcuts for group names
  * better filtering of states and partition names with '*'
  * added job bindings
@thiell thiell added this to the 1.8 milestone Oct 11, 2017
Copy link
Collaborator

@degremont degremont left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this patch. I'm curious about how it works.

If you refresh the commit message, please add "Closes #359"

list: sinfo -h -o "%T" | tr -d '~#$*'
reverse: sinfo -h -N -o "%T" -n $NODE
cache_time: 300
list: sinfo -h -o "%T" | sed 's/[*$]//g'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain why this sed is better? Do you have examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although the current regex doesn't use it, sed could allow to only remove the last character of the string if it matches the blacklist, while tr will blindly remove all occurrences. Granted, those characters don't really happen in node state names, but what if Slurm someday introduces a "SuperB0o$t" state, huh? 😁

Performance wise, sed is anecdotally faster than tr, but the tr syntax is maybe easier to read.
So, for readability's sake, let's stick with tr. And add @ and + to the list of characters to remove. From the sinfo man page:

NODE STATE CODES
       Node state codes are shortened as required for the field size.  These node states may be followed by a special character to identify state flags associated with the node.  The following node sufficies and states are used:
       *   The node is presently not responding and will not be allocated any new work.  If the node remains non-responsive, it will be placed in the DOWN state (except in the case of COMPLETING, DRAINED, DRAINING, FAIL, FAILING nodes).
       ~   The node is presently in a power saving mode (typically running at reduced frequency).
       #   The node is presently being powered up or configured.
       $   The node is currently in a reservation with a flag value of "maintenance".
       @   The node is pending reboot.
       ALLOCATED   The node has been allocated to one or more jobs.
       ALLOCATED+  The node is allocated to one or more active jobs plus one or more jobs are in the process of COMPLETING.
       COMPLETING [...]

# SLURM job bindings
#
[slurmjob,sj]
map: squeue -j $GROUP -h -o "%N"
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you refresh this patch, keep -h at the beginning of each command. It makes things simpler to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

        * Use `tr` instead of `sed`, fro readability
        * remove additional trailing characters in node state
        * reorder options for consistency.

Closes cea-hpc#359
Copy link
Collaborator

@degremont degremont left a comment

Choose a reason for hiding this comment

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

Thanks!

@thiell thiell merged commit 386286a into cea-hpc:master Oct 12, 2017
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.

None yet

3 participants