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

graduate HPAContainerMetrics to stable #123482

Merged
merged 2 commits into from
Mar 7, 2024
Merged

graduate HPAContainerMetrics to stable #123482

merged 2 commits into from
Mar 7, 2024

Conversation

sanposhiho
Copy link
Member

@sanposhiho sanposhiho commented Feb 24, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

graduate HPAContainerMetrics to stable

Which issue(s) this PR fixes:

Part of kubernetes/enhancements#1610

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Graduated HorizontalPodAutoscaler support for per-container metrics to stable.

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

- [KEP]: https://github.com/kubernetes/enhancements/issues/1610

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. needs-priority Indicates a PR lacks a `priority/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/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 24, 2024
@sanposhiho
Copy link
Member Author

/cc @gjtempleton

@sanposhiho
Copy link
Member Author

/retest

@sanposhiho sanposhiho force-pushed the hpa-containerresource-graduation branch from e195246 to 07e0a80 Compare February 25, 2024 00:58
@sftim
Copy link
Contributor

sftim commented Feb 25, 2024

Changelog suggestion

-graduate HPAContainerMetrics to stable
+Graduated HorizontalPodAutoscaler support for per-container metrics to stable.

gjtempleton
gjtempleton suggested changes Feb 27, 2024
View reviewed changes
Copy link
Member

@gjtempleton gjtempleton left a comment

Choose a reason for hiding this comment

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

One minor change and one query from me.

pkg/features/kube_features.go Outdated Show resolved Hide resolved
pkg/registry/autoscaling/horizontalpodautoscaler/strategy.go Outdated
Comment on lines 70 to 68
func (autoscalerStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
newHPA := obj.(*autoscaling.HorizontalPodAutoscaler)

// create cannot set status
newHPA.Status = autoscaling.HorizontalPodAutoscalerStatus{}

if !utilfeature.DefaultFeatureGate.Enabled(features.HPAContainerMetrics) {
dropContainerMetricSources(newHPA.Spec.Metrics)
}
}
func (autoscalerStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {}
Copy link
Member

Choose a reason for hiding this comment

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

Am I missing some context as to why we're wiping this entire function instead of just the feature gate check?

Same holds true for PrepareForUpdate below.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. I overlooked it prevents the operation from changing status.

sanposhiho
sanposhiho commented Mar 2, 2024
View reviewed changes
pkg/features/kube_features.go
@@ -994,7 +995,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS

GracefulNodeShutdownBasedOnPodPriority: {Default: true, PreRelease: featuregate.Beta},

HPAContainerMetrics: {Default: true, PreRelease: featuregate.Beta},
HPAContainerMetrics: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.32
Copy link
Member Author

Choose a reason for hiding this comment

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

Added LockToDefault as I was suggested in another GA graduation PR #123481.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good. Note that in future releases the logic using the featuregate must remain in order to support the compatibility versions: https://github.com/kubernetes/enhancements/tree/master/keps/sig-architecture/4330-compatibility-versions .

@sanposhiho
Copy link
Member Author

/label api-review

@k8s-ci-robot k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Mar 2, 2024
@gjtempleton
Copy link
Member

/lgtm

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

LGTM label has been added.

Git tree hash: 94c119f1be96ba38dd53039fb43d52bd77ee72ca

@gjtempleton gjtempleton mentioned this pull request Mar 4, 2024
Closed
@deads2k
Copy link
Contributor

deads2k commented Mar 6, 2024

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sanposhiho

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2024
@pacoxu
Copy link
Member

pacoxu commented Mar 6, 2024

Code freeze began early today.

Is this target to v1.30?

@gjtempleton
Copy link
Member

It is, yes. Have just submitted an exception request (currently waiting for the message to be approved.)

@katcosgrove
Copy link

/milestone v1.30

@k8s-ci-robot k8s-ci-robot added this to the v1.30 milestone Mar 6, 2024
@k8s-ci-robot k8s-ci-robot merged commit 2ec63e0 into kubernetes:master Mar 7, 2024
17 checks passed
@jthomperoo jthomperoo mentioned this pull request Mar 19, 2024
Open
@cici37
Copy link
Contributor

cici37 commented Apr 11, 2024

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 11, 2024
@gjtempleton gjtempleton mentioned this pull request Apr 29, 2024
Open
@sanposhiho
Copy link
Member Author

sanposhiho commented May 6, 2024 •
edited
Loading

@praparn Can you file a GitHub issue instead of here. But, note that v1.26 or older already reached end of Life.,

@praparn
Copy link

praparn commented May 7, 2024

As test operate HPAContainerMetrics on Kubernetes 1.30. Just wonder how to make it happen. Base on HPA/V2beta2 was removed since Kubernetes 1.26 and current HPA/V2 is not support "unknown field "spec.containerMetrics", unknown field "spec.metrics[0].resource.container"

@sanposhiho Note with thanks will delete and post on issue na krab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gjtempleton gjtempleton gjtempleton requested changes

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@gjtempleton gjtempleton

Labels
api-review Categorizes an issue or PR as actively needing an API review. 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. 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
API Reviews
Status: API review completed, 1.30
SIG Apps
Archived in project
Milestone
v1.30
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@sanposhiho @sftim @gjtempleton @k8s-ci-robot @deads2k @pacoxu @katcosgrove @cici37 @praparn