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

Drop descheduler server from strategies #267

Conversation

ingvagabund
Copy link
Contributor

Removing sigs.k8s.io/descheduler/cmd/descheduler/app/options dependency from all strategies so they can be run independently of options.DeschedulerServer.

Building on top of #266

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 20, 2020
Comment on lines 88 to 105
if s := deschedulerPolicy.Strategies["RemoveDuplicates"]; s.Enabled {
strategies.RemoveDuplicatePods(rs.Client, nodes, rs.EvictLocalStoragePods, podEvictor)
}
if s := deschedulerPolicy.Strategies["LowNodeUtilization"]; s.Enabled {
strategies.LowNodeUtilization(rs.Client, s, nodes, rs.EvictLocalStoragePods, podEvictor)
}
if s := deschedulerPolicy.Strategies["RemovePodsViolatingInterPodAntiAffinity"]; s.Enabled {
strategies.RemovePodsViolatingInterPodAntiAffinity(rs.Client, nodes, rs.EvictLocalStoragePods, podEvictor)
}
if s := deschedulerPolicy.Strategies["RemovePodsViolatingNodeAffinity"]; s.Enabled {
strategies.RemovePodsViolatingNodeAffinity(rs.Client, s, nodes, rs.EvictLocalStoragePods, podEvictor)
}
if s := deschedulerPolicy.Strategies["RemovePodsViolatingNodeTaints"]; s.Enabled {
strategies.RemovePodsViolatingNodeTaints(rs.Client, nodes, rs.EvictLocalStoragePods, podEvictor)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we could initialize a map of name:strategyFunction and put these all in a loop? That would look more like what the scheduler does and could open us up for better user configuration in the future (like being able to specify a custom ordering of the strategies to run). Something like

type strategyFunction func(clientset.Interface, []v1*Node, bool, *evictions.PodEvictor)

strategyFuncs := map[string]strategyFunction{
  "RemoveDuplicates": strategies.RemoveDuplicatePods,
  "LowNodeUtilization": strategies.LowNodeUtilization,
  "RemovePodsViolatingInterPodAntiAffinity": strategies.RemovePodsViolatingInterPodAntiAffinity,
  "RemovePodsViolatingNodeAffinity": strategies.RemovePodsViolatingNodeAffinity,
  "RemovePodsViolatingNodeTaints": strategies.RemovePodsViolatingNodeTaints,
}

for name, f := range strategyFuncs {
  if s := deschedulerPolicy.Strategies[name]; s.Enabled {
    f(rs.Client, nodes, rs.EvictLocalStoragePods, podEvictor)
  }
}

The map could be declared at the package level so we have a good DefaultStrategy map

Copy link
Contributor Author

@ingvagabund ingvagabund Apr 21, 2020

Choose a reason for hiding this comment

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

Yeah, I wanted to talk about this in more detail. Right now every strategy has free will about arguments that go in. Should we create an interface for a strategy? Or, use a data type for it as you suggest? Interface might be too restrictive atm. Private strategyFunction is a low hanging fruit that can be extended as other strategies goes in.

tldr; Though, the list of arguments might grow too much. Later, we might implement some initialization/factory part that will get the config and produces a list of enabled strategies so we don't have to check for s.Enabled at all. Each strategy might register itself (as is done in the scheduler) as plugins. The factory will just accept rs.EvictLocalStoragePods and rs.Client for now. Each strategy implementing Run([]*v1.Node, *evictions.PodEvictor)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, there is a lot of discussion that could be had around implementing more of a "framework" for the descheduler strategies. I was just thinking my change could be a quick fix to clean up the chain of if-statements, but with the side benefit of suggesting a starting framework somewhere down the line. I don't think we need to get too much into it for this PR though

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2020
@ingvagabund ingvagabund force-pushed the drop-DeschedulerServer-from-strategies branch from 89bc603 to 54c833d Compare April 24, 2020 00:15
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 24, 2020
@ingvagabund ingvagabund force-pushed the drop-DeschedulerServer-from-strategies branch from 54c833d to fbada51 Compare April 24, 2020 15:08
@ingvagabund ingvagabund force-pushed the drop-DeschedulerServer-from-strategies branch from fbada51 to cbcefb5 Compare April 28, 2020 14:15
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

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

@ravisantoshgudimetla ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

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

Mostly LGTM just a nit, it's fine if you don't want to address it

pkg/descheduler/descheduler.go Show resolved Hide resolved
Copy link
Contributor

@ravisantoshgudimetla ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

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

/approve

Thanks for working on this @ingvagabund

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund, 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:
  • OWNERS [ravisantoshgudimetla]

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 Apr 28, 2020
@k8s-ci-robot k8s-ci-robot merged commit 78eef6c into kubernetes-sigs:master Apr 28, 2020
@ingvagabund ingvagabund deleted the drop-DeschedulerServer-from-strategies branch April 28, 2020 15:37
ingvagabund added a commit to ingvagabund/descheduler that referenced this pull request Jun 4, 2020
Based on https://github.com/kubernetes/community/blob/master/community-membership.md#requirements-1:

The following apply to the part of codebase for which one would be a reviewer in an OWNERS file (for repos using the bot).

> member for at least 3 months

For a couple of years now

> Primary reviewer for at least 5 PRs to the codebase

kubernetes-sigs#285
kubernetes-sigs#275
kubernetes-sigs#267
kubernetes-sigs#254
kubernetes-sigs#181

> Reviewed or merged at least 20 substantial PRs to the codebase

https://github.com/kubernetes-sigs/descheduler/pulls?q=is%3Apr+is%3Aclosed+assignee%3Aingvagabund

> Knowledgeable about the codebase

yes

> Sponsored by a subproject approver
> With no objections from other approvers
> Done through PR to update the OWNERS file

this PR

> May either self-nominate, be nominated by an approver in this subproject, or be nominated by a robot

self-nominating
briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022
…lerServer-from-strategies

Drop descheduler server from strategies
briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022
Based on https://github.com/kubernetes/community/blob/master/community-membership.md#requirements-1:

The following apply to the part of codebase for which one would be a reviewer in an OWNERS file (for repos using the bot).

> member for at least 3 months

For a couple of years now

> Primary reviewer for at least 5 PRs to the codebase

kubernetes-sigs#285
kubernetes-sigs#275
kubernetes-sigs#267
kubernetes-sigs#254
kubernetes-sigs#181

> Reviewed or merged at least 20 substantial PRs to the codebase

https://github.com/kubernetes-sigs/descheduler/pulls?q=is%3Apr+is%3Aclosed+assignee%3Aingvagabund

> Knowledgeable about the codebase

yes

> Sponsored by a subproject approver
> With no objections from other approvers
> Done through PR to update the OWNERS file

this PR

> May either self-nominate, be nominated by an approver in this subproject, or be nominated by a robot

self-nominating
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. 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
Development

Successfully merging this pull request may close these issues.

4 participants