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.

Sign up for GitHub

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

Jump to bottom

Rollforward of api: Add total_issued_requests to Upstream Locality and Endpoint Stats. (#6692) #6784

Merged
merged 1 commit into from
May 10, 2019
Merged

Rollforward of api: Add total_issued_requests to Upstream Locality and Endpoint Stats. (#6692) #6784

merged 1 commit into from
May 10, 2019

Conversation

karthikbox
Copy link
Contributor

No new changes.

Signed-off-by: Karthik Reddy rekarthik@google.com

@karthikbox
Copy link
Contributor Author

Waiting to see if coverage passes.

@karthikbox
Copy link
Contributor Author

Coverage still hangs at IpVersions/LoadStatsIntegrationTest.LocalityWeighted/IPv4 even after increasing coverage timeout to 8000secs. This looks like a real issue. Not sure why this manifests only during coverage run.

Any pointers to debug this further @lizan ?

The following PASS locally:
bazel test --test_output=streamed //test/integration:load_stats_integration_test
bazel test --test_output=streamed --test_timeout=4000 //test/coverage:coverage_tests
The LoadStatsIntegrationTest takes 12756 ms total.

./ci/run_envoy_docker.sh './ci/do_ci.sh bazel.coverage' locally fails with some other error:
https://pastebin.com/eqT93Fe0

@htuch
Copy link
Member

htuch commented May 3, 2019

@karthikbox please merge master and try again, there has been a lot of churn recently in CI.

@lizan
Copy link
Member

lizan commented May 6, 2019

@karthikbox

It is a real issue because coverage run link all tests into a single binary and run all tests in one process, it may keep stats from other tests in that case.

@htuch htuch added the waiting label May 6, 2019
@karthikbox karthikbox force-pushed the rollforward_lrs branch 9 times, most recently from 1c05216 to 3e0a634 Compare May 8, 2019 22:24
@karthikbox karthikbox marked this pull request as ready for review May 9, 2019 00:14
htuch
htuch suggested changes May 9, 2019
View reviewed changes
Copy link
Member

@htuch htuch 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 do a --runs_per_test=500 on the load_stats_integration_test to provide some confidence that flakes are gone? Thanks. LGTM and ready to ship modulo the comment followup and this.
/wait

api/envoy/api/v2/endpoint/load_report.proto Outdated Show resolved Hide resolved
@karthikbox
Copy link
Contributor Author

Ran test //test/integration:load_stats_integration_test 1000 times locally. No failures.

htuch
htuch previously approved these changes May 10, 2019
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

@karthikbox
Rollforward of api: Add total_issued_requests to Upstream Locality an…
d86a5c0 
…d Endpoint Stats. (envoyproxy#6692)

Changes: Fixes coverage test hang.
No change to core logic from envoyproxy#6692.

Signed-off-by: Karthik Reddy <rekarthik@google.com>
@lizan lizan merged commit 27b3e48 into envoyproxy:master May 10, 2019
This was referenced May 10, 2019
Closed
Merged
mpuncel added a commit to mpuncel/envoy that referenced this pull request May 10, 2019
@mpuncel
Merge branch 'master' into mpuncel/hedging-impl
7b721be 
* master: (88 commits)
  upstream: Null-deref on TCP health checker if setsockopt fails  (envoyproxy#6793)
  ci: switch macOS CI to azure pipelines (envoyproxy#6889)
  os syscalls lib: break apart syscalls used for hot restart (envoyproxy#6880)
  Kafka codec: precompute request size before serialization, so we do n… (envoyproxy#6862)
  upstream: move static and strict_dns clusters to dedicated files (envoyproxy#6886)
  Rollforward of api: Add total_issued_requests to Upstream Locality and Endpoint Stats. (envoyproxy#6692) (envoyproxy#6784)
  fix explicit constructor in copy-initialization (envoyproxy#6884)
  stats: use tag iterator rather than constructing the tag-array and searching that. (envoyproxy#6853)
  common: use unscoped build target in generate_version_linkstamp (envoyproxy#6877)
  Addendum to envoyproxy#6778 (envoyproxy#6882)
  ci: add minimum Linux build for Azure Pipelines (envoyproxy#6881)
  grpc: utilities for inter-converting grpc::ByteBuffer and Buffer::Instance. (envoyproxy#6732)
  upstream: allow excluding hosts from lb calculations until initial health check (envoyproxy#6794)
  stats: prevent unused counters from leaking across hot restart (envoyproxy#6850)
  network filters: add `injectDataToFilterChain(data, end_stream)` method to network filter callbacks (envoyproxy#6750)
  delete things that snuck back in (envoyproxy#6873)
  config: scoped rds (2b): support delta APIs in ConfigProvider framework (envoyproxy#6781)
  string == string! (envoyproxy#6868)
  config: add mssing imports to delta_subscription_state (envoyproxy#6869)
  protobuf: add missing default case to enum (envoyproxy#6870)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
@stevenzzzz stevenzzzz mentioned this pull request Nov 12, 2024
Open
@stevenzzzz stevenzzzz mentioned this pull request Dec 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizan lizan lizan approved these changes

@htuch htuch htuch left review comments

Assignees

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@karthikbox @htuch @lizan