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

Enhancement proposal to add minReplicas per VPA Object (see #4560) #4566

Merged
merged 3 commits into from
Jan 13, 2022
Merged

Enhancement proposal to add minReplicas per VPA Object (see #4560) #4566

merged 3 commits into from
Jan 13, 2022

Conversation

kgolab
Copy link
Collaborator

@kgolab kgolab commented Dec 28, 2021

Which component this PR applies to?

vertical-pod-autoscaler (VPA API)

What type of PR is this?

/kind documentation
/kind feature
/kind api-change

What this PR does / why we need it:

To allow for API review before merging (or rejecting) #4560.

@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 28, 2021
@kgolab
Copy link
Collaborator Author

kgolab commented Dec 28, 2021

@mwielgus , you asked for the KEP to accompany my PR #4560. Please have a look if this is what you had in mind. Thanks!

@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@kgolab
Copy link
Collaborator Author

kgolab commented Jan 4, 2022

As per yesterday's SIG meeting: @jbartosik , @wangchen615, you might be interested in looking into this.

jbartosik
jbartosik approved these changes Jan 10, 2022
View reviewed changes
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.

Generally looks good to me, I left some small comments about phrasing.

vertical-pod-autoscaler/enhancements/4566-min-replicas/README.md Outdated Show resolved Hide resolved
vertical-pod-autoscaler/enhancements/4566-min-replicas/README.md Outdated Show resolved Hide resolved
vertical-pod-autoscaler/enhancements/4566-min-replicas/README.md Outdated
change is to set global `--min-replicas` flag to `1` and Pod Disruption Budget
to protect single-replica workloads from being disrupted, if needed.

While this would end up in a very similar (if not identical) cluster behaviour,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think cluster behavior is different.

With this KEP we can protect 1 pod controllers from eviction by default (and can override that per-VPA).
With the alternative described in this section we evict 1-pod controllers by default (and can override that per-controller).

I'd rephrase to something like:

While this allows the same configuration for each individual controller it changes the default behavior to an unsafe one. As a result it requires cluster-wide coordination...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Took an attempt at simplifying, PTAL.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2022
wangchen615
wangchen615 reviewed Jan 10, 2022
View reviewed changes
Copy link
Contributor

@wangchen615 wangchen615 left a comment

Choose a reason for hiding this comment

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

Just some minor issues. I like the idea, thanks.

vertical-pod-autoscaler/enhancements/4566-min-replicas/README.md Outdated Show resolved Hide resolved
vertical-pod-autoscaler/enhancements/4566-min-replicas/README.md Outdated Show resolved Hide resolved
vertical-pod-autoscaler/enhancements/4566-min-replicas/README.md Outdated Show resolved Hide resolved
vertical-pod-autoscaler/enhancements/4566-min-replicas/README.md Outdated Show resolved Hide resolved
vertical-pod-autoscaler/enhancements/4566-min-replicas/README.md Outdated

### Test Plan

Add automated tests that update VPA objects, altering the value of
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean Add e2e tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually they're already there in PR #4560, please have a look at e2e/v1/actuation.go. The testEvictsSingletonPod verifies exactly the behaviour described here: no eviction w/o minReplicas being set and eviction happening after minReplicas is set to 1.

Added 'E2E' in the KEP to make it clear.

@kgolab
Copy link
Collaborator Author

kgolab commented Jan 11, 2022

@jbartosik , @wangchen615 . hopefully all your comments are addressed. Thanks for the review :-)

@kgolab
Copy link
Collaborator Author

kgolab commented Jan 11, 2022 •
edited
Loading

The test failure seemed completely unrelated (CA test) so I relaunched test workflow after syncing with master but the problem persists.
Blocked on #4594 or #4592

jbartosik
jbartosik approved these changes Jan 12, 2022
View reviewed changes
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.

Looks good to me.

I'm not giving the label yet because that would merge the PR and I want to give others some time to take a look. (feel free to ping me if you think it's time to merge but I didn't give the label yet)

/retest

@jbartosik
Copy link
Collaborator

@wangchen615 please let me know you want to take another look (a comment telling me I should wait more is enough). If this PR doesn't get any more comments I'll give it LGTM and it'll merge on Monday (2022-01-17) during my work hours.

I'd like to merge this, then #4560 then do a release #4404. I want to make a release next week. Or if we need more discussion on this I'd like to know that by Monday so that I can go ahead and release without waiting for this.

@wangchen615
Copy link
Contributor

@wangchen615 please let me know you want to take another look (a comment telling me I should wait more is enough). If this PR doesn't get any more comments I'll give it LGTM and it'll merge on Monday (2022-01-17) during my work hours.

I'd like to merge this, then #4560 then do a release #4404. I want to make a release next week. Or if we need more discussion on this I'd like to know that by Monday so that I can go ahead and release without waiting for this.

@jbartosik I already went through it. It looked good to me. No more comments.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbartosik, kgolab, wangchen615

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

jbartosik
jbartosik reviewed Jan 13, 2022
View reviewed changes
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.

Thanks :)

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2022
@k8s-ci-robot k8s-ci-robot merged commit 24d9b2e into kubernetes:master Jan 13, 2022
@kgolab kgolab mentioned this pull request Jan 14, 2022
Merged
@kgolab kgolab mentioned this pull request Oct 20, 2022
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbartosik jbartosik jbartosik approved these changes

@wangchen615 wangchen615 wangchen615 approved these changes

@krzysied krzysied Awaiting requested review from krzysied

Assignees

@jbartosik jbartosik

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/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/documentation Categorizes issue or PR as related to documentation. 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.
5 participants
@kgolab @k8s-triage-robot @jbartosik @wangchen615 @k8s-ci-robot