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

List nodes through informer in every iteration #249

Merged
merged 1 commit into from
Mar 4, 2020
Merged

List nodes through informer in every iteration #249

merged 1 commit into from
Mar 4, 2020

Conversation

ingvagabund
Copy link
Contributor

@ingvagabund ingvagabund commented Mar 3, 2020 •
edited
Loading

Also, refactor the code a bit so it can be tested without checking for eviction support.

Fixes: #245

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 3, 2020
@ingvagabund ingvagabund force-pushed the list-node-through-informer-in-every-iteration branch from 31de11f to 2bfddc8 Compare March 3, 2020 17:00
dharmab
dharmab reviewed Mar 3, 2020
View reviewed changes
pkg/descheduler/descheduler.go
return
}

if len(nodes) <= 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is number this a good candidate for a configurable option in a future PR? e.g. a user might want to set this to match the number of AZs in their cluster.

Copy link
Contributor Author

@ingvagabund ingvagabund Mar 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Two nodes is a minimal condition for running the descheduler. In case you need to also verify each AZ has at list one node available, what are the chances you will not have it before you run the descheduler. Also, it's maybe more convenient for individual strategies. No strategy should evict a pod if there's no other node where the new pod can land. Including constraining strategies to be zone aware.

@dharmab dharmab mentioned this pull request Mar 3, 2020
Closed
@ingvagabund
List nodes through informer in every iteration
9593ce1 
Also, refactor the code a bit so it can be tested without checking for eviction support.
@ingvagabund ingvagabund force-pushed the list-node-through-informer-in-every-iteration branch from 2bfddc8 to 9593ce1 Compare March 4, 2020 15:27
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 4, 2020
@ingvagabund ingvagabund changed the title WIP: List nodes through informer in every iteration List nodes through informer in every iteration Mar 4, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2020
@ingvagabund
Copy link
Contributor Author

@damemi @aveshagarwal PTAL

damemi
damemi approved these changes Mar 4, 2020
View reviewed changes
Copy link
Contributor

@damemi damemi left a comment

Choose a reason for hiding this comment

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

/lgtm
thanks @ingvagabund!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2020
@aveshagarwal
Copy link
Contributor

/lgtm
/approve

thanks @ingvagabund

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aveshagarwal, ingvagabund

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 Mar 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit 682e07c into kubernetes-sigs:master Mar 4, 2020
@ingvagabund ingvagabund deleted the list-node-through-informer-in-every-iteration branch March 4, 2020 21:17
@damemi damemi mentioned this pull request Apr 2, 2020
Merged
@seanmalloy
Copy link
Member

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label May 20, 2020
@seanmalloy seanmalloy mentioned this pull request May 20, 2020
Closed
4 tasks
briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022
Merge pull request kubernetes-sigs#249 from ingvagabund/list-node-thr…
274bb7a 
…ough-informer-in-every-iteration

List nodes through informer in every iteration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dharmab dharmab dharmab left review comments

@damemi damemi damemi approved these changes

@seanmalloy seanmalloy Awaiting requested review from seanmalloy

Assignees

@damemi damemi

@aveshagarwal aveshagarwal

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/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RemovePodsViolatingNodeTaints policy not working with --descheduling-interval option
6 participants
@ingvagabund @aveshagarwal @k8s-ci-robot @seanmalloy @damemi @dharmab