[feat] Improve handling of maintenance reservations in Slurm #3573
Conversation
ekouts
changed the title
MAINTENANCE to the available states for nodes that belong in a Slurm reservation with the MAINT flag.
… feat/maint_nodes
ekouts
changed the title
MAINTENANCE to the available states for nodes that belong in a Slurm reservation with the MAINT flag.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3573 +/- ##
===========================================
- Coverage 91.28% 91.26% -0.03%
===========================================
Files 62 62
Lines 13583 13582 -1
===========================================
- Hits 12399 12395 -4
- Misses 1184 1187 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ekouts
changed the title
… feat/maint_nodes
vkarak
changed the title
vkarak
left a comment
vkarak
left a comment
There was a problem hiding this comment.
I agree with the overall fix; I have just some suggestions on the implementation.
|
We would also need a unit test. |
… feat/maint_nodes
vkarak
left a comment
vkarak
left a comment
There was a problem hiding this comment.
The implementation looks good to me. I have some minor suggestions. As for the unit tests we can still test these changes by creating a fake reservation output that includes some of the nodes here. Ideally, we should change how we get the reservation nodes and retrieve them from a fixture. I could try adding something along these lines.
|
|
||
|
|
||
| def filter_nodes_by_state(nodelist, state): | ||
| def filter_nodes_by_state(nodelist, state, scheduler): |
There was a problem hiding this comment.
What if made this a scheduler's method?
| self._sched_access_in_submit = self.get_option( | ||
| 'sched_access_in_submit' | ||
| ) | ||
| self.node_available_states = { |
There was a problem hiding this comment.
I don't think this needs to be public:
| self.node_available_states = { | |
| self._available_states = { |
| else: | ||
| self.log(f"could not extract the reservation flags for " | ||
| f"reservation '{reservation}'") |
There was a problem hiding this comment.
What if instead of logging only the else here, we logged the full completed.stdout once we get it?
self.log(f'reservation info\n{completed.stdout}')2 participants
Add
MAINTENANCEto the available states for nodes that belong in a reservation with theMAINTflag, when the user explicitly requests for this reservation.Fixes #3572 .