[RelayMiner]: add proxy.Ping(...) capability to test connectivity between relay servers and backend URLs
#1037
Conversation
…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
…s based on localnet config
Olshansk
added
tooling
There was a problem hiding this comment.
Thanks for picking this back up @eddyzags! 🙌
I have to stop here for today but this is looking great so far! 🚀
The biggest thing I haven't reviewed yet is the test (but I already saw the addition of go-mockdns, and I skimmed the test names 😉) and am looking forward to it.
There was a problem hiding this comment.
Was this change intentionally persisted, and if so, how is it related to this feature?
I think this change should be reverted. My assumption is that this is the result of an older commit which was never reconciled completely with main:
- The yaml files referenced don't exist.
- The flags seem to be specifying the same/similar config as what's been removed from the relayminer configs that do exist. 🤔
There was a problem hiding this comment.
Sorry, I wasn't clear in my previous comments.
Was this change intentionally persisted, and if so, how is it related to this feature?
Yes, this change was intentionally made to ensure the Ping safeguard at startup succeeds for the Relayminer with the localnet default configuration, and/or any custom localnet configuration in that regard (link to localnet default configuration in the main branch). In the default localnet configuration, the Ollama Kubernetes deployment is not applied (ollama.enabled=false). However, the relayminer configuration still referenced Ollama suppliers in its configuration files, even though the container wasn’t deployed (link to relayminer-1 configuration for localnet). With the newly introduced mechanism of the Ping safeguard at startup, this will cause the relayminer to fail continuously because the Ollama container isn't deployed.
To solve this issue, I found a way to dynamically define the relayminer's configuration based on the localnet configuration by modifying the poktrolld/Tiltfile. Hence, those modifications.
For poktrolld users that are deploying a Relayminer without relying on the localnet, they will have to make sure that their config.suppliers[*].service_config.backend_url are up and running and reachable before deploying a Relayminer.
The yaml files referenced don't exist.
I disagree, they exists:
- values-common.yaml is defined here
- values-relayminer-common.yaml is defined here
- values-relayminer- + str(actor_number) + ".yaml" is defined here, here and here
The flags seem to be specifying the same/similar config as what's been removed from the relayminer configs that do exist.
I cannot find that. Can you link me to the precise line in my fork that makes you think that please? 🙏🏾
There was a problem hiding this comment.
It seems to me that these relayminer configs (1-3) should be reverted.
| server.Handler = http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
| sendJSONRPCResponse(test.t, w) | ||
| }) | ||
| listener, err := net.Listen("tcp", supplierConfig.ServiceConfig.BackendUrl.Host) |
There was a problem hiding this comment.
Why separate the listener from the server?
There was a problem hiding this comment.
By using a custom listener, and thereby decoupling the listener from the serve action, we ensure that the HTTP server is fully prepared to listen on a specific port in the test's main Go routine. This guarantees that the HTTP server(s) is ready before proceeding to the actual test cases.
Previously, listening and serving were handled within the Go routine using http.ListenAndServe function. This approach sometimes led to the HTTP server not being ready when the test cases began execution, resulting in test failures and flaky behavior.
There was a problem hiding this comment.
Amazing! 👍 #PUC with that explanation, perhaps condensed, if possible.
|
Thanks for reviewing @bryanchriswhite ! Waiting for the rest of the review 🚀 |
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