MCPServer: allow default image injection (npx/uvx) and set Healthy wh… #85
Conversation
intojhanurag
force-pushed
the
fix/mcpserver-default-image-health
branch
from
565157d to
4394a4c
Compare
|
Hey @jmhbh @rinormaloku , this is ready for review |
jmhbh
left a comment
jmhbh
left a comment
There was a problem hiding this comment.
Hey @intojhanurag sorry for the super late review. This slipped through the cracks for me.
| log.FromContext(ctx).Info("MCPServer deployment not found", "name", server.Name) | ||
| setReadyCondition(server, false, kagentdevv1alpha1.MCPServerReasonPodsNotReady, "Deployment not found") | ||
| } else { | ||
| log.FromContext(ctx).Error(err, "Error getting MCPServer deployment", "name", server.Name) |
There was a problem hiding this comment.
This log feels unnecessary given that we already write the error to the status
| deploymentName := server.Name | ||
| if err := r.Get(ctx, client.ObjectKey{Name: deploymentName, Namespace: server.Namespace}, deployment); err != nil { | ||
| if client.IgnoreNotFound(err) == nil { | ||
| log.FromContext(ctx).Info("MCPServer deployment not found", "name", server.Name) |
There was a problem hiding this comment.
This log feels unnecessary given that we already write the error to the status
| // Check if deployment is available | ||
| // A deployment is considered ready when it has the desired number of available replicas | ||
| if deployment.Status.AvailableReplicas > 0 && deployment.Status.AvailableReplicas == deployment.Status.Replicas { | ||
| log.FromContext(ctx).Info("MCPServer status transitioned to Healthy", |
| } else { | ||
| message := fmt.Sprintf("Deployment not ready: %d/%d replicas available", | ||
| deployment.Status.AvailableReplicas, deployment.Status.Replicas) | ||
| log.FromContext(ctx).Info("MCPServer deployment not ready", |
| server *v1alpha1.MCPServer, | ||
| ) (*appsv1.Deployment, error) { | ||
| image := server.Spec.Deployment.Image | ||
| originalImage := image |
There was a problem hiding this comment.
just curious what the original intention with this change is? If this was a leftover change I think we can remove this and the conditional for this variable.
There was a problem hiding this comment.
The originalImage variable is used to detect if an image was explicitly provided by the user or if a default was injected for uvx/npx. This allows the logs to show when a default image is being used, which is useful for debugging. Btw I removed this part.
|
|
||
| // Set Accepted condition based on validation | ||
| if err := r.validateMCPServer(server); err != nil { | ||
| log.FromContext(ctx).Info("MCPServer validation failed", "name", server.Name, "error", err.Error()) |
There was a problem hiding this comment.
I think all the logs added here are not necessary as they are all reflected in the MCPServer status so I would remove them.
|
|
||
| // Set Programmed condition based on reconcile result | ||
| if reconcileErr != nil { | ||
| log.FromContext(ctx).Error(reconcileErr, "MCPServer reconcile failed", "name", server.Name) |
| "Configuration validation failed", | ||
| ) | ||
| } else { | ||
| log.FromContext(ctx).Info("MCPServer validation passed", "name", server.Name) |
| "Resources failed to be created", | ||
| ) | ||
| } else { | ||
| log.FromContext(ctx).Info("MCPServer reconcile succeeded", "name", server.Name) |
intojhanurag
force-pushed
the
fix/mcpserver-default-image-health
branch
from
7b76718 to
9e0a08a
Compare
…en pods ready; add logs Signed-off-by: Anurag Ojha <aojharaj2004@gmail.com>
intojhanurag
force-pushed
the
fix/mcpserver-default-image-health
branch
from
9e0a08a to
dfa3a59
Compare
|
Hey @jmhbh , I applied the all suggestions . Feel free to take a look if you have a moment. |
|
@intojhanurag Thanks! looks good please remove the kind binary and then I'll merge this. |
Signed-off-by: Anurag Ojha <aojharaj2004@gmail.com>
intojhanurag
force-pushed
the
fix/mcpserver-default-image-health
branch
from
f33032c to
fba8f91
Compare
|
Hey @jmhbh , Feel free to take a look . I removed the binary file. |
|
thanks for your contribution @intojhanurag ! |
2 participants
This PR fixes #76 a bug where MCPServer resources that use cmd: npx (or uvx) stay in Unhealthy state unless spec.deployment.image is explicitly set.
Now, the controller correctly injects a default image (node:24-alpine3.21) during translation, and the status is updated to Healthy.