-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Avoid panic when deleted object is cache.DeletionFinalStateUnknown #5951
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I think this is fine given that the pod IPs and names are generally stable. The bad case is when the pod gets changed right before deletion and the deletions event somehow gets dropped on the way to the cache. That can make the local pod cache inconsistent. I think we should eventually get rid of this cache once we start using instance IDs (pod IDs) consistently and no longer need to do the reverse mapping from an IP to a pod. /lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also merge this into release-0.8 ?
* Contract between the config builder and the runtime. Add new configuration data to the snapshot. Ephemeral.go builds the snapshot. Snapshot is used by the runtime dispatch table. * address Oz's comments. * Renames old style config as legacy.
* Tests for istioctl get/delete/etc * Use subtests for test cases
* add README file to chart. * update installation steps for README.
Similar to #2460. We should not rely on GOPATH being a single value. Also, while on that, replace $TOP with $GO_TOP in testEnvLocalK8S.sh, to make it consistent to Makefile.
Mixer developer doc has moved to github wiki page, update the link url for README file.
…5798) By making publicRootCABundlePath a var instead of a const. By using the -X ldflag option, the user bulding istio can override the default location for this file.
* Remove mTLS enablement via service annotation. See #5826. * Remove AuthenticationPolicy from Port struct (model/service.go) * Address comments. * Fix e2e tests. * Try to fix the e2e test again.
We already have an imagePullPolicy flag in the e2e test framework, but we're not currently passing it to istioctl. In local testing, this has resulted in stale istio-init images, which do not respect the new iptables flags.
otherwise, pilot will not enumerate the routes past the base route
* use Gateway and VirtualService to expose helloworld service (#5791) * use Gateway and VirtualService to expose helloworld service * readme fix * Make istio work without rbac. (#5877) Fixed istio/old_issues_repo#255 * Update Jaeger version and add limit on the number of traces held in m… (#5873) * Update Jaeger version and add limit on the number of traces held in memory * Use tag 1 to pick up latest stable versions * Use tag 1.5 * enable mixer alpha3_v2 tests (#5844) * enable mixer alpha3_v2 tests * fix capture logs for new target * make rate limit test more resilient * linter errors * fix fmt * update tests * update api sha to head of release-0.8 (#5939) * loose the validation on istioctl to allow users to pass config (#5955) * remove the unecessary check
* Update the helm cluster to v2 * More files for the test env * Make script customizable
* Implement a spy IBP backend. This will help in easy testing of our OOP adapter implementation. Now, this backend only implements static IBP interface. After my template specific IBP Handle service PR goes in, I will enhance this spy backend to have two flavors, session plan and no-session plan. * fix lint
- Patch up URLs and front-matter to make the new Hugo-based doc setup happy.
* Add buffering in front of REPORT adapters. The proxy calls Mixer with large batches of reports (on the order of 1000 items). Instead of calling adapters for each of these reports, Mixer buffers the generated instances and calls into the adapters a single time. This eliminates a fairly large amount of computation in Mixer proper. But more importantly, adapters can leverage these large incoming batches to use batches when talking to their back end, potentially reducing RPC overhead considerably. Also, remove the request_count and request_duration metrics. They weren't actually measuring the right thing and they're redundant with gRPC metrics we already have.
* copy license file to chart. * Update date in LICENSE files. * rollback the change for origin license.
New changes are detected. LGTM label has been removed. |
Codecov Report
@@ Coverage Diff @@
## master #5951 +/- ##
=======================================
- Coverage 69% 69% -<1%
=======================================
Files 321 321
Lines 27800 27787 -13
=======================================
- Hits 18922 18906 -16
- Misses 8149 8153 +4
+ Partials 729 728 -1
Continue to review full report at Codecov.
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kyessenov, ymesika Assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@ymesika: The following test failed, say
Instructions 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. |
Rebased to resolve test errors (which indeed went away) but now it holds a bunch of commits from other. Sorry for the mess. |
@ymesika: PR needs rebase. Instructions 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 will re-open a new one. |
As reported in #5739, there are cases when the deleted object is not a Pod but cache.DeletionFinalStateUnknown.
The piece of code in this pull request handle such cases and avoid panic/crash trying to cast wrong type.