[RelayMiner]: add proxy.Ping(...) capability to test connectivity between relay servers and backend URLs
#744
Conversation
WalkthroughThis update introduces a new Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (2)
pkg/relayer/relayminer.go (1)
138-154: Log error details for better debugging.In the
ServePingfunction, when an error occurs during thePingcall, consider logging the error details before returning a 500 status. This will help in diagnosing issues more effectively.if errs := rel.relayerProxy.Ping(ctx); errs != nil { + rel.logger.Error().Err(errs).Msg("failed to ping relay servers") w.WriteHeader(http.StatusInternalServerError) return }pkg/relayer/proxy/proxy_test.go (1)
155-158: Consider extending test coverage forPingmethod.While the current test ensures no errors are returned, consider adding scenarios to test edge cases, such as:
- Handling of unreachable URLs.
- Different HTTP status codes.
- Timeout scenarios.
This will ensure comprehensive coverage of the
Pingmethod's behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
pkg/relayer/proxy/proxy.go (1)
203-203: Fix typographical error in log message.The log message contains a typographical error: "occured" should be "occurred".
- Msg("an unexpected error occured while pinging backend URL") + Msg("an unexpected error occurred while pinging backend URL")
Olshansk
added
the
community
…ty between relay servers and backend URLs (#1) * relayer: add RelayServers() method to RelayProxy interface; Add Ping(), ServiceIDs(), Forward() method to RelayServer interface; add RelayServers slice with helper method byServiceID * relayer: add forward config entry * relayer: implement ServiceIDs, Forward, and Ping method for synchrounous RPC server * relayer: add RelayServers implementation for RelayProxy * relayer: add Ping and Forward options * relayer: integrate ping option * relayer: add ServePing and ServeForward method to RelayMiner * test proxy.Ping() in test + remove forward feature * add serve ping test * add doc
|
Hey @Olshansk 👋🏾 ! Thanks for taking the time to review the code and the feedback.
Yes let's tackle that. I will make changes to the latest comments by Monday at the latest. I am also working on the forward feature (see issue comment). I'll open a PR as soon as I can.
And yes, I am definitely interested in contributing more 🚀 |
|
@eddyzags Sounds great! Please re-request a review when you're ready. I will take another look then. |
|
@eddyzags Just FYI: #744 (comment) |
|
@eddyzags Another bump |
|
Hey @Olshansk I've just added the last requested modifications:
As the Ping safeguard implementation won't start the Relay Miner pod if the suppliers aren't reachable, I had to clean the Relay Miner's Helm value files when starting the Localnet environment based on the constraints below:
In other words, this new logic forces us to only reference the available suppliers in the Relay Miner's configuration. Error message: The Relay miner configuration seems to expects all the suppliers list (Ollama, Anvi & Rest) even if the supplier's container isn't deployed. If this is true, the Relay Miner won't be able to start with the Ping safeguard if one supplier isn't deployed. |
…s based on localnet config
|
@eddyzags Appreciate the detailed feedback. Re review - I will take a look next week but hopefully @bryanchriswhite can help out as well.
Re this error. I'll take a deeper look next week but in the meantime, can you try:
This might be something historical we've already solved |
| go install golang.org/x/tools/cmd/goimports@latest | ||
| go install github.com/mikefarah/yq/v4@latest | ||
|
|
||
| .PHONY: install_cosmovisor |
There was a problem hiding this comment.
Super cool feature @eddyzags 😎 - thanks for the contribution! 🚀 🙌
(And apologies for the review delay 😅)
There was a problem hiding this comment.
Were any of the makefile changes intentional? Otherwise, let's revert. I split the makefile up in #816.
There was a problem hiding this comment.
Same question here as above.
| @@ -173,6 +174,21 @@ pprof: | |||
|
|
|||
| You can learn how to use that endpoint on the [Performance Troubleshooting](../../develop/developer_guide/performance_troubleshooting.md) page. | |||
|
|
|||
| ### `ping` | |||
| Configures a `ping` server to test the connectivity of every backend URLs. If | ||
| all the backend URLs are reachable, the endpoint returns a 200 HTTP | ||
| Code. Otherwise, if one or more backend URLs aren't reachable, the service | ||
| returns an 500 HTTP Internal server error. |
There was a problem hiding this comment.
Have you considered using 204 No Content for the success and 503 Service Unavailable for the error status codes? I think they could be more appropriate than 200 and 500.
| Configures a `ping` server to test the connectivity of every backend URLs. If | |
| all the backend URLs are reachable, the endpoint returns a 200 HTTP | |
| Code. Otherwise, if one or more backend URLs aren't reachable, the service | |
| returns an 500 HTTP Internal server error. | |
| Configures a `ping` server to test the connectivity of all backend URLs. If | |
| all the backend URLs are reachable, the endpoint returns a 200 HTTP | |
| Code. If one or more backend URLs aren't reachable, the service | |
| returns an 500 HTTP Internal server error. |
There was a problem hiding this comment.
Intentional diff?
Same goes for all files in the loalnet/kubernetes directory.
One simple way to revert these would be to do something like:
git fetch origin
git checkout origin/main -- localnet/kubernetes
git add localnet/kubernetes
git commit| filename := "/tmp/relayerminer.ping.sock" | ||
|
|
||
| ln, err := net.Listen("unix", filename) | ||
| require.NoError(t, err) | ||
| defer os.Remove(filename) |
|
|
||
| time.Sleep(time.Millisecond) | ||
|
|
||
| c := http.Client{ |
There was a problem hiding this comment.
Same as above:
| c := http.Client{ | |
| httpClient := http.Client{ |
| Transport: &http.Transport{ | ||
| DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| return net.Dial(ln.Addr().Network(), ln.Addr().String()) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
- Can we please extract the
*http.Transportvalue (i.e. keep the&here) to avar? - Are we sure that it's necessary to provide a
DialContextfunc? If so, please add a comment explaining.
| _, err = c.Get("http://unix") | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
#PUC (please update comment). It's not clear to me what's going on here and what this assertion implies.
I was expecting to see an assertion regarding the contents of the socket file or something to ensure that the http request made it through.
| relayerProxyMock := mockrelayer.NewMockRelayerProxy(ctrl) | ||
| relayerProxyMock.EXPECT(). | ||
| Start(gomock.Eq(ctx)). | ||
| Times(1) | ||
| relayerProxyMock.EXPECT(). | ||
| Stop(gomock.Eq(ctx)). | ||
| Times(1) | ||
| relayerProxyMock.EXPECT(). | ||
| ServedRelays(). | ||
| Return(returnedRelaysObs). | ||
| Times(1) |
There was a problem hiding this comment.
I think this can this be simplified to:
| relayerProxyMock := mockrelayer.NewMockRelayerProxy(ctrl) | |
| relayerProxyMock.EXPECT(). | |
| Start(gomock.Eq(ctx)). | |
| Times(1) | |
| relayerProxyMock.EXPECT(). | |
| Stop(gomock.Eq(ctx)). | |
| Times(1) | |
| relayerProxyMock.EXPECT(). | |
| ServedRelays(). | |
| Return(returnedRelaysObs). | |
| Times(1) | |
| relayerProxyMock := NewMockOneTimeRelayerProxy(ctx, t, returnedRelaysObs relayer.RelaysObservable) |
Summary
This PR adds the capability to test the connectivity between the Relay Servers and the Backend URLs in two ways.
Safeguard at Startup:
For every
suppliers.[].service_config.backend_urlreferenced as input inside the Relay Miner Configuration file, the Relay Proxy will verify wether the network connection between the targetedbackend_urland the relayerminer process is functioning properly. If one or more connections aren't possible, the relay miner won't be able to start.Configurable Ping HTTP server:
The Relay Miner process will listen for incoming request to synchronously test the connectivity of every referenced
suppliers.[].service_config.backend_url. If one or more backend URLs aren't reachable, the incoming request will fail.Based on the
serverConfig.ServerType(Example: HTTP), each Server Type will implement their own logic to implement to test the connectivity.Issue
Type of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_testmake test_e2ePR Testing (only if making code changes)
devnet-test-e2elabel to the PR.make trigger_ciif you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
pingfunctionality, allowing users to test backend connectivity within the relay miner's setup.Bug Fixes
Tests