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

A30: xDS v3 Support #189

Merged
merged 7 commits into from
Jul 23, 2020
Merged

A30: xDS v3 Support #189

merged 7 commits into from
Jul 23, 2020

Conversation

markdroth
Copy link
Member

@markdroth markdroth commented Jun 23, 2020

No description provided.

@markdroth markdroth marked this pull request as ready for review June 25, 2020 17:51
@markdroth markdroth requested review from ejona86 and dfawley and removed request for ejona86 June 25, 2020 17:51
@markdroth markdroth changed the title WIP: A30: xDS v3 Support A30: xDS v3 Support Jun 25, 2020
A30-xds-v3.md Outdated Show resolved Hide resolved
A30-xds-v3.md Outdated Show resolved Hide resolved
A30-xds-v3.md Outdated Show resolved Hide resolved
A30-xds-v3.md Show resolved Hide resolved
request v2 resources, but it's fine for the server to send us either v2
or v3 resources. Similarly, if we're using the v3 transport protocol,
we will request v3 resources, but it's fine for the server to send us
either v2 or v3 resources.
Copy link
Member

Choose a reason for hiding this comment

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

Except that the server doesn't know that the client supports v2. When we add support for v3.1, we would drop support for these v2 resources and newer clients would break.

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, because of make-before-break, I think this is okay. The client will still deserialize the resources as whatever version it wants, regardless of which version the server used when serializing.

Copy link
Member

Choose a reason for hiding this comment

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

There is a mapping from v2 resource names to v3 resource names. You don't just look at the message name, "oh, this ends with Listener, let's assume it is an envoy.config.listener.v3.Listener". The version is part of the Any and type_url. When the client drops v2 support, are you saying it will keep the v2→v3 message name mapping and continue supporting v2 protos? That is not at all obvious to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see what you're saying. I agree that this is a concern.

I see two possible options here:

  1. We continue to accept the v2 type_url even after we drop v2 support, within some bounds (e.g., maybe we could say that we will continue to support v2 type_urls for as long as we're supporting v3, just to allow this type of transition).
  2. Stick with the plan of dropping v2 support and just document the fact that servers can use this as a transition mechanism but need to stop sending v2 protos to v3 clients before they can upgrade to a version of gRPC that does not have v2 support.

My inclination is to go with (2), but I'd be willing to do (1) if there's a need for it.

@htuch, how is Envoy handling this?

Copy link

Choose a reason for hiding this comment

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

As above. The idea is that mixed version support only exists during the 1 year transition period and is only there for supporting rollouts where they may be mixed client/server features as an ephemeral process.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a paragraph clarifying that this is just intended for use as a rollout strategy, not something that can be assumed in the general case.

A30-xds-v3.md Outdated Show resolved Hide resolved
A30-xds-v3.md Outdated Show resolved Hide resolved
A30-xds-v3.md Outdated Show resolved Hide resolved
A30-xds-v3.md Outdated
It will assume that all servers speak version N-1, and there will be a
server feature defined in the bootstrap file (see below) to tell it that
the server supports version N. When speaking to the server, gRPC will use
the highest version that is common to both itself and the server.
Copy link

Choose a reason for hiding this comment

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

The minor versioning proposal features version negotiation, should this be the preferred mechanism all things being equal?

Copy link
Member Author

Choose a reason for hiding this comment

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

The minor versioning proposal says that there will be some mechanism for version negotiation, but it does not actually define what that mechanism will be. The current text of the proposal suggests using context params, but in offline conversation, you and I and Matt agreed that that won't actually work due to the exact-match semantics of context_params. I believe that where we left this was that when someone actually gets around to implementing the minor version proposal, they will need to come up with a concrete proposal for version negotiation. So I think that how this works is an open question, and I don't want to say anything about that here until we have a concrete proposal to evaluate.

It is also worth noting that the way our implementation is going to work, the version configured in the bootstrap file is effectively the transport version, not the resource version, whereas the version negotiation mechanism is by necessity primarily about resource version. As an example, the version negotiation mechanism probably won't be usable to control things like the "xds transport next steps" design that you're currently working on implementing, because the client needs to know whether to use it before it sends the initial request containing its supported versions. This is another reason why I believe it would be beneficial to consider the transport version and the resource version independently: in terms of mechanism, transport changes are generally going to be configured via client-side configuration rather than version negotiation.

A30-xds-v3.md Outdated Show resolved Hide resolved
A30-xds-v3.md Show resolved Hide resolved
A30-xds-v3.md Outdated
field being added in https://github.com/envoyproxy/envoy/pull/11754 to
specify the transport version of LRS. However, since this field will exist
only in xDS v3, this would still not provide a clean way to specify use
of LRS v3 while using xDS v2.
Copy link

Choose a reason for hiding this comment

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

I think maybe we could add this to v2. The v2 freeze is there to encourage folks to move to v3, if we're adding a field to explicitly make this easier, then why not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. I've sent you envoyproxy/envoy#11824.

A30-xds-v3.md Outdated Show resolved Hide resolved
A30-xds-v3.md Show resolved Hide resolved
request v2 resources, but it's fine for the server to send us either v2
or v3 resources. Similarly, if we're using the v3 transport protocol,
we will request v3 resources, but it's fine for the server to send us
either v2 or v3 resources.
Copy link

Choose a reason for hiding this comment

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

As above. The idea is that mixed version support only exists during the 1 year transition period and is only there for supporting rollouts where they may be mixed client/server features as an ephemeral process.

call this the *resource version*.

When gRPC uses v3 to talk to a server, the primary thing that it is
choosing is the transport protocol version, not the resource version.
Copy link

Choose a reason for hiding this comment

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

One consideration worth noting is that the transport version only can currently change based on major version. There is no minor/patch in the gRPC endpoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I realize that. But this section of the doc is specific to the v2->v3 transition, and in this case, the major version number is the only relevant part.

A30-xds-v3.md Outdated Show resolved Hide resolved
A30-xds-v3.md Outdated

Note that the ability to accept resource versions that are different
than what was requested is only intended for use as a rollout strategy
during transitions where the management server has some a prior
Copy link
Member

Choose a reason for hiding this comment

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

"a priori" or "prior"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Looks good, just one thing to consider.


### Version Support Policy

gRPC's policy will be to support two versions of xDS at any given time.
Copy link
Member

Choose a reason for hiding this comment

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

Is this "at most two versions"? Or "exactly two versions"? If v4 isn't released for 5 years, will we support v2 for that long?

Copy link
Member Author

Choose a reason for hiding this comment

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

The current plan is to have one new version per year, so I think this distinction isn't important. If the version release schedule changes, then we can change our policy at that point.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds fair. It does seem likely there will be an xDS version bump every time there is an opportunity to do so.


### Version Support Policy

gRPC's policy will be to support two versions of xDS at any given time.
Copy link
Member

Choose a reason for hiding this comment

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

That sounds fair. It does seem likely there will be an xDS version bump every time there is an opportunity to do so.

@markdroth markdroth merged commit 2ffadbb into grpc:master Jul 23, 2020
@markdroth markdroth deleted the xds_v3 branch July 23, 2020 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants