-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[loki-distributed] Add 'grpclb' port to query-frontend service #803
[loki-distributed] Add 'grpclb' port to query-frontend service #803
Conversation
5cd6aec
to
2bdd242
Compare
@Tristan971 please update the docs. @unguiculus what do you think about the PR? |
For the target port name change, I would certainly not mind, but was similarly thinking "technically breaking change" if someone made their own service for some reason, so refrained from touching it I guess we could introduce an extra named port on the deployment rather than renaming the existing? Will get docs fixed in a second |
Makes sense so we could leave the targetPort as is. |
Fixes grafana#801 Signed-off-by: Tristan Deloche <tde@hey.com>
2bdd242
to
60a5a0c
Compare
Had never tried, and can't find any doc to explain it, but it would seem that you cannot have 2 portMappings on a single container that have the same number/protocol but different names... At least on the k8s version I'm on (1.21.5) it just discards one of them. I'm afraid we'll need to stick to it being 'grpc' at the pod level until 1.0 hits |
Any updates on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #801, albeit not necessarily the right way about it...
From a maintenance perspective it's fragile to just add a duplicate port like that, and from a BC perspective we can't necessarily just rename the old one either.
There's definitely something that needs to happen, but I'm not sure this is it, even though it does the trick.