fix: bcj doesn't make pod on node that has erased taint #1204
Conversation
|
Welcome @weldonlwz! It looks like this is your first PR to openkruise/kruise 🎉 |
|
/lgtm |
|
@veophi it said check not success, does the commit need any update? |
@weldonlwz Retried. It is e2e accidental failure. Maybe we need another reviewer to approve this pr. /cc @zmberg |
|
@caniszczyk very good, can you add e2e test in e2e file for this scenarios? |
| if err != nil { | ||
| klog.Errorf("pod %s failed to checkNodeFitness for node %s, %v", pod.Name, node.Name, err) | ||
| continue | ||
| } | ||
| if !canFit { |
There was a problem hiding this comment.
if !canFit && pod.DeletionTimestamp == nil {
.......
}
There was a problem hiding this comment.
if !canFit && pod.DeletionTimestamp == nil {
klog.Infof("Pod %s does not fit on node %s due to %v", pod.Name, node.Name, err)
podsToDelete = append(podsToDelete, pod)
continue
}
Am i correct?
Signed-off-by: weldonli <[email protected]>
weldonlwz
force-pushed
the
feature/bugfix
branch
from
0122e69 to
7b82a58
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1204 +/- ##
=======================================
Coverage 50.02% 50.02%
=======================================
Files 143 143
Lines 19912 19897 -15
=======================================
- Hits 9960 9954 -6
+ Misses 8856 8847 -9
Partials 1096 1096
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@weldonlwz Can you add some e2e test, so it can verify whether the function works. |
|
it occurs to me that when I taint a node with 'unschedule', we dont expect the pod on that node to be removed, right? if then the logic to set |
yes, i think so. And what do you want to update |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zmberg 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 |
Signed-off-by: weldonli <[email protected]> Co-authored-by: weldonli <[email protected]>
Signed-off-by: weldonli <[email protected]> Co-authored-by: weldonli <[email protected]>
Signed-off-by: weldonli <[email protected]> Co-authored-by: weldonli <[email protected]>
Co-authored-by: weldonli <[email protected]>
Ⅰ. Describe what this PR does
fix the bug that broadcastjob won't make pod when a tainted node has erased its taint
Ⅱ. Does this pull request fix one issue?
fixes #1199
Ⅲ. Describe how to verify it
1.cordon a node
2.apply a new broadcastjob, and the pod will not appear on that cordoned node
3.uncordon that node, a new pod is made
Ⅳ. Special notes for reviews
the returned err of function
checkNodeFitnessis not error actually, but the reason why the node is not fit