ray-project / kuberay

A toolkit to run Ray applications on Kubernetes
Apache License 2.0
990 stars 330 forks source link

[Refactor][RayCluster] RayClusterHeadPodsAssociationOptions and RayClusterWorkerPodsAssociationOptions #2023

Closed rueian closed 3 months ago

rueian commented 3 months ago

Why are these changes needed?

Introduce the following association functions to centralize many ad-hoc MatchingLabels that are scattered around:

These functions return options of a new type AssociationOptions that can convert the options into either a []client.ListOption or a []client.DeleteAllOfOption so that we can use them like this:

filters := common.RayClusterWorkerPodsAssociationOptions(instance)

r.List(ctx, &corev1.Pod{}, filters.ToListOptions()...)

r.DeleteAllOf(ctx, &corev1.Pod{}, filters.ToDeleteOptions()...)

Checks

rueian commented 3 months ago

Hi @kevin85421,

What do you think about the new AssociationOptions that allows us to convert associations to either a slice of ListOption or DeleteAllOfOption?

I'd like to have your feedback, thanks!

kevin85421 commented 3 months ago

What do you think about the new AssociationOptions that allows us to convert associations to either a slice of ListOption or DeleteAllOfOption?

LGTM

rueian commented 3 months ago

Hi @evalaiyc98, could you also help review this?

evalaiyc98 commented 3 months ago

Sure!

evalaiyc98 commented 3 months ago

I've noticed that the logic used here still resembles the original approach. I believe it would be beneficial to maintain consistency with the previous refactor part. What are your thoughts on this?

https://github.com/ray-project/kuberay/blob/a87f9a6a6d8ead0ffd81dc823f2fd7f38f2af027/ray-operator/controllers/ray/suite_helpers_test.go#L206-L209