[RayService][Bug] applyServeTargetCapacity: accept int64/float64 for cached target_capacity (#4777)#4778
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9341b20. Configure here.
…cached target_capacity (ray-project#4777) Closes ray-project#4777. `applyServeTargetCapacity` reads the cached `ServeConfigV2` string, unmarshals it via `k8s.io/apimachinery/util/yaml`, and asserts `serveConfig["target_capacity"].(float64)` to skip `UpdateDeployments` when the cached value already matches the goal. `util/yaml` decodes plain integer scalars as `int64` (it routes JSON-compatible input through encoding/json with UseNumber off, and `gopkg.in/yaml.v2` also returns int64 for integer scalars). The `float64` assertion therefore always failed, the idempotency branch was always skipped, and `UpdateDeployments` ran on every reconcile even when the value was already current. Existing `TestReconcileServeTargetCapacity` did not catch this because every case used a cached/goal mismatch (0 → 30 / 60 → 60), so the controller proceeded to the update branch in both the intended-skip and intended-update flows — masking the bug. Fix - New `targetCapacityAsInt32(any) (int32, bool)` helper that accepts int64, float64, int, and int32 and returns int32; rejects nil/string/map/etc. - `applyServeTargetCapacity` now uses the helper for the idempotency check. Tests - New `TestApplyServeTargetCapacity_SkipsUpdateWhenCachedMatchesGoal` — the cached/goal scenario the previous suite never covered. Cached ServeConfigV2 with `target_capacity: 60` and goal 60 must NOT call `UpdateDeployments`. Asserts `fakeDashboard.LastUpdatedConfig` stays empty. - New `TestTargetCapacityAsInt32` — 7 sub-cases covering int64, float64, int, int32, nil, string, map. - Existing `TestReconcileServeTargetCapacity` still passes (unchanged behaviour for cached/goal mismatch paths). Verified locally: all subtests pass with the fix, and reverting just the controller change makes the new `TestApplyServeTargetCapacity_SkipsUpdateWhenCachedMatchesGoal` fail with `Should be empty, but was [123 34 116 97 114 103 101 116 95 99 97 112 97 99 105 116 121 34 58 54 48 125]` — the encoded `{"target_capacity":60}` body that `UpdateDeployments` should never have been called with. Signed-off-by: SAY-5 <say.apm35@gmail.com>
9341b20 to
e4d3f64
Compare
|
Pushed an update that addresses the gosec G115 lint failures from the previous CI run. The two flagged conversions in Now Test Also rebased onto current master to clear the staleness. |
Signed-off-by: SAY-5 <say.apm35@gmail.com>
…versions Signed-off-by: SAY-5 <say.apm35@gmail.com>
Closes #4777.
Why
applyServeTargetCapacityreads the cachedServeConfigV2string,unmarshals it with
k8s.io/apimachinery/util/yaml, and assertsserveConfig["target_capacity"].(float64)so it can skipUpdateDeploymentswhen the cached value already matches the goal.util/yamldecodes plain integer scalars asint64(it routesJSON-compatible input through
encoding/jsonwithUseNumberoff,and
gopkg.in/yaml.v2also returns int64 for integer scalars). Thefloat64assertion therefore always failed for anytarget_capacitywritten as a plain integer, the idempotency branchwas always skipped, and the controller called
UpdateDeploymentsonevery reconcile even when the value was already current.
The existing
TestReconcileServeTargetCapacitydidn't catch thisbecause every case used a cached/goal mismatch (0 → 30 / 60 → 30
etc.), so the controller proceeded to the update branch in both the
intended-skip and intended-update flows — masking the bug entirely.
Fix
targetCapacityAsInt32(any) (int32, bool)helper that acceptsint64,float64,int, andint32, returningint32. Rejectsnil/string/map/ other types.applyServeTargetCapacitynow uses the helper for the idempotencycheck.
Tests
ray-operator/controllers/ray/rayservice_controller_unit_test.go:TestApplyServeTargetCapacity_SkipsUpdateWhenCachedMatchesGoal— the cached/goal scenario the previous suite never covered. Cached
ServeConfigV2 = '{"target_capacity": 60}'and goal 60 must NOTcall
UpdateDeployments. AssertsfakeDashboard.LastUpdatedConfigstays empty.
TestTargetCapacityAsInt32— 7 sub-cases covering int64,float64, int, int32, nil, string, map.
TestReconcileServeTargetCapacitystill passesunchanged.
Verification
go test ./controllers/ray/ -run "TestApplyServeTargetCapacity_SkipsUpdateWhenCachedMatchesGoal|TestTargetCapacityAsInt32|TestReconcileServeTargetCapacity"→ all green.while keeping the test makes the new
TestApplyServeTargetCapacity_SkipsUpdateWhenCachedMatchesGoalfail with
Should be empty, but was [123 34 116 97 ... 54 48 125]— the byte representation of
{"target_capacity":60}thatUpdateDeploymentsshould never have been called with — i.e. thetest catches the regression.