Fix envoy initial fetch time out when CDS updated #1034
Conversation
haorenfsa
changed the title
haorenfsa
changed the title
|
@arkodg Please help review, thank you in advance |
haorenfsa
changed the title
haorenfsa
force-pushed
the
bugfix
branch
2 times, most recently
from
fda1676 to
b76edf9
Compare
haorenfsa
changed the title
|
I encountered the same issue. Here is my PR, but unfortunately, it hasn't received a review #981 |
pkg/cache/v3/simple.go
Outdated
| // otherwise, envoy won't complete initialization | ||
| if len(resp.Resources) > 0 || len(resp.RemovedResources) > 0 || (state.IsWildcard() && state.IsFirst()) { | ||
| if len(resp.Resources) > 0 || |
There was a problem hiding this comment.
Does this make more sense as a switch{} now where we can fall through cases? Having this many || seems a bit unreadable
| @@ -137,3 +142,11 @@ func (s *StreamState) SetKnownResourceNamesAsList(url string, names []string) { | |||
| } | |||
| s.knownResourceNames[url] = m | |||
| } | |||
|
|
|||
| func (s *StreamState) HasWarmingCluster() bool { | |||
There was a problem hiding this comment.
Should this be more generic given that envoy also supports listener warming? Maybe HasResourceWarming()?
There was a problem hiding this comment.
Seems only Cluster warming needs to be considered, see below comments.
| if typ == resource.EndpointType { | ||
| watch.state.SetClusterWarming(false) | ||
| } | ||
| if typ == resource.ClusterType { |
There was a problem hiding this comment.
What about the cases where we get a warming listener?
There was a problem hiding this comment.
Yes! When I wrote this patch, I only had dynamic Cluster, so only ClusterType was considered. If we're to merge this patch, I'll add the rest resources need warming.
There was a problem hiding this comment.
@alecholmez Thank you for taking time reviewing this.
I just read the envoy xDS protocol document again https://www.envoyproxy.io/docs/envoy/latest/api-docs/xds_protocol#resource-warming, and found that only Cluster warming needs an additional EDS response. No additional RDS response is required for Listener warming. So I think we only need consider about the ClusterType in this patch, what do you think?
There was a problem hiding this comment.
Hi @lukidzi, I checked your PR. As I stated above, seems only CDS & EDS needs to be taking care of ? Anything I missed, please help point it out.
There was a problem hiding this comment.
Hi, if both LDS and RDS are in use, you may encounter an issue when a Listener enters the warming phase and depends on RDS data to proceed. My PR only addressed the issue for CDS/EDS, so it doesn’t currently resolve the LDS/RDS dependency during the warming phase.
Signed-off-by: haorenfsa <[email protected]>
| @@ -54,6 +58,7 @@ func NewStreamState(wildcard bool, initialResourceVersions map[string]string) St | |||
| first: true, | |||
| knownResourceNames: map[string]map[string]struct{}{}, | |||
| ordered: false, // Ordered comes from the first request since that's when we discover if they want ADS | |||
| hasWarmingCluster: false, | |||
There was a problem hiding this comment.
As @valerian-roche suggested in my PR envoyproxy/go-control-plane#981, it would be beneficial to push only the endpoints of updated clusters. When your Envoy instance is watching over 1000 clusters, forcing a full update for all of them can be excessive if only one has changed. I could reopen my PR and finish it up if it makes sense to do it. Envoy has a new feature to load cached EDS but it works ONLY when initial_fetch_timeout is not disabled.
There was a problem hiding this comment.
It seems to me, only 1 extra empty EDS response is needed no matter how many Clusters were updated.
So my patch will only send 1 empty EDS response, if nothing changes.
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Fix #1035
related: envoyproxy/gateway#4445