[baseserver] Support common config struct #9999
Conversation
csweichel
force-pushed
the
cw/baseserver-cfg
branch
from
3c5fdf7 to
3b6aa28
Compare
specifically debug, health and readiness
csweichel
force-pushed
the
cw/baseserver-cfg
branch
6 times, most recently
from
a735ea1 to
7e6cd6e
Compare
csweichel
force-pushed
the
cw/baseserver-cfg
branch
3 times, most recently
from
6c600ef to
1940499
Compare
github-actions
bot
added
team: delivery
| } | ||
| } | ||
|
|
||
| func MustFindFreePort(t *testing.T) int { |
There was a problem hiding this comment.
I think this is possibly racy. We're binding a socket to find out it's free, but then we release it immediately and again re-bind during server startup. During tests, we can have a number of servers running concurrently and do not have guarantees in which order they will execute.
I think here we should either make it so we always require an explicit port and never assign one automatically, it then becomes the Test's problem to find an available port, or support binding the socket only once during server startup.
There was a problem hiding this comment.
This is indeed racy. Short of exposing the listener through API I fret there's no good way to not make it racy.
I think here we should either make it so we always require an explicit port and never assign one automatically
That's what's implemented now. In addition there's the helper func to try and find a free port. I'd vote for going with this for - if the raciness proves to be a problem in practice, we extend the API so that tests can hand in a bound listener.
There was a problem hiding this comment.
Let's see how this goes. I think testing with a baseserver is already an improvement compared to what we have today for most things. We can always follow-up on this.
| return s.cfg.httpPort >= 0 | ||
| func (s *builtinServices) Close() error { | ||
| var eg errgroup.Group | ||
| eg.Go(func() error { return s.Debug.Close() }) |
There was a problem hiding this comment.
Neat! This does however not ensure that the Debug server is closed last. In general, it's beneficial to ensure it's last as it allows you to get the most metrics out of the system before it shuts down, and that way it also does contain metrics from the http/grpc servers shutting down.
|
/werft run 👍 started the job as gitpod-build-cw-baseserver-cfg.15 |
|
/werft run 👍 started the job as gitpod-build-cw-baseserver-cfg.16 |
|
I've addressed the comments, and rebased the stacked PRs. |
| @@ -10,10 +10,6 @@ import ( | |||
|
|
|||
| func service(ctx *common.RenderContext) ([]runtime.Object, error) { | |||
| return common.GenerateService(Component, map[string]common.ServicePort{ | |||
| DebugPortName: { | |||
There was a problem hiding this comment.
Thanks for removing this. It was the next for me to clean up after I landed the "debug/built-in server" last Friday.
Description
This PR refactors baseserver so that it's easy to integrate into the existing components, all the while harmonising the RPC/debug/HTTP configuration surface.
There's a stack of follow-up PRs which demonstrate the use of these changes in the real world:
How to test
See unit tests
Release Notes
/werft no-preview