[Refactor][RayCluster] RayClusterHeadPodsAssociationOptions and RayClusterWorkerPodsAssociationOptions #2023

rueian · 2024-03-15T16:21:12Z

Why are these changes needed?

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

RayClusterHeadPodsAssociationOptions
RayClusterWorkerPodsAssociationOptions
RayClusterGroupPodsAssociationOptions
RayClusterAllPodsAssociationOptions

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

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- This PR is not tested :(

…usterWorkerPodsAssociationOptions

…RayClusterWorkerPodsAssociationOptions

rueian · 2024-03-16T03:11:05Z

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 · 2024-03-16T06:07:20Z

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

LGTM

rueian · 2024-03-18T12:47:51Z

Hi @evalaiyc98, could you also help review this?

evalaiyc98 · 2024-03-18T13:06:49Z

Sure!

kevin85421 · 2024-03-19T08:49:20Z

ray-operator/controllers/ray/common/association.go

@@ -33,6 +33,66 @@ func RayClusterHeadlessServiceListOptions(instance *rayv1.RayCluster) []client.L
 	}
 }

+type AssociationOption interface {


not important: maybe we can use generic.

kevin85421 · 2024-03-19T09:00:00Z

ray-operator/controllers/ray/common/association_test.go

@@ -149,6 +149,113 @@ func TestRayClusterHeadlessServiceListOptions(t *testing.T) {
 	}
 }

+func TestRayClusterHeadPodsListOptions(t *testing.T) {


The tests can't ensure that the functions work as expected. It is very easy to forget to update listOptions below if we update labels. How about create a test in raycluster_controller_test.go to test whether the functions can get the correct number of Pods? The test will create a RayCluster with 2 worker groups so that we can test the four association functions above.

Instead of creating a new test, how about replacing the usages of MatchingLabels with the new AssociationOption in existing tests? I found the existing tests can cover all the four association functions above.

We should use the new AssociationOption to associate anything in the codebase (not necessary to include all of them in this PR).

I found the existing tests can cover all the four association functions above.

Which existing tests are you referring to? Do you mean envtest or others? I think envtest is necessary.

Yes, the existing envtests in the raycluster_controller_test.go.
I refactored them in the latest commit 8e7a7c3

…oller_test with AssociationOptions

kevin85421 · 2024-03-21T14:51:19Z

ray-operator/controllers/ray/raycluster_controller_test.go

-		workerFilterLabels := client.MatchingLabels{utils.RayClusterLabelKey: rayCluster.Name, utils.RayNodeGroupLabelKey: rayCluster.Spec.WorkerGroupSpecs[0].GroupName}
-		headFilterLabels := client.MatchingLabels{utils.RayClusterLabelKey: rayCluster.Name, utils.RayNodeGroupLabelKey: utils.RayNodeHeadGroupLabelValue}
+		workerFilters := common.RayClusterGroupPodsAssociationOptions(rayCluster, rayCluster.Spec.WorkerGroupSpecs[0].GroupName).ToListOptions()
+		headGroupFilters := common.RayClusterGroupPodsAssociationOptions(rayCluster, utils.RayNodeHeadGroupLabelValue).ToListOptions()


Why do we need to have both headGroupFilters and headFilters? The primary goal of this refactoring is to maintain consistency in the association method by always using the same function to associate two Kubernetes resources. If there is no special reason, we should remove headGroupFilters.

Hi @kevin85421, actually the original headFilterLabels were equivalent to headGroupFilters. The headFilters are newcomers.

I found that the ray.io/group is still needed for a head Pod due to this:

kuberay/ray-operator/controllers/ray/common/pod.go

Lines 585 to 593 in a87f9a6

// RAY_NODE_TYPE_NAME is used by Ray Autoscaler V2 (alpha). See https://github.com/ray-project/kuberay/issues/1965 for more details.

nodeGroupNameEnv := corev1.EnvVar{

Name: utils.RAY_NODE_TYPE_NAME,

ValueFrom: &corev1.EnvVarSource{

FieldRef: &corev1.ObjectFieldSelector{

FieldPath: fmt.Sprintf("metadata.labels['%s']", utils.RayNodeGroupLabelKey),

},

},

}

Shouldn't we still test ray.io/group for a head Pod? We can leave a warning that we should normally use headFilters instead of headGroupFilters to do associations. The headGroupFilters are only meant to test the ray.io/group=headgroup.

Shouldn't we still test ray.io/group for a head Pod?

We have already had unit tests for ray.io/group. It's better to remove headGroupFilters from the env tests.

evalaiyc98 · 2024-03-21T15:21:45Z

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?

kuberay/ray-operator/controllers/ray/suite_helpers_test.go

Lines 206 to 209 in a87f9a6

    
           headFilterLabels := client.MatchingLabels{ 
        
           	utils.RayClusterLabelKey:  rayClusterName, 
        
           	utils.RayNodeTypeLabelKey: string(rayv1.HeadNode), 
        
           }

…er_test.go

…usterWorkerPodsAssociationOptions (ray-project#2023)

…ons and RayClusterWorkerPodsAssociationOptions (#2023) (#2035)

rueian force-pushed the refactor-raycluster-association-options branch 2 times, most recently from 4870479 to f073326 Compare March 15, 2024 17:12

[Refactor][RayCluster] RayClusterHeadPodsAssociationOptions and RayCl…

6b8f9e6

…usterWorkerPodsAssociationOptions

rueian force-pushed the refactor-raycluster-association-options branch from f073326 to 6b8f9e6 Compare March 15, 2024 17:16

[Refactor][RayCluster] test RayClusterHeadPodsAssociationOptions and …

70596d7

…RayClusterWorkerPodsAssociationOptions

rueian marked this pull request as ready for review March 16, 2024 03:08

kevin85421 self-requested a review March 16, 2024 06:07

kevin85421 self-assigned this Mar 16, 2024

kevin85421 reviewed Mar 19, 2024

View reviewed changes

rueian added 2 commits March 20, 2024 20:18

[Refactor][RayCluster] replace all MatchingLabels in raycluster_contr…

8e7a7c3

…oller_test with AssociationOptions

[Refactor][RayCluster] remove naive AssociationOptions testing

cf9eaa0

kevin85421 reviewed Mar 21, 2024

View reviewed changes

[Refactor][RayCluster] remove headGroupFilters in raycluster_controll…

b5bee80

…er_test.go

kevin85421 approved these changes Mar 21, 2024

View reviewed changes

kevin85421 merged commit 83f8dcc into ray-project:master Mar 21, 2024
23 checks passed

kevin85421 pushed a commit to kevin85421/kuberay that referenced this pull request Mar 22, 2024

[Refactor][RayCluster] RayClusterHeadPodsAssociationOptions and RayCl…

f06e3a6

…usterWorkerPodsAssociationOptions (ray-project#2023)

kevin85421 added a commit that referenced this pull request Mar 22, 2024

[Cherry-pick][Refactor][RayCluster] RayClusterHeadPodsAssociationOpti…

32f56c3

…ons and RayClusterWorkerPodsAssociationOptions (#2023) (#2035)

rueian mentioned this pull request Mar 26, 2024

[Refactor] Move MatchingLabels to association.go #2045

Open

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor][RayCluster] RayClusterHeadPodsAssociationOptions and RayClusterWorkerPodsAssociationOptions #2023

[Refactor][RayCluster] RayClusterHeadPodsAssociationOptions and RayClusterWorkerPodsAssociationOptions #2023

rueian commented Mar 15, 2024 •

edited

rueian commented Mar 16, 2024 •

edited

kevin85421 commented Mar 16, 2024

rueian commented Mar 18, 2024

evalaiyc98 commented Mar 18, 2024

kevin85421 Mar 19, 2024

kevin85421 Mar 19, 2024

rueian Mar 21, 2024 •

edited

kevin85421 Mar 21, 2024

rueian Mar 21, 2024

kevin85421 Mar 21, 2024

kevin85421 Mar 21, 2024

rueian Mar 21, 2024

kevin85421 Mar 21, 2024

evalaiyc98 commented Mar 21, 2024

	// RAY_NODE_TYPE_NAME is used by Ray Autoscaler V2 (alpha). See https://github.com/ray-project/kuberay/issues/1965 for more details.
	nodeGroupNameEnv := corev1.EnvVar{
	Name: utils.RAY_NODE_TYPE_NAME,
	ValueFrom: &corev1.EnvVarSource{
	FieldRef: &corev1.ObjectFieldSelector{
	FieldPath: fmt.Sprintf("metadata.labels['%s']", utils.RayNodeGroupLabelKey),
	},
	},
	}

[Refactor][RayCluster] RayClusterHeadPodsAssociationOptions and RayClusterWorkerPodsAssociationOptions #2023

[Refactor][RayCluster] RayClusterHeadPodsAssociationOptions and RayClusterWorkerPodsAssociationOptions #2023

Conversation

rueian commented Mar 15, 2024 • edited

Why are these changes needed?

Checks

rueian commented Mar 16, 2024 • edited

kevin85421 commented Mar 16, 2024

rueian commented Mar 18, 2024

evalaiyc98 commented Mar 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rueian Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evalaiyc98 commented Mar 21, 2024

rueian commented Mar 15, 2024 •

edited

rueian commented Mar 16, 2024 •

edited

rueian Mar 21, 2024 •

edited