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

neofs-node: Reload config for pprof and metrics on SIGHUP #2094

Conversation

acid-ant
Copy link
Contributor

Close #1868

Signed-off-by: Anton Nikiforov an.nikiforov@yadro.com

@sami-nspcc
Copy link

I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3705

@codecov
Copy link

codecov bot commented Nov 24, 2022

Codecov Report

Merging #2094 (e8324d4) into master (7e8fee2) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head e8324d4 differs from pull request most recent head 49de91a. Consider uploading reports for the commit 49de91a to get more accurate results

@@            Coverage Diff             @@
##           master    #2094      +/-   ##
==========================================
- Coverage   30.71%   30.70%   -0.02%     
==========================================
  Files         383      382       -1     
  Lines       28277    28121     -156     
==========================================
- Hits         8685     8634      -51     
+ Misses      18862    18754     -108     
- Partials      730      733       +3     
Impacted Files Coverage Δ
cmd/neofs-node/closer.go 0.00% <0.00%> (ø)
cmd/neofs-node/config.go 0.00% <0.00%> (ø)
cmd/neofs-node/httpcomponent.go 0.00% <0.00%> (ø)
cmd/neofs-node/main.go 0.00% <0.00%> (ø)
cmd/neofs-node/object.go 0.00% <0.00%> (ø)
cmd/neofs-node/worker.go 0.00% <0.00%> (ø)
pkg/local_object_storage/shard/get.go 77.58% <0.00%> (-9.09%) ⬇️
cmd/neofs-cli/internal/key/wallet.go 87.50% <0.00%> (-5.61%) ⬇️
pkg/local_object_storage/writecache/put.go 65.85% <0.00%> (-5.58%) ⬇️
pkg/local_object_storage/shard/lock.go 50.00% <0.00%> (-4.55%) ⬇️
... and 40 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

is it possible to provide some sort of a dynamic configuration to our http servers the same way it is done for logger? at least that is how i planned SIGHUP unification

@sami-nspcc
Copy link

I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3751

cmd/neofs-node/config.go Outdated Show resolved Hide resolved
cmd/neofs-node/pprof.go Outdated Show resolved Hide resolved
cmd/neofs-node/pprof.go Outdated Show resolved Hide resolved
@sami-nspcc
Copy link

I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3766

@sami-nspcc
Copy link

I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3767

@sami-nspcc
Copy link

I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3768

@sami-nspcc
Copy link

I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3769

cmd/neofs-node/httpcmp.go Outdated Show resolved Hide resolved
@sami-nspcc
Copy link

I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3772

cmd/neofs-node/closer.go Outdated Show resolved Hide resolved
cmd/neofs-node/config.go Outdated Show resolved Hide resolved
cmd/neofs-node/httpcmp.go Outdated Show resolved Hide resolved
pkg/util/logger/logger.go Outdated Show resolved Hide resolved
cmd/neofs-node/httpcmp.go Outdated Show resolved Hide resolved
@sami-nspcc
Copy link

I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3778

@acid-ant
Copy link
Contributor Author

acid-ant commented Dec 8, 2022

Current behavior for metrics:

  • exclude from requests processing when disabling via SIGHUP
  • initialized always
  • metrics related to node state set always

func delCloser(c *cfg, name string) {
for i, clsr := range c.closers {
if clsr.name == name {
c.closers[i] = c.closers[len(c.closers)-1]
Copy link
Member

Choose a reason for hiding this comment

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

can't say for sure but think that we can rely on the closers' order now or in the future. not sure

Copy link
Contributor Author

@acid-ant acid-ant Jan 11, 2023

Choose a reason for hiding this comment

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

It is a simplest way to detect do we have closer/worker for the httpcomponent or not. In other case we need to remember id for worker and closer and do not forget to set it to negative value to mark that closer/worker for component deleted or not initialized at that moment, and do not forget to check this when reloading, and at the end when removing closer/worker for pprof we need to decrement index for metrics and the same for metrics. We have less than 20 workers\closers currently to iterate over. You prefer this approach?

cmd/neofs-node/config.go Show resolved Hide resolved
cmd/neofs-node/config.go Outdated Show resolved Hide resolved
cmd/neofs-node/httpcomponent.go Show resolved Hide resolved
@carpawell
Copy link
Member

Changelog conflicts.

pkg/util/http/server.go Outdated Show resolved Hide resolved
pkg/network/transport/object/grpc/service.go Outdated Show resolved Hide resolved
cmd/neofs-node/worker.go Outdated Show resolved Hide resolved
Signed-off-by: Anton Nikiforov <an.nikiforov@yadro.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.

Reload pprof/metrics services on SIGHUP
5 participants