Skip to content

test: Fix hot restart test under --runs_per_test#2609

Merged
dnoe merged 3 commits intoenvoyproxy:masterfrom
dnoe:hot-restart-test-concurrency
Feb 15, 2018
Merged

test: Fix hot restart test under --runs_per_test#2609
dnoe merged 3 commits intoenvoyproxy:masterfrom
dnoe:hot-restart-test-concurrency

Conversation

@dnoe
Copy link
Contributor

@dnoe dnoe commented Feb 14, 2018

Description:
When using --runs_per_test with concurrency the individual invocations
of the hot restart test may collide. Bazel provides an environment
variable when using --runs_per_test that allows us to set --base_id in
each parallel invocation to a guaranteed unique value so this doesn't
happen.

Risk Level: Low

Testing: bazel test //test/integration:hotrestart_test --runs_per_test=100

Signed-off-by: Dan Noé dpn@google.com

When using --runs_per_test with concurrency the individual invocations
of the hot restart test may collide. Bazel provides an environment
variable when using --runs_per_test that allows us to set --base_id in
each parallel invocation to a guaranteed unique value so this doesn't
happen.

Signed-off-by: Dan Noé <dpn@google.com>
@dnoe dnoe requested a review from ggreenway February 14, 2018 18:44
ggreenway
ggreenway previously approved these changes Feb 14, 2018
Copy link
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Nice.

htuch
htuch previously approved these changes Feb 14, 2018
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!

if [[ -z "${TEST_RANDOM_SEED}" ]]; then
BASE_ID=1
else
BASE_ID="${TEST_RANDOM_SEED}"
Copy link
Member

Choose a reason for hiding this comment

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

Arguably this still has the issue that if you run two distinct bazel test on the same machine, with different run IDs, you will have potential conflict. I think it's fine for now, this is a useful improvement over status quo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't Bazel serialize separate build or test invocations made on the same machine in the same code base? When I try it in a separate window it tells me it's waiting for the first command to complete.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's per-bazel-daemon. On an interactive system, that's generally 1 per user (I think). On a build machine, it's probably 1 per container.

Copy link
Member

Choose a reason for hiding this comment

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

It's per source directory actually (I know because I run multiple concurrently in different Envoy trees :).

@dnoe
Copy link
Contributor Author

dnoe commented Feb 14, 2018

Amusingly, looks like it has failed in CI on the real flake in hotrestart test in async grpc client

Envoy sets up SHM even when not starting up fully, so even cases where
the binary is not started with a full config want --base-id. Otherwise
we can get "file exists" when running many tests in parallel.

Signed-off-by: Dan Noé <dpn@google.com>
@dnoe dnoe dismissed stale reviews from htuch and ggreenway via 320cd11 February 14, 2018 21:30
@dnoe
Copy link
Contributor Author

dnoe commented Feb 14, 2018

I think all the cases where the envoy binary is launched from the hot restart test are now properly configured with base-id.

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.

This was invaluable in figuring out #2613. We can merge that one first, then we should be able to have this pass CI and help deflake the roll forward.

@htuch
Copy link
Member

htuch commented Feb 15, 2018

Please merge master to pick up #2613, this will unblock CI.

@dnoe dnoe merged commit acf8ad4 into envoyproxy:master Feb 15, 2018
@dnoe dnoe deleted the hot-restart-test-concurrency branch February 15, 2018 15:17
Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020
* Update proxy to envoy-wasm@9dc356, 12/17

* ReSync to latest envoyproxy/envoy-wasm@9cf22b8

* Fix build

* Lint
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action

Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action

Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants