Skip to content
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

Fill response flag for stats plugin #2527

Merged
merged 5 commits into from
Nov 7, 2019

Conversation

bianpengyuan
Copy link
Contributor

This needs istio/envoy#116 to be in to work with 1.4 and envoyproxy/envoy-wasm to be updated to work with master.

@bianpengyuan bianpengyuan requested review from mandarjog, kyessenov and a team November 6, 2019 01:36
@googlebot 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 6, 2019
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 6, 2019
Copy link
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. The only suggestion I have is that unrecognized bits should be supplied since this may need to work with future Envoy versions. E.g. drop LastFlag and just print out the flags as-is in case they overflow.

@bianpengyuan bianpengyuan added the cherrypick/release-1.4 Set this label on a PR to auto-merge it to the release-1.4 branch label Nov 6, 2019
@bianpengyuan
Copy link
Contributor Author

Can I get some review on this? Thanks!

@bianpengyuan
Copy link
Contributor Author

bianpengyuan commented Nov 7, 2019

@kyessenov Does this look like a flake to you? It seems to be different from client timeout we saw before. There is no crashes and does not return any mismatches.. so likely the prometheus endpoint does not return metrics. Could this also happen because of stale listeners?

--- FAIL: TestStatsParallel (48.68s)
    stats_xds_test.go:233: failed to match all stats
FAIL
FAIL	istio.io/proxy/test/envoye2e/stats	66.504s
ok  	istio.io/proxy/test/envoye2e/stats_plugin	23.067s
ok  	istio.io/proxy/test/envoye2e/tcp_metadata_exchange	5.045s
Makefile.core.mk:79: recipe for target 'test_tsan' failed
make: *** [test_tsan] Error 1

@kyessenov
Copy link
Contributor

@bianpengyuan Yeah this is not a timeout. Either prometheus reports incorrect stats, or we failed to report one. Does this happen consistently? We can disable the test for now to get this in.

@bianpengyuan
Copy link
Contributor Author

@kyessenov haven't retried it. Let me retest it.

@bianpengyuan
Copy link
Contributor Author

/test proxy-presubmit-tsan

@istio-testing
Copy link
Collaborator

In response to a cherrypick label: new pull request created: #2534

@kyessenov
Copy link
Contributor

I noticed that it was polling stats endpoint for ~20s. I think it was just starved of CPU resources since TSAN builds are slow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick/release-1.4 Set this label on a PR to auto-merge it to the release-1.4 branch cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants