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

Implementation for KEP-4831: Control VPA eviction behavior based on scaling direction and resource #5599

Conversation

voelzmo
Copy link
Contributor

@voelzmo voelzmo commented Mar 15, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

  • Implements ScalingDirectionPodEvictionAdmission, which controls Pod admission to eviction based on EvictionRequirements defined in a VPA's .spec.updatePolicy.evictionRequirements
  • Makes Updater use the new ScalingDirectionPodEvictionAdmission instead of DefaultPodEvictionAdmission
  • Change e2e tests to use VPA builder that's already used in the unit tests
    • Changes to VPA builder to allow for multiple containers and more detailed settings per container

Which issue(s) this PR fixes:

Implements #4831
API PR done in #5176

Special notes for your reviewer:

Depends on the API changes in #5176

Does this PR introduce a user-facing change?

Added `.updatePolicy.evictionRequirements` to the VPA spec allowing to constrain Pod eviction based on scaling direction (scaling up or scaling down) and resource (cpu or memory).

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [AEP]: https://github.com/kubernetes/autoscaler/tree/master/vertical-pod-autoscaler/enhancements/4831-control-eviction-behavior

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 15, 2023
@k8s-ci-robot k8s-ci-robot added area/vertical-pod-autoscaler 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 Mar 15, 2023
@voelzmo voelzmo force-pushed the enh/implement-eviction-behavior-control-kep-4831 branch 2 times, most recently from 9049e36 to c31607c Compare March 15, 2023 13:49
@voelzmo voelzmo force-pushed the enh/implement-eviction-behavior-control-kep-4831 branch from d44239d to c76a13f Compare March 23, 2023 09:05
@voelzmo
Copy link
Contributor Author

voelzmo commented Apr 19, 2023

@jbartosik Can you please take a look? Thanks!

@voelzmo
Copy link
Contributor Author

voelzmo commented May 3, 2023

@jbartosik Can you try to give this a review, please? Thanks!

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2023
@voelzmo voelzmo force-pushed the enh/implement-eviction-behavior-control-kep-4831 branch from 7e50e27 to c05301d Compare July 26, 2023 12:59
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2023
@voelzmo
Copy link
Contributor Author

voelzmo commented Jul 26, 2023

Force-pushed an update:

vertical-pod-autoscaler/README.md Outdated Show resolved Hide resolved
vertical-pod-autoscaler/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not reviewing this here; I'm instead looking at version in #5176 and assuming we'll sync this to not make ny changes from there

vertical-pod-autoscaler/e2e/v1/common.go Outdated Show resolved Hide resolved
vertical-pod-autoscaler/e2e/v1/updater.go Show resolved Hide resolved
vertical-pod-autoscaler/e2e/v1/updater.go Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm reviewing this in #5176 and presume we'll not change this file in this PR

@voelzmo voelzmo force-pushed the enh/implement-eviction-behavior-control-kep-4831 branch from 3bd99c1 to 4a5e88a Compare August 8, 2023 14:26
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 8, 2023
@voelzmo
Copy link
Contributor Author

voelzmo commented Aug 8, 2023

force-pushed an update:

  • rebased on current master that includes the merged API PR
  • refactorings to expose VPA builder in e2e tests
  • modifications to VPA builder such that it can replace the previous SetupVPAForNHamstersWithMinReplicas functions

Open points:

  • one TODO in the VPA builder
  • Simplifications/inlining for the SetupVPAForNHamstersWithMinReplicas functions

I can get to that next week

@voelzmo voelzmo force-pushed the enh/implement-eviction-behavior-control-kep-4831 branch from 4a5e88a to 35e7fbf Compare August 14, 2023 16:02
@voelzmo
Copy link
Contributor Author

voelzmo commented Aug 14, 2023

Next set of updates, containing the first inline of the test setup helper functions.
Still todo:

  • inline SetupVPA helper function and have tests directly call the builder to create VPA objects

@voelzmo voelzmo force-pushed the enh/implement-eviction-behavior-control-kep-4831 branch 3 times, most recently from f6aacee to a56a76d Compare August 15, 2023 11:50
@voelzmo voelzmo force-pushed the enh/implement-eviction-behavior-control-kep-4831 branch from 7130a53 to 6380df0 Compare September 22, 2023 12:33
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 22, 2023
@voelzmo
Copy link
Contributor Author

voelzmo commented Sep 22, 2023

@jbartosik rebased on master, please take another look. Unfortunately, the bot removes LGTM labels on new changes ;(

@voelzmo
Copy link
Contributor Author

voelzmo commented Oct 12, 2023

@jbartosik can we merge this before the next rebase is necessary? I’m kind of afraid having to go through a lot of files again when other changes get merged 😱
Thanks!

Copy link
Collaborator

@jbartosik jbartosik left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbartosik, voelzmo

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 merged commit ffec40d into kubernetes:master Nov 2, 2023
6 checks passed
This was referenced Dec 20, 2023
@voelzmo voelzmo mentioned this pull request Feb 7, 2024
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. area/vertical-pod-autoscaler 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants