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

fix query-frontend request load balancing when using k8s service #7966

Merged
merged 3 commits into from
Dec 19, 2022
Merged

fix query-frontend request load balancing when using k8s service #7966

merged 3 commits into from
Dec 19, 2022

Conversation

sandeepsukhani
Copy link
Contributor

@sandeepsukhani sandeepsukhani commented Dec 19, 2022 •
edited
Loading

What this PR does / why we need it:
We noticed an imbalance in the requests sent to the Query Frontend pods using query-frontend k8s service. This seems to be caused by query-frontend being a headless service that resolves to each QF pod IP and leaves it up to the client to load balance the requests as mentioned here.

This PR fixes the issue by creating two separate services for pod IP discovery and load balancing of queries:

  • query-frontend to be used for load balancing incoming Loki queries.
  • query-frontend-headless to be used for discovering QF pod IPs from queriers to connect as workers.

Checklist

  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@sandeepsukhani sandeepsukhani requested a review from a team as a code owner December 19, 2022 06:25
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@sandeepsukhani sandeepsukhani changed the title use separate query-frontend services for pod IP discovery and load balancing of queries fix query-frontend request load balancing when using k8s service Dec 19, 2022
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

dannykopping
dannykopping approved these changes Dec 19, 2022
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM!
I think we should add this to the upgrade guide as well

@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 19, 2022
@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Dec 19, 2022
dannykopping
dannykopping approved these changes Dec 19, 2022
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM!

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@sandeepsukhani sandeepsukhani merged commit 9e7ac3d into grafana:main Dec 19, 2022
@sandeepsukhani sandeepsukhani mentioned this pull request Jan 2, 2023
Merged
sandeepsukhani added a commit that referenced this pull request Jan 2, 2023
@sandeepsukhani
make query-frontend service grpc lb friendly (#8025)
3a8325d 
**What this PR does / why we need it**:
In PR #7966, I made changes to Query Frontend service. Part of it was
creating a new service.
To make the service properly load balance grpc requests, we need to use
`grpclbServiceFor`, but since we do not use grpc in QF, I went with the
usual method, i.e. I used `serviceFor`. Thinking more about it, I think
there is no harm in keeping it grpc LB friendly, and it would save us
from any possible bugs if we make QF accept queries via grpc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dannykopping dannykopping dannykopping approved these changes

Assignees
No one assigned
Labels
size/M type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sandeepsukhani @grafanabot @dannykopping