Skip to content

ads: aggregated discovery service.#124

Merged
htuch merged 1 commit intoenvoyproxy:masterfrom
htuch:ads
Jul 27, 2017
Merged

ads: aggregated discovery service.#124
htuch merged 1 commit intoenvoyproxy:masterfrom
htuch:ads

Conversation

@htuch
Copy link
Copy Markdown
Member

@htuch htuch commented Jul 26, 2017

See README.md for motivation and usage.

Fixes #102.

See README.md for motivation and usage.
@htuch
Copy link
Copy Markdown
Member Author

htuch commented Jul 26, 2017

service AggregatedDiscoveryService {
// This is a gRPC-only API.
rpc StreamAggregatedResources(stream DiscoveryRequest)
returns (stream DiscoveryResponse) {
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Jul 26, 2017

Choose a reason for hiding this comment

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

Even though we have dropped the canary conversion for now, do we still want to make the API a stream of "atomic" lists of xDS updates? I see no downside since from an implementation perspective initially it could just still go through them in order, but in the future it would allow us to to "atomic" things more easily without an API change?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I considered this and opted for simplicity in this PR, but I'm open to adopting the atomic list concept if we can provide a concrete use case. By moving to lists of updates in each response we also make the requests more complicated, as they need to express multiple version numbers (for each singleton xDS API that gets updated or fails its update). I think it's worth doing if we have a strong motivator here, but all things being equal I'm tending towards simplicity over future proofing.

There's a nice property of the existing API that a server that can handle ADS requests can also handle arbitrary singleton xDS requests as well, if we add more structure to the ADS request then this goes away, but maybe it's not that important.

distinct streams, one for *X* and one for *Y*. ADS allows these to be
coalesced to a single stream to a single management server, avoiding the need
for distributed synchronization to correctly sequence the update. With ADS,
the management server would deliver the CDS, EDS and then RDS updates on a
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about LDS updates? Should the sequence be CDS, EDS,RDS and then LDS?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LDS must come before RDS so the need for RDS is setup. LDS update will not become active until RDS is initialized (which could happen very quickly if it's the next message processed).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In this example, the idea was that LDS wasn't changing, it was purely routes and clusters. Above I mention the sequence as [CDS, EDS, LDS, RDS]. This is because LDS will warm on RDS and we allow Envoy to be aware of all the expected RDS resources via LDS ahead of the RDS delivery. If there' some reason it should be RDS and then LDS, then we should change things.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah good point about version numbers. OK let's just keep it simple. We can always add another API later if needed.

@htuch htuch merged commit fefd8a6 into envoyproxy:master Jul 27, 2017
@htuch htuch deleted the ads branch July 27, 2017 17:22
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.

3 participants