csds: implement CSDS service handler #4243

menghanl · 2021-03-04T07:26:23Z

No description provided.

easwars · 2021-03-10T17:49:02Z

xds/csds/csds.go

+	ret := &clientStatusServer{}
+	ret.xdsClient, ret.xdsClientErr = newXDSClient()
+	if ret.xdsClientErr != nil {
+		logger.Errorf("failed to create xds client: %v", ret.xdsClientErr)


Why not return an error as a second return value instead? What good is it to have a CSDS server without an xdsClient?

It's harder to use if this function returns an error, because they cannot register this in one line.

If this fails, the server will fail all RPCs.

And also, I think the error returned here is either useless, or should have been returned by other functions.
Before this happens, the gRPC client/server that uses xDS would also fail, and the user will get the returned error from there.

I'm not convinced by the argument that returning an error makes the function harder to use. And that if the client creation runs into an error here, it would have elsewhere as well.
Given that this is a public facing API, I feel it would be better to have it stand on its own rather than have these unsaid dependencies.

I changed this to return an error.

Also the returned struct is a concrete struct, instead of the interface. So we can also get rid of the finalizer.

xds/csds/csds.go

easwars · 2021-03-10T17:52:23Z

xds/csds/csds.go

+		if err != nil {
+			return err
+		}
+		resp, errResp := s.buildClientStatusRespForReq(req)


Nit: s/errResp/err ?

easwars · 2021-03-10T17:54:16Z

xds/csds/csds.go

+// If it returns an error, the error is a status error.
+func (s *clientStatusServer) buildClientStatusRespForReq(req *v3statuspb.ClientStatusRequest) (*v3statuspb.ClientStatusResponse, error) {
+	if len(req.NodeMatchers) != 0 {
+		return nil, status.Errorf(codes.InvalidArgument, "NodeMatchers are not supported, request contains NodeMatchers: %v", req.NodeMatchers)


Nit: s/NodeMatchers/node_matchers/ in the error message.

easwars · 2021-03-10T17:54:58Z

xds/csds/csds.go

+		return nil, status.Errorf(codes.InvalidArgument, "NodeMatchers are not supported, request contains NodeMatchers: %v", req.NodeMatchers)
+	}
+	if s.xdsClient == nil {
+		return nil, status.Errorf(codes.FailedPrecondition, "xds client creation failed with error: %v", s.xdsClientErr)


Same question here? What purpose does it serve to have a CSDS server without an xdsClient?

Replied in the previous comment.

easwars · 2021-03-10T18:38:27Z

xds/csds/csds.go

+	DumpCDS() (string, map[string]client.UpdateWithMD)
+	DumpEDS() (string, map[string]client.UpdateWithMD)
+	BootstrapConfig() *bootstrap.Config
+	Close()


Same for Close().

This is a good question actually. The client used by CSDS is never closed. We should close it, but there's no easy way.

What's I'm thinking is to set a Finalizer: https://golang.org/pkg/runtime/#SetFinalizer
But not sure if it's a good idea

Added a finalizer. Does seems too bad. Let me know what you think.

Also, cc @dfawley What do you think of a finalizer?

easwars · 2021-03-10T18:39:24Z

xds/csds/csds_test.go

+		xdsC.WatchEndpoints(target, func(client.EndpointsUpdate, error) {})
+	}
+
+	for i := 0; i < retryCount; i++ {


Is there something better than retrying in a loop and sleeping in between that we can do here?

I didn't find any better way. There are real xDS RPCs involved, and we don't have control of the timing.

xds/csds/csds_test.go

easwars · 2021-03-10T18:44:55Z

xds/csds/csds_test.go

+	if err := stream.Send(&v3statuspb.ClientStatusRequest{Node: nil}); err != nil {
+		return fmt.Errorf("failed to send: %v", err)
+	}
+	r, err := stream.Recv()


We are using the streaming method in all tests. For completeness sake, should one of the checkForXxx use the unary method?

I think it would be OK, because they both simply call the same function.

But isn't that an implementation detail. Hmm ... maybe its ok.

easwars

I think the only sticking point is the API of NewClientStatusDiscoveryServer. Otherwise looks good.

easwars · 2021-03-16T17:07:23Z

xds/csds/csds.go

+	ret := &clientStatusServer{}
+	ret.xdsClient, ret.xdsClientErr = newXDSClient()
+	if ret.xdsClientErr != nil {
+		logger.Errorf("failed to create xds client: %v", ret.xdsClientErr)


I'm not convinced by the argument that returning an error makes the function harder to use. And that if the client creation runs into an error here, it would have elsewhere as well.
Given that this is a public facing API, I feel it would be better to have it stand on its own rather than have these unsaid dependencies.

easwars · 2021-03-16T17:08:05Z

xds/csds/csds.go

+		//
+		// Besides, if client cannot be created, the users should have received
+		// the error, because their client/server cannot work, either.
+		logger.Errorf("failed to create xds client: %v", ret.xdsClientErr)


Also, return from inside the if, so that the code under else need not be indented.

easwars · 2021-03-16T17:10:31Z

xds/csds/csds.go

+	case client.ServiceStatusNACKed:
+		return v3adminpb.ClientResourceStatus_NACKED
+	}
+	return v3adminpb.ClientResourceStatus_UNKNOWN


Nit: move the last return into a default case?

easwars · 2021-03-16T17:12:50Z

xds/csds/csds_test.go

+	if err := stream.Send(&v3statuspb.ClientStatusRequest{Node: nil}); err != nil {
+		return fmt.Errorf("failed to send: %v", err)
+	}
+	r, err := stream.Recv()


But isn't that an implementation detail. Hmm ... maybe its ok.

No more finalizer

easwars · 2021-03-16T18:38:37Z

xds/csds/csds.go

+	if s.xdsClient == nil {
+		// This should never happen.
+		return nil, status.Errorf(codes.FailedPrecondition, "xds client is nil")
+	}


We don't need this check anymore, right?

Removed. Should be safe now.

menghanl requested a review from easwars March 4, 2021 07:26

menghanl assigned easwars Mar 4, 2021

menghanl added the Type: Feature label Mar 4, 2021

menghanl added this to the 1.37 Release milestone Mar 4, 2021

easwars requested changes Mar 10, 2021

View reviewed changes

easwars assigned menghanl and unassigned easwars Mar 10, 2021

menghanl force-pushed the csds_real branch from e85c6b0 to 5090248 Compare March 12, 2021 00:02

menghanl assigned easwars and unassigned menghanl Mar 12, 2021

easwars reviewed Mar 16, 2021

View reviewed changes

easwars assigned menghanl and unassigned easwars Mar 16, 2021

menghanl force-pushed the csds_real branch from 7cf46bc to e485311 Compare March 16, 2021 18:15

menghanl added 9 commits March 16, 2021 11:17

[csds_real] csds

19d8087

[csds_real] node

c6436d2

[csds_real] cleanup

304ec3a

[csds_real] 222

e4d0bca

[csds_real] c1 simple

6f56a7c

[csds_real] set finalizer to close xds client

b0738ef

[csds_real] typed nil

6875ddc

[csds_real] c2

5de9e2b

[csds_real] error and cleanup in return

343bcf2

No more finalizer

menghanl force-pushed the csds_real branch from e485311 to 343bcf2 Compare March 16, 2021 18:17

easwars approved these changes Mar 16, 2021

View reviewed changes

[csds_real] no nil check

cd444ae

menghanl merged commit 95173a5 into grpc:master Mar 16, 2021

menghanl deleted the csds_real branch March 16, 2021 21:05

dependabot bot mentioned this pull request Mar 17, 2021

Bump src/google.golang.org/grpc from 61f0b5f to 967933b pivotal-cf/service-backup-release#56

Closed

github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

csds: implement CSDS service handler #4243

csds: implement CSDS service handler #4243

menghanl commented Mar 4, 2021

easwars Mar 10, 2021

menghanl Mar 11, 2021

easwars Mar 16, 2021

menghanl Mar 16, 2021

easwars Mar 10, 2021

menghanl Mar 11, 2021

easwars Mar 10, 2021

menghanl Mar 11, 2021

easwars Mar 10, 2021

menghanl Mar 11, 2021

easwars Mar 10, 2021

menghanl Mar 11, 2021

menghanl Mar 12, 2021

easwars Mar 10, 2021

menghanl Mar 11, 2021

easwars Mar 10, 2021

menghanl Mar 11, 2021

easwars Mar 16, 2021

easwars left a comment

easwars Mar 16, 2021

easwars Mar 16, 2021

menghanl Mar 16, 2021

easwars Mar 16, 2021

menghanl Mar 16, 2021

easwars Mar 16, 2021

easwars Mar 16, 2021

menghanl Mar 16, 2021

csds: implement CSDS service handler #4243

csds: implement CSDS service handler #4243

Conversation

menghanl commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment