Move matching labels to association.go #2734

owenowenisme · 2025-01-13T00:58:10Z

Why are these changes needed?

#2733

Related issue number

Closes #2733

Checks

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

owenowenisme · 2025-01-13T01:31:25Z

@rueian please take a look🙏🙏🙏

rueian · 2025-01-13T01:43:50Z

Hi @owenowenisme,

I noticed that there are RayClusterHeadPodsAssociationOptions and RayClusterWorkerPodsAssociationOptions in the association.go. Could you reuse them?

rueian · 2025-01-13T19:02:37Z

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

@@ -100,6 +100,16 @@ func RayClusterWorkerPodsAssociationOptions(instance *rayv1.RayCluster) Associat
 	}
 }

+func RayClusterRedisPodsAssociationOptions(instance *rayv1.RayCluster) AssociationOptions {


Could you add a test for this new method?

kevin85421 · 2025-01-15T00:26:56Z

ray-operator/controllers/ray/raycluster_controller.go

-		utils.RayNodeTypeLabelKey: "head",
-	}
-	if err := r.List(ctx, &rayHeadSvc, client.InNamespace(instance.Namespace), filterLabels); err != nil {
+	filterLabels := common.RayClusterAllPodsAssociationOptions(instance).ToListOptions()


Use RayClusterHeadServiceListOptions instead?

kevin85421 · 2025-01-15T00:30:21Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -544,9 +544,9 @@ func (r *RayClusterReconciler) reconcileIngressKubernetes(ctx context.Context, i
 func (r *RayClusterReconciler) reconcileHeadService(ctx context.Context, instance *rayv1.RayCluster) error {
 	logger := ctrl.LoggerFrom(ctx)
 	services := corev1.ServiceList{}
-	filterLabels := client.MatchingLabels{utils.RayClusterLabelKey: instance.Name, utils.RayNodeTypeLabelKey: string(rayv1.HeadNode)}
+	filterLabels := common.RayClusterHeadPodsAssociationOptions(instance).ToListOptions()


Suggested change

filterLabels := common.RayClusterHeadPodsAssociationOptions(instance).ToListOptions()

filterLabels := common.RayClusterHeadServiceListOptions(instance).ToListOptions()

kevin85421 · 2025-01-15T00:35:03Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -286,9 +286,9 @@ func (r *RayClusterReconciler) rayClusterReconcile(ctx context.Context, instance
 			}

 			// We can start the Redis cleanup process now because the head Pod has been terminated.
-			filterLabels := client.MatchingLabels{utils.RayClusterLabelKey: instance.Name, utils.RayNodeTypeLabelKey: string(rayv1.RedisCleanupNode)}
+			filterLabels := common.RayClusterRedisPodsAssociationOptions(instance).ToListOptions()


It is used to associate a K8s Job instead of Pod.

It is not a Redis Pod.

Please rename to RayClusterRedisCleanupJobAssociationOptions

kevin85421 · 2025-01-15T00:37:36Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -481,8 +481,8 @@ func (r *RayClusterReconciler) reconcileIngress(ctx context.Context, instance *r
 func (r *RayClusterReconciler) reconcileRouteOpenShift(ctx context.Context, instance *rayv1.RayCluster) error {
 	logger := ctrl.LoggerFrom(ctx)
 	headRoutes := routev1.RouteList{}
-	filterLabels := client.MatchingLabels{utils.RayClusterLabelKey: instance.Name}
-	if err := r.List(ctx, &headRoutes, client.InNamespace(instance.Namespace), filterLabels); err != nil {
+	filterLabels := common.RayClusterAllPodsAssociationOptions(instance).ToListOptions()


It's misleading to use RayClusterAllPodsAssociationOptions to associate K8s ingress / route. Please create a new function RayClusterNetworkResourcesOptions for both ingress and route instead.

rueian · 2025-01-15T05:55:12Z

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

@@ -194,3 +194,12 @@ func GetRayClusterHeadPod(ctx context.Context, reader client.Reader, instance *r
 	}
 	return &runtimePods.Items[0], nil
 }
+
+func RayClusterNetworkResourcesOptions(instance *rayv1.RayCluster) AssociationOptions {


Since the new function is created, please add a test for it.

rueian · 2025-01-16T19:12:23Z

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

@@ -292,3 +293,77 @@ func TestGetRayClusterHeadPod(t *testing.T) {
 	assert.Nil(t, err)
 	assert.Equal(t, ret, headPod)
 }
+
+func TestRayClusterRedisPodsAssociationOptions(t *testing.T) {


Suggested change

func TestRayClusterRedisPodsAssociationOptions(t *testing.T) {

func TestRayClusterRedisCleanupJobAssociationOptions(t *testing.T) {

rueian · 2025-01-16T19:12:56Z

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

+
+	_ = &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "redis-pod",


Suggested change

Name: "redis-pod",

Name: "redis-cleanup",

owenowenisme · 2025-01-17T03:33:47Z

@rueian @kevin85421
Thanks for the detailed review 🙏.

rueian · 2025-01-17T19:16:34Z

Just a note: these failing tests seem unrelated to this PR

kevin85421 · 2025-01-17T20:23:16Z

rerun the test

kevin85421 · 2025-01-17T21:25:35Z

CI still fails. @owenowenisme could you rebase with the master branch?

Signed-off-by: owenowenisme <[email protected]>

owenowenisme · 2025-01-17T23:49:15Z

@kevin85421 Only unrelated test failed is remaining.

kevin85421 · 2025-01-17T23:59:28Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -573,8 +572,8 @@ func (r *RayClusterReconciler) reconcileRouteOpenShift(ctx context.Context, inst
 func (r *RayClusterReconciler) reconcileIngressKubernetes(ctx context.Context, instance *rayv1.RayCluster) error {
 	logger := ctrl.LoggerFrom(ctx)
 	headIngresses := networkingv1.IngressList{}
-	filterLabels := client.MatchingLabels{utils.RayClusterLabelKey: instance.Name}
-	if err := r.List(ctx, &headIngresses, client.InNamespace(instance.Namespace), filterLabels); err != nil {
+	filterLabels := common.RayClusterAllPodsAssociationOptions(instance).ToListOptions()


Suggested change

filterLabels := common.RayClusterAllPodsAssociationOptions(instance).ToListOptions()

filterLabels := common.RayClusterNetworkResourcesOptions(instance).ToListOptions()

kevin85421 · 2025-01-18T00:07:31Z

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

+			utils.RayNodeTypeLabelKey: string(rayv1.RedisCleanupNode),
+		}),
+	}
+	for k, v := range expected {


Signed-off-by: owenowenisme <[email protected]>

owenowenisme · 2025-01-18T00:42:17Z

@kevin85421
Sorry for such careless mistakes, I'll pay attention more in the future.

owenowenisme force-pushed the move-matchingLabels-to-association.go branch from f89cc2d to 9e9031f Compare January 13, 2025 01:01

kevin85421 assigned kevin85421 and rueian Jan 13, 2025

rueian reviewed Jan 13, 2025

View reviewed changes

rueian approved these changes Jan 14, 2025

View reviewed changes

kevin85421 requested changes Jan 15, 2025

View reviewed changes

owenowenisme force-pushed the move-matchingLabels-to-association.go branch from 4fa5f6a to 1c1c6b7 Compare January 15, 2025 03:09

rueian reviewed Jan 15, 2025

View reviewed changes

fscnick mentioned this pull request Jan 15, 2025

[RayCluster][Refactor] use RayClusterAllPodsAssociationOptions instead #2756

Merged

4 tasks

owenowenisme force-pushed the move-matchingLabels-to-association.go branch 2 times, most recently from 72c369f to e570111 Compare January 15, 2025 18:12

rueian reviewed Jan 16, 2025

View reviewed changes

owenowenisme requested a review from kevin85421 January 17, 2025 03:33

owenowenisme added 8 commits January 18, 2025 08:22

moved client.matching label into associate.go

229324c

Signed-off-by: owenowenisme <[email protected]>

use existing AssociationOptions function

c2b44fd

Signed-off-by: owenowenisme <[email protected]>

fix import path typo

0840c84

Signed-off-by: owenowenisme <[email protected]>

added test for RayClusterRedisPodsAssociationOptions

ebf254a

Signed-off-by: owenowenisme <[email protected]>

use specific existing associateOption

f730064

Signed-off-by: owenowenisme <[email protected]>

added TestRayClusterNetworkResourcesOptions

3305569

Signed-off-by: owenowenisme <[email protected]>

rename redis cleanup pod

0304b1f

Signed-off-by: owenowenisme <[email protected]>

added utils.RayIDLabelKey in testService in TestReconcileHeadService

e8ddab1

Signed-off-by: owenowenisme <[email protected]>

owenowenisme force-pushed the move-matchingLabels-to-association.go branch from b36aea0 to e8ddab1 Compare January 17, 2025 23:22

kevin85421 reviewed Jan 18, 2025

View reviewed changes

remove debugging log and use RayClusterNetworkResourcesOptions correctly

bb01605

Signed-off-by: owenowenisme <[email protected]>

kevin85421 approved these changes Jan 18, 2025

View reviewed changes

kevin85421 merged commit 5fde3c6 into ray-project:master Jan 18, 2025
23 of 24 checks passed

owenowenisme deleted the move-matchingLabels-to-association.go branch January 18, 2025 02:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move matching labels to association.go #2734

Move matching labels to association.go #2734

owenowenisme commented Jan 13, 2025 •

edited

Loading

owenowenisme commented Jan 13, 2025

rueian commented Jan 13, 2025

rueian Jan 13, 2025

owenowenisme Jan 14, 2025

kevin85421 Jan 15, 2025

kevin85421 Jan 15, 2025

kevin85421 Jan 15, 2025

kevin85421 Jan 15, 2025

rueian Jan 15, 2025

rueian Jan 16, 2025

rueian Jan 16, 2025

owenowenisme commented Jan 17, 2025

rueian commented Jan 17, 2025

kevin85421 commented Jan 17, 2025

kevin85421 commented Jan 17, 2025

owenowenisme commented Jan 17, 2025

kevin85421 Jan 17, 2025

kevin85421 Jan 18, 2025

owenowenisme commented Jan 18, 2025

	filterLabels := common.RayClusterHeadPodsAssociationOptions(instance).ToListOptions()
	filterLabels := common.RayClusterHeadServiceListOptions(instance).ToListOptions()

	func TestRayClusterRedisPodsAssociationOptions(t *testing.T) {
	func TestRayClusterRedisCleanupJobAssociationOptions(t *testing.T) {

	filterLabels := common.RayClusterAllPodsAssociationOptions(instance).ToListOptions()
	filterLabels := common.RayClusterNetworkResourcesOptions(instance).ToListOptions()

Move matching labels to association.go #2734

Move matching labels to association.go #2734

Conversation

owenowenisme commented Jan 13, 2025 • edited Loading

Why are these changes needed?

Related issue number

Checks

owenowenisme commented Jan 13, 2025

rueian commented Jan 13, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owenowenisme commented Jan 17, 2025

rueian commented Jan 17, 2025

kevin85421 commented Jan 17, 2025

kevin85421 commented Jan 17, 2025

owenowenisme commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owenowenisme commented Jan 18, 2025

owenowenisme commented Jan 13, 2025 •

edited

Loading