-
-
Notifications
You must be signed in to change notification settings - Fork 44
Fix watch for changes to LeaderWorkerSet created by llmaz and trigger a Reconcile for the owner #108
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
Conversation
|
/cc @kerthcet |
|
/kind bug |
| return ctrl.NewControllerManagedBy(mgr). | ||
| For(&inferenceapi.Playground{}). | ||
| Watches(&inferenceapi.Service{}, &handler.EnqueueRequestForObject{}, | ||
| Watches(&inferenceapi.Service{}, handler.EnqueueRequestForOwner(r.Scheme, r.RESTMapper(), &inferenceapi.Playground{}, handler.OnlyControllerOwner()), |
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.
Inference service can be created manually, so maybe this is not right? Playground is for quick start, once people has forked inference backend, like vllm-on-ascend-chip, playground couldn't support such backends, but inference could.
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.
If the inference service is manually created by users or external controllers, Can the playground own the inference service?
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.
Then there's no Playground, so
- Playground is for quick start, it's optional, if not found, skip.
- Inference Service is for advanced deployment, it's the real service.
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.
- A user (Bob) creates an Inference Service (foo)
- Another user (Li) doesn't know the foo service already exists, then creates a Playground with the same name.
- The service foo will be owned by the Playground, and the service foo may be deleted by Li via the Playground.
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.
We may need to handle this case when reconcile Playground
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.
Good catch, then the Playground is illegal, but seems we can't block the Playground creation, not supported in webhook, we may need to add another condition in Playground to report the error, what do you think?
condition := metav1.Condition{
Type: inferenceapi.PlaygroundProgressing,
Status: metav1.ConditionFalse,
Reason: "AbortProcessing",
Message: "Playground owns the same name with a Service",
}
77a6977 to
9469714
Compare
9469714 to
bc74b66
Compare
| service := &inferenceapi.Service{} | ||
| if err := r.Get(ctx, types.NamespacedName{Name: playground.Name, Namespace: playground.Namespace}, service); err == nil { | ||
| if !controllerutil.HasControllerReference(service) { | ||
| condition := metav1.Condition{ |
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.
We should collect all the condition handlers to one place just like setPlaygroundCondition but this can be a follow up when implementing the Pending status for waiting for model creation.
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.
ok
|
/lgtm Thanks! |
|
Seems we have a test failure. |
bc74b66 to
2216220
Compare
|
/hold For review |
…Reconcile for the owner
2216220 to
fa158df
Compare
|
/hold cancel |
| func setPlaygroundCondition(playground *inferenceapi.Playground, service *inferenceapi.Service) { | ||
| // For the start up. | ||
| if len(playground.Status.Conditions) == 0 { | ||
| if len(playground.Status.Conditions) == 0 || !apimeta.IsStatusConditionTrue(service.Status.Conditions, inferenceapi.PlaygroundProgressing) { |
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.
This should also be refactored once introducing the pending for model creation condition. Because not progressing doesn't mean it's waiting for inferenceService ready.
|
/lgtm We need a followup to address the concerns then. |
What this PR does / why we need it
If an object is not created by llmaz controllers, we shouldn't handle it.
Which issue(s) this PR fixes
Fixes #
Special notes for your reviewer
Does this PR introduce a user-facing change?