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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

admin: implement admin services #4274

Merged
merged 11 commits into from Mar 22, 2021
Merged

admin: implement admin services #4274

merged 11 commits into from Mar 22, 2021

Conversation

menghanl
Copy link
Contributor

Admin service contains channelz and CSDS.
CSDS is registered by the xds package, so that the service is registered (and dependency is imported) only when xds is imported.

This PR also moves some packages from xds/internal to internal/xds, for testing purposes

  • internal/xds/env
  • testing bootstrap generation

admin/admin.go Outdated Show resolved Hide resolved
admin/admin.go Outdated Show resolved Hide resolved
admin/admin.go Outdated Show resolved Hide resolved
)

func TestRegisterWithCSDS(t *testing.T) {
if err := test.RunRegisterTests([]test.RunAndCode{
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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)
	}
})

Copy link
Contributor Author

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.

admin/test/utils.go Outdated Show resolved Hide resolved
admin/test/utils.go Outdated Show resolved Hide resolved

var (
// services is a map from name to service register functions.
services = make(map[string]func(grpc.ServiceRegistrar) (func(), error))
Copy link
Member

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?

Copy link
Contributor Author

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:

grpc-go/server.go

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)
}

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@dfawley dfawley assigned menghanl and unassigned dfawley Mar 19, 2021
@menghanl menghanl merged commit d26af8e into grpc:master Mar 22, 2021
@menghanl menghanl deleted the admin_api branch March 22, 2021 22:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants