use EndpointBuilder in CDS by stevenctl · Pull Request #45733 · istio/istio

stevenctl · 2023-06-29T16:02:08Z

Changes:

Use EndpointBuilder in favor of buildLocalityLbEndpoints in cluster builder. That cluster builder method is a partial duplication of a lot of the same logic we have for EDS.
Enable multi-network at CDS level

Benchmarks are no longer affected.

                                              │ /home/landow/old.txt │        /home/landow/new.txt        │
                                                │        sec/op        │   sec/op     vs base               │
ClusterGeneration/gateways-24                              4.844m ± 2%   4.875m ± 4%       ~ (p=0.529 n=10)
ClusterGeneration/gateways-shared-24                       4.864m ± 4%   4.873m ± 3%       ~ (p=0.796 n=10)
ClusterGeneration/knative-gateway-24                       12.55µ ± 2%   12.71µ ± 4%       ~ (p=0.393 n=10)
ClusterGeneration/http-24                                  868.8µ ± 4%   877.8µ ± 4%       ~ (p=0.631 n=10)
ClusterGeneration/tcp-24                                   867.0µ ± 4%   874.0µ ± 3%       ~ (p=0.579 n=10)
ClusterGeneration/tls-24                                   881.4µ ± 4%   884.2µ ± 4%       ~ (p=0.912 n=10)
ClusterGeneration/auto-24                                 1021.5µ ± 5%   994.1µ ± 7%       ~ (p=0.247 n=10)
ClusterGeneration/telemetry-api-24                         2.420m ± 5%   2.461m ± 3%       ~ (p=0.436 n=10)
ClusterGeneration/virtualservice-24                        565.5µ ± 3%   573.3µ ± 4%       ~ (p=0.143 n=10)
ClusterGeneration/serviceentry-workloadentry-24            3.318m ± 5%   3.238m ± 6%       ~ (p=0.529 n=10)
geomean                                                    990.7µ        992.7µ       +0.21%

                                                │ /home/landow/old.txt │        /home/landow/new.txt         │
                                                │        kb/msg        │   kb/msg    vs base                 │
ClusterGeneration/gateways-24                               390.3 ± 0%   390.3 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/gateways-shared-24                        390.3 ± 0%   390.3 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/knative-gateway-24                        1.058 ± 0%   1.058 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/http-24                                   86.34 ± 0%   86.34 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/tcp-24                                    86.95 ± 0%   86.95 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/tls-24                                    86.95 ± 0%   86.95 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/auto-24                                   101.8 ± 0%   101.8 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/telemetry-api-24                          172.9 ± 0%   172.9 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/virtualservice-24                         39.70 ± 0%   39.70 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/serviceentry-workloadentry-24             379.1 ± 0%   379.1 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                     88.01        88.01       +0.00%
¹ all samples are equal

                                                │ /home/landow/old.txt │          /home/landow/new.txt          │
                                                │    resources/msg     │ resources/msg  vs base                 │
ClusterGeneration/gateways-24                              1.003k ± 0%     1.003k ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/gateways-shared-24                       1.003k ± 0%     1.003k ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/knative-gateway-24                        3.000 ± 0%      3.000 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/http-24                                   105.0 ± 0%      105.0 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/tcp-24                                    105.0 ± 0%      105.0 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/tls-24                                    105.0 ± 0%      105.0 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/auto-24                                   105.0 ± 0%      105.0 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/telemetry-api-24                          411.0 ± 0%      411.0 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/virtualservice-24                         105.0 ± 0%      105.0 ± 0%       ~ (p=1.000 n=10) ¹
ClusterGeneration/serviceentry-workloadentry-24             411.0 ± 0%      411.0 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                     151.8           151.8       +0.00%
¹ all samples are equal

cc @kdorosh @howardjohn

stevenctl · 2023-06-29T16:06:58Z

Next, I'd like to consolidate on shards vs service instances but that has implications for cds cache.

howardjohn

didn't read the code yet, but initial reaction: there is a lot of effort to handle the special case of EDS that it doesn't use snapshoting mechanism (push context) and allows state to change at any time. Spreading that property made be tricky

stevenctl · 2023-06-29T17:37:33Z

/test benchmark_istio

kdorosh · 2023-06-30T14:34:23Z

pilot/pkg/networking/core/v1alpha3/cluster_builder.go

generally agree that removing this code duplication is probably worth it, unless spreading the mutability of EDS that @howardjohn was talking about is both too difficult and completely necessary. because off the cuff id rather remove the mutability property for EDS and use snapshotting or something else since that's easier to reason about.

why is EDS handled differently? performance reasons? EDS being more dynamic?

pilot/pkg/networking/core/v1alpha3/cluster_cache.go

hzxuzhonghu · 2023-07-20T02:16:25Z

I feel a little concerned about such big refactor, since it touches too many files. With CI, it has failed multi times, so need much more look

stevenctl · 2023-07-20T05:42:33Z

With CI, it has failed multi times, so need much more look

FWIW, it wasn't really done yet. The failures from earlier were all due to make format breaking some import statements making tests not compile.

stevenctl · 2023-08-22T20:22:18Z

/test all

stevenctl · 2023-08-22T20:27:22Z

pilot/pkg/xds/endpoints/endpoint_builder.go

 	return locEps
 }

+func (b *EndpointBuilder) filterIstioEndpoint(ep *model.IstioEndpoint, svcPort *model.Port) bool {


This needs to be looked at closely. Must cover what we have in the original CDS code.

hzxuzhonghu · 2023-08-23T02:58:10Z

pilot/pkg/networking/core/v1alpha3/cluster_cache.go

 	}
 	h.Write(Separator)

+	t.endpointBuilder.WriteHash(h)


why do we need to add more factor for cache key calculation when we just do a cleanup

My concern is mostly that EndpointBuilder takes a model.Proxy and PushContext. It can decide to use whatever parts of that it needs. The cache key for EndpointBuilder will be updated to include those, but now that affects CDS.
I want to make it hard(er) to write caching bugs.

Right now endpoint builder has these things that CDS dosn't:

isClusterLocal

node type

failover priority labels

we just do a cleanup

I'm not sure what you mean.

pilot/pkg/networking/util/util.go

hzxuzhonghu · 2023-08-23T03:13:50Z

pilot/pkg/xds/endpoints/endpoint_builder.go

+		b.subsetName = strings.TrimPrefix(b.subsetName, "http/")
+		b.subsetName = strings.TrimPrefix(b.subsetName, "tcp/")
+	}
+	b.mtlsChecker = newMtlsChecker(b.push, b.port, b.destinationRule.GetRule(), b.subsetName)


nit: donot couple mtlsChecker with subsetInfo

mtlsChecker depends on the subset name. I could make it so we build it in the constructor, and the build it again if the subset changes?

hzxuzhonghu · 2023-08-23T03:35:22Z

pilot/pkg/xds/endpoints/endpoint_builder.go

+	// If we don't know the address we must eventually use a gateway address
+	if ep.Address == "" && ep.Network == b.network {
+		return false


have we supported this

This was already in endpoint_builder

hzxuzhonghu · 2023-08-23T03:41:19Z

pilot/pkg/xds/endpoints/endpoint_builder.go

+			if noCache {
+				ep.ComputeEnvoyEndpoint(eep)


confused why set it for cds endpoint, previously it is for eds

Should be !noCache.

hzxuzhonghu · 2023-08-23T03:45:09Z

pilot/pkg/xds/endpoints/endpoint_builder.go

 	}

+	// Apply the Split Horizon EDS filter, if applicable.
+	locEps = b.EndpointsByNetworkFilter(locEps)


Suggest not apply to cds endpoint, just keep as before. Not quite sure whether we set the network label correct

I'm not sure what you mean. The original motivation (for me) was to get multi-network working for non EDS. It's possible I have a ServiceEntry that has mixed IPs/Hostnames so it must be a DNS cluster. I want the IPs to get mapped.

I mean for cds, we donot apply filter now. This pr is already big, if we want to apply network filter for cds endpoint, we can do it later

hzxuzhonghu · 2023-08-23T03:48:23Z

pilot/pkg/xds/endpoints/endpoint_builder.go

+		if shardKey.Cluster != b.clusterID {
+			// If the downstream service is configured as cluster-local, only include endpoints that
+			// reside in the same cluster.
+			if isClusterLocal || b.service.Attributes.NodeLocal {


duplicate with the filter later

This is a very slight optimization allowing us to not loop over endpoints[] because every one of them will be a miss.

stevenctl · 2023-08-23T18:05:22Z

pilot/pkg/networking/core/v1alpha3/cluster_cache.go

 		metadataCerts:   cb.metadataCerts,
 		peerAuthVersion: cb.req.Push.AuthnPolicies.GetVersion(),
 		serviceAccounts: cb.req.Push.ServiceAccounts(service.Hostname, service.Attributes.Namespace, port.Port),
+		endpointBuilder: endpoints.NewCDSEndpointBuilder(


I think some of the slowdown might be that we do this unconditionally. None of our benchmarks are DNS resolution. This patch should have almost no impact.

howardjohn

General idea is fine but need to fix the performance

howardjohn · 2023-08-23T20:36:42Z

pilot/pkg/networking/core/v1alpha3/cluster.go


 			// We have a cache miss, so we will re-generate the cluster and later store it in the cache.
-			lbEndpoints := cb.buildLocalityLbEndpoints(clusterKey.proxyView, service, port.Port, nil)
+			lbEndpoints := clusterKey.endpointBuilder.FromServiceEndpoints()


Suggested change

lbEndpoints := clusterKey.endpointBuilder.FromServiceEndpoints()

var lbEndpoints []*endpoint.LocalityLbEndpoints

// We have a cache miss, so we will re-generate the cluster and later store it in the cache.

if (service.Resolution == model.DNSLB || service.Resolution == model.DNSRoundRobinLB) {

lbEndpoints = clusterKey.endpointBuilder.FromServiceEndpoints()

}

I think this fixes the performance regression, which is due to always computing endpoints even if its an EDS cluster.

Right.. I did this on the original draft and un-did it. Thanks!

Still +5% mean. The ones that have a higher regression are on the order of us.

Turns out it was passing around/copying the EndpointBuilder struct. Made that a pointer and everything looks good.

Everything is equal now.

Co-authored-by: John Howard <howardjohn@google.com>

stevenctl · 2023-08-23T22:11:31Z

/retest

This reverts commit 0057716.

stevenctl requested review from a team as code owners June 29, 2023 16:02

istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 29, 2023

howardjohn reviewed Jun 29, 2023

View reviewed changes

kdorosh reviewed Jun 30, 2023

View reviewed changes

howardjohn reviewed Jul 6, 2023

View reviewed changes

pilot/pkg/networking/core/v1alpha3/cluster_cache.go Outdated Show resolved Hide resolved

howardjohn mentioned this pull request Jul 6, 2023

optimize EnvoyFilter rebuild #45791

Merged

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 13, 2023

stevenctl force-pushed the ep-builder-cds branch from 39c9e36 to db333e7 Compare July 14, 2023 18:34

istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged and removed needs-rebase Indicates a PR needs to be rebased before being merged labels Jul 14, 2023

stevenctl force-pushed the ep-builder-cds branch from d71d2d6 to 4360e46 Compare July 18, 2023 15:11

stevenctl linked an issue Jul 18, 2023 that may be closed by this pull request

multi-network filters are not applied to non-EDS clusters #45506

Closed

stevenctl force-pushed the ep-builder-cds branch from 4360e46 to 5983803 Compare July 19, 2023 14:39

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 19, 2023

stevenctl force-pushed the ep-builder-cds branch from 5983803 to 8b2cb10 Compare July 19, 2023 14:58

istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 19, 2023

stevenctl force-pushed the ep-builder-cds branch 3 times, most recently from b5a9bba to cdc8548 Compare July 19, 2023 15:07

stevenctl mentioned this pull request Jul 20, 2023

allow unresolved gateway addresses #46088

Merged

stevenctl force-pushed the ep-builder-cds branch from cdc8548 to 108a3c4 Compare July 20, 2023 05:41

istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 20, 2023

test fixes

24d1643

stevenctl force-pushed the ep-builder-cds branch from 2a27c06 to 24d1643 Compare August 22, 2023 20:23

stevenctl marked this pull request as ready for review August 22, 2023 20:23

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Aug 22, 2023

stevenctl commented Aug 22, 2023

View reviewed changes

copy over missing checks

c8d60b0

hzxuzhonghu reviewed Aug 23, 2023

View reviewed changes

stevenctl added 3 commits August 23, 2023 10:40

clarify precomputed endpoint logic

9627b69

release note

765baa9

move TestMergeTrafficPolicy

86b08be

istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 23, 2023

stevenctl commented Aug 23, 2023

View reviewed changes

howardjohn requested changes Aug 23, 2023

View reviewed changes

stevenctl and others added 3 commits August 23, 2023 13:58

conditionally build endpoints

c0162ec

Co-authored-by: John Howard <howardjohn@google.com>

format

0c097ec

conditionally include epbuilder cache key

0057716

stevenctl added 6 commits August 25, 2023 08:07

Revert "conditionally include epbuilder cache key"

13e1d6d

This reverts commit 0057716.

one more case of always building eps

15e7228

conditional build epbuilder

abd041b

conditional hash

d5080cb

make ep_builder a pointer

f7fb3a8

cleanups

2400d82

howardjohn approved these changes Aug 31, 2023

View reviewed changes

istio-testing merged commit d4f548a into istio:master Aug 31, 2023

hzxuzhonghu mentioned this pull request Feb 23, 2024

Regression in service entry merging in 1.19 -> 1.20 #49489

Closed

-			lbEndpoints := clusterKey.endpointBuilder.FromServiceEndpoints()
+			var lbEndpoints []*endpoint.LocalityLbEndpoints
+			// We have a cache miss, so we will re-generate the cluster and later store it in the cache.
+			if (service.Resolution == model.DNSLB || service.Resolution == model.DNSRoundRobinLB) {
+				lbEndpoints = clusterKey.endpointBuilder.FromServiceEndpoints()
+			}

Conversation

stevenctl commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenctl commented Jun 29, 2023

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

stevenctl commented Jun 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hzxuzhonghu commented Jul 20, 2023

Uh oh!

stevenctl commented Jul 20, 2023

Uh oh!

stevenctl commented Aug 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenctl commented Aug 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

stevenctl commented Jun 29, 2023 •

edited

Loading