internal/cli/server: fix flag behaviour#529
Merged
Merged
Conversation
JekaMas
reviewed
Sep 23, 2022
Codecov ReportBase: 56.84% // Head: 56.83% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #529 +/- ##
===========================================
- Coverage 56.84% 56.83% -0.01%
===========================================
Files 607 607
Lines 69996 69997 +1
===========================================
- Hits 39787 39785 -2
- Misses 26792 26805 +13
+ Partials 3417 3407 -10
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
cffls
approved these changes
Sep 23, 2022
pratikspatil024
approved these changes
Sep 24, 2022
pratikspatil024
approved these changes
Sep 27, 2022
pratikspatil024
added a commit
that referenced
this pull request
Sep 27, 2022
* remove setting maxpeers to 0 for nodiscover flag * set default prometheus and open-collector endpoint * skip building grpc address from pprof address and port * fix: linters * fix and improve tests * use loopback address for prometheus and open-collector endpoint * add logs for prometheus and open-collector setup * updated the script to handle prometheus-addr * updated builder/files/config.toml Co-authored-by: Pratik Patil <pratikspatil024@gmail.com>
pratikspatil024
added a commit
that referenced
this pull request
Sep 27, 2022
* remove setting maxpeers to 0 for nodiscover flag * set default prometheus and open-collector endpoint * skip building grpc address from pprof address and port * fix: linters * fix and improve tests * use loopback address for prometheus and open-collector endpoint * add logs for prometheus and open-collector setup * updated the script to handle prometheus-addr * updated builder/files/config.toml Co-authored-by: Pratik Patil <pratikspatil024@gmail.com>
pratikspatil024
added a commit
that referenced
this pull request
Sep 29, 2022
* Added script to generate config.toml fromstart.sh (#518) * added go and bash script to get config out of start.sh and updated flagset.go * changed 'requiredblocks' flag back to 'eth.requiredblocks' * updated script * changed 'requiredblocks' flag back to 'eth.requiredblocks' * updated tests, and removed requiredblocks from json and hcl * addressed comments * internal/cli/server: fix flag behaviour (#529) * remove setting maxpeers to 0 for nodiscover flag * set default prometheus and open-collector endpoint * skip building grpc address from pprof address and port * fix: linters * fix and improve tests * use loopback address for prometheus and open-collector endpoint * add logs for prometheus and open-collector setup * updated the script to handle prometheus-addr * updated builder/files/config.toml Co-authored-by: Pratik Patil <pratikspatil024@gmail.com> Co-authored-by: Manav Darji <manavdarji.india@gmail.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
This PR achieves the following things
When the flag
nodiscoverwas enabled in the old-cli, the node would not discover other peers but still connected to static and trusted peers if required, which was much convenient for our validator and sentry architecture. But, in the new-cli, when we set this flag, we also used to setmaxpeersto 0. This would not allow the node to connect to any static peers. Ideally, we've suggested people to use this setting of flags (nodiscoveralong withmaxpeers=0). Now, we can either get rid of this flag and keep the same behaviour in client as it is now by not translating it the new toml file using @pratikpatil024's migration script or we can keep the same behaviour ofnodiscoverin the client (as geth does). I prefer the latter one.We were unable to send metrics to datadog from some of our internal validator nodes running the new-cli. On investigating further, we found out that in the old-cli, prometheus address was set on based on
metrics.addrandmetrics.portflags (orpprof.addrandpprof.portflag if the earlier ones are not available). The defaults address in any case is127.0.0.1:7071. So, ideally even if the users have not provided these flags, the prometheus metrics are exported on the default address. In case of the new-cli, currently we need to explicitly set the address for prometheus. Instead we can set them as default so that the changes on our end (w.r.t to metrics collector) are minimized. Also, if these flags are available, use them to derive prometheus address in the migration script.As finalized in (2), the default port for prometheus is 7071 and with the migration script, we're translating the pprof address + port combination to grpc address. Most of the nodes have pprof port set to 7071 and hence it would not allow prometheus to work. My suggestion is to ignore all pprof flags and just rely on default grpc port which is 3131.