fail the snapshotter immediately if metrics server can't be bound #363
Conversation
pkg/metrics/listener.go
Outdated
| } | ||
| go func() { | ||
| if err := http.ListenAndServe(addr, nil); err != nil { | ||
| log.L.Errorf("error serve on %s: %v", addr, err) |
There was a problem hiding this comment.
Can we make sure to exit snapshotter once http.ListenAndServe fails?
There was a problem hiding this comment.
Better not to, we should try to maintain the snapshots service , I suppose.
There was a problem hiding this comment.
So the user can only determine if the metrics server is working by checking the error log? I think it's a not good design.
There was a problem hiding this comment.
Um. If we indeed need to ensure that the metrics server is successfully started, perhaps we can try to connect to the listener in the main goroutine
There was a problem hiding this comment.
I think we just need to move the connection listen out from ListenAndServe. listen is likely to fail when the port is already bound when serve can only fail when the connection is closed by accident
There was a problem hiding this comment.
Agree. It is sufficient to quit snapshotter if the port is already bound.
pkg/metrics/listener.go
Outdated
| } | ||
| go func() { | ||
| if err := http.ListenAndServe(addr, nil); err != nil { | ||
| log.L.Errorf("error serve on %s: %v", addr, err) |
There was a problem hiding this comment.
Could we display information such as "Failed to serve metrics HTTP endpoint on %s: %v" as this error message is in a go routine.
There was a problem hiding this comment.
Good point. We'd better give more information about error context.
changweige
force-pushed
the
fail-start-metrics-address
branch
2 times, most recently
from
da14195
to
9f7ad26
Compare
| } | ||
|
|
||
| go func() { | ||
| log.L.Infof("Start metrics HTTP server on %s", addr) | ||
| if err := http.Serve(l, nil); trapClosedConnErr(err) != nil { |
There was a problem hiding this comment.
Perhaps err != nil && !errors.Is(err, net.ErrClosed) will also work?
We'd better to exit from snapshotter's startup if the metrics server address can't be bound. Signed-off-by: Changwei Ge <gechangwei@bytedance.com>
changweige
force-pushed
the
fail-start-metrics-address
branch
from
9f7ad26
to
7354a06
Compare
We should fail snapshotter's startup if the metrics address weren't bound.