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

Add check for taints to determine if pod can run #29793

Merged
merged 1 commit into from Oct 29, 2020

Conversation

rmweir
Copy link
Contributor

@rmweir rmweir commented Oct 27, 2020

Version2 of: #29759

Problem
Even when some nodes are labelled as control plane, they still have the ability to run pods

Solution
Check for taints to determine if pods can be executed on that node and count it towards worker stats if it can

Issue
#29139

@rmweir rmweir requested a review from brandond October 27, 2020 22:44
brandond
brandond previously approved these changes Oct 27, 2020
Copy link
Contributor

@brandond brandond left a comment

Choose a reason for hiding this comment

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

LGTM!

cbron
cbron previously approved these changes Oct 28, 2020
if taint.Key != nodeRoleControlPlane && taint.Key != nodeRoleETCD &&
taint.Key != nodeRoleControlPlaneHyphen && taint.Key != nodeRoleMaster {
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: feel like this is confusing conditional. You could do notWorker := a || b || c then if notWorker && taint == NoSchedule; return true

Copy link
Contributor Author

@rmweir rmweir Oct 28, 2020

Choose a reason for hiding this comment

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

@cbron Did something equivalent but without the inverted logic. Also, I try to avoid return the true case inside of conditionals as it's more idiomatic per: https://dave.cheney.net/2020/02/23/the-zen-of-go, but I think this reads fine now.

Copy link
Contributor

@cbron cbron left a comment

Choose a reason for hiding this comment

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

looks great

@rmweir rmweir merged commit 019940a into rancher:master Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants