feat: inbound only graceful draining by keithmattix · Pull Request #38430 · envoyproxy/envoy

keithmattix · 2025-02-12T16:41:22Z

Trying #37873 again to get to the bottom of the flakes that caused us to need to revert. This got a lot more complex after #38333, so I may need @yanavlasov to help me out with the proper semantics

Commit Message:

Fixes https://github.com/envoyproxy/envoy/issues/35020. The inbound_only query param for the drain_listeners admin
endpoint doesn't work with the graceful query parameter. As a result,
outbound listeners will send connection: close headers to upstreams
which is undesired. This PR adds the ability for drain_manager to drain
in a single direction.

Additional Description:
Risk Level: Medium
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix · 2025-02-13T04:13:55Z

Can someone from @envoyproxy/envoy-mobile-triage help me with these Android test failures? The smoking gun appears to be errors like these:

java.lang.UnsatisfiedLinkError: /mnt/engflow/worker/work/3/exec/bazel-out/k8-fastbuild/bin/test/java/io/envoyproxy/envoymobile/engine/testing/quic_test_server_test.runfiles/envoy_mobile/test/jni/libenvoy_jni_with_test_extensions.so: /mnt/engflow/worker/work/3/exec/bazel-out/k8-fastbuild/bin/test/java/io/envoyproxy/envoymobile/engine/testing/quic_test_server_test.runfiles/envoy_mobile/test/jni/libenvoy_jni_with_test_extensions.so: undefined symbol: __atomic_load

I tried to add a link command to the jni bazel config but no dice. I've never touched Envoy native development, so I truly have no idea what I'm doing

abeyad · 2025-02-13T16:57:29Z

java.lang.UnsatisfiedLinkError: /mnt/engflow/worker/work/3/exec/bazel-out/k8-fastbuild/bin/test/java/io/envoyproxy/envoymobile/engine/testing/quic_test_server_test.runfiles/envoy_mobile/test/jni/libenvoy_jni_with_test_extensions.so: /mnt/engflow/worker/work/3/exec/bazel-out/k8-fastbuild/bin/test/java/io/envoyproxy/envoymobile/engine/testing/quic_test_server_test.runfiles/envoy_mobile/test/jni/libenvoy_jni_with_test_extensions.so: undefined symbol: __atomic_load

You added the -latomic to libenvoy_jni.so, not to libenvoy_jni_with_test_extensions.so. Can you try adding it to

envoy/mobile/test/jni/BUILD

Line 110 in 87b4ae8

name = "libenvoy_jni_with_test_extensions.so",

? That said, it's unclear why you would get an std::atomic linker error with your change, as we've had std::atomic in Envoy code prior and no linker issues on Android.

fredyw · 2025-02-13T17:08:44Z

java.lang.UnsatisfiedLinkError: /mnt/engflow/worker/work/3/exec/bazel-out/k8-fastbuild/bin/test/java/io/envoyproxy/envoymobile/engine/testing/quic_test_server_test.runfiles/envoy_mobile/test/jni/libenvoy_jni_with_test_extensions.so: /mnt/engflow/worker/work/3/exec/bazel-out/k8-fastbuild/bin/test/java/io/envoyproxy/envoymobile/engine/testing/quic_test_server_test.runfiles/envoy_mobile/test/jni/libenvoy_jni_with_test_extensions.so: undefined symbol: __atomic_load

You added the -latomic to libenvoy_jni.so, not to libenvoy_jni_with_test_extensions.so. Can you try adding it to

envoy/mobile/test/jni/BUILD

Line 110 in 87b4ae8

name = "libenvoy_jni_with_test_extensions.so",

? That said, it's unclear why you would get an std::atomic linker error with your change, as we've had std::atomic in Envoy code prior and no linker issues on Android.

That's what I thought, too but when I tested it in 40faf7e, it didn't help, though: https://github.com/envoyproxy/envoy/actions/runs/13312980039/job/37180020453

I'm also not able to reproduce this locally.

fredyw · 2025-02-13T17:11:46Z

java.lang.UnsatisfiedLinkError: /mnt/engflow/worker/work/3/exec/bazel-out/k8-fastbuild/bin/test/java/io/envoyproxy/envoymobile/engine/testing/quic_test_server_test.runfiles/envoy_mobile/test/jni/libenvoy_jni_with_test_extensions.so: /mnt/engflow/worker/work/3/exec/bazel-out/k8-fastbuild/bin/test/java/io/envoyproxy/envoymobile/engine/testing/quic_test_server_test.runfiles/envoy_mobile/test/jni/libenvoy_jni_with_test_extensions.so: undefined symbol: __atomic_load

You added the -latomic to libenvoy_jni.so, not to libenvoy_jni_with_test_extensions.so. Can you try adding it to

envoy/mobile/test/jni/BUILD

Line 110 in 87b4ae8

name = "libenvoy_jni_with_test_extensions.so",

? That said, it's unclear why you would get an std::atomic linker error with your change, as we've had std::atomic in Envoy code prior and no linker issues on Android.

That's what I thought, too but when I tested it in 40faf7e, it didn't help, though: https://github.com/envoyproxy/envoy/actions/runs/13312980039/job/37180020453

I'm also not able to reproduce this locally.

Oh looks like it did help. I just need to link it with -latomic in a different shared library. Let me test it again.

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix · 2025-02-13T18:38:42Z

Android is now passing after integrating @fredyw's changes; thanks for the assist!

This is now ready for review

keithmattix · 2025-02-13T19:37:16Z

/retest

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

yanavlasov

Which test was flaky?

/wait

yanavlasov · 2025-02-14T15:16:38Z

.bazelrc

 # leave room for compiler/linker.
 # The number 3G is chosen heuristically to both support large VM and small VM with RBE.
 # Startup options cannot be selected via config.
+# TODO: Adding just to test android


Leftover from testing?

Yes android doesn't run in PRs by default; I wanted to get an approval on the code first before removing so there aren't surprises on main post merge

source/server/drain_manager_impl.h

yanavlasov · 2025-02-14T15:47:35Z

source/server/drain_manager_impl.h

+  std::atomic<DrainPair> draining_{DrainPair{false, Network::DrainDirection::None}};
+  // A map of timers keyed by the direction that triggered the drain
+  std::map<Network::DrainDirection, Event::TimerPtr> drain_tick_timers_;
+  std::map<Network::DrainDirection, MonotonicTime> drain_deadlines_ = {


This looks like a struct would be a better choice? Are the timer keys dynamic?

No we only expect a max of two keys (All and InboundOnly) at any time (and we have an assertion in the code to check).

keithmattix · 2025-02-14T15:55:49Z

Which test was flaky?

/wait

AdminDrainInboundOnlyIdempotent (or something similar). I believe I addressed it by adding the stopped_listener flatHashSet check into a callback function that runs on the main thread which should prevent the race conditions that led to the flaky test. Here's a failed run: https://github.com/envoyproxy/envoy/actions/runs/13172376287/job/36764947882

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix · 2025-02-20T15:10:22Z

@yanavlasov do you have any additional feedback?

yanavlasov

/wait

source/server/drain_manager_impl.h

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix · 2025-02-25T18:06:39Z

/retest

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix · 2025-03-02T18:32:37Z

/retest

@yanavlasov

Trying envoyproxy#37873 again to get to the bottom of the flakes that caused us to need to revert. This got a lot more complex after envoyproxy#38333, so I may need @yanavlasov to help me out with the proper semantics Commit Message: ``` Fixes envoyproxy#35020. The inbound_only query param for the drain_listeners admin endpoint doesn't work with the graceful query parameter. As a result, outbound listeners will send connection: close headers to upstreams which is undesired. This PR adds the ability for drain_manager to drain in a single direction. ``` Additional Description: Risk Level: Medium Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

@yanavlasov

Trying envoyproxy#37873 again to get to the bottom of the flakes that caused us to need to revert. This got a lot more complex after envoyproxy#38333, so I may need @yanavlasov to help me out with the proper semantics Commit Message: ``` Fixes envoyproxy#35020. The inbound_only query param for the drain_listeners admin endpoint doesn't work with the graceful query parameter. As a result, outbound listeners will send connection: close headers to upstreams which is undesired. This PR adds the ability for drain_manager to drain in a single direction. ``` Additional Description: Risk Level: Medium Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

@yanavlasov

Trying envoyproxy#37873 again to get to the bottom of the flakes that caused us to need to revert. This got a lot more complex after envoyproxy#38333, so I may need @yanavlasov to help me out with the proper semantics Commit Message: ``` Fixes envoyproxy#35020. The inbound_only query param for the drain_listeners admin endpoint doesn't work with the graceful query parameter. As a result, outbound listeners will send connection: close headers to upstreams which is undesired. This PR adds the ability for drain_manager to drain in a single direction. ``` Additional Description: Risk Level: Medium Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix added 2 commits February 12, 2025 16:38

Merge feature/inbound-only-graceful-draining

8b87b2e

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

Fixup

ed1132c

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix requested review from adisuissa, alyssawilk, mattklein123, wbpcode and zuercher as code owners February 12, 2025 16:41

keithmattix changed the title ~~Directional Draining~~ feat: inbound only graceful draining Feb 12, 2025

keithmattix added 2 commits February 12, 2025 22:43

Move stopped_listener check to callback on main thread

9b00aaa

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

Add libatomic to jni

c7f67fd

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix requested review from RyanTheOptimist, abeyad and fredyw as code owners February 13, 2025 03:26

mattklein123 assigned yanavlasov Feb 13, 2025

keithmattix added 2 commits February 13, 2025 18:10

Fix android linking issues

5efe6c2

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

Fix format

6ef466d

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix added 2 commits February 13, 2025 23:12

Link atomic for mobile binary size

e7510dd

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

Format

6240ea2

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

yanavlasov reviewed Feb 14, 2025

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 14, 2025

Access draining_ in an atomic fashion

97508bf

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

repokitteh-read-only bot removed the waiting label Feb 14, 2025

keithmattix requested a review from yanavlasov February 24, 2025 23:35

yanavlasov requested changes Feb 25, 2025

View reviewed changes

source/server/drain_manager_impl.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added waiting and removed waiting labels Feb 25, 2025

keithmattix requested a review from yanavlasov February 25, 2025 02:46

keithmattix added 2 commits February 25, 2025 02:46

Fix double load

e57511e

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

Fix other double load()

4b23d16

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

Move a comment

38bfe30

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

yanavlasov approved these changes Mar 4, 2025

View reviewed changes

yanavlasov merged commit c53f917 into envoyproxy:main Mar 4, 2025
25 checks passed

mikemorris mentioned this pull request Mar 5, 2025

Native Sidecars draining erroneously sending connection: close for outbound requests istio/istio#51855

Closed

2 tasks

eric846 mentioned this pull request Mar 8, 2025

Update Envoy to 78a8841 (Mar 07, 2025) envoyproxy/nighthawk#1309

Merged

Conversation

keithmattix commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keithmattix commented Feb 13, 2025

Uh oh!

abeyad commented Feb 13, 2025

Uh oh!

fredyw commented Feb 13, 2025

Uh oh!

fredyw commented Feb 13, 2025

Uh oh!

keithmattix commented Feb 13, 2025

Uh oh!

keithmattix commented Feb 13, 2025

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

yanavlasov Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

keithmattix Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yanavlasov Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

keithmattix Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

keithmattix commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keithmattix commented Feb 20, 2025

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keithmattix commented Feb 25, 2025

Uh oh!

keithmattix commented Mar 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

keithmattix commented Feb 12, 2025 •

edited

Loading

keithmattix commented Feb 14, 2025 •

edited

Loading