Fix Service and WorkloadEntry updates #27184

howardjohn · 2020-09-09T22:23:04Z

PR is split into many commits

does some testing refactoring to support this, and adds 2 failing tests
Fixes bug: workloadentry + service stay on incorrect port #27151, enables the test
Fixes Update workload entry causes duplicate address #27183, enables the test
Fixes pod updates for SE selects pod
Fixes SE updates for SE selects pod
Fixes WorkloadEntry mis attributed to kubernetes service #27185

Somehow we managed to have 4 different bugs covering all 4 update scenarios. They should be all fixed and tested now.

kyessenov · 2020-09-09T23:11:35Z

pilot/pkg/serviceregistry/kube/controller/controller.go

@@ -206,10 +206,12 @@ type Controller struct {
 	// we run through the label selectors here to pick only ones that we need.
 	// Only nodes with ExternalIP addresses are included in this map !
 	nodeInfoMap map[string]kubernetesNode
-	// externalNameSvcInstanceMap stores hostname ==> instance, is used to store instances for ExternalName k8s services
+	// externalNameSvcInstanceMap stores hostname ==2> instance, is used to store instances for ExternalName k8s services


stevenctl · 2020-09-09T23:50:52Z

pilot/pkg/xds/fake.go

+	if err := cg.ServiceEntryRegistry.AppendWorkloadHandler(k8s.WorkloadInstanceHandler); err != nil {
+		t.Fatal(err)
+	}
+	if err := k8s.AppendWorkloadHandler(cg.ServiceEntryRegistry.WorkloadInstanceHandler); err != nil {


You will may need to call SyncAll again after this. We call k8s.Run() inside of its constructor, so the WorkloadHandler could miss the initial SyncAll from inside the k8s controller.

This ended up causing a race so I think Ill pass it the handler in the constructor

Either that, or fake kube controller should have Run called outside the constructor and share the same stop chan as the other controllers.

iandyh · 2020-09-10T01:20:46Z

Thanks for the fix! Will also take a look later.

Hopefully it can address the issues described in #25110

howardjohn · 2020-09-10T05:01:52Z

/retest

too see if its a flake

howardjohn · 2020-09-10T16:02:03Z

/retest

howardjohn · 2020-09-10T21:55:54Z

/retest

ramaraochavali

LGTM with some minor comments. @hzxuzhonghu Can you also PTAL?

ramaraochavali · 2020-09-11T05:26:02Z

pilot/pkg/serviceregistry/kube/controller/controller.go

@@ -708,8 +712,9 @@ func (c *Controller) serviceInstancesFromWorkloadInstances(svc *model.Service, r
 	workloadInstancesExist = len(c.workloadInstancesByIP) > 0
 	c.RUnlock()

+	// Only select internal Kubernetes services with selectors


nit: This comment and code below is little confusing. It took a while for me to understand we are doing an exclusion here rather than selecting

ramaraochavali · 2020-09-11T05:28:44Z

pilot/pkg/serviceregistry/kube/controller/controller.go

@@ -904,7 +909,14 @@ func (c *Controller) WorkloadInstanceHandler(si *model.WorkloadInstance, event m
 	case model.EventDelete:
 		delete(c.workloadInstancesByIP, si.Endpoint.Address)
 	default: // add or update
+		// Check to see if the workload entry changed. If it did, clear the old entry
+		k := si.Name + "~" + si.Namespace


Not related to this but - We are using "~" separator at multiple places in the code, May be we should move that to some constant like ISTIO_SEPARATOR :-) or some thing and use it every where so that in future for some odd reason if we have to change, we can change easilt

ramaraochavali · 2020-09-11T05:32:26Z

pilot/pkg/serviceregistry/kube/controller/controller.go

@@ -210,6 +210,8 @@ type Controller struct {
 	externalNameSvcInstanceMap map[host.Name][]*model.ServiceInstance
 	// workload instances from workload entries  - map of ip -> workload instance
 	workloadInstancesByIP map[string]*model.WorkloadInstance
+	// Stores a map of workload instance name/namespace to address
+	workloadInstancesIPsByName map[string]string


nit: workloadInstancesIPsByName-> workloadInstanceIPByName /workloadInstanceAddressByName?

ramaraochavali · 2020-09-11T05:47:16Z

pilot/pkg/serviceregistry/kube/controller/controller.go

+		// Check to see if the workload entry changed. If it did, clear the old entry
+		k := si.Name + "~" + si.Namespace
+		existing := c.workloadInstancesIPsByName[k]
+		if existing != si.Endpoint.Address {


nit: I think in add case, delete will always be executed which is redundant may be only do this for update case?.

pilot/pkg/serviceregistry/serviceentry/servicediscovery.go

howardjohn · 2020-09-14T17:42:51Z

/retest

istio-testing · 2020-09-15T00:38:30Z

In response to a cherrypick label: #27184 failed to apply on top of branch "release-1.7":

Applying: Updates to the testing infra to support testing this change
Using index info to reconstruct a base tree...
A	pilot/pkg/networking/core/v1alpha3/fake.go
M	pilot/pkg/serviceregistry/kube/controller/controller.go
M	pilot/pkg/serviceregistry/kube/controller/fake.go
M	pilot/pkg/serviceregistry/serviceregistry_test.go
M	pilot/pkg/xds/ads_test.go
M	pilot/pkg/xds/fake.go
M	pilot/pkg/xds/xds_test.go
A	pilot/test/xdstest/extract.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): pilot/test/xdstest/extract.go deleted in HEAD and modified in Updates to the testing infra to support testing this change. Version Updates to the testing infra to support testing this change of pilot/test/xdstest/extract.go left in tree.
Auto-merging pilot/pkg/xds/xds_test.go
CONFLICT (content): Merge conflict in pilot/pkg/xds/xds_test.go
Auto-merging pilot/pkg/xds/fake.go
CONFLICT (content): Merge conflict in pilot/pkg/xds/fake.go
Auto-merging pilot/pkg/xds/ads_test.go
CONFLICT (content): Merge conflict in pilot/pkg/xds/ads_test.go
Auto-merging pilot/pkg/serviceregistry/serviceregistry_test.go
CONFLICT (content): Merge conflict in pilot/pkg/serviceregistry/serviceregistry_test.go
Auto-merging pilot/pkg/serviceregistry/kube/controller/fake.go
CONFLICT (content): Merge conflict in pilot/pkg/serviceregistry/kube/controller/fake.go
Auto-merging pilot/pkg/serviceregistry/kube/controller/controller.go
CONFLICT (modify/delete): pilot/pkg/networking/core/v1alpha3/fake.go deleted in HEAD and modified in Updates to the testing infra to support testing this change. Version Updates to the testing infra to support testing this change of pilot/pkg/networking/core/v1alpha3/fake.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Updates to the testing infra to support testing this change
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

istio-testing · 2020-09-15T00:38:32Z

In response to a cherrypick label: new issue created for failed cherrypick: #27304

howardjohn requested review from a team as code owners September 9, 2020 22:23

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Sep 9, 2020

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 9, 2020

howardjohn requested review from a team as code owners September 9, 2020 22:24

kyessenov reviewed Sep 9, 2020

View reviewed changes

stevenctl reviewed Sep 9, 2020

View reviewed changes

howardjohn added the cherrypick/release-1.7 label Sep 10, 2020

howardjohn assigned hzxuzhonghu and ramaraochavali Sep 10, 2020

ramaraochavali approved these changes Sep 11, 2020

View reviewed changes

howardjohn force-pushed the pilot/fix-we-service-update branch from 4be8693 to aa678e7 Compare September 14, 2020 16:25

jwendell approved these changes Sep 14, 2020

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 14, 2020

howardjohn added 10 commits September 14, 2020 16:41

Updates to the testing infra to support testing this change

d48feaa

Fix Service update for workload entry

cef1406

Fix workload entry update

7482ceb

add release note

cfa84d7

Handle updated pod

8823ff0

handle pod IP update

7171d13

Fix SE update for SE selects pod

d385dbc

fix test failures

368ed94

Fix bad selector on k8s service

f26c0d5

fix lint

72be29b

howardjohn added 2 commits September 14, 2020 16:41

fix tests

dc6d38c

Add note

0b90c3d

howardjohn force-pushed the pilot/fix-we-service-update branch from aa678e7 to 0b90c3d Compare September 14, 2020 23:41

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 14, 2020

istio-testing merged commit 195a47c into istio:master Sep 15, 2020

istio-testing mentioned this pull request Sep 15, 2020

[release-1.7] Fix Service and WorkloadEntry updates #27304

Closed

iandyh mentioned this pull request Oct 29, 2020

HYBRID DNS resolution support in ServiceEntry construct #25898

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Service and WorkloadEntry updates #27184

Fix Service and WorkloadEntry updates #27184

howardjohn commented Sep 9, 2020 •

edited

Loading

kyessenov Sep 9, 2020

stevenctl Sep 9, 2020

howardjohn Sep 9, 2020

stevenctl Sep 9, 2020 •

edited

Loading

iandyh commented Sep 10, 2020 •

edited

Loading

howardjohn commented Sep 10, 2020

howardjohn commented Sep 10, 2020

howardjohn commented Sep 10, 2020

ramaraochavali left a comment

ramaraochavali Sep 11, 2020

ramaraochavali Sep 11, 2020

ramaraochavali Sep 11, 2020

ramaraochavali Sep 11, 2020

howardjohn commented Sep 14, 2020

istio-testing commented Sep 15, 2020

istio-testing commented Sep 15, 2020

Fix Service and WorkloadEntry updates #27184

Fix Service and WorkloadEntry updates #27184

Conversation

howardjohn commented Sep 9, 2020 • edited Loading

kyessenov Sep 9, 2020

Choose a reason for hiding this comment

stevenctl Sep 9, 2020

Choose a reason for hiding this comment

howardjohn Sep 9, 2020

Choose a reason for hiding this comment

stevenctl Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

iandyh commented Sep 10, 2020 • edited Loading

howardjohn commented Sep 10, 2020

howardjohn commented Sep 10, 2020

howardjohn commented Sep 10, 2020

ramaraochavali left a comment

Choose a reason for hiding this comment

ramaraochavali Sep 11, 2020

Choose a reason for hiding this comment

ramaraochavali Sep 11, 2020

Choose a reason for hiding this comment

ramaraochavali Sep 11, 2020

Choose a reason for hiding this comment

ramaraochavali Sep 11, 2020

Choose a reason for hiding this comment

howardjohn commented Sep 14, 2020

istio-testing commented Sep 15, 2020

istio-testing commented Sep 15, 2020

howardjohn commented Sep 9, 2020 •

edited

Loading

stevenctl Sep 9, 2020 •

edited

Loading

iandyh commented Sep 10, 2020 •

edited

Loading