Implement extension plugin framework #588
Conversation
Summary of ChangesHello @hzxuzhonghu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request proposes and details a new plugin framework for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
cc @VanderChen @LiZhenCheng9527 @JesseStutler @zl-cheng This is much like volcano job's plugin framework |
![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 introduces a well-structured design proposal for a ModelServing plugin framework. The proposal is comprehensive, covering motivation, API changes, hook points, and implementation details. My review focuses on improving the clarity and consistency of the proposed API, ensuring that the design is robust and easy for developers to understand and use. The feedback includes suggestions to formally document all API fields shown in examples, clarify ambiguous fields, resolve inconsistencies in function signatures, and enhance the context provided to plugins.
| - `name` (string) – plugin identifier | ||
| - `type` (string) – plugin kind (e.g., `BuiltIn`, `Webhook`), to allow future extension | ||
| - `config` (raw JSON) – plugin-specific configuration (opaque to the API server) | ||
| - `scope` (optional) – apply to entry/worker, roles, or all |
There was a problem hiding this comment.
The scope field is a great addition for targeting plugins. To improve clarity, it would be beneficial to better define its possible values and behavior. For example:
- What are the exact enum values? e.g.,
All,Role,PodType. - If
scopeisRole, how does a user specify which roles to apply the plugin to? Perhapsscopecould be an object likescope: { roles: ["role-a", "role-b"] }. - What is the distinction between
entry/workerandroles? Doesentry/workerapply to all roles but only to specific pod types within them?
Clarifying these aspects will make the feature easier to understand and use correctly.
| gpuCount: 1 | ||
| - name: huawei-ascend-defaults | ||
| type: BuiltIn | ||
| disabled: true |
There was a problem hiding this comment.
The example YAML includes a disabled: true field, which is a very useful feature for toggling plugins without removing them from the spec. However, this field is not mentioned in the API changes description (lines 65-69). It should be formally documented as part of the plugin API to ensure it is a supported and well-understood feature.
There was a problem hiding this comment.
good catch, will remove
|
|
||
| Initial hook points: | ||
|
|
||
| - `OnPodCreate(ctx, req) -> (podMutations, error)` |
There was a problem hiding this comment.
There's a discrepancy in the described signature for the OnPodCreate hook between this section and the Go interface draft.
- Here on line 106, the signature is
-> (podMutations, error), suggesting it returns a patch. - In the Go interface draft on line 194, it's
... error, implying in-place mutation of thePodobject within theHookRequest.
For built-in plugins, in-place mutation is generally more straightforward. It would be good to clarify this. For instance, you could state that the (podMutations, error) return signature is intended for future remote plugins (like webhooks), while built-in plugins perform in-place mutations. Please update the documentation to be consistent.
There was a problem hiding this comment.
Pull request overview
This PR introduces a design proposal for a ModelServing plugin framework that enables users to customize Pods generated by the ModelServing controller for different inference engines and accelerators through an extensible plugin mechanism.
Key changes:
- Proposes a new
spec.pluginsfield in ModelServing API for configuring optional plugins - Defines hook points (
OnPodCreate,OnPodReady) for plugin lifecycle integration - Outlines implementation strategy with built-in plugins as the initial approach
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hzxuzhonghu
force-pushed
the
add-extension-proposal
branch
from
4320be5 to
6f07111
Compare
hzxuzhonghu
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/model-serving-controller/controller/model_serving_controller.go
Outdated
Show resolved
Hide resolved
pkg/model-serving-controller/controller/model_serving_controller.go
Outdated
Show resolved
Hide resolved
pkg/model-serving-controller/controller/model_serving_controller.go
Outdated
Show resolved
Hide resolved
pkg/model-serving-controller/controller/model_serving_controller.go
Outdated
Show resolved
Hide resolved
|
ping @VanderChen @LiZhenCheng9527 @JesseStutler @zl-cheng ptal |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
pkg/model-serving-controller/controller/model_serving_controller.go:1238
- The controller integration for plugin hooks lacks test coverage. While the plugin manager itself has comprehensive unit tests in manager_test.go, there are no tests validating that the controller correctly calls the plugin hooks during pod creation (CreatePodsByRole) and readiness (handleReadyPod). Tests should verify that plugins are invoked at the correct lifecycle points and that errors are properly handled.
func (c *ModelServingController) CreatePodsByRole(ctx context.Context, role workloadv1alpha1.Role, mi *workloadv1alpha1.ModelServing, roleIndex int, servingGroupOrdinal int, revision string) error {
servingGroupName := utils.GenerateServingGroupName(mi.Name, servingGroupOrdinal)
// TODO: build the plugin chain only once for the ModelServing
chain, err := c.buildPluginChain(mi)
if err != nil {
return fmt.Errorf("build plugin chain: %w", err)
}
roleID := utils.GenerateRoleID(role.Name, roleIndex)
entryPod := utils.GenerateEntryPod(role, mi, servingGroupName, roleIndex, revision)
taskName := c.gangManager.GenerateTaskName(role.Name, roleIndex)
c.gangManager.AnnotatePodWithPodGroup(entryPod, mi, servingGroupName, taskName)
entryReq := &plugins.HookRequest{
ModelServing: mi,
ServingGroup: servingGroupName,
RoleName: role.Name,
RoleID: roleID,
IsEntry: true,
Pod: entryPod,
}
if err := chain.OnPodCreate(ctx, entryReq); err != nil {
return fmt.Errorf("execute OnPodCreate failed for entry pod %s: %v", entryPod.Name, err)
}
_, err = c.kubeClientSet.CoreV1().Pods(mi.Namespace).Create(ctx, entryPod, metav1.CreateOptions{})
if err != nil && !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("failed to create entry pod %s: %v", entryPod.Name, err)
}
serviceSelector := map[string]string{
workloadv1alpha1.GroupNameLabelKey: servingGroupName,
workloadv1alpha1.RoleLabelKey: role.Name,
workloadv1alpha1.RoleIDKey: utils.GenerateRoleID(role.Name, roleIndex),
workloadv1alpha1.EntryLabelKey: utils.Entry,
}
if role.WorkerTemplate != nil {
if err := utils.CreateHeadlessService(ctx, c.kubeClientSet, mi, serviceSelector, servingGroupName, role.Name, roleIndex); err != nil {
return fmt.Errorf("failed to create headless service: %v", err)
}
}
for i := 1; i <= int(role.WorkerReplicas); i++ {
workerPod := utils.GenerateWorkerPod(role, mi, entryPod, servingGroupName, roleIndex, i, revision)
c.gangManager.AnnotatePodWithPodGroup(workerPod, mi, servingGroupName, taskName)
workerReq := &plugins.HookRequest{
ModelServing: mi,
ServingGroup: servingGroupName,
RoleName: role.Name,
RoleID: roleID,
IsEntry: false,
Pod: workerPod,
}
if err := chain.OnPodCreate(ctx, workerReq); err != nil {
return fmt.Errorf("execute OnPodCreate failed for worker pod %s: %v", workerPod.Name, err)
}
_, err := c.kubeClientSet.CoreV1().Pods(mi.Namespace).Create(ctx, workerPod, metav1.CreateOptions{})
if err != nil && !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("failed to create worker pod %s: %v", workerPod.Name, err)
}
}
return nil
pkg/model-serving-controller/controller/model_serving_controller.go:790
- The controller integration for plugin hooks lacks test coverage. While the plugin manager itself has comprehensive unit tests, there are no tests validating that the controller correctly calls the plugin ready hooks and handles errors appropriately. Tests should verify the hook invocation and error propagation in the ready pod handling flow.
func (c *ModelServingController) handleReadyPod(mi *workloadv1alpha1.ModelServing, servingGroupName string, newPod *corev1.Pod) error {
chain, err := c.buildPluginChain(mi)
if err != nil {
return fmt.Errorf("build plugin chain: %w", err)
}
if chain != nil {
if err := chain.OnPodReady(context.TODO(), &plugins.HookRequest{
ModelServing: mi,
ServingGroup: servingGroupName,
RoleName: utils.PodRoleName(newPod),
RoleID: utils.PodRoleID(newPod),
IsEntry: newPod.Labels[workloadv1alpha1.EntryLabelKey] == utils.Entry,
Pod: newPod,
}); err != nil {
return err
}
}
// Add the running pod to the global storage and try to update the ServingGroup status
c.store.AddRunningPodToServingGroup(types.NamespacedName{
Namespace: mi.Namespace,
Name: mi.Name,
}, servingGroupName, newPod.Name, utils.PodRevision(newPod), utils.PodRoleName(newPod), utils.PodRoleID(newPod))
ready, err := c.checkServingGroupReady(mi, servingGroupName)
if err != nil {
return fmt.Errorf("failed to check ServingGroup status, err: %v", err)
}
if ready {
// All pods in the ServingGroup are running, so the ServingGroup status also needs to be set to running
err = c.store.UpdateServingGroupStatus(utils.GetNamespaceName(mi), servingGroupName, datastore.ServingGroupRunning)
if err != nil {
return fmt.Errorf("failed to set ServingGroup %s status: %v", servingGroupName, err)
}
klog.V(2).Infof("Update ServingGroup %s status to Running", servingGroupName)
c.enqueueModelServing(mi)
} else {
klog.V(4).Infof("ServingGroup %s still creating", servingGroupName)
}
return nil
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/model-serving-controller/controller/model_serving_controller.go
Outdated
Show resolved
Hide resolved
pkg/model-serving-controller/controller/model_serving_controller.go
Outdated
Show resolved
Hide resolved
| return fmt.Errorf("build plugin chain: %w", err) | ||
| } | ||
| if chain != nil { | ||
| if err := chain.OnPodReady(context.TODO(), &plugins.HookRequest{ |
There was a problem hiding this comment.
Using context.TODO() is inappropriate here. The parent context (ctx) should be passed through to maintain proper context propagation, cancellation signals, and deadline handling. This is especially important for plugin hooks that may perform external operations.
| if err := chain.OnPodReady(context.TODO(), &plugins.HookRequest{ | |
| if err := chain.OnPodReady(context.Background(), &plugins.HookRequest{ |
|
|
||
| // Plugins defines optional plugin chain to customize serving pods. | ||
| // +optional | ||
| Plugins []PluginSpec `json:"plugins,omitempty"` |
There was a problem hiding this comment.
nit: Should we create a separate plugin.go file to store plugin-related definitions? I believe this approach would be more concise and easier to extend.
|
|
||
| // OnPodCreate executes plugins in order. Mutations are applied to req.Pod. | ||
| func (c *Chain) OnPodCreate(ctx context.Context, req *HookRequest) error { | ||
| if c == nil { |
There was a problem hiding this comment.
If c == nil, a panic will occur when this method is called.
Therefore, I consider the inspection here unnecessary.
There was a problem hiding this comment.
Nope, if c is nil, calling c.OnPodCreate will not panic. But accessing its field <c.fieldA> will panic
|
|
||
| // OnPodReady executes plugins' ready hooks in order. | ||
| func (c *Chain) OnPodReady(ctx context.Context, req *HookRequest) error { | ||
| if c == nil { |
hzxuzhonghu
force-pushed
the
add-extension-proposal
branch
from
55d4548 to
3b5c611
Compare
|
@VanderChen @LiZhenCheng9527 @zl-cheng any other comments? |
| RoleName string | ||
| RoleID string | ||
| IsEntry bool | ||
| Pod *corev1.Pod |
There was a problem hiding this comment.
I have a question. The Pods specified here are the ones waiting to be processed. Is "isEntry" used to handle different situations? Can we either specify specific Pods for processing or specify entryPods or WorkerPods of a particular role.
|
/lgtm |
|
Is there any update on this PR? |
|
Will resolve conflict today |
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
hzxuzhonghu
force-pushed
the
add-extension-proposal
branch
from
3b5c611 to
cd5abe8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/model-serving-controller/controller/model_serving_controller.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
3 participants
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
Which issue(s) this PR fixes:
For #563
Special notes for your reviewer:
Does this PR introduce a user-facing change?: