descheduler
descheduler copied to clipboard
kubernetes-sigs
Introducing descheduler profiles with v1alpha2 api version
Closes #486
- Have added a new version
v1alpha2while providing a backward compatibility withv1alpha1as defined in design doc - Enabled all profiles and strategies by default and simplified strategy parameter struct as defined in the doc
The way I have converted two api policy versions may not be the best way as I have limited understanding of CRD's. Also, the changes in readme may not convey the details in the right way. Happy to fix both based on comments.
Hi @hanumanthan. Thanks for your PR.
I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.
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.
Thank you for starting this work @hanumanthan. From a quick overview this looks to be the right direction but I will give it a more in depth review when I have a chance.
The one part I am still unsure about is if we will actually need the version conversions, or if we will be fine to just consume v1alpha2 in our code. So, I will need to look more into how those work.
I might recommend holding off on the embedded types for now, just so we can get the version bump with profiles merged smoothly. We could then merge the embedded types in the same release (or discuss if we would rather go with something like defining a params interface https://github.com/kubernetes-sigs/descheduler/issues/586#issuecomment-858675447).
Great @damemi I will wait for the review comments and remove the embedded struct changes along with fixing the review comments.
Regarding the param interface, I am happy to pick that up. Have raised it here
Hi @hanumanthan, very sorry but I haven't been able to take a deep look yet. I will prioritize that this or next week.
In the meantime, could you please remove the changes related to embedding the strategy params? We won't do that in this PR, and it will make it easier to review & test the changes around just the new api version
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: hanumanthan To complete the pull request process, please ask for approval from damemi after the PR has been reviewed.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
This is related to the error I'm facing installing it from helm chart?
~# helm install descheduler --namespace kube-system descheduler/descheduler
Error: unable to build kubernetes objects from release manifest: unable to recognize "": no matches for kind "CronJob" in version "batch/v1"
@Svisstack no, this PR is to add a new version for the Descheduler's own config API
Your error is that your cluster is not recognizing the v1 CronJob type. What version of kubernetes are you on? I believe CronJobs were just promoted to v1 in 1.21. You can set the cronJobApiVersion value in the helm chart to batch/v1beta1 if you are on an older version of kubernetes.
@hanumanthan: PR needs rebase.
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.
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle stale - Mark this issue or PR as rotten with
/lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle stale - Mark this issue or PR as rotten with
/lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle stale - Mark this issue or PR as rotten with
/lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
@damemi @ingvagabund now that the strategies are migrated to the plugin approach, I imagine moving to v1alpha2 will be on the of next steps. I wanted to check what will be the plan regarding this new sub-effort. (EDIT: We still need to get rid of workarounds and other glues for the plugin migration https://github.com/kubernetes-sigs/descheduler/issues/557#issuecomment-1219737145)
1- https://github.com/kubernetes-sigs/descheduler/pull/587#issuecomment-861821017
The one part I am still unsure about is if we will actually need the version conversions, or if we will be fine to just consume v1alpha2 in our code
I think that, before the framework idea, conversion made some sense since the config would still be a bit similar. Now the config will look like something completely different, passing arguments to strategies in a different way, and so on. Making conversion now seems very very complicated and weird to maintain. Probably better to drop v1alpha1 altogether and directly move to v1alpha2 but with a big release note announcement... What are your thoughts on this? This will require a warning somewhere making clear to users, that if they want to upgrade, and they forget to change the config file version and structure, things will break.
EDIT: Discussed with Jan, conversion and support to v1alpha1 should be in place.
2- @hanumantha
Do you still want to push this effort forward? Probably with getting familiar with https://github.com/kubernetes-sigs/descheduler/issues/837 and related issues and PRs. v1alpha2 would take another shape and would probably need to be another PR.
We need to support v1alpha1 for few releases to provide some deprecated time before users convert their configuration to v1alpha2. The conversion will be automatic without user intervention.
@hanumanthan: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
| Test name | Commit | Details | Required | Rerun command |
|---|---|---|---|---|
| pull-descheduler-helm-test | 78433ae9f8992686afd50aaf57471e26f12655a0 | link | /test pull-descheduler-helm-test |
|
| pull-descheduler-test-e2e-k8s-master-1-22 | 78433ae9f8992686afd50aaf57471e26f12655a0 | link | /test pull-descheduler-test-e2e-k8s-master-1-22 |
|
| pull-descheduler-test-e2e-k8s-1-21-1-21 | 78433ae9f8992686afd50aaf57471e26f12655a0 | link | /test pull-descheduler-test-e2e-k8s-1-21-1-21 |
|
| pull-descheduler-unit-test-master-master | 78433ae9f8992686afd50aaf57471e26f12655a0 | link | true | /test pull-descheduler-unit-test-master-master |
| pull-descheduler-test-e2e-k8s-master-1-23 | 78433ae9f8992686afd50aaf57471e26f12655a0 | link | true | /test pull-descheduler-test-e2e-k8s-master-1-23 |
| pull-descheduler-test-e2e-k8s-master-1-24 | 78433ae9f8992686afd50aaf57471e26f12655a0 | link | true | /test pull-descheduler-test-e2e-k8s-master-1-24 |
| pull-descheduler-test-e2e-k8s-master-1-25 | 78433ae9f8992686afd50aaf57471e26f12655a0 | link | true | /test pull-descheduler-test-e2e-k8s-master-1-25 |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
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. I understand the commands that are listed here.
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.
This bot triages PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the PR is closed
You can:
- Reopen this PR with
/reopen - Mark this PR as fresh with
/remove-lifecycle rotten - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/close
@k8s-triage-robot: Closed this PR.
In response to this:
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.
This bot triages PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied- After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied- After 30d of inactivity since
lifecycle/rottenwas applied, the PR is closedYou can:
- Reopen this PR with
/reopen- Mark this PR as fresh with
/remove-lifecycle rotten- Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/close
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.