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

xRFC TP2: Dynamically Generated Cacheable xDS Resources #10

Merged
merged 15 commits into from
May 5, 2022

Conversation

markdroth
Copy link
Contributor

Signed-off-by: Mark D. Roth <roth@google.com>
@howardjohn
Copy link
Contributor

@hzxuzhonghu @ramaraochavali should take a look as we have been looking into caching XDS in Istio. I haven't had a chance to take a look yet but planning to soon.

@hzxuzhonghu
Copy link

What's tricky for istio is: we donot know which are the dynamic parameters for xds clients in advance, because users may have policies with workload selector.

Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
Copy link
Contributor

@htuch htuch left a comment

Choose a reason for hiding this comment

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

I've read up to the API protos, looks good, but would like to discuss a little bit on the tradeoffs around matching semantics for missing labels.

Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
@markdroth
Copy link
Contributor Author

I've removed the DPDS functionality from this xRFC, since I don't think it fully solves the problem of server-specified constraints: the DPDS resource itself would be uncacheable, which means that the client needs to talk to the authoritiative server anyway, but the whole point of dynamic parameters is to make resources cacheable. Therefore, DPDS seems no better than simply marking the resources that we wanted to dynamically generate as being non-cacheable.

I do think this issue will probably need more attention later, but I think we can decouple that from the basic dynamic parameter design here.

…ic params instead

Signed-off-by: Mark D. Roth <roth@google.com>
Copy link

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks Mark, this is a great!
Left a few comments.

Copy link
Contributor

@htuch htuch left a comment

Choose a reason for hiding this comment

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

At high-level looks great. I can do a nit pick pass again when threads are wrapped.

Copy link

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

I've not read every word of the proposal, so I apologize if something was discussed and I missed it.

…rces

Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
Copy link
Contributor Author

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

I've done a major rewrite of this design, now focusing solely on the "dynamic route selection" use-case, which I think will be the overwhelmingly common case. The design has gone back to an earlier approach where the client sends dynamic parameters and the constraints are associated with the resources sent from the server. And I've simplified the constraints representation to handle the cases that I think we actually need.

@mattklein123 @htuch @fuqianggao @adisuissa @ejona86 Please review. Thanks!

Signed-off-by: Mark D. Roth <roth@google.com>
Copy link
Contributor Author

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed review, Eric!

Signed-off-by: Mark D. Roth <roth@google.com>
Copy link
Contributor

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Overall proposal looks good, a bunch of tiny and bigger comments. Thanks for putting this together @markdroth!
/wait

@mattklein123 mattklein123 self-assigned this Nov 29, 2021
Copy link
Contributor

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Sorry for the really, really, really late review. Overall this looks good. I left some comments. Thank you!

Signed-off-by: Mark D. Roth <roth@google.com>
Copy link
Contributor

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM modulo minor comments on writeup.

@mattklein123
Copy link
Contributor

@markdroth sorry catching up from leave. Where are we at with this? Should I do another pass and then we can get this merged or are there other pending changes I should wait for?

Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
@htuch
Copy link
Contributor

htuch commented Feb 10, 2022

LGTM, will defer to @mattklein123 for final approval/merge.

@markdroth
Copy link
Contributor Author

The main open issue that I think we still need to resolve here is where to land on the trade-off between flexibility of matching semantics vs. complexity of server and cache implementations, especially given how hard it will be to extend this later. I'd like to make sure we have consensus on that before merging this.

Copy link
Contributor

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. In general this looks great. My general feeling though is we should plan for the expansion of matching capabilities. Personally, I like the idea of client capabilities, as that would allow us to not only handle additions, but also potentially features that certain implementations don't want to support (e.g., "crazy regex matching option").

Signed-off-by: Mark D. Roth <roth@google.com>
Copy link
Contributor Author

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

I've updated the doc as per our recent discussions. PTAL.

@markdroth
Copy link
Contributor Author

I've sent envoyproxy/envoy#20926 with the proto changes.

@@ -562,25 +515,15 @@ that a server has the following two variants of a resource:

Choose a reason for hiding this comment

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

Are you saying that this type of configurations are not allowed based on the best practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. If the server always ensures that all variants of a resource have non-overlapping constraints for the same set of keys, then this kind of conflict cannot happen.

Choose a reason for hiding this comment

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

OK. Can you make this a bit more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the existing wording here covers this:

This situation will be avoided via a best practice that all authoritative
xDS servers should have all variants of a given resource specify
non-overlapping constraints for the same set of keys
.

Can you suggest what wording change(s) might make this clearer?

Choose a reason for hiding this comment

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

Looks good.

@fuqianggao
Copy link

/lgtm

Copy link
Contributor

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all the back and forth!

Copy link
Contributor

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mattklein123 mattklein123 merged commit 70da609 into cncf:main May 5, 2022
@markdroth markdroth deleted the dynamic_parameters branch May 6, 2022 15:25
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.

None yet

8 participants