DELETE_RAYJOB_CR_AFTER_JOB_FINISHES doesn't work with Configuration API #2260
Conversation
| type RayClientProvider struct { | ||
| configapi.Configuration | ||
| } | ||
|
|
||
| func (r *RayClientProvider) GetDashboardClient(mgr manager.Manager) func() RayDashboardClientInterface { | ||
| return GetRayDashboardClientFunc(mgr, r.UseKubernetesProxy) | ||
| } | ||
|
|
||
| func (r *RayClientProvider) GetHttpProxyClient(mgr manager.Manager) func() RayHttpProxyClientInterface { | ||
| return GetRayHttpProxyClientFunc(mgr, r.UseKubernetesProxy) | ||
| } | ||
|
|
There was a problem hiding this comment.
Why a new type RayClientProvider is introduced here? Is this for refactoring purpose to make sure that the type Configuration has no other methods except for deepcopy? If yes it might be better to put this in a separate PR or at least mentioned this in the PR description.
There was a problem hiding this comment.
This refactor was needed because otherwise in NewRayJobReconciler we pass in two copies of the Configuration, one implementing util.ClientProvider and one containing the actual type. Also as you mentioned, Kubernetes API types should not implement other methods outside standrdard DeepCopy and other generated methods.
Signed-off-by: Andrew Sy Kim <[email protected]>
andrewsykim
force-pushed
the
fix-ray-job-delete
branch
from
5de9dab
to
5f06fa8
Compare
|
@MortalHappiness @kevin85421 should we include this in v1.2? |
|
I am okay to defer this given we don't have much usage of the Configuration API. We can revisit if/when we want to promote it |
|
This PR looks good to me. I think it can be included in v1.2 |
Why are these changes needed?
This is a follow-up to #2225
The
deleteRayJobAfterJobFinishesfield in the Configuration API does not actually work because the controller only checks for environment variables. This PR changes the behavior so that KubeRay checks both environment variables and the Configuration API.Related issue number
Checks