Skip to content

Prevent ModelServing templates from overwriting controller Pod labels - #1593

Open
miantalha45 wants to merge 5 commits into
volcano-sh:mainfrom
miantalha45:fix/modelserving-reserved-template-labels
Open

Prevent ModelServing templates from overwriting controller Pod labels#1593
miantalha45 wants to merge 5 commits into
volcano-sh:mainfrom
miantalha45:fix/modelserving-reserved-template-labels

Conversation

@miantalha45

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

Prevents entryTemplate and workerTemplate metadata from overwriting labels owned by the ModelServing controller.

The webhook now rejects template label keys under modelserving.volcano.sh/. Pod generation also ignores this label namespace as a defensive fallback, keeping controller generated identity, routing, and rollout labels authoritative.

Custom user labels remain supported.

Which issue(s) this PR fixes:

Fixes #1582

Bug evidence (required for bug-related PRs):

Before this change, a valid ModelServing template could set:

entryTemplate:
  metadata:
    labels:
      modelserving.volcano.sh/entry: "false"

The controller first set modelserving.volcano.sh/entry: "true" on the generated Entry Pod, then copied template labels into the same map. The template value overwrote the controller value.

This caused the generated Entry Pod to have:

modelserving.volcano.sh/entry=false

Headless Service selectors use the Entry label to find the Role Entry Pod, so the Service could have no endpoints.

Regression coverage verifies that:

  • Reserved labels in Entry templates are rejected.
  • Reserved labels in Worker templates are rejected.
  • Normal custom labels are accepted.
  • Pod generation preserves controller owned labels.

Tests run:

go test ./pkg/model-serving-controller/utils ./pkg/model-serving-controller/webhook -count=1
go test ./pkg/model-serving-controller/... -count=1

Special notes for your reviewer:

The Pod generation guard protects controller labels if an existing resource, an older client, or an admission bypass supplies a reserved label. The webhook provides clear feedback for new or updated ModelServing resources.

Does this PR introduce a user-facing change?:

ModelServing now rejects Pod template labels in the modelserving.volcano.sh namespace, which is reserved for controller-managed labels.

Signed-off-by: Talha Amjad <myown4500@gmail.com>
Copilot AI lite review requested due to automatic review settings August 12, 2026 17:11

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@miantalha45

Copy link
Copy Markdown
Contributor Author

Signed-off-by: Talha Amjad <myown4500@gmail.com>
Signed-off-by: Talha Amjad <myown4500@gmail.com>
Copilot AI review requested due to automatic review settings August 13, 2026 02:46

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

// ModelServing controller when it creates Pods.
func IsControllerReservedPodLabel(key string) bool {
switch key {
case ModelServingNameLabelKey,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we reserve the entire modelserving.volcano.sh/ prefix instead of enumerating the current labels? Any future controller-managed label will remain user-overridable unless it is also added to this list.

@acsoto

acsoto commented Aug 13, 2026

Copy link
Copy Markdown
Member

The public examples/kthena-router/ModelServing-ds1.5b-pd-disaggregation.yaml example and the matching router-routing.md snippet still set modelserving.volcano.sh/group-name, so the documented flow will now be rejected at admission.

Signed-off-by: Talha Amjad <myown4500@gmail.com>
Copilot AI review requested due to automatic review settings August 13, 2026 08:53

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@miantalha45

Copy link
Copy Markdown
Contributor Author

Thanks for review @acsoto . I reserved the full modelserving.volcano.sh/ prefix so future controller-managed labels are protected automatically.

I migrated the P/D role classification label from modelserving.volcano.sh/rolename to the user-owned role-name label in the E2E fixtures, public example, documentation asset, and router guide. I also removed the user-supplied modelserving.volcano.sh/group-name labels from the ModelServing example.

Comment on lines +158 to +160
if workloadv1alpha1.IsControllerReservedPodLabel(k) {
continue
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Have you considered cluster upgrade scenarios? The code design might discard non-conflicting labels

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

examples/kthena-router/ModelServing-ds1.5b-pd-disaggregation.yaml use modelserving.volcano.sh/rolename

Signed-off-by: Talha Amjad <myown4500@gmail.com>
Copilot AI review requested due to automatic review settings August 16, 2026 09:18

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@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

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.

ModelServing template metadata can overwrite controller-reserved Pod labels

5 participants