Skip to content

[WIP] HPA - pod selection strategy #132018

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
Draft
wants to merge 55 commits into
base: master
Choose a base branch
from
Draft

[WIP] HPA - pod selection strategy #132018

wants to merge 55 commits into from
+2,110 −217

Conversation

omerap12
Copy link
Member

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot
Copy link
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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-sigs/prow repository.

@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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 29, 2025
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label May 29, 2025
@k8s-ci-robot k8s-ci-robot requested review from deads2k and mwielgus May 29, 2025 10:02
@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels May 29, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label May 29, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in SIG Apps May 29, 2025
omerap12 added 2 commits May 31, 2025 15:07
@omerap12
Change to SelectionStrategy
2b8c5ee 
Signed-off-by: Omer Aplatony <[email protected]>
@omerap12
Add converstions
9490b89 
Signed-off-by: Omer Aplatony <[email protected]>
@k8s-ci-robot k8s-ci-robot added area/code-generation sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels May 31, 2025
omerap12 added 2 commits May 31, 2025 16:33
@omerap12
Add converstions
46b9688 
Signed-off-by: Omer Aplatony <[email protected]>
@omerap12
Fixed bug
a94de2a 
Signed-off-by: Omer Aplatony <[email protected]>
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 31, 2025
@omerap12
mute unit tests for now
3fea554 
Signed-off-by: Omer Aplatony <[email protected]>
@k8s-ci-robot k8s-ci-robot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Jun 20, 2025
@k8s-ci-robot
Copy link
Contributor

Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found 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-sigs/prow repository.

adrianmoisey and others added 12 commits June 21, 2025 08:52
@adrianmoisey
Should be false
5edfe9b
@omerap12
Add test for TestPrepareForUpdateSelectionStrategyEnabled func
76a495e 
Signed-off-by: Omer Aplatony <[email protected]>
@omerap12
Add tests for pod_filters and cache
c31ff3f 
Signed-off-by: Omer Aplatony <[email protected]>
@omerap12
Add metrics for cache hit & miss
bf26585 
Signed-off-by: Omer Aplatony <[email protected]>
@omerap12
Add mock monitor
37f4a92 
Signed-off-by: Omer Aplatony <[email protected]>
@adrianmoisey
Fix test
161ce2d
@omerap12
Remove strategy status
0796eef 
Signed-off-by: Omer Aplatony <[email protected]>
@omerap12
Revert "Remove strategy status"
ba2d026 
This reverts commit 0796eef.

Signed-off-by: Omer Aplatony <[email protected]>
@omerap12
Removed CurrentSelectionStrategy from HPA status
306c4ce 
Signed-off-by: Omer Aplatony <[email protected]>
@adrianmoisey
Update SelectionStrategy description
1eaa40e
@adrianmoisey
Handle v1 round-tripping
a83fa6e
@adrianmoisey
Remove status and handle v2beta1 round tripping
0b429b9
adrianmoisey
adrianmoisey reviewed Jun 24, 2025
View reviewed changes
pkg/controller/podautoscaler/horizontal.go
Comment on lines +252 to +262
oldStrategy := autoscalingv2.LabelSelector // default
if oldHPA.Spec.SelectionStrategy != nil {
oldStrategy = *oldHPA.Spec.SelectionStrategy
}

newStrategy := autoscalingv2.LabelSelector // default
if newHPA.Spec.SelectionStrategy != nil {
newStrategy = *newHPA.Spec.SelectionStrategy
}

if oldStrategy != newStrategy {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this can't be simplified into:

Suggested change
oldStrategy := autoscalingv2.LabelSelector // default
if oldHPA.Spec.SelectionStrategy != nil {
oldStrategy = *oldHPA.Spec.SelectionStrategy
}
newStrategy := autoscalingv2.LabelSelector // default
if newHPA.Spec.SelectionStrategy != nil {
newStrategy = *newHPA.Spec.SelectionStrategy
}
if oldStrategy != newStrategy {
if oldHPA.Spec.SelectionStrategy != newHPA.Spec.SelectionStrategy {

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I guess the variables are used on line 272

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2025
@adrianmoisey
Add other round-trip direction
299ab19 
And don't json Marshall it, just stringify
@adrianmoisey
Round-trip v2beta1
664eb9f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrianmoisey adrianmoisey adrianmoisey left review comments

@deads2k deads2k Awaiting requested review from deads2k

@mwielgus mwielgus Awaiting requested review from mwielgus

Assignees
No one assigned
Labels
area/code-generation area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
SIG Apps
Status: Needs Triage
SIG Auth
Status: Needs Triage
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@omerap12 @k8s-ci-robot @adrianmoisey