Skip to content

Conversation

@kamarabbas99
Copy link
Contributor

@kamarabbas99 kamarabbas99 commented Nov 19, 2025

This is to address comments in #8813

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area area/vertical-pod-autoscaler kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed do-not-merge/needs-area labels Nov 19, 2025
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 19, 2025
@kamarabbas99
Copy link
Contributor Author

/cc soltysh

@k8s-ci-robot k8s-ci-robot requested a review from soltysh November 19, 2025 18:35
@kamarabbas99 kamarabbas99 changed the title Address comments on https://github.com/kubernetes/autoscaler/pull/8813 Address comments on PR 8813 Nov 19, 2025
@kamarabbas99 kamarabbas99 force-pushed the experimental-cpu-boost-v2 branch from 9cbd887 to 1c2f5c0 Compare November 19, 2025 18:42
@omerap12
Copy link
Member

/assign @soltysh

Type StartupBoostType `json:"type" protobuf:"bytes,1,opt,name=type"`
// factor specifies the factor to apply to the resource request.
// This field is to be used only when Type is "Factor".
// This field is required when Type is "Factor".
Copy link
Contributor

Choose a reason for hiding this comment

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

The schema sets this to +optional. How are we going to inforce this conditional requirement?

Copy link
Member

Choose a reason for hiding this comment

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

It may be best to rather review #8813, this PR is just addressing comments made in that PR, and it's missing lots of context. (note that this is a PR into a feature branch)

See https://github.com/kubernetes/autoscaler/pull/8813/files#diff-ad66c76a76541b7991631925641b989fc402901440d8e0dcbc3591009eef52b9

Copy link
Contributor

Choose a reason for hiding this comment

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

We will error out in the handler, which should be then exposed to the user that the object is miconfigured. Sadly we can't express that condition in through annotations.

@jackfrancis
Copy link
Contributor

/retitle CPU startup boost: add tests

@k8s-ci-robot k8s-ci-robot changed the title Address comments on PR 8813 CPU startup boost: add tests Nov 21, 2025
@adrianmoisey
Copy link
Member

Given that this is going to a feature branch, and the final review happens on #8813
I'll LGTM
/lgtm

cc @omerap12

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2025
@omerap12
Copy link
Member

makes sense.
/approve

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm

Type StartupBoostType `json:"type" protobuf:"bytes,1,opt,name=type"`
// factor specifies the factor to apply to the resource request.
// This field is to be used only when Type is "Factor".
// This field is required when Type is "Factor".
Copy link
Contributor

Choose a reason for hiding this comment

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

We will error out in the handler, which should be then exposed to the user that the object is miconfigured. Sadly we can't express that condition in through annotations.

@adrianmoisey
Copy link
Member

I'm manually adding the approved label here, since this PR is into a feature branch, and review can take place on #8813

@adrianmoisey adrianmoisey added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: kamarabbas99, omerap12, soltysh

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 d7c901f into kubernetes:experimental-cpu-boost-v2 Nov 21, 2025
13 checks passed
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/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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants