-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Load --grpc_auth_static_client_creds
file once
#15030
Load --grpc_auth_static_client_creds
file once
#15030
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
--grpc_auth_static_client_creds
file once
5340793
to
b1e2631
Compare
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.
Nice work! This is a much better way forward than imposing a concurrency limit on vtgate's processing of tablet records. I can re-review and approve once the feedback is addressed. They are all small things, the basic approach is obviously correct and what everyone has agreed on.
Thanks @deepthi! |
This PR seems to be causing this test failure, but I can't spot why. More 👀 appreciated! --- FAIL: TestVtctlAuthClient (0.00s)
client.go:88: failed to get first line: rpc error: code = Unauthenticated desc = username and password must be provided
FAIL
FAIL vitess.io/vitess/go/vt/vtctl/grpcvtctlclient 0.116s |
7b1556d
to
02d012f
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #15030 +/- ##
===========================================
- Coverage 47.36% 39.76% -7.60%
===========================================
Files 1144 1639 +495
Lines 238995 380084 +141089
===========================================
+ Hits 113197 151155 +37958
- Misses 117213 212721 +95508
- Partials 8585 16208 +7623 ☔ View full report in Codecov by Sentry. |
d6eed90
to
2277828
Compare
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
56fe3e3
to
a69e2a3
Compare
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.
one minor bug fix then lgtm
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
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
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
…izations (#227) * Load `--grpc_auth_static_client_creds` file once (vitessio#15030) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Filter by keyspace earlier in `tabletgateway`s `WaitForTablets(...)` (vitessio#15347) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Limit concurrent creation of healthcheck gRPC connections (vitessio#15053) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * go mod tidy Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Update MySQL apt package and GPG signature (vitessio#14785) Signed-off-by: Matt Lord <mattalord@gmail.com> * remove unrelated workflow files from v20 Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Signed-off-by: Matt Lord <mattalord@gmail.com> Co-authored-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
* Make `Durabler` interface methods public (vitessio#15548) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Signed-off-by: Manan Gupta <manan@planetscale.com> Co-authored-by: Manan Gupta <manan@planetscale.com> * Load `--grpc_auth_static_client_creds` file once (vitessio#15030) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Limit concurrent creation of healthcheck gRPC connections (vitessio#15053) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Filter by keyspace earlier in `tabletgateway`s `WaitForTablets(...)` (vitessio#15347) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Use slack-15.0 as previous release Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * empty commit Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * force ci to run * Update GH Action runners Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * test templates Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * set GH access token in build Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix reparent old tests Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Remove CIs we don't need Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Remove CIs we don't need again Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Add private repo setup to upgrade_downgrade_test_backups_e2e.yml Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Add private repo setup to more CI Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * remove CI skip logic for upstream stuff Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * CODEOWNERS Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * [release-19.0] Add timeout to all the contexts used for RPC calls in vtorc (vitessio#15991) (vitessio#16103) Signed-off-by: Manan Gupta <manan@planetscale.com> * `slack-vitess-r15.0.5`: forward-port consul topo limits PR #111 (#297) * `slack-vitess-r14.0.5`: allow conn overrides in consul topo (#111) * `slack-vitess-r14.0.5`: allow conn overrides in consul topo Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * fix e2e test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Update flags tests that didn't exist in v14 Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * update vtcombo e2e Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix err with installing percona-xtrabackup-24 Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * `slack-vitess-r15.0.5`: fix races in `Unit Test (Race)` CI, fix "old" reparent CIs (#356) * update vtcombo e2e test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix bad merge conflict fix Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * go mod tidy Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * update vtcombo e2e test again Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * [release-19.0] Upgrade the Golang version to `go1.22.5` (vitessio#16322) Signed-off-by: GitHub <noreply@github.com> Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> Co-authored-by: frouioui <frouioui@users.noreply.github.com> Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr> * merge conflict fixes Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * make vtadmin_web_proto_types Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: GitHub <noreply@github.com> Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> Co-authored-by: Manan Gupta <manan@planetscale.com> Co-authored-by: Manan Gupta <35839558+GuptaManan100@users.noreply.github.com> Co-authored-by: vitess-bot <139342327+vitess-bot@users.noreply.github.com> Co-authored-by: frouioui <frouioui@users.noreply.github.com> Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr>
Description
This PR causes the
--grpc_auth_static_client_creds
file to be loaded once in order to avoid high OS-thread usage whenvtgate
starts up. In the right circumstances, the repeat loading of this file can contribute tovtgate
crashing onruntime: program exceeds 10000-thread limit
This stack is seen using a significant amount of blocking syscalls on every spawned healthcheck stream:
At startup
vtgate
opens healthcheck streams to every tablet it is watching in parallel, and right now each of those goroutines are loading/unmarshalling the static client creds fileA consequence of this change is a process reload is required to change the file, whereas today you could change the file and it would be re-loaded (sadly by N x goroutines) on each loop of the topology watcher
.Start(...)
ticker, which to me isn't a dealbreaker (I was surprised it's reloaded actually), but I'm curious what everyone thought? I'm happy to adjust this to supportSIGHUP
and I'm curious if there are helpers for that sort of thingRelated Issue(s)
#15031
Checklist
Deployment Notes