-
Notifications
You must be signed in to change notification settings - Fork 84
Add LWS Labels Extension Plugin for ModelServing Pods #767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,110 @@ | ||||||
| /* | ||||||
| Copyright The Volcano Authors. | ||||||
|
|
||||||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||||||
| you may not use this file except in compliance with the License. | ||||||
| You may obtain a copy of the License at | ||||||
|
|
||||||
| http://www.apache.org/licenses/LICENSE-2.0 | ||||||
|
|
||||||
| Unless required by applicable law or agreed to in writing, software | ||||||
| distributed under the License is distributed on an "AS IS" BASIS, | ||||||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||
| See the License for the specific language governing permissions and | ||||||
| limitations under the License. | ||||||
| */ | ||||||
|
|
||||||
| package plugins | ||||||
|
|
||||||
| import ( | ||||||
| "context" | ||||||
| "fmt" | ||||||
| "strconv" | ||||||
|
|
||||||
| workloadv1alpha1 "github.com/volcano-sh/kthena/pkg/apis/workload/v1alpha1" | ||||||
| "github.com/volcano-sh/kthena/pkg/model-serving-controller/utils" | ||||||
| ) | ||||||
|
|
||||||
| const ( | ||||||
| // LWSLabelsPluginName is the registered name of the LWS labels plugin. | ||||||
| LWSLabelsPluginName = "lws-labels" | ||||||
|
|
||||||
| // Standard LWS label keys as defined by the LeaderWorkerSet API. | ||||||
| LWSLabelName = "leaderworkerset.sigs.k8s.io/name" | ||||||
| LWSLabelGroupIndex = "leaderworkerset.sigs.k8s.io/group-index" | ||||||
| LWSLabelWorkerIndex = "leaderworkerset.sigs.k8s.io/worker-index" | ||||||
| LWSLabelGroupKey = "leaderworkerset.sigs.k8s.io/group-key" | ||||||
| ) | ||||||
|
|
||||||
| // LWSLabelsPlugin injects standard LeaderWorkerSet labels into pods | ||||||
| // created for LWS workloads, enabling compatibility with the LWS | ||||||
| // ecosystem (monitoring, logging, controllers). | ||||||
| type LWSLabelsPlugin struct { | ||||||
| name string | ||||||
| } | ||||||
|
|
||||||
| func init() { | ||||||
| DefaultRegistry.Register(LWSLabelsPluginName, NewLWSLabelsPlugin) | ||||||
| } | ||||||
|
|
||||||
| // NewLWSLabelsPlugin constructs the LWS labels plugin from a PluginSpec. | ||||||
| // This plugin does not require any configuration. | ||||||
| func NewLWSLabelsPlugin(spec workloadv1alpha1.PluginSpec) (Plugin, error) { | ||||||
| return &LWSLabelsPlugin{name: spec.Name}, nil | ||||||
| } | ||||||
|
|
||||||
| func (p *LWSLabelsPlugin) Name() string { return p.name } | ||||||
|
|
||||||
| // OnPodCreate injects the four standard LWS labels into the pod. | ||||||
| // Labels are merged safely: existing user-defined labels are never overwritten. | ||||||
| func (p *LWSLabelsPlugin) OnPodCreate(_ context.Context, req *HookRequest) error { | ||||||
| if req == nil || req.Pod == nil || req.ModelServing == nil { | ||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| // Derive label values from the HookRequest context. | ||||||
| lwsName := req.ModelServing.Name | ||||||
|
|
||||||
| // Extract group index from the serving group name (e.g. "my-lws-0" → "0"). | ||||||
| _, groupIndex := utils.GetParentNameAndOrdinal(req.ServingGroup) | ||||||
| if groupIndex < 0 { | ||||||
| return fmt.Errorf("cannot extract group index from serving group name %q", req.ServingGroup) | ||||||
| } | ||||||
| groupIndexStr := strconv.Itoa(groupIndex) | ||||||
|
|
||||||
| // Extract worker index from the pod name (trailing ordinal, e.g. "my-lws-0-default-0-1" → "1"). | ||||||
| _, workerIndex := utils.GetParentNameAndOrdinal(req.Pod.Name) | ||||||
| if workerIndex < 0 { | ||||||
| return fmt.Errorf("cannot extract worker index from pod name %q", req.Pod.Name) | ||||||
| } | ||||||
|
Comment on lines
+60
to
+79
|
||||||
| workerIndexStr := strconv.Itoa(workerIndex) | ||||||
|
|
||||||
| // Group key uniquely identifies the group within the LWS. | ||||||
| groupKey := fmt.Sprintf("%s-%s", lwsName, groupIndexStr) | ||||||
|
||||||
| groupKey := fmt.Sprintf("%s-%s", lwsName, groupIndexStr) | |
| groupKey := req.ServingGroup |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,211 @@ | ||||
| /* | ||||
| Copyright The Volcano Authors. | ||||
|
|
||||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||||
| you may not use this file except in compliance with the License. | ||||
| You may obtain a copy of the License at | ||||
|
|
||||
| http://www.apache.org/licenses/LICENSE-2.0 | ||||
|
|
||||
| Unless required by applicable law or agreed to in writing, software | ||||
| distributed under the License is distributed on an "AS IS" BASIS, | ||||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||
| See the License for the specific language governing permissions and | ||||
| limitations under the License. | ||||
| */ | ||||
|
|
||||
| package plugins | ||||
|
|
||||
| import ( | ||||
| "context" | ||||
| "testing" | ||||
|
|
||||
| corev1 "k8s.io/api/core/v1" | ||||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||
|
|
||||
| workloadv1alpha1 "github.com/volcano-sh/kthena/pkg/apis/workload/v1alpha1" | ||||
| ) | ||||
|
|
||||
| func TestLWSLabelsPluginOnPodCreate(t *testing.T) { | ||||
| tests := []struct { | ||||
| name string | ||||
| req *HookRequest | ||||
| expectLabels map[string]string | ||||
| expectError bool | ||||
| expectNoChange bool // true if req/pod is nil and no mutation expected | ||||
| }{ | ||||
| { | ||||
| name: "entry pod gets all four LWS labels", | ||||
| req: &HookRequest{ | ||||
| ModelServing: &workloadv1alpha1.ModelServing{ | ||||
| ObjectMeta: metav1.ObjectMeta{Name: "my-lws"}, | ||||
| }, | ||||
| ServingGroup: "my-lws-0", | ||||
| RoleName: "default", | ||||
| RoleID: "default-0", | ||||
| IsEntry: true, | ||||
| Pod: &corev1.Pod{ | ||||
| ObjectMeta: metav1.ObjectMeta{ | ||||
| Name: "my-lws-0-default-0-0", | ||||
| Labels: map[string]string{}, | ||||
| }, | ||||
| }, | ||||
| }, | ||||
| expectLabels: map[string]string{ | ||||
| LWSLabelName: "my-lws", | ||||
| LWSLabelGroupIndex: "0", | ||||
| LWSLabelWorkerIndex: "0", | ||||
| LWSLabelGroupKey: "my-lws-0", | ||||
| }, | ||||
| }, | ||||
| { | ||||
| name: "worker pod gets correct worker index", | ||||
| req: &HookRequest{ | ||||
| ModelServing: &workloadv1alpha1.ModelServing{ | ||||
| ObjectMeta: metav1.ObjectMeta{Name: "my-lws"}, | ||||
| }, | ||||
| ServingGroup: "my-lws-2", | ||||
| RoleName: "default", | ||||
| RoleID: "default-0", | ||||
| IsEntry: false, | ||||
| Pod: &corev1.Pod{ | ||||
| ObjectMeta: metav1.ObjectMeta{ | ||||
| Name: "my-lws-2-default-0-3", | ||||
| Labels: map[string]string{}, | ||||
| }, | ||||
| }, | ||||
| }, | ||||
| expectLabels: map[string]string{ | ||||
| LWSLabelName: "my-lws", | ||||
| LWSLabelGroupIndex: "2", | ||||
| LWSLabelWorkerIndex: "3", | ||||
| LWSLabelGroupKey: "my-lws-2", | ||||
| }, | ||||
| }, | ||||
| { | ||||
| name: "existing user labels are not overwritten", | ||||
| req: &HookRequest{ | ||||
| ModelServing: &workloadv1alpha1.ModelServing{ | ||||
| ObjectMeta: metav1.ObjectMeta{Name: "my-lws"}, | ||||
| }, | ||||
| ServingGroup: "my-lws-0", | ||||
| Pod: &corev1.Pod{ | ||||
| ObjectMeta: metav1.ObjectMeta{ | ||||
| Name: "my-lws-0-default-0-0", | ||||
| Labels: map[string]string{ | ||||
| LWSLabelName: "user-override", | ||||
| LWSLabelGroupIndex: "99", | ||||
| "custom-label": "keep-me", | ||||
| }, | ||||
| }, | ||||
| }, | ||||
| }, | ||||
| expectLabels: map[string]string{ | ||||
| LWSLabelName: "user-override", // preserved | ||||
| LWSLabelGroupIndex: "99", // preserved | ||||
| LWSLabelWorkerIndex: "0", // injected | ||||
| LWSLabelGroupKey: "my-lws-0", // injected | ||||
|
||||
| LWSLabelGroupKey: "my-lws-0", // injected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For more robust test assertions, it's better to compare the entire labels map using reflect.DeepEqual. This ensures that no unexpected labels are added and that the final state of the labels is exactly as expected. The current loop-based check only verifies that a subset of expected labels exist, but it wouldn't catch any extra, erroneously added labels.
Note: You will need to import the reflect package to use this function.
if !reflect.DeepEqual(pod.Labels, tt.expectLabels) {
t.Errorf("labels mismatch.\nGot: %v\nWant: %v", pod.Labels, tt.expectLabels)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As implemented, this plugin only runs when it is explicitly listed in
ModelServing.spec.plugins. The current LWS->ModelServing translation (pkg/model-serving-controller/controller/lws_controller.go:constructModelServing) does not add this plugin, so LWS-originated pods will still miss theleaderworkerset.sigs.k8s.io/*labels by default. Consider wiring this in (e.g., have the LWS controller inject a built-inPluginSpec{Name: "lws-labels", Type: BuiltIn}or add defaulting based on the LWS ownerRef), otherwise the PR won't actually fix #759 as described.