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

Filter pods by labelSelector #510

Merged
merged 4 commits into from
Mar 5, 2021
Merged

Filter pods by labelSelector #510

merged 4 commits into from
Mar 5, 2021

Conversation

lixiang233
Copy link
Contributor

@lixiang233 lixiang233 commented Mar 1, 2021 •
edited
Loading

Fixes: #195

TODO:

  • API change
  • new labelSelector option
  • Add option to strategies
  • README and e2e-tests

/kind feature

@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. kind/feature Categorizes issue or PR as related to a new feature. 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 1, 2021
@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 1, 2021
lixiang233
lixiang233 commented Mar 1, 2021
View reviewed changes
pkg/descheduler/strategies/topologyspreadconstraint.go Outdated
Comment on lines 113 to 114
if (len(includedNamespaces) > 0 && !includedNamespaces.Has(namespace.Name)) ||
(len(excludedNamespaces) > 0 && excludedNamespaces.Has(namespace.Name)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

modify this to pass cyclomatic complexity check.

@lixiang233 lixiang233 changed the title [WIP] Filter pods by labelSelector Filter pods by labelSelector Mar 2, 2021
@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 2, 2021
@seanmalloy
Copy link
Member

@lixiang233 is this PR ready for review?

@lixiang233
Copy link
Contributor Author

@lixiang233 is this PR ready for review?

yes, PTAL when you have time.

@lixiang233
Copy link
Contributor Author

/cc @ingvagabund

ingvagabund
ingvagabund reviewed Mar 3, 2021
View reviewed changes
Copy link
Contributor

@ingvagabund ingvagabund left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Allowing to set label selector in RemoveDuplicates and RemovePodsViolatingTopologySpreadConstraint improperly might result in the opposite of what those strategies wants to achieve. The skew of pods between topology domains might actually increase.

test/e2e/e2e_test.go Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
damemi
damemi reviewed Mar 3, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
@lixiang233
Copy link
Contributor Author

Allowing to set label selector in RemoveDuplicates and RemovePodsViolatingTopologySpreadConstraint improperly might result in the opposite of what those strategies wants to achieve. The skew of pods between topology domains might actually increase.

You are right, so should we mention this in README to tell users the risk of using this parameter or just disable it in these strategies?

@seanmalloy
Copy link
Member

Allowing to set label selector in RemoveDuplicates and RemovePodsViolatingTopologySpreadConstraint improperly might result in the opposite of what those strategies wants to achieve. The skew of pods between topology domains might actually increase.

You are right, so should we mention this in README to tell users the risk of using this parameter or just disable it in these strategies?

Disable it in these strategies.

@ingvagabund
Copy link
Contributor

To disable. We don't want to allow users to configure a strategy the way it malfunctions.

@lixiang233
Copy link
Contributor Author

Disabled. PTAL @ingvagabund @seanmalloy

@ingvagabund
Copy link
Contributor

Worth changing Strategies: Add labelSelector to all strategies except LowNodeUtilization commit message.

@lixiang233
Copy link
Contributor Author

Worth changing Strategies: Add labelSelector to all strategies except LowNodeUtilization commit message.

updated.

@damemi
Copy link
Contributor

damemi commented Mar 4, 2021

I think this looks all good to me, anyone else have any suggestions?
/approve

@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, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, lixiang233, seanmalloy

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

@seanmalloy
Copy link
Member

seanmalloy commented Mar 5, 2021 •
edited
Loading

It looks good to me. I did not find anything that needs changing. I'll give @ingvagabund some time to review just in case he wants to take one more look.

Also FYI @killianmuldoon the long awaited label filtering feature that you suggested has been implemented. Feel free to review if you have time. Thanks!

/cc @killianmuldoon

@k8s-ci-robot
Copy link
Contributor

@seanmalloy: GitHub didn't allow me to request PR reviews from the following users: killianmuldoon.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

It looks good to me. I did not find anything that needs changing. I'll give @ingvagabund some time to review just in case he want to take one more look.

Also FYI @killianmuldoon the long awaited label filtering feature that you suggested has been implemented. Feel free to review if you have time. Thanks!

/cc @killianmuldoon

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ingvagabund
Copy link
Contributor

/lgtm

Thank you @lixiang233!!!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2021
@k8s-ci-robot k8s-ci-robot merged commit 9e14f73 into kubernetes-sigs:master Mar 5, 2021
@lixiang233 lixiang233 deleted the Ft_filter_by_label branch March 6, 2021 16:41
@ingvagabund ingvagabund mentioned this pull request Apr 23, 2021
Closed
wking added a commit to wking/kubernetes-descheduler that referenced this pull request Oct 7, 2021
@wking
README: Clarify podRestartThreshold applying to the sum over containers
6fd80ba 
calcContainerRestarts sums over containers.  The new language makes
that clear, avoiding potential confusion vs. an altenative that looked
for pods where a single container had passed the configured threshold.
For example, with three containers with 50 restarts and a threshold of
100, the actual "sum over containers" logic makes that pod a candidate
for descheduling, but the "largest single container restart count"
hypothetical would not have made it a candidate.

Also shifts labelSelector into the parameter table, because when it
was added in 29ade13 (README and e2e-testcase add for
labelSelector, 2021-03-02, kubernetes-sigs#510), it landed a few lines too high.
@wking wking mentioned this pull request Oct 7, 2021
Merged
@damemi damemi mentioned this pull request Nov 10, 2021
Closed
briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022
Merge pull request kubernetes-sigs#510 from lixiang233/Ft_filter_by_l…
c901cc4 
…abel

Filter pods by labelSelector
briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022
README: Clarify podRestartThreshold applying to the sum over containers
9d282c1 
calcContainerRestarts sums over containers.  The new language makes
that clear, avoiding potential confusion vs. an altenative that looked
for pods where a single container had passed the configured threshold.
For example, with three containers with 50 restarts and a threshold of
100, the actual "sum over containers" logic makes that pod a candidate
for descheduling, but the "largest single container restart count"
hypothetical would not have made it a candidate.

Also shifts labelSelector into the parameter table, because when it
was added in 29ade13 (README and e2e-testcase add for
labelSelector, 2021-03-02, kubernetes-sigs#510), it landed a few lines too high.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damemi damemi damemi left review comments

@ingvagabund ingvagabund ingvagabund left review comments

@seanmalloy seanmalloy seanmalloy approved these changes

Assignees

@ingvagabund ingvagabund

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. 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.

Filter by label
5 participants
@lixiang233 @seanmalloy @ingvagabund @damemi @k8s-ci-robot