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.

Sign up for GitHub

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

Jump to bottom

Add test cases for taintbasedevictions #69788

Merged
merged 1 commit into from
Oct 18, 2018
Merged

Add test cases for taintbasedevictions #69788

merged 1 commit into from
Oct 18, 2018

Conversation

ravisantoshgudimetla
Copy link
Contributor

What this PR does / why we need it:
Add unit tests for basic taint based evictions.
/cc @Huang-Wei @k82cn @bsalamat
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 #

Special notes for your reviewer:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 15, 2018
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 15, 2018
@ravisantoshgudimetla
Copy link
Contributor Author

/sig scheduling
/kind enhancement

@k8s-ci-robot k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 15, 2018
@Huang-Wei
Copy link
Member

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 15, 2018
@k82cn
Copy link
Member

k82cn commented Oct 15, 2018

Thanks, I'm going to take time to review this :)

/assign

@Huang-Wei Huang-Wei mentioned this pull request Oct 15, 2018
Closed
7 tasks
Huang-Wei
Huang-Wei reviewed Oct 15, 2018
View reviewed changes
pkg/controller/nodelifecycle/node_lifecycle_controller_test.go Outdated
// TestEvictPodsByTaints, ensures we just have a NoExecute taint applied to node.
// NodeController is just responsible for enqueuing the node to tainting queue from which taint manager picks up
// and evicts the pods on the node.
func TestEvictPodsByTaints(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

change func name to "TestApplyNoExecuteTaints"?

Huang-Wei
Huang-Wei reviewed Oct 15, 2018
View reviewed changes
Copy link
Member

@Huang-Wei Huang-Wei left a comment •
edited

Choose a reason for hiding this comment

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

Could you add more cases to cover the scenario that by giving nodes having NoExecute taints, once we got a Ready=true condition, those NoExecute taints are expected to take off those nodes?

Otherwise lgtm. Thanks @ravisantoshgudimetla !

@ravisantoshgudimetla ravisantoshgudimetla force-pushed the taint-based-eviction branch 2 times, most recently from b4d8bc9 to 42b2fee Compare October 16, 2018 17:53
@Huang-Wei
Copy link
Member

/retest

bsalamat
bsalamat reviewed Oct 16, 2018
View reviewed changes
Copy link
Member

@bsalamat bsalamat left a comment

Choose a reason for hiding this comment

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

LGTM, but I would like someone more familiar with the logic to take a look.

Huang-Wei
Huang-Wei reviewed Oct 16, 2018
View reviewed changes
pkg/controller/nodelifecycle/node_lifecycle_controller_test.go Outdated
// NotReady Taint with NoExecute effect should be applied to this node.
{
ObjectMeta: metav1.ObjectMeta{
Name: "node3",
Copy link
Member

Choose a reason for hiding this comment

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

where is node2...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

node3 preempted node2, lol, I missed it. I will change it.

Huang-Wei
Huang-Wei approved these changes Oct 16, 2018
View reviewed changes
Copy link
Member

@Huang-Wei Huang-Wei left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2018
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2018
@Huang-Wei
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2018
@spiffxp
Copy link
Member

spiffxp commented Oct 17, 2018

/milestone v1.13
I'm adding this to the v1.13 milestone since it relates to kubernetes/enhancements#166 which is currently planned for v1.13

(also, thanks for adding unit test coverage!)

@k8s-ci-robot k8s-ci-robot added this to the v1.13 milestone Oct 17, 2018
@k82cn
Copy link
Member

k82cn commented Oct 18, 2018

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2018
@k8s-ci-robot k8s-ci-robot merged commit d425258 into kubernetes:master Oct 18, 2018
@Huang-Wei
Copy link
Member

Huang-Wei commented Oct 18, 2018 •
edited

@ravisantoshgudimetla I think #54297 can be resolved by this PR?

@Huang-Wei
Copy link
Member

ping @ravisantoshgudimetla ^^

@Huang-Wei Huang-Wei mentioned this pull request Nov 1, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsalamat bsalamat bsalamat left review comments

@Huang-Wei Huang-Wei Huang-Wei approved these changes

@k82cn k82cn Awaiting requested review from k82cn

Assignees

@Huang-Wei Huang-Wei

@k82cn k82cn

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.13
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ravisantoshgudimetla @Huang-Wei @k82cn @spiffxp @k8s-ci-robot @bsalamat