-
Notifications
You must be signed in to change notification settings - Fork 264
Add Unschedulable PodCondition for pods in pending #535
Add Unschedulable PodCondition for pods in pending #535
Conversation
pkg/scheduler/cache/cache.go
Outdated
@@ -51,7 +51,7 @@ func New(config *rest.Config, schedulerName string, nsAsQueue bool) Cache { | |||
type SchedulerCache struct { | |||
sync.Mutex | |||
|
|||
kubeclient *kubernetes.Clientset | |||
Kubeclient *kubernetes.Clientset |
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 have to use clientset and recorder and I'd like to reuse libs from cache object. let me know if this is fine.
@k82cn I update the code to address review feedbacks. Please let me know if there's additional changes needed.
|
@Jeffwan , would you help to manage those commits into only 2 commits, major logic and vendor? |
2ef97c8
to
981868c
Compare
@k82cn Yeah. Definitely. I rearrange commits to two. One or code change and the other one for vensor dependencies. |
981868c
to
a419905
Compare
Looks like CI has updates but has not reflected on Github. @k82cn Do you have an idea of this problem? |
a419905
to
5a33753
Compare
jobErrMsg := job.FitError() | ||
|
||
// Update podCondition for tasks Allocated and Pending before job discarded | ||
for _, taskInfo := range job.TaskStatusIndex[api.Pending] { |
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.
@k82cn Thanks for careful examination. I already make clean up and please have a look at this revision.
Move Task status update in Backoff
and Allocated task included now.
The reason I use jobErrMsg
and FailedSchedulingEvent
here is to follow same message as default scheduler in case some components relied on it.
Leave ssn.TaskUnschedulable public for now in case somewhere outside use it to update status.
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.
my pleasure :)
5a33753
to
6c85f6e
Compare
Resolve vendor conflicts from #551 |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Jeffwan, k82cn The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add Unschedulable PodCondition for pods in pending
Add Unschedulable PodCondition for pods in pending
Add Unschedulable PodCondition for pods in pending
What this PR does / why we need it:
Please check #526 for details. Pod scheduled by kube-batch doesn't have rich pod conditions which makes other components like cluster autoscaler hard to interact with.
This PR will add PodConditions for pending pods in order for other kubernetes components to be aware.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #526
Special notes for your reviewer:
I tried following two alternatives, but both of them fails to create new nodes. At the time, CA detect pending pods but ignore scale up action after simulation. This is not accurate because it only consider resource putting pod one by one but not all the pending pods together. (Since CA project is designed for default scheduler, I am not sure if they will accept a change here)
Update status of specific pod failed in assignment here. https://github.com/kubernetes-sigs/kube-batch/blob/2bd479115971cf0a68f7a1384fc1875fc9f60073/pkg/scheduler/actions/allocate/allocate.go#L162-L164
Update entire pods status (same as this PR). https://github.com/kubernetes-sigs/kube-batch/blob/2bd479115971cf0a68f7a1384fc1875fc9f60073/pkg/scheduler/plugins/gang/gang.go#L158-L161
Logs from CA
Logs from kube-batch (2 pods allocated and 2 pending)
Release note: