eds: do not trigger continuous full pushes when a pod is in crash loop by ramaraochavali · Pull Request #18574 · istio/istio

ramaraochavali · 2019-11-02T03:37:20Z

Prevents full push being triggered when a pod is in crash loop and thus its endpoints flipflop between 0 and 1. Fixes #13822

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2019-11-02T03:38:16Z

@hzxuzhonghu @rshriram @howardjohn PTAL

ramaraochavali · 2019-11-02T04:20:27Z

/test integ-pilot-k8s-tests_istio

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2019-11-02T11:46:47Z

SyntheticServiceEntryController seems to be incorrectly dependent on removing the service key from EndpointShardsByService when endpoints are zero (and then subsequent updates triggering full push) - It also not triggering full push when service changes in end-end-pilot integ test. So I changed it to trigger full push on every configUpdate (Similar to how service entries work) with a TODO to fix later. If there are better ideas/simple fix - I can try but it seems to be problem with SyntheticServiceEntryController which should be handled separately. @hzxuzhonghu any idea?

howardjohn

overall lgtm, thanks for working on this and adding good tests.

I only skimmed it right now so I'll give a chance for Nino to look at the SSE code or others then give a proper review during the week

howardjohn · 2019-11-03T15:22:03Z

pilot/pkg/proxy/envoy/v2/eds_test.go

+		t.Fatal("Expecting only EDS update as part of a partial push. But received CDS also +v", upd)
+	}
+
+	if len(upd) > 0 && !contains(upd, "eds") {


missing % on the format

howardjohn · 2019-11-03T15:25:36Z

@Nino-K

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

hzxuzhonghu · 2019-11-04T03:33:16Z

pilot/pkg/proxy/envoy/v2/eds.go

+// deleteEndpointShards deletes matching endpoints from EndpointShardsByService map. This is called when
+// endpoints are deleted or the service is deleted. If deleteKeys is true, this method will also delete the
+// associated entries from map.
+func (s *DiscoveryServer) deleteEndpointShards(cluster, serviceName, namespace string, deleteKeys bool) {


Donot like deleteKeys param, this kind of param is confusing.

Can you only do delete in SvcUpdate? Thus we don't need to care about endpoints=0 or not?

Ok. I changed it to two functions and called delete only from SvcUpdate and deleting shards from edsupdate.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Nino-K · 2019-11-04T19:35:46Z

pilot/pkg/config/coredatamodel/syntheticserviceentrycontroller.go

+	// functioning correctly. Currently it is working because on edsUpdate if we set endpoints to 0, we remove
+	// the service from EndpointShardsByService and subsequent eds updates trigger a full push. That is being
+	// fixed in https://github.com/istio/istio/pull/18574. Need to fix this issue and re-enable conditional
+	// full push. For now, any configupdate triggers a full push much like service entries.


@ramaraochavali can you please put this in an issue, being able to do a EDSUpdate vs ConfigUpdate was one of the main goals of this controller.

Ok. Created this issue #18625

hzxuzhonghu · 2019-11-05T03:16:59Z

pilot/pkg/config/coredatamodel/syntheticserviceentrycontroller.go

-		oldEpVersion := c.endpointVersion(conf.Namespace, conf.Name)
-		newEpVersion := version(conf.Annotations, endpointKey)
-		if oldEpVersion != newEpVersion {
-			if err := c.edsUpdate(conf); err != nil {


why remove this?

because it is ineffective now - any update needs a full push

Didn't we only do eds push when svc not changed?

Yes. That is what we have to do. But it is not working for SSE. See the TODO below and #18625

hzxuzhonghu · 2019-11-05T03:42:43Z

pilot/pkg/proxy/envoy/v2/eds.go

+		delete(s.EndpointShardsByService[serviceName][namespace].Shards, cluster)
+		s.EndpointShardsByService[serviceName][namespace].mutex.Unlock()
+		delete(s.EndpointShardsByService[serviceName], namespace)
+		delete(s.EndpointShardsByService, serviceName)


This is not right, we can delete EndpointShardsByService[hostname] only when len(EndpointShardsByService[hostname]) = 0

hzxuzhonghu · 2019-11-05T03:43:31Z

pilot/pkg/proxy/envoy/v2/eds.go

+		s.EndpointShardsByService[serviceName][namespace].mutex.Lock()
+		delete(s.EndpointShardsByService[serviceName][namespace].Shards, cluster)
+		s.EndpointShardsByService[serviceName][namespace].mutex.Unlock()
+		delete(s.EndpointShardsByService[serviceName], namespace)


Same only can delete when s.EndpointShardsByService[serviceName][namesapce].Shards = 0

hzxuzhonghu · 2019-11-05T03:46:59Z

pilot/pkg/model/push_context.go

 	// updated to force a EDS and CDS recomputation and incremental push, as it doesn't affect
 	// LDS/RDS.
-	SvcUpdate(shard, hostname string, ports map[string]uint32, rports map[uint32]string)
+	SvcUpdate(shard, hostname string, namespace string, event Event, ports map[string]uint32, rports map[uint32]string)


We donot need the ports and rports param

I think may be better to leave them for future? Because they were there from the beginning.

or if you are ok to clean it up - I can do that. I do not have strong opinion on that.

Let's clean up it.

Ok. Cleaned it up. PTAL

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2019-11-05T08:49:57Z

/test unit-tests_istio

ramaraochavali · 2019-11-06T03:59:50Z

@rshriram @howardjohn @hzxuzhonghu - Can you PTAL? Addressed all the comments

istio#18574) * wip Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * add test cases Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * revert controller change Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * add debug error Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * try deleting completely Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * trigger full push for ss3 config update Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * change sse to trigger full push Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * lint Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * review comments Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * split delete in to two functions Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * address comments Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * clean up ports Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali added 3 commits November 1, 2019 17:56

wip

f94c5ba

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

add test cases

ebd5e2b

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Merge branch 'master' into fix/eds_full_push

52fed50

ramaraochavali requested a review from a team as a code owner November 2, 2019 03:37

istio-policy-bot added the area/networking label Nov 2, 2019

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 2, 2019

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Nov 2, 2019

ramaraochavali added 2 commits November 2, 2019 10:45

revert controller change

258cb05

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

add debug error

29b3978

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali requested a review from a team as a code owner November 2, 2019 06:37

ramaraochavali added 4 commits November 2, 2019 12:53

try deleting completely

55bf5fb

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

trigger full push for ss3 config update

09f9dc3

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

change sse to trigger full push

0188d05

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

lint

7b45849

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn reviewed Nov 3, 2019

View reviewed changes

review comments

ca37252

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

hzxuzhonghu reviewed Nov 4, 2019

View reviewed changes

split delete in to two functions

a9aee2a

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Nino-K reviewed Nov 4, 2019

View reviewed changes

hzxuzhonghu reviewed Nov 5, 2019

View reviewed changes

ramaraochavali mentioned this pull request Nov 5, 2019

synthetic service entries need full push for eds/config updates #18625

Closed

hzxuzhonghu reviewed Nov 5, 2019

View reviewed changes

ramaraochavali added 2 commits November 5, 2019 10:02

address comments

6c307a9

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

clean up ports

f198890

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn approved these changes Nov 6, 2019

View reviewed changes

istio-testing merged commit c5d10e6 into istio:master Nov 6, 2019

ramaraochavali deleted the fix/eds_full_push branch November 6, 2019 04:56

Conversation

ramaraochavali commented Nov 2, 2019 • edited by istio-policy-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramaraochavali commented Nov 2, 2019

Uh oh!

ramaraochavali commented Nov 2, 2019

Uh oh!

ramaraochavali commented Nov 2, 2019

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

howardjohn commented Nov 3, 2019

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!

ramaraochavali Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ramaraochavali commented Nov 5, 2019

Uh oh!

ramaraochavali commented Nov 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ramaraochavali commented Nov 2, 2019 •

edited by istio-policy-bot

Loading

ramaraochavali Nov 5, 2019 •

edited

Loading