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

[cluster/vminsert]:add reload -relabelConfig on the request to /-/reload #3923

Open
wants to merge 2 commits into
base: cluster
Choose a base branch
from

Conversation

z-anshun
Copy link
Contributor

@z-anshun z-anshun commented Mar 7, 2023

When I use vminsert's relabelConfig, I found that now there is no reloaded api. However, vminsert under VM-Single has it. So, I hope to add it to the cluster/vminster.

@z-anshun z-anshun force-pushed the add-reload-cluster-vminsert branch from bbf1a17 to 26ea65b Compare March 7, 2023 09:15
@z-anshun z-anshun force-pushed the add-reload-cluster-vminsert branch from 26ea65b to f29def3 Compare March 7, 2023 09:54
@hagen1778 hagen1778 requested review from f41gh7 and valyala March 9, 2023 08:45
Copy link
Collaborator

@hagen1778 hagen1778 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@f41gh7 f41gh7 left a comment

Choose a reason for hiding this comment

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

LGTM

@z-anshun
Copy link
Contributor Author

@valyala ,please help review the code.

Copy link
Collaborator

@valyala valyala left a comment

Choose a reason for hiding this comment

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

The code looks good except one thing - usually the cluster contains multiple vminsert nodes hidden behind a load balancer. So the load balancer sends every request to /-/reload to different vminsert node. This may lead to inconsistent state at vminsert nodes, where different vmagent nodes have different relabeling configs. This adds confusion on how to properly reload relabeling configs at vminsert nodes. That's why the /-/reload endpoint wasn't added to vminsert. Right now it is possible to reload relabeling configs at vminsert nodes by sending SIGHUP signal to them.

So I'm not sure this is a good enhancement. Probably, it would be better adding an option such as -relabelConfigCheckInterval in the way similar to -promscrape.configCheckInterval at vmagent.

@z-anshun
Copy link
Contributor Author

z-anshun commented Mar 28, 2023

The code looks good except one thing - usually the cluster contains multiple vminsert nodes hidden behind a load balancer. So the load balancer sends every request to /-/reload to different vminsert node. This may lead to inconsistent state at vminsert nodes, where different vmagent nodes have different relabeling configs. This adds confusion on how to properly reload relabeling configs at vminsert nodes. That's why the /-/reload endpoint wasn't added to vminsert. Right now it is possible to reload relabeling configs at vminsert nodes by sending SIGHUP signal to them.

So I'm not sure this is a good enhancement. Probably, it would be better adding an option such as -relabelConfigCheckInterval in the way similar to -promscrape.configCheckInterval at vmagent.

@valyala very good. I found that vmagent has -promscrape.configCheckInterval and /-/reload, so I thought it would be better to add both of them to vminert.

Signed-off-by: z-anshun <1179798460@qq.com>
@z-anshun z-anshun force-pushed the add-reload-cluster-vminsert branch from e1132dc to 4cc910b Compare March 28, 2023 03:21
@z-anshun
Copy link
Contributor Author

@valyala ,please review

@Haleygo
Copy link
Collaborator

Haleygo commented Apr 25, 2024

This may lead to inconsistent state at vminsert nodes, where different vmagent nodes have different relabeling configs.

@valyala I'm not quite understand your point here. As you said, component like vmagent or vmalert already have this /-/reload endpoint, all of them can get inconsistent state if user put load balancer in front of them and use it to perform reloading. So it's a generic wrong operation for reloading multiple instances.
The right move here should be putting a sidecar for each instance, like in k8s, it's common to having a config-reloader container as sidecar along with the service, so each instance will be updated when the config changed.
I think we should expose this /-/reload for vminsert, since it gives user an option to do so, instead of waiting for configCheckInterval.

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.

None yet

5 participants