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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[vtadmin] grpc healthserver + channelz #10038

Merged
merged 6 commits into from
Apr 5, 2022

Conversation

ajm188
Copy link
Contributor

@ajm188 ajm188 commented Apr 5, 2022

Description

This started off as a branch to more properly use the health server, but then ended up doing some misc cleanup and adding in channelz, which I wanted to do as well.

Main changes:

  1. Add --rbac to the vtadmin-up example script.
  2. Remove Services from grpcserver.Options, since (a) we're gonna inspect the server for what services are registered and (b) there was no flag to ever let users set custom services anyway!! 馃檲
  3. Add an option to enable channelz, which is useful for debugging (I was playing around with this when working on the custom resolver stuff)

channelz demo

First, I started vtadmin without --grpc-enable-channelz and tried to run a client against it. As expected, it failed with:

F0405 09:07:41.291721   63470 main.go:22] rpc error: code = Unimplemented desc = unknown service grpc.channelz.v1.Channelz

Then, I restarted vtadmin-api with --grpc-enable-channelz, and ran the following (linking to gist because it will probably exceed GitHub's PR body size limit). It starts one goroutine that polls the GetClusters endpoint, and then runs GetTopChannels from channelz in a loop until we get something non-empty (to avoid races between this and the goroutine).

Gist

Related Issue(s)

Checklist

  • Should this PR be backported? no
  • Tests were added or are not required n/a
  • Documentation was added or is not required

Deployment Notes

Andrew Mason added 6 commits April 5, 2022 06:27
This was introduced and made a required flag (with mutually-exclusive `--no-rbac`)
in vitessio#9972.

Signed-off-by: Andrew Mason <andrew@planetscale.com>
Instead of requiring users to know in advance (and put into their startup
scripts), which services will run, we can (correctly) inspect the service
names that have been registered on the underlying grpc.Server and mark
them as SERVING in the health service.

_Alsooooo_, this was never bound to a flag, so users couldn't use it
anyway! Whoops.

Signed-off-by: Andrew Mason <andrew@planetscale.com>
Signed-off-by: Andrew Mason <andrew@planetscale.com>
Signed-off-by: Andrew Mason <andrew@planetscale.com>
Signed-off-by: Andrew Mason <andrew@planetscale.com>
Signed-off-by: Andrew Mason <andrew@planetscale.com>
@ajm188 ajm188 added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: VTAdmin VTadmin interface release notes labels Apr 5, 2022
@ajm188 ajm188 added this to the v14.0 milestone Apr 5, 2022
@ajm188 ajm188 requested a review from vmg April 5, 2022 13:22
@ajm188 ajm188 added this to In progress in VTAdmin via automation Apr 5, 2022
@ajm188 ajm188 merged commit 2d1681a into vitessio:main Apr 5, 2022
VTAdmin automation moved this from In progress to Done Apr 5, 2022
@ajm188 ajm188 deleted the andrew/vtadmin-grpc-healthserver branch April 5, 2022 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: VTAdmin VTadmin interface Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants