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

Incremental XDS proposal. #3470

Merged
merged 11 commits into from
Jul 2, 2018
Merged
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 99 additions & 0 deletions api/envoy/api/v2/discovery.proto
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,102 @@ message DiscoveryResponse {
// required for non-stream based xDS implementations.
string nonce = 5;
}

Copy link
Member

Choose a reason for hiding this comment

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

When we get agreement on the basic protos and sequencing, we should add sequence diagrams and a section to https://github.com/envoyproxy/envoy/blob/master/api/XDS_PROTOCOL.md as part of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

// IncrementalDiscoveryRequest and IncrementalDiscoveryResponse are used in a
// new gRPC endpoint for Incremental xDS. The feature is not supported for REST
// management servers.
//
// With Incremental xDS, the IncrementalDiscoveryResponses do not need to
// include a full snapshot of the tracked resources. Instead
// IncrementalDiscoveryResponses are a diff to the state of a xDS client.
// In Incremental XDS there are per resource versions which allows to track
// state at the resource granularity.
// An xDS Incremental session is always in the context of a gRPC bidirectional
// stream. This allows the xDS server to keep track of the state of xDS clients
// connected to it.
//
// In Incremental xDS the nonce field is required and used to pair
// IncrementalDiscoveryResponse to a IncrementalDiscoveryRequest ACK or NACK.
// Optionaly, a response message level system_version_info is present for
// debugging purposes only.
//
// IncrementalDiscoveryRequest can be sent in 3 situations:
// 1. Initial message in a xDS bidirectional gRPC stream.
// 2. As a ACK or NACK response to a previous IncrementalDiscoveryResponse.
// In this case the response_nonce is set to the nonce value in the Response.
// ACK or NACK is determined by the absence or presence of error_detail.
// 3. Spontaneous IncrementalDiscoveryRequest from the client.
// This can be done to dynamically add or remove elements from the tracked
// resource_names set. In this case response_nonce must be omitted.
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to convince and explain why this doesn't have the racy behavior that originally motivated the nonce. I.e. we need sequence diagrams considering races. Can we make the changes to https://github.com/envoyproxy/envoy/blob/master/api/XDS_PROTOCOL.md as part of this PR?

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. Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good, I think that even if a nonce-less incremental subscribe races with some other request/response, it's largely harmless; the set of resources being tracked by the management server should eventually converge on what the client is interested in tracking.

Consider this example (M is management server, E is Envoy, X/Y/Z are resources):

  1. M->E (CDS): {X}
  2. E-> M (CDS): subscribe Y
  3. E->M (CDS): unsubscribe Y
  4. M->E (CDS): {X, Y, Z}

What if steps 3 and 4 race? I don't think there is a problem here, since we eventually converge on the right thing, but there will be periods of inconsistency perhaps.

If someone can persuade me that the above is valid, I think we're good to ship, but want to just raise this.

message IncrementalDiscoveryRequest {
// The node making the request.
core.Node node = 1;

// Type of the resource that is being requested, e.g.
// "type.googleapis.com/envoy.api.v2.ClusterLoadAssignment". This is implicit
// in requests made via singleton xDS APIs such as CDS, LDS, etc. but is
// required for ADS.
string type_url = 2;

// IncrementalDiscoveryRequests allow the client to add or remove individual
// resources to the set of tracked resources in the context of a stream.
// All resource names in the resource_names_subscribe list are added to the
// set of tracked resources and all resource names in the resource_names_unsubscribe
// list are removed from the set of tracked resources.
// Unlike in non incremental xDS, an empty resource_names_subscribe or
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 we should still describe here this is effectively a hint, right? Meaning, the management server is still allowed to send additional resources? (Though it must send the requested ones) I think it's worth clarifying this as it's similar to non-incremental in this regard?

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.

// resource_names_unsubscribe list simply means that no resources are to be
// added or removed to the resource list.
// The xDS server must send updates for all tracked resources but can also
// send updates for resources the client has not subscribed to. This behavior
// is similar to non incremental xDS.
// These two fields can be set for all types of IncrementalDiscoveryRequests
// (initial, ACK/NACK or spontaneous).
//
// A list of Resource names to add to the list of tracked resources.
Copy link
Member

Choose a reason for hiding this comment

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

Can we clarify here when this should be empty? For example, on an initial request for CDS when Envoy wants to be told about all relevant clusters by the management server and isn't doing any Envoy side dynamic loading stuff? I guess in general, given that we send this message in multiple different situations, it would be useful for each field to have more detail on when it is and is not set. I think it would make the documentation a lot better.

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 added a paragraph to clarify the use of these two fields. Let me know if it makes sense to you.

repeated string resource_names_subscribe = 3;
// A list of Resource names to remove from the list of tracked resources.
Copy link
Member

Choose a reason for hiding this comment

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

nit: newline before this line

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.

repeated string resource_names_unsubscribe = 4;

// This map must be populated when the IncrementalDiscoveryRequest is the
// first in a stream. The keys are the resources names of the xDS resources
// known to the xDS client. The values in the map are the associated resource
// level version info.
map<string, string> initial_resource_versions = 5;

// When the IncrementalDiscoveryRequest is a ACK or NACK message in response
// to a previous IncrementalDiscoveryResponse, the response_nonce must be the
// nonce in the IncrementalDiscoveryResponse.
// Otherwise response_nonce must be omitted.
string response_nonce = 6;

// This is populated when the previous :ref:`DiscoveryResponse <envoy_api_msg_DiscoveryResponse>`
Copy link
Member

Choose a reason for hiding this comment

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

Nit: needs blank line separator.

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.

// failed to update configuration. The *message* field in *error_details*
// provides the Envoy internal exception related to the failure.
google.rpc.Status error_detail = 7;
}

message IncrementalDiscoveryResponse {
// The version of the response data (used for debugging).
string system_version_info = 1;

// The response resources. These are typed resources that match the type url
// in the IncrementalDiscoveryRequest.
repeated Resource resources = 2 [(gogoproto.nullable) = false];

// Resources names of resources that have be deleted and to be removed from the xDS Client.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: needs blank line separator.

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.

// Removed resources for missing resources can be ignored.
repeated string removed_resources = 6;

// The nonce provides a way for IncrementalDiscoveryRequests to uniquely
// reference a IncrementalDiscoveryResponse. The nonce is required.
string nonce = 5;
}

message Resource {
// The resource level version. It allows xDS to track the state of individual
// resources.
string version = 1;

// The resource being tracked.
google.protobuf.Any resource = 2;
}