-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
admin: implement admin services #4274
Conversation
admin/test/admin_test.go
Outdated
) | ||
|
||
func TestRegisterWithCSDS(t *testing.T) { | ||
if err := test.RunRegisterTests([]test.RunAndCode{ |
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.
I think it might be nicer to pass t
to RunRegisterTests
so it will print the proper line where the failure happened.
You can also do a t.Run
then.
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.
Done.
How to do t.Run
?
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.
t.Run("channelz", func(t *testing.T) {
if err := RunChannelz(conn); status.Code(err) != ec.ChannelzCode {
t.Fatalf("%s test failed with error %v, want code %v", "channelz", err, ec.ChannelzCode)
}
})
t.Run("csds", func(t *testing.T) {
if err := RunCSDS(conn); status.Code(err) != ec.CSDSCode {
t.Fatalf("%s test failed with error %v, want code %v", "CSDS", err, ec.CSDSCode)
}
})
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.
Done. Maybe we don't need "%s test failed"
any more? But I kept them.
internal/admin/admin.go
Outdated
|
||
var ( | ||
// services is a map from name to service register functions. | ||
services = make(map[string]func(grpc.ServiceRegistrar) (func(), error)) |
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.
Maybe remove the name? Just make it a slice?
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.
I wanted to avoid duplicates, because they panic:
Lines 596 to 598 in 967933b
if _, ok := s.services[sd.ServiceName]; ok { | |
logger.Fatalf("grpc: Server.RegisterService found duplicate service registration for %q", sd.ServiceName) | |
} |
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.
But why would we register the same service multiple times?
Also there's no guarantee that the name used when registering with admin
is the same as the name used when registering the service with the server.
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.
Maybe this should take a list of grpc.ServiceDesc
, then we can dedup based on the service name.
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.
That doesn't work yet, because CSDS's service desc isn't exported.
Removed name. This is internal anyway, should be safe.
- package for env vars - e2e test function for creating bootstrap file
Admin service contains channelz and CSDS.
CSDS is registered by the
xds
package, so that the service is registered (and dependency is imported) only whenxds
is imported.This PR also moves some packages from
xds/internal
tointernal/xds
, for testing purposesinternal/xds/env