[Windows] Address status/fix various flaky_on_windows tests by wrowe · Pull Request #13837 · envoyproxy/envoy

wrowe · 2020-10-30T20:42:34Z

Commit Message: [Windows] Address status/fix various flaky_on_windows tests
Additional Description:

Correct listener_impl_test, fixes
TcpListenerImplTest.SetListenerRejectFractionIntermediate
These fixes reflect that the timing on windows is often less
synchronous than linux observations, due to the design of the
underlying socket stack.
In QuicHttpIntegrationTest, the MultipleQuicConnectionsWithBPF is
never relevant to Windows or macOS (and took a not-insignificant
amount of time per test case.)
Enable no-longer-failing tests (no failure observed in RBE or locally)
//test/extensions/grpc_credentials/file_based_metadata:integration_test
//test/extensions/transport_sockets/alts:tsi_handshaker_test
//test/integration:http2_flood_integration_test
//test/integration:http2_upstream_integration_test
//test/integration:overload_integration_test
//test/server:guarddog_impl_test

Co-authored-by: Sunjay Bhatia sunjayb@vmware.com
Co-authored-by: William A Rowe Jr wrowe@vmware.com

Risk Level: Low (test enablement only)
Testing: Windows msvc-cl and clang-cl RBE and locally
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

These fixes reflect that the timing on windows is often less synchronous than linux observations, due to the design of the underlying socket stack. - Correct listener_impl_test, fixes TcpListenerImplTest.SetListenerRejectFractionIntermediate - In QuicHttpIntegrationTest, the MultipleQuicConnectionsWithBPF is never relevant to Windows or macOS (and took a not-insigificant amount of time per test case.) - Enable no-longer-failing tests (no failure observed in RBE or locally) //test/extensions/grpc_credentials/file_based_metadata:integration_test //test/extensions/transport_sockets/alts:tsi_handshaker_test //test/integration:http2_flood_integration_test //test/integration:http2_upstream_integration_test //test/integration:overload_integration_test //test/server:guarddog_impl_test Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com> Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

sunjayBhatia · 2020-10-30T20:45:12Z

@envoyproxy/windows-dev

davinci26 · 2020-11-02T16:58:28Z

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

 }

+// Only run on platforms that support BPF
+#if defined(SO_ATTACH_REUSEPORT_CBPF) && defined(__linux__)


why not just if defined(SO_ATTACH_REUSEPORT_CBPF)?

we matched what was in here:

envoy/source/extensions/quic_listeners/quiche/active_quic_listener.cc

Line 229 in 9025fba

#if defined(SO_ATTACH_REUSEPORT_CBPF) && defined(__linux__)

looks like the test has undergone this change already looking back at the blame

see this diff: f8c3acc#diff-5872f75ef2969ee19f048c4dfc487bb7acc649021940a8acca7a4921b2d5c910

not sure why exactly the ifdef was taken out for platforms that do not support BPF

originally added here: e176b30#diff-5872f75ef2969ee19f048c4dfc487bb7acc649021940a8acca7a4921b2d5c910R233

davinci26 · 2020-11-02T16:59:19Z

LGTM

ggreenway · 2020-11-02T17:42:51Z

test/common/network/listener_impl_test.cc

    testing::InSequence s1;
    EXPECT_CALL(random_generator, random()).WillOnce(Return(std::numeric_limits<uint64_t>::max()));
-    EXPECT_CALL(listener_callbacks, onAccept_(_));
+    EXPECT_CALL(listener_callbacks, onAccept_(_)).WillOnce([&] { dispatcher_->exit(); });


Can you explain this change? It's not clear to me why this is better than the old version.

making the dispatcher exit on the connected event makes the test racy, the dispatcher could exit before there is a chance to invoke the tcp listener onAccept callback which is effectively the last thing that happens when the connection is established/accepted

Oh I see. I thought the events happened in the opposite order. Sounds good.

ggreenway · 2020-11-02T17:46:49Z

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

 }

+// Only run on platforms that support BPF
+#if defined(SO_ATTACH_REUSEPORT_CBPF) && defined(__linux__)


Why ifdef this out? Doesn't it pass on non-bpf platforms? (I'm basing that assumption on the comment below that the runtime override is a no-op on non-bpf platforms).

It does pass, but may give the wrong impress on non-BPF platforms for someone taking a quick glance that BPF support somehow works, ifdef-ing it out is mainly a clarification

Also, we are having issues with this test timing out, not running the various parameterized runs of this test limits test runtime a bit

Ah, I think the test really should be renamed, and always run. The name should probably be "Default mode" or something. It is testing that, given no extra config, quic works on this platform, which is why I think it should run on all platforms.

It is doing the same thing as the next test on non-bpf platforms, but the goal is to ensure that both configurations work on all platforms.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia · 2020-11-02T18:48:23Z

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc


-TEST_P(QuicHttpIntegrationTest, MultipleQuicConnectionsWithBPF) { testMultipleQuicConnections(); }
+// Ensure multiple quic connections work, regardless of platform BPF support
+TEST_P(QuicHttpIntegrationTest, MultipleQuicConnectionsDefaultMode) {


@ggreenway renamed, let me know what you think (removed BPF from the name)

Yes, that looks better, thanks

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

ggreenway

LGTM, modulo one minor nit

ggreenway · 2020-11-02T19:15:33Z

test/common/network/listener_impl_test.cc

    testing::InSequence s1;
    EXPECT_CALL(random_generator, random()).WillOnce(Return(std::numeric_limits<uint64_t>::max()));
+    // Exiting dispatcher on client side connect event can cause a race, listener accept callback
+    // may not have been triggered, exit dispatcher here to prevent this


Nit: period at end of sentence.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

wrowe requested review from alyssawilk, htuch and mattklein123 as code owners October 30, 2020 20:42

wrowe added area/build area/test flakes area/windows labels Oct 31, 2020

wrowe added this to the windows-beta milestone Oct 31, 2020

mattklein123 assigned davinci26 Nov 2, 2020

davinci26 reviewed Nov 2, 2020

View reviewed changes

davinci26 previously approved these changes Nov 2, 2020

View reviewed changes

ggreenway requested changes Nov 2, 2020

View reviewed changes

Rename test

030868f

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia dismissed davinci26’s stale review via 030868f November 2, 2020 18:37

Rename test

307317d

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia reviewed Nov 2, 2020

View reviewed changes

Comment

0e2b70f

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia requested a review from ggreenway November 2, 2020 18:57

ggreenway requested changes Nov 2, 2020

View reviewed changes

add period

ec01ee9

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

ggreenway approved these changes Nov 2, 2020

View reviewed changes

ggreenway self-assigned this Nov 2, 2020

ggreenway merged commit 5df0a6b into envoyproxy:master Nov 3, 2020

sunjayBhatia deleted the windows-fix-flakes branch November 3, 2020 20:09

wrowe removed this from the windows-ga milestone Jan 15, 2021

Conversation

wrowe commented Oct 30, 2020

Uh oh!

sunjayBhatia commented Oct 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davinci26 commented Nov 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants