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

KEP-1610: graduate ContainerResource to stable #4406

Merged
merged 3 commits into from Feb 8, 2024
Merged

KEP-1610: graduate ContainerResource to stable #4406

merged 3 commits into from Feb 8, 2024
+38 −21

Conversation

sanposhiho
Copy link
Member

  • One-line PR description: graduate ContainerResource to stable
  • Other comments:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 15, 2024
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 15, 2024
@sanposhiho
Copy link
Member Author

/cc @mwielgus @pbetkier @gjtempleton

@sanposhiho
Copy link
Member Author

/cc @johnbelamaric

johnbelamaric
johnbelamaric reviewed Feb 6, 2024
View reviewed changes
Copy link
Member

@johnbelamaric johnbelamaric left a comment

Choose a reason for hiding this comment

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

Unchanged line: https://github.com/kubernetes/enhancements/pull/4406/files#diff-48a4e697d44bcfcacdc3b875b4c4c654d479ee4416e3904db42371973903299eR785

I assume this testing was already done back in beta time frame, might want to clean up the KEP. Please confirm.

Unchanged line: https://github.com/kubernetes/enhancements/pull/4406/files#diff-48a4e697d44bcfcacdc3b875b4c4c654d479ee4416e3904db42371973903299eR829

Was this done?

Unchanged lines:

These promise some metrics in beta. Looks like some were implemented, but kubernetes/kubernetes#115639 is still open. Is there a plan to add additional metrics in 1.30? Can you provide any guidance on SLOs for the current ones and any new ones?

Unchanged line: https://github.com/kubernetes/enhancements/pull/4406/files#diff-48a4e697d44bcfcacdc3b875b4c4c654d479ee4416e3904db42371973903299eR1052

Can you add any helpful tips here?

@johnbelamaric johnbelamaric mentioned this pull request Feb 7, 2024
Closed
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 8, 2024
@sanposhiho
Copy link
Member Author

I assume this testing was already done back in beta time frame, might want to clean up the KEP. Please confirm.

Sorry, it looks like I missed it in beta graduation actually. I implemented the tests under the feature gate is enabled/disabled though, we do not have "switching" (= changing) them. Let me follow up it in this release. Tracked in kubernetes/kubernetes#123189
(The switching test will be useless after reaching GA though, unfortunately, it seems like we don't have enough time to have this PR merged before the enhancement freeze. So still the switching test would be beneficial)

Was this done?

Yes, just clean up the mention.

These promise some metrics in beta. Looks like some were implemented, but kubernetes/kubernetes#115639 is still open. Is there a plan to add additional metrics in 1.30?

We only implemented the ones need for the graduation for now. #115639 is not closed because it contains a lot of other metrics, which is not necessary in this enhancement.
So, no additional metric plan for 1.30.

Can you provide any guidance on SLOs for the current ones and any new ones?

Yes, done.

johnbelamaric
johnbelamaric reviewed Feb 8, 2024
View reviewed changes
keps/sig-autoscaling/1610-container-resource-autoscaling/README.md
- Many error occurrence on the container resource metrics
which can be monitored via the 3rd metrics described in [What are the SLIs (Service Level Indicators) an operator can use to determine the health of the service?](#what-are-the-slis-service-level-indicators-an-operator-can-use-to-determine-the-health-of-the-service) section.
- `reconciliation_duration_seconds`: The time(seconds) that the HPA controller takes to reconcile once.
- You should rollback if you see an increase in the overall performance of HPA controller
Copy link
Member

Choose a reason for hiding this comment

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

either "decrease in overall performance" or "increase in overall reconciliation duration". Probably the latter. "increase in performance" sounds like "going faster"...

@johnbelamaric
Copy link
Member

Ok, this is good for PRR. You'll need SIG approval before I can do the actual approval though.

@gjtempleton
Copy link
Member

With those comments addressed, happy to
/approve

@johnbelamaric
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gjtempleton, johnbelamaric, 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 Feb 8, 2024
@johnbelamaric
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 Feb 8, 2024
@k8s-ci-robot k8s-ci-robot merged commit b3fadb9 into kubernetes:master Feb 8, 2024
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.30 milestone Feb 8, 2024
@sanposhiho
Copy link
Member Author

Sorry I'm too busy to get back here. Thanks @gjtempleton @johnbelamaric for the reviews at such a last minute!

@johnbelamaric If it graduates to GA in this release, we don't support the enablement/disablement anymore. So, I'm gonna just close kubernetes/kubernetes#123189. What do you think?

@sanposhiho sanposhiho mentioned this pull request Feb 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnbelamaric johnbelamaric johnbelamaric left review comments

@kikisdeliveryservice kikisdeliveryservice Awaiting requested review from kikisdeliveryservice

@mwielgus mwielgus Awaiting requested review from mwielgus

@gjtempleton gjtempleton Awaiting requested review from gjtempleton

@pbetkier pbetkier Awaiting requested review from pbetkier

Assignees

@johnbelamaric johnbelamaric

Labels
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/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.30
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sanposhiho @johnbelamaric @gjtempleton @k8s-ci-robot