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

xds: v3 support: client bootstrap #3723

Merged
merged 7 commits into from
Jul 9, 2020
Merged

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Jun 30, 2020

This PR adds support in the client bootstrap code to support xDS v3 API.

  • Server support for v3 is indicated in the bootstrap file by a new field server_features.
  • On the client side, v3 API is enabled when the GRPC_XDS_EXPERIMENTAL_V3_SUPPORT environment variable is set to true.

Rest of the client side v3 support will be added in follow up PRs.


This change is Reviewable

@easwars easwars requested a review from menghanl June 30, 2020 22:01
@easwars easwars added no release notes Type: Feature New features or improvements in behavior labels Jun 30, 2020
@easwars
Copy link
Contributor Author

easwars commented Jul 7, 2020

I don't understand the travis test failures. I have locally been able to run the tests with -race and -count=1000 and they pass all the time.

Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 9 files at r1.
Reviewable status: 8 of 9 files reviewed, 5 unresolved discussions (waiting on @easwars and @menghanl)


xds/internal/client/client.go, line 137 at r1 (raw file):

	if opts.Config.TransportAPI == version.TransportV2 {
		c.v2c = newXDSV2Client(c, cc, opts.Config.NodeProto.(*corepb.Node), backoff.DefaultExponential.Backoff, c.logger)
	}

else? Return error? Or this will cause nil panic?


xds/internal/client/bootstrap/bootstrap.go, line 145 at r1 (raw file):

			// does not contain the deprecated field "build_version" from the
			// v2.Node proto. We expect this to succeed because we do not expect
			// the bootstrap file to contain the "build_version" field.

Even if it does, unmarshal won't fail, because of AllowUnknownFields


xds/internal/client/bootstrap/bootstrap.go, line 226 at r1 (raw file):

Quoted 4 lines of code…
		v3 := &v3corepb.Node{}
		if c.NodeProto != nil {
			v3 = c.NodeProto.(*v3corepb.Node)
		}

Does this save an allocation?

v3, _ := c.NodeProto.(*v3corepb.Node)
if v3 == nil {
  v3 = &v3corepb.Node{}
}

xds/internal/client/bootstrap/bootstrap.go, line 249 at r1 (raw file):

	// user_agent_version. But the management servers are still using the old
	// field, so we will keep both set.
	// API version for xDS transport protocol. This describes the xDS gRPC/REST

Is this comment right?


xds/internal/version/version.go, line 20 at r1 (raw file):

// Package version defines supported xDS API versions.
package version

Move to client/?
This is only used by clients, not by the balancers/resolvers, right?

@menghanl
Copy link
Contributor

menghanl commented Jul 8, 2020

Travis failure could be googleapis/google-cloud-go#2417
Fixed by their 0.59.0 release (though there's a newer 0.60.0)

But we don't explicitly depend on them. Try to update golang.org/x/oauth2 maybe

Copy link
Contributor Author

@easwars easwars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 8 of 9 files reviewed, 5 unresolved discussions (waiting on @menghanl)


xds/internal/client/client.go, line 137 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

else? Return error? Or this will cause nil panic?

Done.


xds/internal/client/bootstrap/bootstrap.go, line 145 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Even if it does, unmarshal won't fail, because of AllowUnknownFields

Updated the comment.


xds/internal/client/bootstrap/bootstrap.go, line 226 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…
		v3 := &v3corepb.Node{}
		if c.NodeProto != nil {
			v3 = c.NodeProto.(*v3corepb.Node)
		}

Does this save an allocation?

v3, _ := c.NodeProto.(*v3corepb.Node)
if v3 == nil {
  v3 = &v3corepb.Node{}
}

Done. Thanks.


xds/internal/client/bootstrap/bootstrap.go, line 249 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Is this comment right?

The comment by itself is correct :), but I don't remember why I put it here. Removed it.


xds/internal/version/version.go, line 20 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Move to client/?
This is only used by clients, not by the balancers/resolvers, right?

Will probably be used on the server side as well.

@easwars
Copy link
Contributor Author

easwars commented Jul 9, 2020

Travis failure could be googleapis/google-cloud-go#2417
Fixed by their 0.59.0 release (though there's a newer 0.60.0)

But we don't explicitly depend on them. Try to update golang.org/x/oauth2 maybe

Updating didn't help. Although that issue is marked as closed, it does look like the problem is not completely fixed. I reverted the change to enable the leak check. Let's see if travis becomes happy.

@menghanl
Copy link
Contributor

menghanl commented Jul 9, 2020

Try to explicitly ping cloud libraries to 0.60.0?
If the leak still happens, we should ping the issue.

@easwars
Copy link
Contributor Author

easwars commented Jul 9, 2020

Travis is happy.

Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 9 files at r1, 3 of 5 files at r2, 1 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @easwars)


go.sum, line 69 at r3 (raw file):

honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc h1:/hemPrYIhOhy8zYrNj+069zDB68us2sMGsfkFJO0iZs=
honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=

Revert?

@easwars easwars merged commit d8193ee into grpc:master Jul 9, 2020
@easwars easwars deleted the node_proto_cleanup branch August 4, 2020 21:11
@easwars easwars changed the title xds: Add v3 support for client bootstrap. xds: v3 support: client bootstrap Mar 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants