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

scheduler: assure Fairness and DeadlockFree #1991

Closed
wants to merge 1 commit into from

Conversation

ZiMengSheng
Copy link
Contributor

@ZiMengSheng ZiMengSheng commented Apr 8, 2024

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

V. Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in make test

@koordinator-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign buptcozy after the PR has been reviewed.
You can assign the PR to them by writing /assign @buptcozy in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ZiMengSheng ZiMengSheng force-pushed the attempt-v1 branch 2 times, most recently from 275f95f to d15a90b Compare April 8, 2024 12:26
Copy link

codecov bot commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 58.95522% with 55 lines in your changes are missing coverage. Please review.

Project coverage is 68.66%. Comparing base (ae23cc9) to head (ac22aa2).
Report is 3 commits behind head on main.

Files Patch % Lines
pkg/scheduler/plugins/coscheduling/core/core.go 27.65% 34 Missing ⚠️
.../scheduler/plugins/coscheduling/core/gang_cache.go 68.42% 12 Missing ⚠️
pkg/scheduler/plugins/coscheduling/core/gang.go 76.47% 4 Missing ⚠️
pkg/scheduler/plugins/coscheduling/coscheduling.go 87.09% 4 Missing ⚠️
...cheduler/plugins/coscheduling/core/gang_summary.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1991      +/-   ##
==========================================
- Coverage   68.67%   68.66%   -0.02%     
==========================================
  Files         421      421              
  Lines       38779    38871      +92     
==========================================
+ Hits        26632    26689      +57     
- Misses       9835     9868      +33     
- Partials     2312     2314       +2     
Flag Coverage Δ
unittests 68.66% <58.95%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ZiMengSheng ZiMengSheng force-pushed the attempt-v1 branch 2 times, most recently from a60bfd2 to 0e1ec3e Compare April 9, 2024 10:32
2. A1-A5 assumed in gangA scheduleCycle 2, C1-C5 assumed in gangA scheduleCycle 2, C7 failed due to insufficient resource and gangA\B\C cycle set to invalid,
3. then B10 failed due to cycle invalid, B1 comes and find its gang scheduling cycle 2 can be valid, so it's assumed. however, we expect B1 should fail because schedule cycle 2 already failed.
*/
gang.ScheduleCycle += 1
Copy link
Contributor

@buptcozy buptcozy Apr 10, 2024

Choose a reason for hiding this comment

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

这里scheduleCycle的增加是什么情况,是想提前开启下一轮吗?感觉好像没什么必要; 感觉注释也有些问题,gang的scheduleCycle是在A\B\C内是独立的,为什么C7 insufficient resource会导致A6-A10的scheduleCycle校验失败

Copy link
Contributor

Choose a reason for hiding this comment

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

经过讨论,这个地方修改可以去掉

Signed-off-by: wangjianyu.wjy <wangjianyu.wjy@alibaba-inc.com>
@ZiMengSheng
Copy link
Contributor Author

this pr is replaced by #1996 and #1997, so closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants