Refactor ModelServing tests to use workloadRoleReplicas#867
Refactor ModelServing tests to use workloadRoleReplicas#867volcano-sh-bot merged 1 commit intovolcano-sh:mainfrom
Conversation
Updated createBasicModelServing calls to include workloadRoleReplicas parameter for consistency across tests. Signed-off-by: shuguansheng <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the ModelServing E2E tests by updating the createBasicModelServing helper function to include a workloadRoleReplicas parameter and a default RolloutStrategy. Feedback indicates that the new parameter's behavior is misleading because it is ignored when explicit roles are provided. Additionally, hardcoding a non-standard MaxUnavailable value in a basic helper is discouraged as it may mask issues or make test configurations less transparent.
| return workloadRoleReplicas | ||
| } | ||
|
|
||
| func createBasicModelServing(name string, servingGroupReplicas, workloadRoleReplicas int32, roles ...workload.Role) *workload.ModelServing { |
There was a problem hiding this comment.
The workloadRoleReplicas parameter is only utilized when the roles variadic argument is empty. In several test cases (e.g., TestModelServingServingGroupRecreate at line 306 or TestModelServingControllerManagerRestart at line 1242), a value of 0 is passed but silently ignored because explicit roles are provided. This makes the helper's API misleading.
Consider renaming the parameter to defaultRoleReplicas to clarify that it only applies to the generated default role, or refactoring the helper to use a configuration struct to avoid passing ignored positional arguments.
| RolloutStrategy: &workload.RolloutStrategy{ | ||
| Type: workload.ServingGroupRollingUpdate, | ||
| RollingUpdateConfiguration: &workload.RollingUpdateConfiguration{ | ||
| MaxUnavailable: &intstr.IntOrString{ | ||
| IntVal: 2, // maxUnavailable = 2 | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The createBasicModelServing helper now hardcodes a RolloutStrategy with MaxUnavailable: 2. This is problematic for two reasons:
- It deviates from the API default (
1), meaning almost all tests using this helper are now running with a non-standard configuration that might mask or introduce unexpected behaviors. - It makes tests that specifically verify rolling update constraints (like
TestModelServingRollingUpdateMaxUnavailable) less transparent, as the magic number2is hidden inside the helper rather than being explicit in the test body.
It is recommended to remove this hardcoded strategy from the "Basic" helper. Tests that require a specific MaxUnavailable value should configure it explicitly on the returned object.
There was a problem hiding this comment.
Pull request overview
This PR refactors ModelServing E2E tests to use a helper function (createBasicModelServing) consistently across test cases, aiming to reduce maintenance overhead. The PR adds a new workloadRoleReplicas parameter to the helper function and consolidates duplicated ModelServing creation logic.
Changes:
- Added new
workloadRoleReplicasparameter tocreateBasicModelServingfunction signature to control default role replica counts - Created a new
getWorkloadRoleReplicashelper function that defaults to 1 replica when 0 is passed - Refactored test calls to use
createBasicModelServingconsistently, replacing inline ModelServing definitions - Removed unused
k8s.io/utils/ptrimport - Important behavioral change: Added a default
RolloutStrategywithmaxUnavailable=2to all ModelServings created bycreateBasicModelServing
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RolloutStrategy: &workload.RolloutStrategy{ | ||
| Type: workload.ServingGroupRollingUpdate, | ||
| RollingUpdateConfiguration: &workload.RollingUpdateConfiguration{ | ||
| MaxUnavailable: &intstr.IntOrString{ | ||
| IntVal: 2, // maxUnavailable = 2 | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The addition of RolloutStrategy with maxUnavailable=2 to the createBasicModelServing function changes the behavior of ALL tests using this helper. Previously, only the two rolling update-specific tests explicitly set this strategy. Now it's applied globally. This could unintentionally affect tests that don't expect this behavior, such as TestModelServingLifecycle, TestModelServingScaleUp, TestModelServingScaleDown, etc. The RolloutStrategy might alter scaling and update behavior in ways those tests are not designed to handle. Consider either: (1) removing the RolloutStrategy from createBasicModelServing and applying it only where needed, (2) adding a parameter to control whether the strategy is applied, or (3) documenting why all tests should have this strategy.
| RolloutStrategy: &workload.RolloutStrategy{ | |
| Type: workload.ServingGroupRollingUpdate, | |
| RollingUpdateConfiguration: &workload.RollingUpdateConfiguration{ | |
| MaxUnavailable: &intstr.IntOrString{ | |
| IntVal: 2, // maxUnavailable = 2 | |
| }, | |
| }, | |
| }, |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu, LiZhenCheng9527 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 |
Updated createBasicModelServing calls to include workloadRoleReplicas parameter for consistency across tests.
What type of PR is this?
What this PR does / why we need it:
optimize the createBasicModelServing funcs and use the func consistently in the test case, lower maintenance overhead in the future.
Which issue(s) this PR fixes:
Fixes #859
Special notes for your reviewer:
Does this PR introduce a user-facing change?: