[apiserver][feat] Add read and write timeouts to HTTP server

[owenowenisme]

Why are these changes needed?

Related issue number

Closes #3306

Checks

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

[owenowenisme]

@dentiny PTAL
Also, I don't know is 5 seconds for read & write is suitable for us, please let me know if the timeout need to be changed.

[owenowenisme]

kuberay/apiserver/cmd/main.go

Lines 148 to 159 in 8944703

	srv := &http.Server{
	Addr: *httpPortFlag,
	Handler: topMux,
	ReadTimeout: 0, // No timeout
	WriteTimeout: 0, // No timeout
	IdleTimeout: 0, // No timeout
	}
	// Start the server.
	if err := srv.ListenAndServe(); err != nil {
	klog.Fatal(err)
	}

Haha just saw you added the timeout
I'll just close this pr then.

[dentiny]

Hi @owenowenisme , my PR change is only made to make linter happy thus no-op change, while you're adding timeout and delivering true value. :)

[dentiny]

I don't have an ideal recommendation here, usually there're two approaches to set timeout:

1. Set timeout to a large value so it behaves the same as no timeout for most cases, but does help cut down tail latency and trigger retry (i.e. half a minute) (could decrease timeout later after collection stats and user feedbacks);
2. (more scientific) have apiserver and kuberay operator deployed, to check what's the rough duration for operations.

[dentiny]

CC @kevin85421 / @MortalHappiness / @rueian might have much more production experience with kuberay.

[rueian]

We can parse the value from an env variable. And the default 0 is fine, I believe.

[kevin85421]

And the default 0 is fine,

@rueian, would you mind explaining why defaulting to 0 is acceptable? To make sure we are on the same page,

https://pkg.go.dev/net/http#Server

// ReadTimeout is the maximum duration for reading the entire
// request, including the body. A zero or negative value means
// there will be no timeout.

I would prefer to default to a positive timeout to prevent the server from being blocked by a single request for an extended period. In addition, I prefer not to expose the environment variable until we determine that it is necessary.

@dentiny or @owenowenisme, would you mind helping me understand the meaning of ReadTimeout and WriteTimeout here? I have read https://pkg.go.dev/net/http#Server, but I'd like to know what 'request' and 'response' refer to in the context of the KubeRay API server.

[rueian]

I believe that it is the user's responsibility to decide the value they want, so I think it is fine for us to leave it as default.

[dentiny]

The general idea is, we should always apply a limit on resource and operation; otherwise server could get overloaded by slow / malicious clients.

[kevin85421]

I actually think 0 is ok for now, because it's current behavior (no timeout); I would like to reduce risk.

For me, getting stuck on a single request is much more expensive than having a request fail. KubeRay has had related issues complained by users regarding the requests between the KubeRay operator and the Ray dashboard. Hence, I still prefer to set a default timeout.

Set a long enough timeout, as I mentioned #3350 (comment)

SG.

Use flag instead of env to allow user configuration, because env (if used improperly) is shared among all sessions, and prune to security issue.

We can expose this configuration only if some users request it.

[kevin85421]

For the default value, could @owenowenisme investigate the size of requests/responses in different situations so that we can set a reasonable timeout?

[owenowenisme]

Sure, I'll work on that.

[MortalHappiness]

I also think we should provide a default timeout, but also give users an option to configure it.

[kevin85421]

Btw, @owenowenisme would you mind fixing the conflict?