Skip to content

feat(model-serving): support role-scoped network topology policies#1325

Open
vanshika2720 wants to merge 2 commits into
volcano-sh:mainfrom
vanshika2720:feat/role-scoped-network-topology
Open

feat(model-serving): support role-scoped network topology policies#1325
vanshika2720 wants to merge 2 commits into
volcano-sh:mainfrom
vanshika2720:feat/role-scoped-network-topology

Conversation

@vanshika2720

@vanshika2720 vanshika2720 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it

This PR implements the role-scoped network topology design accepted in #1301 and fixes #1278.

Previously, spec.template.networkTopology.rolePolicy was applied to every role within a ModelServing. This prevented heterogeneous deployments (for example, GPU/RDMA inference roles together with CPU-only load-balancer roles) from using different network-topology scheduling behavior.

This change introduces role-level network topology configuration while preserving backward compatibility.

Highlights:

  • Add Role.NetworkTopology for per-role network topology configuration.
  • Introduce a Kthena-owned NetworkTopologySpec adapter type instead of exposing the Volcano scheduler API type directly in the public API.
  • Retype NetworkTopology.GroupPolicy and RolePolicy to use the adapter type without changing the YAML/JSON schema.
  • Resolve network topology precedence as:
    1. Role.NetworkTopology
    2. legacy spec.template.networkTopology.rolePolicy
    3. no topology constraint
  • Convert the adapter type into the Volcano scheduler type only when generating the PodGroup.
  • Reject configurations that specify both Role.NetworkTopology and the legacy rolePolicy using validating webhook checks.
  • Regenerate deepcopy code, CRDs, client-go, and documentation.

Which issue(s) this PR fixes

Fixes #1278

Special notes for your reviewer

This PR follows the design proposal accepted in #1301.

API behavior:

  • Existing users continue to work without changes through the legacy rolePolicy.
  • New users can configure topology policies independently for individual roles.
  • Role.NetworkTopology takes precedence over the legacy rolePolicy.
  • Using both mechanisms simultaneously is rejected by the validating webhook to avoid ambiguous behavior.

Implementation notes:

  • Scheduler-specific types are now isolated at the PodGroup generation boundary.
  • The external YAML/JSON representation remains unchanged for existing fields.
  • Includes regenerated API artifacts and documentation updates.

Validation performed:

  • ✅ gofmt
  • ✅ golangci-lint
  • ✅ unit tests
  • ✅ webhook validation tests
  • ✅ non-e2e test suite

Does this PR introduce a user-facing change?

ModelServing now supports configuring network topology policies per role through `roles[].networkTopology`.

When specified, the role-level policy overrides the legacy `spec.template.networkTopology.rolePolicy`. Existing `rolePolicy` configurations continue to work unchanged for backward compatibility.

Configurations that specify both a role-level policy and the legacy `rolePolicy` are rejected by the validating webhook to prevent ambiguous scheduling behavior.

Copilot AI review requested due to automatic review settings July 8, 2026 14:36
@volcano-sh-bot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign yaozengzeng for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details 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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces role-scoped network topology policies in Kthena, allowing individual roles within a ServingGroup to define their own topology constraints. It decouples Kthena's API from Volcano by introducing a local NetworkTopologySpec struct, updates the controller to resolve role-level policies with backward compatibility, and adds validation to ensure that the legacy rolePolicy and new role-level policies are mutually exclusive. The feedback recommends enhancing API-level and webhook validation to enforce the mutual exclusivity of HighestTierAllowed and HighestTierName within NetworkTopologySpec, along with adding corresponding unit tests.

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.

Comment thread pkg/apis/workload/v1alpha1/servinggroup_types.go
Comment thread pkg/model-serving-controller/webhook/validator.go
Comment thread pkg/model-serving-controller/webhook/validator_test.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Implements role-scoped network topology policies for ModelServing to allow heterogeneous roles to use different topology constraints while keeping the legacy spec.template.networkTopology.rolePolicy behavior for existing manifests.

Changes:

  • Added per-role topology configuration via roles[].networkTopology and introduced a Kthena-owned NetworkTopologySpec adapter type in the workload API.
  • Updated PodGroup generation to resolve network topology per role (role-level first, then legacy rolePolicy) and convert the adapter type to Volcano scheduler types at the boundary.
  • Added validating webhook logic + unit/e2e tests to reject manifests that configure both role-level topology and legacy rolePolicy, plus updated docs/CRDs/client-go artifacts.

Reviewed changes

Copilot reviewed 10 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/e2e/controller-manager/webhook_test.go Adds e2e webhook coverage for rejecting conflicting topology configuration.
test/e2e/controller-manager/model_serving_test.go Adds a helper to build a ModelServing with conflicting topology fields for e2e validation.
pkg/model-serving-controller/webhook/validator.go Enforces mutual exclusion between legacy rolePolicy and role-level roles[].networkTopology.
pkg/model-serving-controller/webhook/validator_test.go Adds unit tests for the new mutual-exclusion validator.
pkg/model-serving-controller/podgroupmanager/manager.go Resolves effective role topology policy and converts adapter type to Volcano scheduler type when building PodGroup/SubGroupPolicy.
pkg/model-serving-controller/podgroupmanager/manager_test.go Updates tests for adapter type + adds cases for role-scoped topology and precedence behavior.
pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go Regenerates deepcopy methods for new/retained API fields and types.
pkg/apis/workload/v1alpha1/servinggroup_types.go Introduces NetworkTopologySpec adapter and adds Role.NetworkTopology field; retypes legacy fields to adapter type.
docs/kthena/docs/user-guide/network-topology.md Documents role-scoped topology policies, precedence rules, and mutual exclusion.
docs/kthena/docs/reference/crd/workload.serving.volcano.sh.md Updates generated CRD reference docs for new fields/types and deprecation notes.
client-go/applyconfiguration/workload/v1alpha1/role.go Adds apply-configuration support for Role.NetworkTopology.
client-go/applyconfiguration/workload/v1alpha1/networktopologyspec.go Adds apply-configuration type/builder for the new NetworkTopologySpec.
client-go/applyconfiguration/workload/v1alpha1/networktopology.go Updates apply-configuration types to use the adapter type instead of Volcano scheduler types.
client-go/applyconfiguration/utils.go Registers apply-configuration mapping for NetworkTopologySpec.
charts/kthena/charts/workload/crds/workload.serving.volcano.sh_modelservings.yaml Regenerates CRD schema to include roles[].networkTopology and updated topology spec typing/docs.
Files not reviewed (5)
  • client-go/applyconfiguration/utils.go: Generated file
  • client-go/applyconfiguration/workload/v1alpha1/networktopology.go: Generated file
  • client-go/applyconfiguration/workload/v1alpha1/networktopologyspec.go: Generated file
  • client-go/applyconfiguration/workload/v1alpha1/role.go: Generated file
  • pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go: Generated file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/apis/workload/v1alpha1/servinggroup_types.go
// HighestTierAllowed specifies the highest tier that a job is allowed to cross when scheduling.
// +kubebuilder:validation:Minimum=0
// +optional
HighestTierAllowed *int32 `json:"highestTierAllowed,omitempty"`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why *int32? In Volcano this filed type is *int

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if role.NetworkTopology != nil {
return role.NetworkTopology
}
if ms.Spec.Template.NetworkTopology != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Don't you check for null pointers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@LiZhenCheng9527 resolveRoleNetworkTopology now guards against both ms == nil and ms.Spec.Template.NetworkTopology == nil before accessing the legacy policy, while still preserving the precedence (Role.NetworkTopology → legacy rolePolicy → none). I also added a unit test covering the nil case to ensure it remains safe.

Add a Role-level NetworkTopology field so heterogeneous roles in a
ModelServing (e.g. GPU/RDMA prefill/decode vs. a CPU-only lb role) can
opt into network-topology-aware scheduling independently, instead of
the ServingGroup-level rolePolicy being applied to every role.

Precedence: a role's own networkTopology wins; otherwise the legacy
spec.template.networkTopology.rolePolicy is applied for backward
compatibility; otherwise no constraint is set. rolePolicy and
roles[*].networkTopology are mutually exclusive and rejected by the
ModelServing validating webhook.

Introduces a Kthena-owned NetworkTopologySpec adapter type so the
ModelServing API no longer re-exports the Volcano scheduler API type
directly; GroupPolicy/RolePolicy are retyped accordingly with no
change to the JSON/YAML shape. Conversion to the scheduler-specific
type happens only at the PodGroup boundary in appendSubGroupPolicy.

Implements the design accepted in volcano-sh#1301 for
volcano-sh#1278.

Signed-off-by: Vanshika <pahalvanshikaa@gmail.com>
…pology

Match Volcano's NetworkTopologySpec.HighestTierAllowed type (*int, not
*int32) exactly in the Kthena adapter type, so no lossy conversion is
needed at the PodGroup conversion boundary.

Make resolveRoleNetworkTopology and syncPodGroupNetworkTopology
explicitly nil-safe for ms and ms.Spec.Template.NetworkTopology,
independent of what today's callers happen to guarantee, matching the
existing nil-guard convention used by extractQueueName in this file.

Add a CEL XValidation rule to NetworkTopologySpec rejecting
highestTierAllowed and highestTierName being set together. This is
enforced identically wherever NetworkTopologySpec is embedded
(groupPolicy, rolePolicy, roles[*].networkTopology), so no duplicate
webhook-side check is added for it.

Regenerate deepcopy, client-go applyconfiguration, CRDs, and CRD
reference docs for the type change and new validation rule.

Signed-off-by: Vanshika <pahalvanshikaa@gmail.com>
Copilot AI review requested due to automatic review settings July 10, 2026 08:27
@vanshika2720 vanshika2720 force-pushed the feat/role-scoped-network-topology branch from c1b8647 to fab9f60 Compare July 10, 2026 08:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 15 changed files in this pull request and generated no new comments.

Files not reviewed (5)
  • client-go/applyconfiguration/utils.go: Generated file
  • client-go/applyconfiguration/workload/v1alpha1/networktopology.go: Generated file
  • client-go/applyconfiguration/workload/v1alpha1/networktopologyspec.go: Generated file
  • client-go/applyconfiguration/workload/v1alpha1/role.go: Generated file
  • pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go: Generated file

@vanshika2720

Copy link
Copy Markdown
Contributor Author

@LiZhenCheng9527 I investigated the failing E2E (controller-manager) check locally. The failure is in TestModelCR/TestModelBoosterSelfHealing timing out while waiting for the Model to become Active, rather than in the code paths modified by this PR. The ModelServing-related tests affected by this change pass locally. From my investigation, this appears unrelated to the role-scoped network topology changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support role-scoped network topology policies in ModelServing

4 participants