-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RelayMiner]: add proxy.Ping(...)
capability to test connectivity between relay servers and backend URLs
#1037
Open
eddyzags
wants to merge
55
commits into
pokt-network:main
Choose a base branch
from
eddyzags:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
55 commits
Select commit
Hold shift + click to select a range
a31e8d5
feat(relayminer): add `proxy.Ping(...)` capability to test connectivi…
eddyzags ecbcb3e
use dynamic slice in Ping error handling
eddyzags 54b79d1
relayer: add godoc to configuration yaml
eddyzags 7bd3223
proxy: add comment explaining application logic
eddyzags 0234978
relayer: change Ping to PingAll in relayproxy interface
eddyzags affc937
proxy: cleanup unused code
eddyzags 9ddc62a
relayer: add godoc explaining ping http serve method
eddyzags 8395970
relayer: remove blank line
eddyzags 2df6479
localnet: add ping helpers for relayminers + logic to define supplier…
eddyzags d3710d8
use errors.Join instead of appending errors slice
eddyzags 8830010
change c to httpClient in sychronous in relay server
eddyzags 86976f1
add relayer miner suppplier not reachable error
eddyzags 493884e
add newpinghandlerfn function
eddyzags 1bf241e
add comment relayminer test
eddyzags c235232
simplified newmockonetimerelayerproxywithping function
eddyzags 587cf44
revert Makefile and add local helpers
eddyzags 44a6545
add 204 no content for ping response
eddyzags 8fdd7e9
add comments to addr for ping config
eddyzags f9d636e
revert Makefile
eddyzags d94ead2
fix typo
eddyzags 5153493
add statuscode assertion while testing ping server
eddyzags 861bf35
add localnet helpers to ping relayminer 1 2 3 + port exposition
eddyzags 4802e86
add more context to synchrounous rpc ping error
eddyzags fda1da3
change c to httpClient in relayminer tests
eddyzags 499ee4e
add more context to transport override in relayminer tests
eddyzags 26dad58
add transport varialbe in relayminer tests
eddyzags 3d5128d
add 502 bad gateway as http code response for /ping
eddyzags 9709983
add tcp listener to relayerminer pkg
eddyzags e3ed522
fix code registration for ErrRelayerProxySupplierNotReachable
eddyzags 8130cf3
add endpointURL variable
eddyzags c164306
improve godoc comments for pingconfig
eddyzags 239f2f1
fix serveping tests + refactor relayminer.ServePing function signature
eddyzags 385b7e9
refactor: dynamically set values for relayminer suppliers list in Til…
eddyzags ad0f0a7
add pingall test suite
eddyzags fe6342a
add proxy different endpoints ping tests
eddyzags d7abf60
stabilize flaky helpers in testproxy
eddyzags e35986e
add comments and refactor variable names
eddyzags dd6b7e8
fix typo in Tiltfile
eddyzags ad39a0c
fix tyop
eddyzags 7f21be9
categorized as errors any HTTP status code higher or equal to 400
eddyzags 446d2f2
improve error handling while serving http request for ping server
eddyzags d2ade64
distinguish 502 from 503 errors in ping handler for error handling
eddyzags 53ca20e
rely on testing temp dir for testing files
eddyzags 39d1458
improve relayminer configuration documentation
eddyzags 07361b9
improve error message for ping request
eddyzags 0c11e02
minor fix
eddyzags 8988335
add comments to explain WithServiceConfigMap() function management to…
eddyzags 2947c2b
rely on defaultService pkg level constant for ping test suite
eddyzags baa9b61
remove unused variable in synchrounous Ping method
eddyzags 3800411
re-using supplierOperatorKeyName package level constant variable for …
eddyzags acdd9d1
add two separate relay servers for all with multiple servers + improv…
eddyzags 2f2d9cc
remove unused variables
eddyzags f4027a5
add TestRelayMiner_NOKPing unit test
eddyzags 6b27a83
refactor Relayminer ping test using suite
eddyzags cc573c7
add optional to ping documentation
eddyzags File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bryanchriswhite marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,3 +11,6 @@ config: | |
pprof: | ||
enabled: true | ||
addr: localhost:6060 | ||
ping: | ||
enabled: true | ||
addr: localhost:8081 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,3 +17,6 @@ suppliers: | |
pprof: | ||
enabled: false | ||
addr: localhost:6060 | ||
ping: | ||
enabled: false | ||
addr: localhost:8082 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,3 +47,6 @@ suppliers: | |
pprof: | ||
enabled: false | ||
addr: localhost:6070 | ||
ping: | ||
enabled: false | ||
addr: localhost:8081 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -42,6 +42,11 @@ func ParseRelayMinerConfigs(configContent []byte) (*RelayMinerConfig, error) { | |||||||||||
Addr: yamlRelayMinerConfig.Pprof.Addr, | ||||||||||||
} | ||||||||||||
|
||||||||||||
relayMinerConfig.Ping = &RelayMinerPingConfig{ | ||||||||||||
Enabled: yamlRelayMinerConfig.Ping.Enabled, | ||||||||||||
Addr: yamlRelayMinerConfig.Ping.Addr, | ||||||||||||
} | ||||||||||||
Comment on lines
+45
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would then reduce:
Suggested change
|
||||||||||||
|
||||||||||||
// Hydrate the pocket node urls | ||||||||||||
if err := relayMinerConfig.HydratePocketNodeUrls(&yamlRelayMinerConfig.PocketNode); err != nil { | ||||||||||||
return nil, err | ||||||||||||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I wasn't clear in my previous comments.
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 theirconfig.suppliers[*].service_config.backend_url
are up and running and reachable before deploying a Relayminer.I disagree, they exists:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eddyzags thanks for the detailed response here! 🙌
I was referring to .yaml files referenced in this commit, but I also see that they're not referenced any more. I just didn't understand the rationale behind moving the config into the Tiltfile.
(@okdas @red-0ne thoughts?)
I was just pointing out that the config fields which you've removed from the relayminer configs correspond to the flags you've added in the Tiltfile. The point being, to question why should we prefer to provide the config via flags over the yaml file, which you answered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, Bryan; I am glad it was clear. Waiting for @okdas and @red-0ne feedback. I am open to suggestions