Skip to content
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

internal/cli/server: fix flag behaviour #529

Merged
merged 9 commits into from
Sep 27, 2022

Conversation

manav2401
Copy link
Contributor

@manav2401 manav2401 commented Sep 23, 2022

This PR achieves the following things

  1. When the flag nodiscover was 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 set maxpeers to 0. This would not allow the node to connect to any static peers. Ideally, we've suggested people to use this setting of flags (nodiscover along with maxpeers=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 of nodiscover in the client (as geth does). I prefer the latter one.

  2. 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.addr and metrics.port flags (or pprof.addr and pprof.port flag if the earlier ones are not available). The defaults address in any case is 127.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.

  3. 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.

@manav2401 manav2401 requested a review from a team September 23, 2022 12:20
scripts/getconfig.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2022

Codecov Report

Base: 56.84% // Head: 56.83% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (6831a1c) compared to base (77db80c).
Patch coverage: 50.00% of modified lines in pull request are covered.

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     
Impacted Files Coverage Δ
internal/cli/server/server.go 30.89% <0.00%> (-0.36%) ⬇️
internal/cli/server/config.go 72.91% <100.00%> (-0.07%) ⬇️
p2p/discover/v4_udp.go 72.75% <0.00%> (-3.19%) ⬇️
les/distributor.go 79.68% <0.00%> (-3.13%) ⬇️
les/utils/limiter.go 90.55% <0.00%> (-1.67%) ⬇️
rpc/client.go 83.72% <0.00%> (-1.17%) ⬇️
p2p/simulations/mocker.go 30.00% <0.00%> (-1.12%) ⬇️
eth/protocols/snap/sync.go 70.30% <0.00%> (-0.98%) ⬇️
les/vflux/server/prioritypool.go 81.25% <0.00%> (-0.94%) ⬇️
p2p/simulations/network.go 57.76% <0.00%> (-0.88%) ⬇️
... and 16 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cffls cffls self-requested a review September 23, 2022 18:07
@manav2401 manav2401 marked this pull request as draft September 24, 2022 05:05
@manav2401 manav2401 marked this pull request as ready for review September 26, 2022 06:43
Copy link
Contributor

@cffls cffls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@pratikspatil024 pratikspatil024 merged commit 4573147 into develop Sep 27, 2022
@pratikspatil024 pratikspatil024 deleted the manav/pos-830-fix-flags branch September 27, 2022 08:06
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>
cffls pushed a commit to cffls/bor that referenced this pull request Sep 29, 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants