Avoid scheduling jobs if not all parallel jobs are ready #6049
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
okurz
approved these changes
Nov 6, 2024
okurz
reviewed
Nov 6, 2024
Some jobs of a parallel cluster can be blocked (by a chained parent or by a pending Gru task) while some jobs can be scheduled. Before this change the scheduler assigns the jobs that can be scheduled which creates a half- scheduled parallel cluster. This is particularly problematic when `PARALLEL_ONE_HOST_ONLY=1` and `git_auto_update = yes` are used because then repairing half-scheduled clusters is more challenging and the likeliness that a cluster is partially blocked is higher. In theory this is also problematic when jobs within the parallel cluster depend on different asset downloads. With this change the scheduler skips the whole cluster until all jobs can be scheduled to avoid half-scheduled clusters. Judging by the previous code and its comments this was already intended but the code didn't actually work correctly. I tested the rewritten version of the code by creating a half-blocked cluster manually and observed the behavior of the scheduler before and after this commit. Related ticket: https://progress.opensuse.org/issues/169342
Martchus
force-pushed
the
fix-scheduler
branch
from
acd1633
to
6045c27
Compare
okurz
approved these changes
Nov 6, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6049 +/- ##
=======================================
Coverage 98.98% 98.98%
=======================================
Files 395 395
Lines 39436 39441 +5
=======================================
+ Hits 39036 39041 +5
Misses 400 400 ☔ View full report in Codecov by Sentry. |
b10n1k
reviewed
Nov 8, 2024
Comment on lines
97
to
+98
| my $tobescheduled = _to_be_scheduled($j, $scheduled_jobs); | ||
| if (!defined $tobescheduled) { |
There was a problem hiding this comment.
correct me but I think _to_be_scheduled_recurse doesnt return undef. shouldnt that be just !$tobescheduled?
There was a problem hiding this comment.
We call _to_be_scheduled here and that returns undef.
b10n1k
approved these changes
Nov 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some jobs of a parallel cluster can be blocked (by a chained parent or by a pending Gru task) while some jobs can be scheduled. Before this change the scheduler assigns the jobs that can be scheduled which creates a half- scheduled parallel cluster.
This is particularly problematic when
PARALLEL_ONE_HOST_ONLY=1andgit_auto_update = yesare used because then repairing half-scheduled clusters is more challenging and the likeliness that a cluster is partially blocked is higher. In theory this is also problematic when jobs within the parallel cluster depend on different asset downloads.With this change the scheduler skips the whole cluster until all jobs can be scheduled to avoid half-scheduled clusters. Judging by the previous code and its comments this was already intended but the code didn't actually work correctly.
I tested the rewritten version of the code by creating a half-blocked cluster manually and observed the behavior of the scheduler before and after this commit.
Related ticket: https://progress.opensuse.org/issues/169342