-
Notifications
You must be signed in to change notification settings - Fork 38.8k
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
Added unschedulable and network-unavailable toleration. #64954
Conversation
/sig apps |
In #61312, |
b5938f9
to
f4be9e7
Compare
/retest |
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.
Could you please add a test to ensure that the tolerations are added properly?
Sure :) |
/retest |
Added integration test, PTAL :) |
/retest |
}) | ||
} | ||
isCritical := utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalCriticalPodAnnotation) && | ||
kubelettypes.IsCriticalPod(newPod) |
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.
Shouldn't it be just kubelettypes.IsCriticalPod(newPod)
?
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.
yes !
node := newNode("unschedulable-node", nil) | ||
node.Spec.Unschedulable = true | ||
// If ScheduleDaemonSetPods enabled, simulate TaintsNodeByCondition to avoid | ||
// race condition. |
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.
Remove "avoid race condition" because we just simulate to avoid the need to start nodelifecycle controller.
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.
done!
} | ||
|
||
// If ScheduleDaemonSetPods enabled, simulate TaintsNodeByCondition to avoid | ||
// race condition. |
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.
Same here
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.
done!
enabled := utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalCriticalPodAnnotation) | ||
defer func() { | ||
utilfeature.DefaultFeatureGate.Set(fmt.Sprintf("%s=%t", | ||
features.ExperimentalCriticalPodAnnotation, enabled)) |
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.
Check error
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.
oh, sorry for the miss; and done :)
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.
also go through the whole file and updated :)
Signed-off-by: Da K. Ma <klaus1982.cn@gmail.com>
/retest |
1 similar comment
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bsalamat, janetkuo, k82cn, mikedanese, ravisantoshgudimetla 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 |
/test all [submit-queue is verifying that this PR is safe to merge] |
/sig scheduling |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. |
This PR appears to have caused a conformance regression: |
:( i will check it |
The latest test is green now (https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-kubernetes-gce-conformance/620) after #68494 merged. |
Signed-off-by: Da K. Ma klaus1982.cn@gmail.com
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):part of #61312
fixes: #67606
Release note: