-
Notifications
You must be signed in to change notification settings - Fork 209
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
Provide parallel_one_host_only via workers config file #5695
Conversation
Great PR! Please pay attention to the following items before merging: Files matching
This is an automatically generated QA checklist based on modified files. |
ab04ef5
to
c8f2488
Compare
c8f2488
to
b396b5b
Compare
add06ea
to
dc4b30a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5695 +/- ##
==========================================
- Coverage 98.42% 98.41% -0.01%
==========================================
Files 394 394
Lines 38516 38447 -69
==========================================
- Hits 37908 37837 -71
- Misses 608 610 +2 ☔ View full report in Codecov by Sentry. |
d4c2491
to
18a6e84
Compare
All the unnecessary changes have been reverted. I couldnt extend the tests further but the existing coverage should be satisfied for now. |
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.
Looks good but I'd move out the two documentation changes just for now.
2d60cc4
to
522dfec
Compare
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 still think this lacks some additions to the scheduler logic. I'll come up with a test case next week to show that.
af76645
to
e3bcfb3
Compare
I pushed a few more commits to add tests for the scheduler logic and extend it. It was not completely trivial because the evaluation of the setting now needs to be done in a much more dynamic way. I haven't done any manual tests yet. |
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 my changes I find it good enough to be merged. I am keeping the "not-ready" label until we/I did some manual testing.
I now tested this manually with a cluster of 2 jobs and all workers with
So I guess that's good enough. |
e3bcfb3
to
2a78ce5
Compare
* Add PARALLEL_ONE_HOST_ONLY to the worker properties like other "capabilities" * Remove absent capabilities from worker properties to make it possible to unconfigure the PARALLEL_ONE_HOST_ONLY setting again Related ticket: https://progress.opensuse.org/issues/158146 Co-authored-by: Marius Kittler <[email protected]> Signed-off-by: ybonatakis <[email protected]>
* Consider the property "PARALLEL_ONE_HOST_ONLY" when assigning a worker slot and it has that worker property set to a truthy value * Fallback to normal scheduling (excluding all worker slots with the "PARALLEL_ONE_HOST_ONLY" property set to a truthy value) so the presence of "PARALLEL_ONE_HOST_ONLY" worker slots will hopefully never make things worse (despite the fact that `WorkerSlotPicker` is not a full SAT solver and therefore limited) * Consider the property "PARALLEL_ONE_HOST_ONLY" when picking siblings of a running job to repair half-scheduled job clusters Related ticket: https://progress.opensuse.org/issues/158146
2a78ce5
to
0e31752
Compare
Really annoying that codecov uploads occaisonally fail:
Since this isn't treated as an error (the CircleCI codecov step passes) I'll have to re-run all CircleCI tests. |
I retried yesterday and also today and Codecov is reliably broken. That's also the case on other PRs like #5723. Strangely, OBS doesn't have the same problem. I checked the upload steps of a few PRs on https://github.com/openSUSE/open-build-service/pull. Maybe we should compare our setup with the one from OBS. For the time being I checked https://output.circle-artifacts.com/output/job/5a4e33c3-6409-401d-b492-b79ba22e4c05/artifacts/0/cover_db/coverage.html manually and it looks good. So I'm merging this PR now. |
https://progress.opensuse.org/issues/158146