Initial pilot api az functionality#1815
Initial pilot api az functionality#1815rshriram merged 3 commits intoistio:masterfrom liamawhite:master
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1815 +/- ##
=========================================
+ Coverage 81.14% 81.65% +0.5%
=========================================
Files 187 197 +10
Lines 18883 19918 +1035
=========================================
+ Hits 15323 16264 +941
- Misses 3127 3211 +84
- Partials 433 443 +10
Continue to review full report at Codecov.
|
|
/test istio-presubmit |
|
@ZackButcher / @nmittler do you guys want to shepherd this through first? This would shed more light into how we name/and control the sidecars. |
|
/test istio-presubmit |
pilot/proxy/envoy/discovery.go
Outdated
ZackButcher
left a comment
There was a problem hiding this comment.
LGTM, just need the missing return and another test case.
pilot/proxy/envoy/discovery_test.go
Outdated
There was a problem hiding this comment.
Can we add a test case for a service with no AZ too?
pilot/proxy/envoy/discovery.go
Outdated
There was a problem hiding this comment.
Is it possible for us to hit cases where services on the same host report different AZs? In other words, is it possible for the notion of an AZ to vary per service (or, what if two different service registries have different notions of AZs and both have entries for services on the same host)?
|
Also, more general note: I couldn't find Envoy's AZ API definition, I could only find references to AZs in the tags returned by SDS queries. Can you point me to the API def? |
|
I think the context is missing here. Envoy has no API for AZs. AZs have to be provided during startup. However, during startup, the only way to get pod AZ info is by querying kube api. That would mean adding the kube client-go into the pilot-agent. Doing so increases the footprint of the pilot agent to 40Mb+ (currently its few Mb only). In order to avoid this, we are introducing a AZ api in pilot. Pilot agent can ping Pilot, to get the AZ for the pod. With this information, it can hot restart Envoy with the right AZ info. (since this happens in early stages of a pod's life, impact should be small). Once Envoy has the AZ info, it will prefer routing to other Envoys in the same AZ over Envoys in other AZs. our EDS already provides Envoy with the AZs of all other pods. What was missing is the AZ of the local pod, which this PR attempts to tackle. |
|
So what component will use this API? The pilot-agent? Where does the local AZ actually manifest in Envoy config? I.e. could we just supply it with listener config or somewhere else w/o having to expose the API on pilot proper? |
|
@rshriram we couldn't pull it in from kube either because that breaks the abstraction for other platforms in the proxy agent right? @ZackButcher My understanding is that it has to be passed into envoy as a flag at start up. We currently expose it in the envoy config already, but thats not enough. @rshriram feel free to correct me! |
|
@liamawhite PR needs rebase |
|
/test istio-presubmit |
|
/test istio-pilot-e2e |
|
/test e2e-smoke |
ZackButcher
left a comment
There was a problem hiding this comment.
Thanks for improving the test coverage!
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ZackButcher The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
@rshriram do you also want service cluster still, too? Look to me like the exact same issue. |
|
Didn't we resolve the service cluster by using the deployment name from kube inject? |
|
Yes, we did, but it's not related to upstream cluster name. Does Envoy care that it's cluster is named same as upstream cluster? |
|
service-cluster is for the envoy CLI. where does upstream cluster come in this picture? |
Automatic merge from submit-queue. Add in consul datacenter (AvailabilityZone) support What this PR does / why we need it: Currently we only support AZ routing for kube based service registries. This enables it for consul. Which issue this PR fixes: Continues on from: istio/old_pilot_repo#1230, #2054 and #1815 Working towards: #1473 Release note: Enable consul intra-datacenter routing with AvailabilityZone
1. Stop trashing build's cache. 2. Split ASan and TSan tests into separate targets. 3. Fix output directory, so that caching works on macOS. Before: - build : 49 mins - macos : 30 mins After: - build : 27 mins - linux_asan: 20 mins - linux_tsan: 16 mins - macos : 30 mins After (with warm cache): - build : 3 mins - linux_asan: 4 mins - linux_tsan: 2 mins - macos : 5 mins Fixes istio#1815. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
What this PR does / why we need it: A new API path for pilot, az. It is needed to allow proxy agent to set the availability zone of a proxy using the --availabilityZone flag
Which issue this PR fixes:
Continues on from: istio/old_pilot_repo#1230
Working towards: #1473
Special notes for your reviewer: This is a quick and dirty PR to get feedback on the API path and general approach. The PR that allows Proxy Agent to consume this endpoint will come later.
Release note: