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.

Sign up for GitHub

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

Jump to bottom

vmstorage: panic when graceful shutdown caused by push metrics #5548

Closed
zhdd99 opened this issue Dec 29, 2023 · 5 comments · Fixed by #5549
Closed

vmstorage: panic when graceful shutdown caused by push metrics #5548

zhdd99 opened this issue Dec 29, 2023 · 5 comments · Fixed by #5549
Assignees
@hagen1778
Labels
bug Something isn't working

Comments

@zhdd99
Copy link
Contributor

zhdd99 commented Dec 29, 2023

Describe the bug

showdown
panic

I found that during the graceful shutdown of a vmstorage node, a panic was caused by pushing metrics。I can fix this bug. I will submit a Merge Request later to help VictoriaMetrics resolve this issue.

To Reproduce

None

Version

vmstorage-20231101-230002-tags-v1.93.7-cluster-0-g1415cd7cd

Logs

No response

Screenshots

No response

Used command-line flags

No response

Additional information

No response
@zhdd99 zhdd99 added the bug Something isn't working label Dec 29, 2023
@zhdd99 zhdd99 mentioned this issue Dec 29, 2023
Merged
hagen1778 added a commit that referenced this issue Jan 9, 2024
@hagen1778
app/all: follow-up after 84d710b
2a7207f 
#5548
Signed-off-by: hagen1778 <roman@victoriametrics.com>
hagen1778 added a commit that referenced this issue Jan 9, 2024
@hagen1778
app/all: follow-up after 84d710b
91ccea2 
#5548
Signed-off-by: hagen1778 <roman@victoriametrics.com>
@hagen1778 hagen1778 self-assigned this Jan 9, 2024
@hagen1778
Copy link
Collaborator

Thanks for the ticket! The #5549 has been merged and will be included into the next release.

valyala added a commit that referenced this issue Jan 15, 2024
@valyala
lib/pushmetrics: wait until the background goroutines, which push met…
be509b3 
…rics, are stopped at pushmetrics.Stop()

Previously the was a race condition when the background goroutine still could try collecting metrics
from already stopped resources after returning from pushmetrics.Stop().
Now the pushmetrics.Stop() waits until the background goroutine is stopped before returning.

This is a follow-up for #5549
and the commit fe2d9f6 .

Updates #5548
valyala added a commit that referenced this issue Jan 15, 2024
@valyala
app/vmstorage: deregister storage metrics before stopping the storage
5106045 
This prevents from possible nil pointer dereference issues when the storage metrics
are read after the storage is stopped.

Updates #5548
@valyala
Copy link
Collaborator

valyala commented Jan 15, 2024

FYI, while the #5549 stops background push process from reading the metrics from the destroyed server.Server instance, it is better fixing the issue in another way - by de-registering server metrics before destroying the server itself. This makes unnecessary to stop background push process before destroying the server. This has been implemented in the commit 5106045 . This commit will be included in the next release.

valyala added a commit that referenced this issue Jan 16, 2024
@valyala
lib/pushmetrics: wait until the background goroutines, which push met…
9e5e514 
…rics, are stopped at pushmetrics.Stop()

Previously the was a race condition when the background goroutine still could try collecting metrics
from already stopped resources after returning from pushmetrics.Stop().
Now the pushmetrics.Stop() waits until the background goroutine is stopped before returning.

This is a follow-up for #5549
and the commit fe2d9f6 .

Updates #5548
valyala added a commit that referenced this issue Jan 16, 2024
@valyala
app/vmstorage: deregister storage metrics before stopping the storage
9b449df 
This prevents from possible nil pointer dereference issues when the storage metrics
are read after the storage is stopped.

Updates #5548
valyala pushed a commit that referenced this issue Jan 16, 2024
@hagen1778 @valyala
app/all: follow-up after 84d710b
978faf5 
#5548
Signed-off-by: hagen1778 <roman@victoriametrics.com>
valyala added a commit that referenced this issue Jan 16, 2024
@valyala
lib/pushmetrics: wait until the background goroutines, which push met…
4f3764b 
…rics, are stopped at pushmetrics.Stop()

Previously the was a race condition when the background goroutine still could try collecting metrics
from already stopped resources after returning from pushmetrics.Stop().
Now the pushmetrics.Stop() waits until the background goroutine is stopped before returning.

This is a follow-up for #5549
and the commit fe2d9f6 .

Updates #5548
valyala pushed a commit that referenced this issue Jan 16, 2024
@hagen1778 @valyala
app/all: follow-up after 84d710b
ce9cd9e 
#5548
Signed-off-by: hagen1778 <roman@victoriametrics.com>
valyala added a commit that referenced this issue Jan 16, 2024
@valyala
lib/pushmetrics: wait until the background goroutines, which push met…
41bdf08 
…rics, are stopped at pushmetrics.Stop()

Previously the was a race condition when the background goroutine still could try collecting metrics
from already stopped resources after returning from pushmetrics.Stop().
Now the pushmetrics.Stop() waits until the background goroutine is stopped before returning.

This is a follow-up for #5549
and the commit fe2d9f6 .

Updates #5548
valyala pushed a commit that referenced this issue Jan 16, 2024
@hagen1778 @valyala
app/all: follow-up after 84d710b
07fe834 
#5548
Signed-off-by: hagen1778 <roman@victoriametrics.com>
valyala added a commit that referenced this issue Jan 16, 2024
@valyala
lib/pushmetrics: wait until the background goroutines, which push met…
fc42900 
…rics, are stopped at pushmetrics.Stop()

Previously the was a race condition when the background goroutine still could try collecting metrics
from already stopped resources after returning from pushmetrics.Stop().
Now the pushmetrics.Stop() waits until the background goroutine is stopped before returning.

This is a follow-up for #5549
and the commit fe2d9f6 .

Updates #5548
valyala added a commit that referenced this issue Jan 16, 2024
@valyala
app/vmstorage: deregister storage metrics before stopping the storage
4b59047 
This prevents from possible nil pointer dereference issues when the storage metrics
are read after the storage is stopped.

Updates #5548
valyala pushed a commit that referenced this issue Jan 16, 2024
@hagen1778 @valyala
app/all: follow-up after 84d710b
f23d23b 
#5548
Signed-off-by: hagen1778 <roman@victoriametrics.com>
valyala added a commit that referenced this issue Jan 16, 2024
@valyala
lib/pushmetrics: wait until the background goroutines, which push met…
3023b68 
…rics, are stopped at pushmetrics.Stop()

Previously the was a race condition when the background goroutine still could try collecting metrics
from already stopped resources after returning from pushmetrics.Stop().
Now the pushmetrics.Stop() waits until the background goroutine is stopped before returning.

This is a follow-up for #5549
and the commit fe2d9f6 .

Updates #5548
valyala added a commit that referenced this issue Jan 16, 2024
@valyala
app/vmstorage: deregister storage metrics before stopping the storage
b546c31 
This prevents from possible nil pointer dereference issues when the storage metrics
are read after the storage is stopped.

Updates #5548
@valyala
Copy link
Collaborator

valyala commented Jan 16, 2024

FYI, the fix for this issue has been included in v1.87.13 LTS release. The fix will be included also into the latest release.

@valyala
Copy link
Collaborator

valyala commented Jan 16, 2024

FYI, the fix for this issue has been included in v1.93.10 LTS release.

@valyala
Copy link
Collaborator

valyala commented Jan 30, 2024

This issue has been addressed in v1.97.0. Closing it as fixed then.

@valyala valyala closed this as completed Jan 30, 2024
@chenrui333 chenrui333 mentioned this issue Jan 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hagen1778 hagen1778

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

vmstorage: fix panic caused by pushing metrics during graceful shutting down zhdd99/VictoriaMetrics
3 participants
@valyala @hagen1778 @zhdd99