Skip to content

Conversation

ymesika
Copy link
Member

@ymesika ymesika commented May 31, 2018

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.

@kyessenov
Copy link
Contributor

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
/approve

Copy link
Contributor

@costinm costinm left a 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 ?

guptasu and others added 21 commits June 3, 2018 08:38
* 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.
@istio-testing istio-testing removed the lgtm label Jun 3, 2018
@istio-testing
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@codecov
Copy link

codecov bot commented Jun 3, 2018

Codecov Report

Merging #5951 into master will decrease coverage by 1%.
The diff coverage is 13%.

Impacted file tree graph

@@           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
Impacted Files Coverage Δ
pilot/pkg/serviceregistry/kube/cache.go 88% <13%> (-12%) ⬇️
mixer/adapter/rbac/controller.go 30% <0%> (-25%) ⬇️
...olarwinds/internal/papertrail/papertrail_logger.go 62% <0%> (-18%) ⬇️
mixer/adapter/stackdriver/metric/distribution.go 89% <0%> (-3%) ⬇️
pilot/pkg/serviceregistry/kube/controller.go 65% <0%> (-2%) ⬇️
mixer/adapter/rbac/rbac.go 11% <0%> (-2%) ⬇️
mixer/adapter/noop/noop.go 100% <0%> (ø) ⬆️
mixer/adapter/prometheus/prometheus.go 100% <0%> (ø) ⬆️
mixer/adapter/cloudwatch/client.go 0% <0%> (ø) ⬆️
mixer/adapter/dogstatsd/dogstatsd.go 100% <0%> (ø) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c119869...01a532c. Read the comment docs.

@googlebot
Copy link
Collaborator

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 cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes labels Jun 3, 2018
@istio-testing
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kyessenov, ymesika
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: linsun

Assign the PR to them by writing /assign @linsun in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@istio-testing
Copy link
Collaborator

@ymesika: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/istio-pilot-e2e.sh 01a532c link /test istio-pilot-e2e

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.

@ymesika
Copy link
Member Author

ymesika commented Jun 3, 2018

Rebased to resolve test errors (which indeed went away) but now it holds a bunch of commits from other. Sorry for the mess.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 3, 2018
@istio-testing
Copy link
Collaborator

@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.

@ymesika
Copy link
Member Author

ymesika commented Jun 4, 2018

I will re-open a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. needs-rebase Indicates a PR needs to be rebased before being merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.