[apiserver] ListAllServices with pagination by tinaxfwu · Pull Request #3490 · ray-project/kuberay

tinaxfwu · 2025-04-26T00:35:03Z

Why are these changes needed?

This PR implements pagination for listing all ray services.

delete ListAllServices in resource_manager
ListAllRayServices in serve_server reuses ListAllServices in resource_manager
An e2e test TestGetAllServicesWithPagination is added to validate the functionality.
print service content or size for debugging purpose

~/Doc/G/kuberay/apiserver feature/pagi..all-services *6 !2 ?8 > go test -v ./test/e2e/...  -timeout 60m  -race 
-coverprofile ray-kube-api-server-e2e-coverage.out -count=1 -parallel 4 -run TestGetAllServicesWithPagination
=== RUN   TestGetAllServicesWithPagination
=== RUN   TestGetAllServicesWithPagination/Test_pagination_return_part_of_the_result_services
=== RUN   TestGetAllServicesWithPagination/Test_pagination_return_all_result_services
--- PASS: TestGetAllServicesWithPagination (5.29s)
    --- PASS: TestGetAllServicesWithPagination/Test_pagination_return_part_of_the_result_services (1.19s)
    --- PASS: TestGetAllServicesWithPagination/Test_pagination_return_all_result_services (1.21s)
PASS
coverage: 39.9% of statements
ok      github.com/ray-project/kuberay/apiserver/test/e2e       6.834s  coverage: 39.9% of statements

Related issue number

Closes #3289

Checks

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

dentiny · 2025-04-26T01:14:31Z

Feel free to ping me when ready to review :)

dentiny · 2025-04-26T01:25:38Z

apiserver/test/e2e/service_server_e2e_test.go

 }

+func createOneServiceInEachNamespace(t *testing.T, numberOfNamespaces int) ([]*End2EndTestingContext, []string) {
+	tCtxs := make([]*End2EndTestingContext, numberOfNamespaces)


nit: can we unify the same array creation pattern? I think it's easy to get wrong.

Fixed in the latest push.
The array creation pattern is now unified using make([]T, 0, cap)

apiserver/test/e2e/service_server_e2e_test.go

dentiny · 2025-04-28T01:15:30Z

apiserver/test/e2e/service_server_e2e_test.go

+			tCtx.DeleteComputeTemplate(t)
+		})
+
+		expectedServiceNames[i] = make([]string, 0, numberOfService)


nit: as I suggested last time, I would appreciate if we could keep consistency for array usage.

dentiny · 2025-04-28T05:08:55Z

apiserver/test/e2e/service_server_e2e_test.go

+	const totalServices = numberOfNamespaces * numberOfService
+
+	tCtxs := make([]*End2EndTestingContext, numberOfNamespaces)
+	expectedServiceNames := make([][]string, numberOfNamespaces)


suggest to add some comments for the 2d array, not easy to figure out at first glance

I think the two-level for loop and 2d array is a little hard to understand, would it be better if we store an array of a struct

type targetSvc struct { namespace string svc string }

Thanks for your suggestion! I’ve updated it in the latest fix.

dentiny · 2025-04-28T05:09:04Z

apiserver/test/e2e/service_server_e2e_test.go

+	t.Run("Test pagination return part of the result services", func(t *testing.T) {
+		pageToken = ""
+		gotServices := make([][]bool, numberOfNamespaces)
+		for i := 0; i < numberOfNamespaces; i++ {


suggest to add some comments for the 2d array, not easy to figure out at first glance

dentiny · 2025-04-28T05:12:11Z

apiserver/test/e2e/service_server_e2e_test.go

+						if service.Namespace == ctx.GetNamespaceName() &&
+							service.Name == expectedServiceNames[namespaceIdx][serviceIdx] {
+							gotServices[namespaceIdx][serviceIdx] = true
+							break


break only breaks out on loop, so even after the target service is found, you still go extra unnecessary for loop (external loop); what about we use a found boolean.

dentiny · 2025-05-01T02:44:37Z

apiserver/test/e2e/service_server_e2e_test.go

+	// Test pagination with limit 1, which is less than the total number of services in all namespaces.
+	t.Run("Test pagination return part of the result services", func(t *testing.T) {
+		pageToken = ""
+		gotServices := make(map[targetService]bool)


this is bug, you need to initialize all possibility with false

otherwise L300 always gets true

@dentiny Thanks for catching that. It has been fixed and it is ready to be reviewed.

troychiu

LGTM except for 1 nit and @dentiny 's comment

apiserver/test/e2e/service_server_e2e_test.go

dentiny · 2025-05-02T16:48:37Z

ping me when ready :)

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

Signed-off-by: Tina <j6vupz97@gmail.com>

dentiny · 2025-05-03T21:58:00Z

apiserver/test/e2e/service_server_e2e_test.go

+			require.Nil(t, actualRPCStatus, "No RPC status expected")
+			require.NotNil(t, response, "A response is expected")
+			require.NotEmpty(t, response.Services, "A list of service is required")
+			require.Len(t, response.Services, 1)


nit: print service content or size for debugging purpose

dentiny

Thanks!

dentiny · 2025-05-03T22:04:43Z

apiserver/test/e2e/service_server_e2e_test.go

+		}
+	})
+
+	// Test pagination with limit 7, which is larger than the total number of services in all namespaces.


nitpick: I suggest to drop the detailed number (7 here), because it could be changed in the future, which adds maintenance overhead.

dentiny · 2025-05-03T22:05:13Z

apiserver/test/e2e/service_server_e2e_test.go

+		require.Empty(t, response.NextPageToken, "Page token should be empty")
+
+		for _, service := range response.Services {
+			gotServices[targetService{


sorry there's bug here, you need to check whether the key exist in the gotServices also

dentiny · 2025-05-03T22:05:33Z

apiserver/test/e2e/service_server_e2e_test.go

+			}
+
+			for _, service := range response.Services {
+				gotServices[targetService{


same here, you need to check service existence

Signed-off-by: Tina <j6vupz97@gmail.com>

dentiny · 2025-05-06T16:34:19Z

Test failed, could you please take a look? @tinaxfwu

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

dentiny · 2025-05-07T03:03:29Z

apiserver/test/e2e/service_server_e2e_test.go

+		// Check all services were found
+		for _, expectedService := range expectedServices {
+			if !gotServices[expectedService] {
+				t.Errorf("ListAllRayServices did not return expected service %s from namespace %s",


Two things,

You use multiple styles to assert in the test, t.Errorf, t.Fatalf and require, can we unify them into one?

t.Errorf doesn't look like a valid assertion

I would suggest use require or assert everywhere

I have replaced t.Errorf and t.Fatalf with require in both TestGetServicesInNamespaceWithPagination and TestGetAllServicesWithPagination.

dentiny · 2025-05-07T03:03:38Z

apiserver/test/e2e/service_server_e2e_test.go

+		// Check all services were found
+		for _, expectedService := range expectedServices {
+			if !gotServices[expectedService] {
+				t.Errorf("ListAllRayServices did not return expected service %s from namespace %s",


Signed-off-by: Tina Wu <j6vupz97@gmail.com>

dentiny · 2025-05-07T05:09:18Z

Feel free to ping me when ready thanks

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

dentiny · 2025-05-08T10:01:22Z

apiserver/test/e2e/service_server_e2e_test.go

+			require.NotNil(t, response, "A response is expected")
+			require.NotEmpty(t, response.Services, "A list of service is required")
+			require.Len(t, response.Services, 1)
+			t.Logf("Got %d services in response, expected %d", len(response.Services), 1)


what's the purpose for this logging?

You left a nit comment earlier about
printing service content or size for debugging purpose. So I add this line to log the size of services received.

Sorry for the confusion, I mean log when error

T. Require(,, message)

dentiny · 2025-05-08T10:01:35Z

apiserver/test/e2e/service_server_e2e_test.go

+			}
+
+			for _, service := range response.Services {
+				t.Logf("Got service: namespace=%s, name=%s", service.Namespace, service.Name)


same here, what's the purpose of logging?

I have removed this line, as the information it printed might be too trivial.

dentiny · 2025-05-08T10:02:32Z

apiserver/test/e2e/service_server_e2e_test.go

+		require.NotEmpty(t, response.Services, "A list of services is required")
+		require.Len(t, response.Services, totalServices)
+		require.Empty(t, response.NextPageToken, "Page token should be empty")
+		t.Logf("Got %d services in response, expected %d", len(response.Services), totalServices)


same question here

@tinaxfwu should this one be removed as well?

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

dentiny

LGTM, thanks for the effort!

dentiny reviewed Apr 26, 2025

View reviewed changes

apiserver/test/e2e/service_server_e2e_test.go Show resolved Hide resolved

tinaxfwu marked this pull request as ready for review April 27, 2025 20:19

tinaxfwu requested a review from dentiny April 27, 2025 20:22

dentiny reviewed Apr 28, 2025

View reviewed changes

dentiny reviewed May 1, 2025

View reviewed changes

troychiu approved these changes May 1, 2025

View reviewed changes

apiserver/test/e2e/service_server_e2e_test.go Show resolved Hide resolved

tinaxfwu force-pushed the feature/pagination-list-all-services branch from f4d121c to 67981e3 Compare May 2, 2025 22:10

tinaxfwu added 9 commits May 2, 2025 22:15

fix potential int32 overflow

dbad35d

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

use list and remove listall

c51babb

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

add test

17f1e9a

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

rm helper func

e8b2af4

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

lint

4909ecc

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

add all in one response

ccf46e1

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

correct

db4211c

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

modify proto

4455c0f

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

array consistency and break loop logic

5ca1ca1

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

tinaxfwu force-pushed the feature/pagination-list-all-services branch from 67981e3 to 5ca1ca1 Compare May 2, 2025 22:15

tinaxfwu added 2 commits May 2, 2025 22:20

fix format

b301cd2

gotServices init

c309510

Signed-off-by: Tina <j6vupz97@gmail.com>

dentiny reviewed May 3, 2025

View reviewed changes

dentiny approved these changes May 3, 2025

View reviewed changes

dentiny reviewed May 3, 2025

View reviewed changes

tinaxfwu added 3 commits May 5, 2025 01:20

change comment

17dad98

Signed-off-by: Tina <j6vupz97@gmail.com>

add logging comments

f125607

Signed-off-by: Tina <j6vupz97@gmail.com>

add existence checking

836a9b1

Signed-off-by: Tina <j6vupz97@gmail.com>

add checks

49dc3b9

Signed-off-by: Tina <j6vupz97@gmail.com>

format and rebase

9ff305a

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

tinaxfwu requested a review from dentiny May 7, 2025 02:49

dentiny reviewed May 7, 2025

View reviewed changes

tinaxfwu added 2 commits May 7, 2025 00:18

use require

bf9f67d

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

trigger CI

5e29a93

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

update format and replace t.errorf

d6694bd

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

tinaxfwu requested a review from dentiny May 7, 2025 06:26

dentiny reviewed May 8, 2025

View reviewed changes

tinaxfwu added 2 commits May 8, 2025 08:10

rm log

b73f747

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

add comment

9d6f938

Signed-off-by: Tina Wu <j6vupz97@gmail.com>

tinaxfwu requested a review from dentiny May 9, 2025 03:19

dentiny approved these changes May 9, 2025

View reviewed changes

dentiny requested a review from kevin85421 May 9, 2025 05:16

dentiny assigned kevin85421 May 9, 2025

kevin85421 approved these changes May 9, 2025

View reviewed changes

kevin85421 merged commit 0721d8f into ray-project:master May 9, 2025
24 checks passed

tinaxfwu deleted the feature/pagination-list-all-services branch May 9, 2025 20:47

pawelpaszki pushed a commit to opendatahub-io/kuberay that referenced this pull request May 26, 2025

[apiserver] ListAllServices with pagination (ray-project#3490)

fe027d0

Conversation

tinaxfwu commented Apr 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

dentiny commented Apr 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

troychiu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dentiny commented May 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dentiny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dentiny commented May 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dentiny commented May 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dentiny left a comment

Choose a reason for hiding this comment

Uh oh!

tinaxfwu commented Apr 26, 2025 •

edited

Loading