fix: handle nil minReplicas in CronFederatedHPA scaling#7725
Conversation
|
@rootp1: The label(s) 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the CronFederatedHPA controller that caused a panic when interacting with FederatedHPA resources that lacked a defined minReplicas value. By adding a null check and implementing proper pointer initialization, the controller now gracefully handles these configurations. The changes are accompanied by a new regression test to ensure stability and prevent future occurrences of this issue. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Welcome @rootp1! It looks like this is your first PR to karmada-io/karmada 🎉 |
There was a problem hiding this comment.
Code Review
This pull request updates the ScaleFHPA method in cronfederatedhpa_job.go to safely handle cases where fhpa.Spec.MinReplicas is nil, preventing potential nil pointer dereferences. It also adds a corresponding unit test to verify the initialization of MinReplicas when it is nil. There are no review comments, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
Fixes a nil-pointer panic in the CronFederatedHPA controller when scaling a target FederatedHPA that omits spec.minReplicas by guarding the dereference and initializing MinReplicas when a target minimum is provided.
Changes:
- Add a nil-safe comparison for
fhpa.Spec.MinReplicasduring scheduled scaling. - Initialize
fhpa.Spec.MinReplicasfromTargetMinReplicaswhen the existing field isnil. - Add a regression test case covering the “existing MinReplicas is nil” path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/controllers/cronfederatedhpa/cronfederatedhpa_job.go | Prevents nil dereference by guarding and initializing MinReplicas during scaling updates. |
| pkg/controllers/cronfederatedhpa/cronfederatedhpa_job_test.go | Adds regression coverage for scaling when the existing FederatedHPA has spec.minReplicas omitted. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7725 +/- ##
=======================================
Coverage 42.06% 42.07%
=======================================
Files 879 879
Lines 54831 54832 +1
=======================================
+ Hits 23065 23068 +3
+ Misses 30022 30020 -2
Partials 1744 1744
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
What type of PR is this?
/kind bug
/kind regression
What this PR does / why we need it:
Prevents
CronFederatedHPAfrom panicking when the targetFederatedHPAomitsspec.minReplicas. The controller now initializesMinReplicaswhen a target minimum is provided, and the regression test covers the nil-pointer path.Summary
ScaleFHPAagainst a nilFederatedHPA.Spec.MinReplicasMinReplicasfromtargetMinReplicaswhen neededWhich issue(s) this PR fixes:
Fixes #7724
Linked Issue
Fixes #7724
What Changed
pkg/controllers/cronfederatedhpa/cronfederatedhpa_job.goto avoid dereferencing a nilMinReplicaspointerInitialize Nil MinReplicascoverage topkg/controllers/cronfederatedhpa/cronfederatedhpa_job_test.goVerification
go test ./pkg/controllers/cronfederatedhpa -run TestScaleFHPA -count=1— passedmake verify— failed:hack/update-codegen.shcleanup could not remove repo-local_gotoolchain files on darwin because the downloaded toolchain cache was read-onlyPATH="$(go env GOPATH)/bin:$PATH" make verify— failed: same_gocleanup permission issue after verification steps completedPATH="$(go env GOPATH)/bin:$PATH" GOTOOLCHAIN=local make verify— failed: local non-auto toolchain isgo1.23.12, butgo.modrequires>= 1.26.4make test— passedSpecial notes for your reviewer:
make verifyreached and passed the substantive verification stages relevant to this change, including staticcheck, mock verification, gofmt, vendor, swagger docs, command-line flags, and CRD generation checks, before failing in temporary_gocleanup. No repository files were left modified by that failure.Scope
Does this PR introduce a user-facing change?: