add tests for aggregate cluster circuit breaker by SaadiaELF · Pull Request #14 · bazmurphy/envoy

SaadiaELF · 2025-01-28T14:08:03Z

Commit Message:
Additional Description:
Risk Level:
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:]

nahratzah

There's a chance the reviewer will ask for only a single test to remain, and the other tests to be removed. In part because the interesting behaviour is getting the circuit breaker, and poking it, which is sufficiently proven with a single test.

I guess we'll find out. ❤️

nahratzah · 2025-01-28T16:21:05Z

test/extensions/clusters/aggregate/cluster_test.cc

  }

+  Stats::Gauge& getCircuitBreakersStatByPriority(std::string priority, std::string stat) {
+    std::string stat_name_ = "circuit_breakers." + priority + "." + stat;


Usually the trailing underscore (_) is only for private class member variables.
This is a local variable, so it should not have a trailing underscore.

Suggested change

std::string stat_name_ = "circuit_breakers." + priority + "." + stat;

std::string stat_name = "circuit_breakers." + priority + "." + stat;

(and similar on the following line)

@SaadiaELF this PR #23 fixes that^

nahratzah · 2025-01-28T16:23:41Z

test/extensions/clusters/aggregate/cluster_test.cc

+TEST_F(AggregateClusterTest, CircuitBreakerDefaultsTest) {
+  initialize(default_yaml_config_);
+
+  Upstream::ResourceManager& resource_manager =
+      cluster_->info()->resourceManager(Upstream::ResourcePriority::Default);
+
+  // the default circuit breaker values are:
+  // max_connections : 1024
+  // max_pending_requests : 1024
+  // max_requests : 1024
+  // max_retries : 3
+
+  EXPECT_EQ(1024U, resource_manager.connections().max());
+  for (int i = 0; i < 1024; ++i) {
+    resource_manager.connections().inc();
+  }
+  EXPECT_EQ(1024U, resource_manager.connections().count());
+  EXPECT_FALSE(resource_manager.connections().canCreate());
+
+  EXPECT_EQ(1024U, resource_manager.pendingRequests().max());
+  for (int i = 0; i < 1024; ++i) {
+    resource_manager.pendingRequests().inc();
+  }
+  EXPECT_EQ(1024U, resource_manager.pendingRequests().count());
+  EXPECT_FALSE(resource_manager.pendingRequests().canCreate());
+
+  EXPECT_EQ(1024U, resource_manager.requests().max());
+  for (int i = 0; i < 1024; ++i) {
+    resource_manager.requests().inc();
+  }
+  EXPECT_EQ(1024U, resource_manager.requests().count());
+  EXPECT_FALSE(resource_manager.requests().canCreate());
+
+  EXPECT_EQ(3U, resource_manager.retries().max());
+  for (int i = 0; i < 3; ++i) {
+    resource_manager.retries().inc();
+  }
+  EXPECT_EQ(3U, resource_manager.retries().count());
+  EXPECT_FALSE(resource_manager.retries().canCreate());
+}


There's a good chance that the envoy reviewer will ask for this test to be removed.

The reason is that if they ever want to change the defaults of the circuit breaker, they'll want to only have tests in the circuit breaker affected. Whereas adding this test would mean if they change the circuit breaker, this test will fail.

You're fine to leave it in, just a heads-up that they may ask to remove this. ❤️

Yes, valid point 👍
See #14 (comment)

nahratzah · 2025-01-28T16:29:54Z

test/extensions/clusters/aggregate/cluster_test.cc

+    EXPECT_EQ(expected_remaining, remaining.value());
+    EXPECT_EQ(expected_open, open.value());


I expect the reviewer will want the metrics removed from the test.
That way, they can change the behaviour of circuit breakers in the future, and it won't break tests here.

You're fine to leave it in if you really really like them, just letting you know. <3

The defaults are set in source/common/upstream/upstream_impl.cc
https://github.com/envoyproxy/envoy/blob/main/source/common/upstream/upstream_impl.cc#L2070-L2075

uint64_t max_connections = 1024; uint64_t max_pending_requests = 1024; uint64_t max_requests = 1024; uint64_t max_retries = 3; uint64_t max_connection_pools = std::numeric_limits<uint64_t>::max(); uint64_t max_connections_per_host = std::numeric_limits<uint64_t>::max();

but I can't think of a way to mirror those to set the defaults for the tests
so that, as you say, if the defaults were changed later (in upstream_impl.cc) that it would also be reflected in the tests

and the same applies to your comment above this one

hmmm how could we do this 🤔

bazmurphy · 2025-01-29T09:19:44Z

We can try leaving the statistic checks in, and see what the reviewer(s) say(s), maybe its an opportunity for us to learn something! 🤓

And we are OK if shred the PR to pieces 😆

If all that remains is what is useful to them, that's OK 😎

What do you think @SaadiaELF ?

Co-authored-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com> Signed-off-by: Baz Murphy <61154071+bazmurphy@users.noreply.github.com> Signed-off-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com>

…iour Co-authored-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com> Signed-off-by: Baz Murphy <61154071+bazmurphy@users.noreply.github.com> Signed-off-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com>

Co-authored-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com> Signed-off-by: Baz Murphy <61154071+bazmurphy@users.noreply.github.com> Signed-off-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com>

Co-authored-by: Baz Murphy <61154071+bazmurphy@users.noreply.github.com> Signed-off-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com> Signed-off-by: Baz Murphy <61154071+bazmurphy@users.noreply.github.com>

bazmurphy · 2025-04-16T14:15:53Z

see review on: #35

When trying to build and run fuzzers that may throw an exception (e.g., `bazel build //test/common/http:path_utility_fuzz_test --config=asan-fuzzer`) the following error occurs: ``` $ bazel-bin/test/common/http/path_utility_fuzz_test /tmp/corpus INFO: found LLVMFuzzerCustomMutator (0x555ec3b27058). Disabling -len_control by default. INFO: Running with entropic power schedule (0xFF, 100). INFO: Seed: 3031084950 INFO: Loaded 1 modules (2436830 inline 8-bit counters): 2436830 [0x555ecf049c40, 0x555ecf29cb1e), INFO: Loaded 1 PC tables (2436830 PCs): 2436830 [0x555ecf29cb20,0x555ed17cb900), INFO: 5 files found in /tmp/corpus INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes INFO: seed corpus: files: 5 min: 91b max: 332b total: 1208b rss: 357Mb libc++abi: terminating due to uncaught exception of type Envoy::EnvoyException ==3072858== ERROR: libFuzzer: deadly signal #0 0x555ec3ae0911 in __sanitizer_print_stack_trace (/usr/local/google/home/adip/.cache/bazel/_bazel_adip/8e88866af51670ff1222d99304421e7c/execroot/envoy/bazel-out/k8-fastbuild/bin/test/common/http/path_utility_fuzz_test+0xa49f911) #1 0x555ec39e56b8 in fuzzer::PrintStackTrace() cxa_noexception.cpp #2 0x555ec39c8a53 in fuzzer::Fuzzer::CrashCallback() cxa_noexception.cpp #3 0x7f5506763e1f (/lib/x86_64-linux-gnu/libc.so.6+0x3fe1f) (BuildId: ea119b374e0f8f858c82ad03a9542414f9ea1c8c) #4 0x7f55067b7e5b in __pthread_kill_implementation nptl/pthread_kill.c:43:17 #5 0x7f5506763d81 in raise signal/../sysdeps/posix/raise.c:26:13 #6 0x7f550674c4ef in abort stdlib/abort.c:79:7 #7 0x555ecc751905 in abort_message abort_message.cpp #8 0x555ecc751ae2 in demangling_terminate_handler() cxa_default_handlers.cpp #9 0x555ecc7519a2 in std::__terminate(void (*)()) cxa_handlers.cpp #10 0x555ecc750fd5 in __cxxabiv1::failed_throw(__cxxabiv1::__cxa_exception*) cxa_exception.cpp #11 0x555ecc750fbf in __cxa_throw (/usr/local/google/home/adip/.cache/bazel/_bazel_adip/8e88866af51670ff1222d99304421e7c/execroot/envoy/bazel-out/k8-fastbuild/bin/test/common/http/path_utility_fuzz_test+0x1310ffbf) #12 0x555ec97f5a1f in Envoy::ProtoExceptionUtil::throwProtoValidationException(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, google::protobuf::Message const&) (/usr/local/google/home/adip/.cache/bazel/_bazel_adip/8e88866af51670ff1222d99304421e7c/execroot/envoy/bazel-out/k8-fastbuild/bin/test/common/http/path_utility_fuzz_test+0x101b4a1f) #13 0x555ec3b87a1c in void Envoy::MessageUtil::validate<test::common::http::PathUtilityTestCase>(test::common::http::PathUtilityTestCase const&, Envoy::ProtobufMessage::ValidationVisitor&, bool) (/usr/local/google/home/adip/.cache/bazel/_bazel_adip/8e88866af51670ff1222d99304421e7c/execroot/envoy/bazel-out/k8-fastbuild/bin/test/common/http/path_utility_fuzz_test+0xa546a1c) #14 0x555ec3b277be in LLVMFuzzerTestOneInput (/usr/local/google/home/adip/.cache/bazel/_bazel_adip/8e88866af51670ff1222d99304421e7c/execroot/envoy/bazel-out/k8-fastbuild/bin/test/common/http/path_utility_fuzz_test+0xa4e67be) #15 0x555ec39c9f60 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) cxa_noexception.cpp #16 0x555ec39c9785 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) cxa_noexception.cpp #17 0x555ec39cb712 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile>>&) cxa_noexception.cpp #18 0x555ec39cba02 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile>>&) cxa_noexception.cpp #19 0x555ec39b9eeb in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) cxa_noexception.cpp #20 0x555ec39e5f12 in main (/usr/local/google/home/adip/.cache/bazel/_bazel_adip/8e88866af51670ff1222d99304421e7c/execroot/envoy/bazel-out/k8-fastbuild/bin/test/common/http/path_utility_fuzz_test+0xa3a4f12) #21 0x7f550674dd67 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 #22 0x7f550674de24 in __libc_start_main csu/../csu/libc-start.c:360:3 #23 0x555ec39ac720 in _start (/usr/local/google/home/adip/.cache/bazel/_bazel_adip/8e88866af51670ff1222d99304421e7c/execroot/envoy/bazel-out/k8-fastbuild/bin/test/common/http/path_utility_fuzz_test+0xa36b720) NOTE: libFuzzer has rudimentary signal handlers. Combine libFuzzer with AddressSanitizer or similar for better crash reports. SUMMARY: libFuzzer: deadly signal ``` This PR adds a linking flag to allow the fuzzers to properly handle exceptions. Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Co-authored-by: Baz Murphy <61154071+bazmurphy@users.noreply.github.com> Signed-off-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com> Signed-off-by: Baz Murphy <61154071+bazmurphy@users.noreply.github.com>

…iour Co-authored-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com> Signed-off-by: Baz Murphy <61154071+bazmurphy@users.noreply.github.com> Signed-off-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com>

SaadiaELF changed the title ~~[DO NOT MERGE] add tests placeholders~~ [DO NOT MERGE] PR Jan 28, 2025

SaadiaELF marked this pull request as ready for review January 28, 2025 16:08

SaadiaELF changed the title ~~[DO NOT MERGE] PR~~ [DO NOT MERGE] tests : add tests for aggregate cluster circuit breaker Jan 28, 2025

nahratzah reviewed Jan 28, 2025

View reviewed changes

bazmurphy force-pushed the test-aggregate-cluster-circuit-breaker-behaviour branch 3 times, most recently from 13a7319 to 40d7a6a Compare January 29, 2025 15:48

bazmurphy force-pushed the test-aggregate-cluster-circuit-breaker-behaviour branch from 40d7a6a to 897727d Compare January 30, 2025 10:44

bazmurphy force-pushed the test-aggregate-cluster-circuit-breaker-behaviour branch from 225be2f to 88ce8db Compare April 16, 2025 11:54

bazmurphy and others added 3 commits April 16, 2025 05:13

bazmurphy changed the title ~~[DO NOT MERGE] tests : add tests for aggregate cluster circuit breaker~~ add tests for aggregate cluster circuit breaker Apr 16, 2025

SaadiaELF and others added 3 commits April 23, 2025 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add tests for aggregate cluster circuit breaker#14

add tests for aggregate cluster circuit breaker#14
SaadiaELF wants to merge 8 commits intomainfrom
test-aggregate-cluster-circuit-breaker-behaviour

SaadiaELF commented Jan 28, 2025 •

edited

Loading

Uh oh!

nahratzah left a comment

Uh oh!

nahratzah Jan 28, 2025

Uh oh!

bazmurphy Jan 29, 2025

Uh oh!

nahratzah Jan 28, 2025

Uh oh!

bazmurphy Jan 29, 2025

Uh oh!

nahratzah Jan 28, 2025

Uh oh!

bazmurphy Jan 29, 2025 •

edited

Loading

Uh oh!

bazmurphy commented Jan 29, 2025 •

edited

Loading

Uh oh!

bazmurphy commented Apr 16, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	std::string stat_name_ = "circuit_breakers." + priority + "." + stat;
	std::string stat_name = "circuit_breakers." + priority + "." + stat;

		EXPECT_EQ(expected_remaining, remaining.value());
		EXPECT_EQ(expected_open, open.value());

Conversation

SaadiaELF commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nahratzah left a comment

Choose a reason for hiding this comment

Uh oh!

nahratzah Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

bazmurphy Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

nahratzah Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

bazmurphy Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

nahratzah Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

bazmurphy Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bazmurphy commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bazmurphy commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SaadiaELF commented Jan 28, 2025 •

edited

Loading

bazmurphy Jan 29, 2025 •

edited

Loading

bazmurphy commented Jan 29, 2025 •

edited

Loading

bazmurphy commented Apr 16, 2025 •

edited

Loading