clarify resource warming docs in xds protocol #22377
Conversation
Signed-off-by: Rama Chavali <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Rama Chavali <[email protected]>
|
I'm not sure how much of the current EDS/RDS warming behavior we really want to document, given that we have agreed in #13009 that the current behavior needs to change. |
I agree that is is going to change - but having the current state updated is important IMO. We can add a disclaimer that it is going to change. |
|
@envoyproxy/api-shepherds looks like this needs a tie-breaker. @adisuissa as the other reviewer do you have thoughts? |
|
My $0.02 is that this is implementation specific, not generic xDS protocol documentation. There should be a place for it and we can accept it now and update later. It might be fine to just put it in a Sphinx RST informational box as an implementation consideration. I agree with @markdroth not to conflate the Envoy specifics with desired protocol behavior. |
I am not suggesting to conflate the Envoy specifics with the desired protocol behaviour. However I think it is important to document what Envoy does as it caused confusion in Istio on multiple occasions . I thought of this place because we documented resource warming here. But happy to move it over to another place if that helps |
|
I like the idea of just doing some type of "client specific note" box for now. We can move this kind of thing out later, and it will be more obvious that it isn't part of the main spec. So for example somewhere near this do something like: I think that should be OK for now? |
Signed-off-by: Rama Chavali <[email protected]>
Signed-off-by: Rama Chavali <[email protected]>
Signed-off-by: Rama Chavali <[email protected]>
|
@mattklein123 changed as your suggestion. PTAL |
docs/root/api-docs/xds_protocol.rst
Outdated
|
|
||
| Envoy specific implementation note: Warming of ``Cluster`` is completed only | ||
| when a new ``ClusterLoadAssignment`` response is supplied by management server | ||
| even though there is no change in endpoints. However, If Envoy is already aware of |
There was a problem hiding this comment.
| even though there is no change in endpoints. However, If Envoy is already aware of | |
| even if there is no change in endpoints. However, If Envoy is already aware of |
?
docs/root/api-docs/xds_protocol.rst
Outdated
| ``RouteConfiguration`` referenced by ``Listener``, it will use it otherwise Envoy will | ||
| initiate a new RDS request with the new route name referenced by ``Listener``. |
There was a problem hiding this comment.
I can't parse this. Is this related to Cluster or unrelated and a different note about warming? Can you clarify and if it's different move it to a separate note?
There was a problem hiding this comment.
It is about listener warming with route. It is under same resource warming category.
There was a problem hiding this comment.
OK then I don't understand this and how it relates to the cluster part. Can you clarify? Thanks.
There was a problem hiding this comment.
It is not related to cluster warming. It is related to resource warming only but talks about how listeners are warmed.
There was a problem hiding this comment.
Then please split it out into a separate note and/or make it a clearly separate statement. I'm having a hard time parsing what this means. Thank you!
There was a problem hiding this comment.
I clarified it. PTAL
Signed-off-by: Rama Chavali <[email protected]>
Signed-off-by: Rama Chavali <[email protected]>
docs/root/api-docs/xds_protocol.rst
Outdated
| Envoy specific implementation note: Warming of ``Cluster`` is completed only | ||
| when a new ``ClusterLoadAssignment`` response is supplied by management server | ||
| even if there is no change in endpoints. | ||
| Warming of ``Listener`` is completed even if management server does not send a | ||
| response for ``RouteConfiguration`` referenced by ``Listener``. Envoy will use the | ||
| previously sent ``RouteConfiguration`` to finish ``Listener`` warming. Management Server | ||
| has to send the ``RouteConfiguration`` response only if it has changed or it was never | ||
| sent in the past. |
There was a problem hiding this comment.
nit: split this into 2 notes, or a bulleted list of 2 notes. It's not super clear which parts are related.
Signed-off-by: Rama Chavali <[email protected]>
Signed-off-by: Rama Chavali <[email protected]>
Signed-off-by: Rama Chavali <[email protected]>
Clusters expect EDS response even if they do not change so EDS response is mandatory to finish warming. But listener use route configuration that Envoy is aware of and response is only needed for new routes. This PR clarifies that part in resource warming docs.
Commit Message:clarify resource warming docs in xds protocol
Additional Description:clarify resource warming docs in xds protocol
Risk Level:N/A
Testing:N/A
Docs Changes:N/A
Release Notes:N/A
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]