Fix bug causing deleted endpoints to not be updated#9805
Fix bug causing deleted endpoints to not be updated#9805mandarjog merged 2 commits intoistio:release-1.0from
Conversation
|
istio/pilot/pkg/proxy/envoy/v2/eds.go Line 408 in fc4d010 Should change this too? |
|
@hzxuzhonghu great catch, I'm impressed ! I'll add more comments on that method and fix it - it is intended as a start on supporting |
|
Good sleuthing. can we add some lower level tests in this area? Specifically why eds unit tests did not catch this? |
|
@mandarjog I'm working on reproducing it in an integration test. Unit tests didn't catch it since the bug was in the pilot bootstrap logic. |
pilot/pkg/bootstrap/server.go
Outdated
| func (s *Server) createK8sServiceControllers(serviceControllers *aggregate.Controller, args *PilotArgs) (err error) { | ||
| clusterID := string(serviceregistry.KubernetesRegistry) | ||
| log.Infof("Primary Cluster name: %s", clusterID) | ||
| args.Config.ControllerOptions.ClusterID = string(serviceregistry.KubernetesRegistry) |
Codecov Report
@@ Coverage Diff @@
## release-1.0 #9805 +/- ##
============================================
- Coverage 72% 72% -<1%
============================================
Files 366 366
Lines 32404 32136 -268
============================================
- Hits 23223 22973 -250
+ Misses 8194 8189 -5
+ Partials 987 974 -13
Continue to review full report at Codecov.
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: costinm, nmittler The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@costinm: The following test failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@costinm just to be clear, you were able to reproduce the problem on k8s? |
The problem was that the 'shard' ID was not matching between incremental updates and full updates.
The 'shard' represents a registry (or in future a subset, for large registries). The registries are stored
in the aggreage map, keyed by the registry name. The bug was that the name for the default registry
was "" when constructing the registry, and "kubernetes" when adding. Adding was ok, and delete after a full push was ok too - but on incremental push, deletes were only deleting the "" shard.
Additional cleanups on 1.1/master, as we convert the other 2 registries to incremental.