[Feature] Add timeout for apiserver grpc server #3427

machichima · 2025-04-19T04:42:37Z

Add time out grpc server interceptor
Add unit test to ensure the timeout interceptor works well
Enable setting the timeout value through flag -grpc_timeout (e.g. -grpc_timeout 30s), default to 60 seconds

Why are these changes needed?

Currently, there's no timeout setting in gRPC server side. This PR added the timeout to ensure resource access doesn't get overloaded in all cases.

Related issue number

Part of #3344

Checks

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

-e Signed-off-by: machichima <[email protected]>

machichima · 2025-04-19T04:46:59Z

Hi @dentiny ,

I noticed that there's a PR related to this which works on http timeout (#3350), do we need to have a same timeout for http and grpc server or is it fine to have them different?

Thanks

dentiny · 2025-04-20T09:33:44Z

I noticed that there's a PR related to this which works on http timeout (#3350), do we need to have a same timeout for http and grpc server or is it fine to have them different?

There're three timeout related issues, as far as I am aware of:

grpc service timeout, which limits the processing time for each request (i.e. query kubernetes apiserver)
- we use grpc gateway, so http server and grpc server share one processing timeout at grpc layer, see https://grpc-ecosystem.github.io/grpc-gateway/
http server timeout, which is used to limit http request read and response write timeout
client side timeout

machichima · 2025-04-20T10:05:42Z

There're three timeout related issues, as far as I am aware of:

Got it, I think this PR is working on grpc service timeout, while PR #3350 works on limiting http request?

As I saw the comment in PR #3350 that they want to decide a default timeout, I am thinking if the default value here should be the same as what they set?

dentiny · 2025-04-20T10:15:22Z

Got it, I think this PR is working on grpc service timeout, while PR #3350 works on limiting http request?

yes

As I saw the comment in PR #3350 that they want to decide a default timeout, I am thinking if the default value here should be the same as what they set?

No, I don't think they need to be the same

apiserver/pkg/interceptor/interceptor.go

bhks · 2025-04-23T05:07:07Z

Got it, I think this PR is working on grpc service timeout, while PR #3350 works on limiting http request?

yes

As I saw the comment in PR #3350 that they want to decide a default timeout, I am thinking if the default value here should be the same as what they set?

No, I don't think they need to be the same

60 seconds should be a good number ?

…server-timeout-grpc-server -e Signed-off-by: machichima <[email protected]>

-e Signed-off-by: machichima <[email protected]>

machichima · 2025-04-23T14:57:17Z

60 seconds should be a good number ?

Yes I think its good for default value. I also add an env variable to let user set the timeout value.

machichima · 2025-04-23T15:12:36Z

@dentiny PTAL

bhks · 2025-04-23T20:44:41Z

apiserver/pkg/interceptor/interceptor.go

+		case <-ctx.Done():
+			// Raise error if time out
+			if ctx.Err() == context.DeadlineExceeded {
+				return nil, fmt.Errorf("grpc server timed out")


Can we name the grpc server with KubeRay API server ?

Sure! Just changed

bhks · 2025-04-23T20:45:28Z

apiserver/cmd/main.go

@@ -50,11 +51,21 @@ func main() {
 		_ = flagSet.Set("log_file", *logFile)
 	}

+	grpcTimeout := 60 * time.Second // Default timeout


Are we following mechanisms to define constants or we are adding to each files where we are using it ?

quickly glancing over the code, we have constants.go for other components (i.e. operator)
https://github.com/ray-project/kuberay/blob/ebb5ba441b0a7f888c17aa5c2d33943084a9a2d9/ray-operator/controllers/ray/utils/constant.go

I usually do this in two ways:

either place it to constant file, just as what we did for kuberay operator

the benefit of which is we group all constants in one place, rather than scattered around the codebase

or define a util function getGrpcServerTimeoutOrDefault and have default timeout besides

the benefit of which is it's easy to locate all timeout related functions and features

Our codebase seems to prefer (1).

Sure, I found that in apiserver, they put constants in config.go, I'll add it here

kuberay/apiserver/pkg/util/config.go

Lines 10 to 14 in a83d3c1

const (

// Label keys

RayClusterNameLabelKey = "ray.io/cluster-name"

RayClusterUserLabelKey = "ray.io/user"

RayClusterVersionLabelKey = "ray.io/version"

dentiny · 2025-04-24T00:08:53Z

apiserver/cmd/main.go

@@ -50,11 +51,21 @@ func main() {
 		_ = flagSet.Set("log_file", *logFile)
 	}

+	grpcTimeout := 60 * time.Second // Default timeout
+	if timeoutStr := os.Getenv("GRPC_SERVER_TIMEOUT"); timeoutStr != "" {


btw why do we use env var instead of flags? I think flags are strictly better in a few ways:

program checks env variables; for example, bazel uses env to decide whether we could reuse cache

impose security issue, because env is shared among all processes which could be accessed everywhere

I almost only use env variables when:

across language boundary

across process boundary, if no other easier way

Thanks for the guidance!

The reason why I put it in environment variable instead of flag is because I search through the code base and find they put this (which I think is a bit similar to timeout?) in the environment variable, so I just simply follow what it does

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

Line 351 in a83d3c1

requeueAfterSeconds, err := strconv.Atoi(os.Getenv(utils.RAYCLUSTER_DEFAULT_REQUEUE_SECONDS_ENV))

I agree to your points, if there's no other places that need this value, I think I'll just move it to flag instead

Moved to flag

dentiny · 2025-04-24T00:09:36Z

apiserver/cmd/main.go

+			grpcTimeout = timeout
+			klog.Infof("gRPC servier timeout set to %v", grpcTimeout)
+		} else {
+			klog.Warningf("Invalid GRPC_SERVER_TIMEOUT value: %v, using default timeout (60 seconds)", err)


use %d to print out default value, in case we change in the future

Thanks! Just added

apiserver/pkg/interceptor/interceptor.go

-e Signed-off-by: machichima <[email protected]>

…server-timeout-grpc-server -e Signed-off-by: machichima <[email protected]>

-e Signed-off-by: machichima <[email protected]>

apiserver/pkg/interceptor/interceptor.go

apiserver/pkg/interceptor/interceptor_test.go

apiserver/pkg/util/config.go

-e Signed-off-by: machichima <[email protected]>

dentiny · 2025-04-28T05:36:31Z

let me know when it's ready for review, feel free to ping me any time :)

-e Signed-off-by: machichima <[email protected]>

machichima · 2025-04-26T08:12:15Z

apiserver/pkg/interceptor/interceptor_test.go

+	select {
+	case <-time.After(delay):
+		return "test_response", h.returnErr
+	case <-ctx.Done():
+		var grpcCode codes.Code
+		switch ctx.Err() {
+		case context.Canceled:
+			grpcCode = codes.Canceled
+		case context.DeadlineExceeded:
+			grpcCode = codes.DeadlineExceeded
+		default:
+			grpcCode = codes.Unknown
+		}
+		return nil, status.Error(grpcCode, ctx.Err().Error())
+	}


Adding this to mimic the grpc IO handler for testing

apiserver/pkg/interceptor/interceptor.go

apiserver/pkg/util/config.go

apiserver/pkg/interceptor/interceptor_test.go

machichima · 2025-04-28T14:15:40Z

apiserver/pkg/manager/client_manager_mock.go

This is automatically updated when running make test

machichima · 2025-04-28T14:16:05Z

apiserver/pkg/client/kubernetes_mock.go

This is automatically updated when running make test

machichima · 2025-04-28T14:16:48Z

apiserver/cmd/main.go

@@ -50,11 +51,21 @@ func main() {
 		_ = flagSet.Set("log_file", *logFile)
 	}

+	grpcTimeout := 60 * time.Second // Default timeout
+	if timeoutStr := os.Getenv("GRPC_SERVER_TIMEOUT"); timeoutStr != "" {


Moved to flag

apiserver/pkg/util/config.go

machichima · 2025-04-29T01:46:30Z

Sorry I just found that I didn't submit the review as comment

machichima · 2025-05-01T22:45:56Z

@dentiny PTAL, Thanks!

dentiny

LGTM, thank you for the help!
And sorry about the delay

dentiny · 2025-05-02T03:58:27Z

apiserver/pkg/client/kubernetes_mock.go

 func (mr *MockKubernetesClientInterfaceMockRecorder) ConfigMapClient(namespace interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()


If the mock call setup fails (e.g., wrong argument types), Go's testing output will show the line in your actual test code where the error originated, rather than pointing you to this ConfigMapClient() method in the mock recorder.

TIL

dentiny · 2025-05-02T04:14:09Z

apiserver/pkg/interceptor/interceptor_test.go

@@ -61,7 +83,7 @@ func TestAPIServerInterceptor(t *testing.T) {
 				req,
 				info,
 				func(ctx context.Context, req interface{}) (interface{}, error) {
-					return tt.handler.Handle(ctx, req)
+					return tt.handler.Handle(ctx, req, 0)


nit: 0 /*delay*/

Fixed! Thanks!

dentiny · 2025-05-02T04:14:27Z

apiserver/pkg/interceptor/interceptor_test.go

@@ -153,7 +175,7 @@ func TestAPIServerInterceptorLogging(t *testing.T) {
 				"test_request",
 				info,
 				func(receivedCtx context.Context, req interface{}) (interface{}, error) {
-					return handler.Handle(receivedCtx, req)
+					return handler.Handle(receivedCtx, req, 0)


nit: add a comment besides constants

Fixed! Thanks!

…server-timeout-grpc-server -e Signed-off-by: machichima <[email protected]>

-e Signed-off-by: machichima <[email protected]>

dentiny · 2025-05-07T09:02:18Z

Enable setting the timeout value through env variable GRPC_SERVER_TIMEOUT

PR description needs to be updated :)

machichima · 2025-05-07T13:46:59Z

PR description needs to be updated :)

Thanks! Just updated!

feat: add grpc server timeout

c677b80

-e Signed-off-by: machichima <[email protected]>

bhks reviewed Apr 23, 2025

View reviewed changes

apiserver/pkg/interceptor/interceptor.go Show resolved Hide resolved

machichima added 4 commits April 23, 2025 22:01

Merge branch 'master' of github.com:ray-project/kuberay into 3344-api…

9cbf138

…server-timeout-grpc-server -e Signed-off-by: machichima <[email protected]>

test: unit test for timeout interceptor

33cf5da

-e Signed-off-by: machichima <[email protected]>

fix: update timeout to 60 seconds

d34686f

-e Signed-off-by: machichima <[email protected]>

feat: enable setting grpc timeout by env variable

ebb5ba4

-e Signed-off-by: machichima <[email protected]>

machichima marked this pull request as ready for review April 23, 2025 14:54

bhks approved these changes Apr 23, 2025

View reviewed changes

dentiny reviewed Apr 24, 2025

View reviewed changes

apiserver/pkg/interceptor/interceptor.go Show resolved Hide resolved

machichima added 8 commits April 24, 2025 23:57

refactor: default timeout to constant

cade329

-e Signed-off-by: machichima <[email protected]>

fix: update time out error message

a8fee22

-e Signed-off-by: machichima <[email protected]>

fix: add default timeout value in warning message

d4a554a

-e Signed-off-by: machichima <[email protected]>

feat: set gRPC timeout in flag instead of env var

f1b5baf

-e Signed-off-by: machichima <[email protected]>

Merge branch 'master' of github.com:ray-project/kuberay into 3344-api…

f8ad90f

…server-timeout-grpc-server -e Signed-off-by: machichima <[email protected]>

test: update mock handler that mimic grpc package handler

4c897aa

-e Signed-off-by: machichima <[email protected]>

feat: simplify timeout interceptor

aa7fcb5

-e Signed-off-by: machichima <[email protected]>

fix: variable type to log

6425633

-e Signed-off-by: machichima <[email protected]>

machichima requested a review from dentiny April 26, 2025 08:12

dentiny reviewed Apr 26, 2025

View reviewed changes

apiserver/pkg/interceptor/interceptor.go Show resolved Hide resolved

dentiny reviewed Apr 26, 2025

View reviewed changes

apiserver/pkg/interceptor/interceptor_test.go Show resolved Hide resolved

dentiny reviewed Apr 26, 2025

View reviewed changes

apiserver/pkg/util/config.go Outdated Show resolved Hide resolved

docs: add docstring for mock Handle

79a6349

-e Signed-off-by: machichima <[email protected]>

fix: remove suffix in timeout constant

54c8d57

-e Signed-off-by: machichima <[email protected]>

machichima commented Apr 29, 2025

View reviewed changes

machichima requested a review from dentiny April 29, 2025 01:45

dentiny approved these changes May 2, 2025

View reviewed changes

dentiny requested a review from kevin85421 May 2, 2025 04:16

dentiny assigned kevin85421 May 2, 2025

machichima added 2 commits May 2, 2025 23:39

Merge branch 'master' of github.com:ray-project/kuberay into 3344-api…

897ad01

…server-timeout-grpc-server -e Signed-off-by: machichima <[email protected]>

style: add comment for constant args

47f872a

-e Signed-off-by: machichima <[email protected]>

kevin85421 approved these changes May 6, 2025

View reviewed changes

kevin85421 merged commit f45155b into ray-project:master May 6, 2025
24 checks passed

laurafitzgerald pushed a commit to laurafitzgerald/kuberay that referenced this pull request May 7, 2025

[Feature] Add timeout for apiserver grpc server (ray-project#3427)

3aed094

laurafitzgerald pushed a commit to laurafitzgerald/kuberay that referenced this pull request May 8, 2025

[Feature] Add timeout for apiserver grpc server (ray-project#3427)

19c273c

	const (
	// Label keys
	RayClusterNameLabelKey = "ray.io/cluster-name"
	RayClusterUserLabelKey = "ray.io/user"
	RayClusterVersionLabelKey = "ray.io/version"

		func (mr MockKubernetesClientInterfaceMockRecorder) ConfigMapClient(namespace interface{}) gomock.Call {
		mr.mock.ctrl.T.Helper()

[Feature] Add timeout for apiserver grpc server #3427

[Feature] Add timeout for apiserver grpc server #3427

Uh oh!

Conversation

machichima commented Apr 19, 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!

machichima commented Apr 19, 2025

Uh oh!

dentiny commented Apr 20, 2025

Uh oh!

machichima commented Apr 20, 2025

Uh oh!

dentiny commented Apr 20, 2025

Uh oh!

Uh oh!

bhks commented Apr 23, 2025

Uh oh!

machichima commented Apr 23, 2025

Uh oh!

machichima commented Apr 23, 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dentiny commented Apr 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

machichima commented Apr 29, 2025

Uh oh!

machichima commented May 1, 2025

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!

Choose a reason for hiding this comment

Uh oh!

machichima commented Apr 19, 2025 •

edited

Loading