-
Notifications
You must be signed in to change notification settings - Fork 808
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
add toleration time to NoExecute effect --- Enable in next release #776
add toleration time to NoExecute effect --- Enable in next release #776
Conversation
Pull Request Test Coverage Report for Build 1677
💛 - Coveralls |
@@ -70,7 +70,7 @@ resources: | |||
|
|||
priorityClassName: "" | |||
nodeSelector: {} | |||
tolerateAllTaints: true |
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 do think we should change the default but unsure if it's "safe" by helm standards to do it in a point release.
wdyt @ayberk
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.
Hmm this should be mostly fine, but it might still break the existing workflow for some customers with basically no heads-up.
I'd rather change the behavior with a release so we can at least have a note in the release notes.
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.
Agree, How about we keep the current behavior for now, and turn this flag off in the next release(which should be pretty soon)
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.
Updated
f568298
to
329d7e7
Compare
329d7e7
to
a724307
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AndyXiangLi, ayberk 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 |
Continue from #775
Is this a bug fix or adding new feature?
Fixes #588
What is this PR about? / Why do we need it?
Not tolerate NoSchedule effect and add default 300s tolerationSeconds to all the NoExecute taints.
As we discussed, will default to tolerateAllTaints for now, will disable this in next release.