DPTP-2938: pod-scaler add cpu/memory authoritative max reduction flags#5254
DPTP-2938: pod-scaler add cpu/memory authoritative max reduction flags#5254deepsm007 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@deepsm007: This pull request references DPTP-2938 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughReplaces a single hardcoded ChangesPer-resource authoritative max reduction percent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/pod-scaler/admission.go (1)
40-40: 🏗️ Heavy liftReduce parameter plumbing by grouping authoritative settings into a config struct.
Line 40, Line 236, and Line 414 continue expanding already-large signatures. This is getting brittle for call-site ordering and future extension; bundle these related knobs into one typed config and pass that through.
As per coding guidelines, “Keep function signatures small — if a function takes more than 3-4 parameters, consider grouping them into an options struct.”
Also applies to: 236-236, 414-414
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/pod-scaler/admission.go` at line 40, The admit function signature on line 40 (and its call sites at lines 236 and 414) has excessive parameters, particularly the authoritative-related settings (authoritativeCPU, authoritativeMemory, authoritativeCPUDryRun, authoritativeMemoryDryRun, authoritativeCPUMaxReductionPercent, authoritativeMemoryMaxReductionPercent). Create a new config struct to bundle these related authoritative settings together, then refactor the admit function to accept this struct instead of individual parameters. Update all call sites at lines 236 and 414 to instantiate and pass this config struct instead of passing the individual authoritative parameters separately.Source: Coding guidelines
cmd/pod-scaler/main.go (1)
111-112: Ensure deployment manifests set the intended 25% cap.Line 111 and Line 112 default both reductions to
1.0(no cap). The linkedopenshift/releaseadmission deployment currently does not set these new flags, so behavior stays uncapped until that follow-up manifest update lands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/pod-scaler/main.go` around lines 111 - 112, The flags authoritativeCPUMaxReductionPercent and authoritativeMemoryMaxReductionPercent currently default to 1.0 (no cap) in the code. To enforce the intended 25% reduction cap, update the deployment manifests in the openshift/release repository to explicitly set the command-line flags --authoritative-cpu-max-reduction-percent and --authoritative-memory-max-reduction-percent to 0.25 when deploying this pod-scaler service, rather than relying on the uncapped defaults.Source: Linked repositories
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/pod-scaler/admission.go`:
- Line 40: The admit function signature on line 40 (and its call sites at lines
236 and 414) has excessive parameters, particularly the authoritative-related
settings (authoritativeCPU, authoritativeMemory, authoritativeCPUDryRun,
authoritativeMemoryDryRun, authoritativeCPUMaxReductionPercent,
authoritativeMemoryMaxReductionPercent). Create a new config struct to bundle
these related authoritative settings together, then refactor the admit function
to accept this struct instead of individual parameters. Update all call sites at
lines 236 and 414 to instantiate and pass this config struct instead of passing
the individual authoritative parameters separately.
In `@cmd/pod-scaler/main.go`:
- Around line 111-112: The flags authoritativeCPUMaxReductionPercent and
authoritativeMemoryMaxReductionPercent currently default to 1.0 (no cap) in the
code. To enforce the intended 25% reduction cap, update the deployment manifests
in the openshift/release repository to explicitly set the command-line flags
--authoritative-cpu-max-reduction-percent and
--authoritative-memory-max-reduction-percent to 0.25 when deploying this
pod-scaler service, rather than relying on the uncapped defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 52112767-757e-4bce-910c-53e3960cd03f
📒 Files selected for processing (3)
cmd/pod-scaler/admission.gocmd/pod-scaler/admission_test.gocmd/pod-scaler/main.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
|
/test images e2e |
2d00cf3 to
90280b3
Compare
|
/retest |
|
Scheduling tests matching the |
|
/override ci/prow/e2e |
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/e2e DetailsIn response to this:
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. |
|
@deepsm007: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
Add configurable per-admission CPU/memory reduction caps for authoritative pod-scaler mode, defaulting to no cap in code with 25% set via release deployment args.
/cc @smg247
openshift/release#80602
Pod-Scaler: Configurable Resource Reduction Caps for Authoritative Mode
This PR updates the pod-scaler admission webhook’s authoritative mode so CPU and memory reductions are no longer limited by a single hardcoded cap. Instead, operators can configure independent maximum reduction limits per resource.
What Changed
--authoritative-cpu-max-reduction-percent(default:1.0, i.e., no cap)--authoritative-memory-max-reduction-percent(default:1.0, i.e., no cap)reduction_cappedfield.Practical Impact for CI Operators
Tests
Issue Reference / Bot Note
mainbranch is 5.0.0.