[RayCluster] Add serviceName to status.headInfo #2089
Conversation
c03543e to
9062dc8
Compare
|
cc @kevin85421 |
| } else if len(runtimeServices.Items) > 1 { | ||
| return "", fmt.Errorf("found multiple head services. cluster name %s, filter labels %v", instance.Name, filterLabels) | ||
| return "", "", fmt.Errorf("found multiple head services. cluster name %s, filter labels %v", instance.Name, filterLabels) | ||
| } else if runtimeServices.Items[0].Spec.ClusterIP == "" { |
There was a problem hiding this comment.
I am not sure if a headless service’s ClusterIP is "" or "None".
There was a problem hiding this comment.
I think it's "None"
(I'm not changing this in this PR anyways)
| var headSvcName string | ||
|
|
||
| headSvcName = rayCluster.Status.Head.ServiceName | ||
| if headSvcName == "" { |
There was a problem hiding this comment.
Which case will go into this if statement? If it is a canary, we may consider removing it at this moment until we find that there is a case that needs to be handled by this if statement. It may hide the actual root causes deeper.
There was a problem hiding this comment.
I was thinking about the version skew problem if user updates operator before CRD when uprading from v1.1 to v1.2
There was a problem hiding this comment.
We can probably remove it though, just being extra safe here
There was a problem hiding this comment.
I was thinking about the version skew problem if user updates operator before CRD when uprading from v1.1 to v1.2
It makes sense to me. We can keep it and add a warning log when it falls back to this if statement.
There was a problem hiding this comment.
We can keep it and add a warning log when it falls back to this if statement.
This would require wiring in the context so we can get the logger with ctrl.LoggerFrom(ctx). Do we still want the warning?
There was a problem hiding this comment.
I am fine with passing context into this function.
There was a problem hiding this comment.
sounds good, let me incoporate this
There was a problem hiding this comment.
updated to pass context to InitClient and log a message when .status.head.serviceName is empty
9062dc8 to
58e5260
Compare
|
@andrewsykim do you want to include this PR into v1.1.1? |
Probably not because it's not fixing any bugs. |
Signed-off-by: Andrew Sy Kim <andrewsy@google.com>
58e5260 to
94aae5d
Compare
Why are these changes needed?
Add service name to HeadInfo
Related issue number
Checks