envoyproxy / java-control-plane

Java implementation of an Envoy gRPC control plane
Apache License 2.0
293 stars 136 forks source link

Send EDS response after CDS even if EDS hasn't change #131

Closed lukidzi closed 4 years ago

lukidzi commented 4 years ago

On our development environment we found that if snapshot already exists and new Envoy connects to EC it might respond immediately in createWatch method. Method setSnapshot can ensure that EDS will be sent after CDS but here we can't be sure that. I've tested this with ADS mode and also made changes for this mode.

This is how it looks like:

  1. Envoy connects to control-plane
  2. Snapshot already exists in control-plane <- other instance share same group
  3. Control-plane respond with CDS in createWatch method
  4. There is snapshot update which change CDS and EDS versions
  5. Envoy sends EDS request
  6. Control-plane respond with EDS in createWatch method
  7. Envoy resume CDS and EDS requests.
  8. Envoy sends request CDS
  9. Control plane respond with CDS in createWatch method
  10. Envoy sends EDS requests
  11. Control plane doesn't respond because version hasn't changed
  12. Cluster of service stays in warming phase

I've prepared a change which sets flag hasClusterChanged after CDS response. Because it will be shared between threads I made it volatile. I would appreciate hearing your thoughts on this.

codecov-io commented 4 years ago

Codecov Report

Merging #131 into master will increase coverage by 0.20%. The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #131      +/-   ##
============================================
+ Coverage     89.56%   89.77%   +0.20%     
- Complexity      195      202       +7     
============================================
  Files            22       22              
  Lines           604      616      +12     
  Branches         49       51       +2     
============================================
+ Hits            541      553      +12     
  Misses           48       48              
  Partials         15       15              
Impacted Files Coverage Δ Complexity Δ
.../io/envoyproxy/controlplane/cache/SimpleCache.java 84.00% <100.00%> (+0.80%) 36.00 <2.00> (+4.00)
...o/envoyproxy/controlplane/cache/TestResources.java 97.77% <100.00%> (+0.02%) 9.00 <1.00> (+1.00)
...lane/server/AdsDiscoveryRequestStreamObserver.java 100.00% <100.00%> (ø) 14.00 <0.00> (+2.00)
...olplane/server/DiscoveryRequestStreamObserver.java 85.54% <100.00%> (+0.17%) 20.00 <1.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8828e23...062872a. Read the comment docs.