From a88ee9ef99a975f284ece55b9aa0bb7beacdd7ed Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 28 May 2024 19:39:23 -0700 Subject: [PATCH] [http2] Add experiment to modify behavior of GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA to throttle pings instead of blocking (#36374) Implements https://github.com/grpc/proposal/pull/429 Currently, the behavior of `GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA` blocks more pings from being sent if we are sending too many pings without a data/header frame being sent as well. The original intention of this channel arg was to play nice with proxies that have restrictive settings when it comes to pings. This causes awkwardness when configuring keepalive pings for transports with long lived streams with sparse communication. In such a case, gRPC Core would stop sending keepalive pings since no data/header frame is being sent, resulting in a situation where we are unable to detect whether the transport is alive or not. This change adds an experiment "max_pings_wo_data_throttle" to modify the behavior of `GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA` to throttle pings to a frequency of 1 minute instead of completely blocking pings when too many pings have been sent without data/header frames. Closes #36374 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36374 from yashykt:ThrottlePings b5bd42a019b7744777da8e1ac9b6d6f3e90e1ae9 PiperOrigin-RevId: 638110795 --- bazel/experiments.bzl | 1 + include/grpc/impl/channel_arg_names.h | 8 ++-- .../chttp2/transport/ping_rate_policy.cc | 42 +++++++++++++------ .../chttp2/transport/ping_rate_policy.h | 9 ++-- src/core/lib/experiments/experiments.cc | 24 +++++++++++ src/core/lib/experiments/experiments.h | 8 ++++ src/core/lib/experiments/experiments.yaml | 8 ++++ src/core/lib/experiments/rollouts.yaml | 2 + test/core/end2end/tests/bad_ping.cc | 3 ++ .../transport/chttp2/ping_rate_policy_test.cc | 35 ++++++++++++++++ 10 files changed, 120 insertions(+), 20 deletions(-) diff --git a/bazel/experiments.bzl b/bazel/experiments.bzl index 053677378a6..7b41b29566a 100644 --- a/bazel/experiments.bzl +++ b/bazel/experiments.bzl @@ -28,6 +28,7 @@ EXPERIMENT_ENABLES = { "http2_stats_fix": "http2_stats_fix", "keepalive_fix": "keepalive_fix", "keepalive_server_fix": "keepalive_server_fix", + "max_pings_wo_data_throttle": "max_pings_wo_data_throttle", "monitoring_experiment": "monitoring_experiment", "multiping": "multiping", "peer_state_based_framing": "peer_state_based_framing", diff --git a/include/grpc/impl/channel_arg_names.h b/include/grpc/impl/channel_arg_names.h index 663e9620a34..efd380b8681 100644 --- a/include/grpc/impl/channel_arg_names.h +++ b/include/grpc/impl/channel_arg_names.h @@ -111,9 +111,11 @@ "grpc.server_max_unrequested_time_in_server" /** Channel arg to override the http2 :scheme header */ #define GRPC_ARG_HTTP2_SCHEME "grpc.http2_scheme" -/** How many pings can the client send before needing to send a - data/header frame? (0 indicates that an infinite number of - pings can be sent without sending a data frame or header frame) */ +/** How many pings can the client send before needing to send a data/header + frame? (0 indicates that an infinite number of pings can be sent without + sending a data frame or header frame). + If experiment "max_pings_wo_data_throttle" is enabled, instead of pings being + completely blocked, they are throttled. */ #define GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA \ "grpc.http2.max_pings_without_data" /** How many misbehaving pings the server can bear before sending goaway and diff --git a/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc b/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc index fed8ca20371..143b08a67c7 100644 --- a/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc +++ b/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc @@ -35,16 +35,18 @@ namespace grpc_core { namespace { -int g_default_max_pings_without_data = 2; +int g_default_max_pings_without_data_sent = 2; +constexpr Duration kThrottleIntervalWithoutDataSent = Duration::Minutes(1); absl::optional g_default_max_inflight_pings; } // namespace Chttp2PingRatePolicy::Chttp2PingRatePolicy(const ChannelArgs& args, bool is_client) - : max_pings_without_data_( + : max_pings_without_data_sent_( is_client - ? std::max(0, args.GetInt(GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA) - .value_or(g_default_max_pings_without_data)) + ? std::max(0, + args.GetInt(GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA) + .value_or(g_default_max_pings_without_data_sent)) : 0), // Configuration via channel arg dominates, otherwise if the multiping // experiment is enabled we use 100, otherwise 1. @@ -54,18 +56,15 @@ Chttp2PingRatePolicy::Chttp2PingRatePolicy(const ChannelArgs& args, IsMultipingEnabled() ? 100 : 1)))) {} void Chttp2PingRatePolicy::SetDefaults(const ChannelArgs& args) { - g_default_max_pings_without_data = + g_default_max_pings_without_data_sent = std::max(0, args.GetInt(GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA) - .value_or(g_default_max_pings_without_data)); + .value_or(g_default_max_pings_without_data_sent)); g_default_max_inflight_pings = args.GetInt(GRPC_ARG_HTTP2_MAX_INFLIGHT_PINGS); } Chttp2PingRatePolicy::RequestSendPingResult Chttp2PingRatePolicy::RequestSendPing(Duration next_allowed_ping_interval, size_t inflight_pings) const { - if (max_pings_without_data_ != 0 && pings_before_data_required_ == 0) { - return TooManyRecentPings{}; - } if (max_inflight_pings_ > 0 && inflight_pings > static_cast(max_inflight_pings_)) { return TooManyRecentPings{}; @@ -77,12 +76,29 @@ Chttp2PingRatePolicy::RequestSendPing(Duration next_allowed_ping_interval, return TooSoon{next_allowed_ping_interval, last_ping_sent_time_, next_allowed_ping - now}; } + // Throttle pings to 1 minute if we haven't sent any data recently + if (max_pings_without_data_sent_ != 0 && + pings_before_data_sending_required_ == 0) { + if (IsMaxPingsWoDataThrottleEnabled()) { + const Timestamp next_allowed_ping = + last_ping_sent_time_ + kThrottleIntervalWithoutDataSent; + if (next_allowed_ping > now) { + return TooSoon{kThrottleIntervalWithoutDataSent, last_ping_sent_time_, + next_allowed_ping - now}; + } + } else { + return TooManyRecentPings{}; + } + } + return SendGranted{}; } void Chttp2PingRatePolicy::SentPing() { last_ping_sent_time_ = Timestamp::Now(); - if (pings_before_data_required_) --pings_before_data_required_; + if (pings_before_data_sending_required_ > 0) { + --pings_before_data_sending_required_; + } } void Chttp2PingRatePolicy::ReceivedDataFrame() { @@ -90,13 +106,13 @@ void Chttp2PingRatePolicy::ReceivedDataFrame() { } void Chttp2PingRatePolicy::ResetPingsBeforeDataRequired() { - pings_before_data_required_ = max_pings_without_data_; + pings_before_data_sending_required_ = max_pings_without_data_sent_; } std::string Chttp2PingRatePolicy::GetDebugString() const { return absl::StrCat( - "max_pings_without_data: ", max_pings_without_data_, - ", pings_before_data_required: ", pings_before_data_required_, + "max_pings_without_data: ", max_pings_without_data_sent_, + ", pings_before_data_required: ", pings_before_data_sending_required_, ", last_ping_sent_time_: ", last_ping_sent_time_.ToString()); } diff --git a/src/core/ext/transport/chttp2/transport/ping_rate_policy.h b/src/core/ext/transport/chttp2/transport/ping_rate_policy.h index b9501686d95..6dab7a361bc 100644 --- a/src/core/ext/transport/chttp2/transport/ping_rate_policy.h +++ b/src/core/ext/transport/chttp2/transport/ping_rate_policy.h @@ -70,13 +70,14 @@ class Chttp2PingRatePolicy { void ReceivedDataFrame(); std::string GetDebugString() const; - int TestOnlyMaxPingsWithoutData() const { return max_pings_without_data_; } + int TestOnlyMaxPingsWithoutData() const { + return max_pings_without_data_sent_; + } private: - const int max_pings_without_data_; + const int max_pings_without_data_sent_; const int max_inflight_pings_; - // No pings allowed before receiving a header or data frame. - int pings_before_data_required_ = 0; + int pings_before_data_sending_required_ = 0; Timestamp last_ping_sent_time_ = Timestamp::InfPast(); }; diff --git a/src/core/lib/experiments/experiments.cc b/src/core/lib/experiments/experiments.cc index f54990cac18..b4ba5bdba89 100644 --- a/src/core/lib/experiments/experiments.cc +++ b/src/core/lib/experiments/experiments.cc @@ -59,6 +59,11 @@ const char* const description_keepalive_server_fix = "Allows overriding keepalive_permit_without_calls for servers. Refer " "https://github.com/grpc/grpc/pull/33917 for more information."; const char* const additional_constraints_keepalive_server_fix = "{}"; +const char* const description_max_pings_wo_data_throttle = + "Experiment to throttle pings to a period of 1 min when " + "GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA limit has reached (instead of " + "completely blocking)."; +const char* const additional_constraints_max_pings_wo_data_throttle = "{}"; const char* const description_monitoring_experiment = "Placeholder experiment to prove/disprove our monitoring is working"; const char* const additional_constraints_monitoring_experiment = "{}"; @@ -157,6 +162,9 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_keepalive_fix, nullptr, 0, false, false}, {"keepalive_server_fix", description_keepalive_server_fix, additional_constraints_keepalive_server_fix, nullptr, 0, false, false}, + {"max_pings_wo_data_throttle", description_max_pings_wo_data_throttle, + additional_constraints_max_pings_wo_data_throttle, nullptr, 0, false, + true}, {"monitoring_experiment", description_monitoring_experiment, additional_constraints_monitoring_experiment, nullptr, 0, true, true}, {"multiping", description_multiping, additional_constraints_multiping, @@ -241,6 +249,11 @@ const char* const description_keepalive_server_fix = "Allows overriding keepalive_permit_without_calls for servers. Refer " "https://github.com/grpc/grpc/pull/33917 for more information."; const char* const additional_constraints_keepalive_server_fix = "{}"; +const char* const description_max_pings_wo_data_throttle = + "Experiment to throttle pings to a period of 1 min when " + "GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA limit has reached (instead of " + "completely blocking)."; +const char* const additional_constraints_max_pings_wo_data_throttle = "{}"; const char* const description_monitoring_experiment = "Placeholder experiment to prove/disprove our monitoring is working"; const char* const additional_constraints_monitoring_experiment = "{}"; @@ -339,6 +352,9 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_keepalive_fix, nullptr, 0, false, false}, {"keepalive_server_fix", description_keepalive_server_fix, additional_constraints_keepalive_server_fix, nullptr, 0, false, false}, + {"max_pings_wo_data_throttle", description_max_pings_wo_data_throttle, + additional_constraints_max_pings_wo_data_throttle, nullptr, 0, false, + true}, {"monitoring_experiment", description_monitoring_experiment, additional_constraints_monitoring_experiment, nullptr, 0, true, true}, {"multiping", description_multiping, additional_constraints_multiping, @@ -423,6 +439,11 @@ const char* const description_keepalive_server_fix = "Allows overriding keepalive_permit_without_calls for servers. Refer " "https://github.com/grpc/grpc/pull/33917 for more information."; const char* const additional_constraints_keepalive_server_fix = "{}"; +const char* const description_max_pings_wo_data_throttle = + "Experiment to throttle pings to a period of 1 min when " + "GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA limit has reached (instead of " + "completely blocking)."; +const char* const additional_constraints_max_pings_wo_data_throttle = "{}"; const char* const description_monitoring_experiment = "Placeholder experiment to prove/disprove our monitoring is working"; const char* const additional_constraints_monitoring_experiment = "{}"; @@ -521,6 +542,9 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_keepalive_fix, nullptr, 0, false, false}, {"keepalive_server_fix", description_keepalive_server_fix, additional_constraints_keepalive_server_fix, nullptr, 0, false, false}, + {"max_pings_wo_data_throttle", description_max_pings_wo_data_throttle, + additional_constraints_max_pings_wo_data_throttle, nullptr, 0, false, + true}, {"monitoring_experiment", description_monitoring_experiment, additional_constraints_monitoring_experiment, nullptr, 0, true, true}, {"multiping", description_multiping, additional_constraints_multiping, diff --git a/src/core/lib/experiments/experiments.h b/src/core/lib/experiments/experiments.h index 999ee24a8ed..50dc88d8269 100644 --- a/src/core/lib/experiments/experiments.h +++ b/src/core/lib/experiments/experiments.h @@ -70,6 +70,7 @@ inline bool IsFreeLargeAllocatorEnabled() { return false; } inline bool IsHttp2StatsFixEnabled() { return true; } inline bool IsKeepaliveFixEnabled() { return false; } inline bool IsKeepaliveServerFixEnabled() { return false; } +inline bool IsMaxPingsWoDataThrottleEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return true; } inline bool IsMultipingEnabled() { return false; } @@ -107,6 +108,7 @@ inline bool IsFreeLargeAllocatorEnabled() { return false; } inline bool IsHttp2StatsFixEnabled() { return true; } inline bool IsKeepaliveFixEnabled() { return false; } inline bool IsKeepaliveServerFixEnabled() { return false; } +inline bool IsMaxPingsWoDataThrottleEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return true; } inline bool IsMultipingEnabled() { return false; } @@ -144,6 +146,7 @@ inline bool IsFreeLargeAllocatorEnabled() { return false; } inline bool IsHttp2StatsFixEnabled() { return true; } inline bool IsKeepaliveFixEnabled() { return false; } inline bool IsKeepaliveServerFixEnabled() { return false; } +inline bool IsMaxPingsWoDataThrottleEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return true; } inline bool IsMultipingEnabled() { return false; } @@ -180,6 +183,7 @@ enum ExperimentIds { kExperimentIdHttp2StatsFix, kExperimentIdKeepaliveFix, kExperimentIdKeepaliveServerFix, + kExperimentIdMaxPingsWoDataThrottle, kExperimentIdMonitoringExperiment, kExperimentIdMultiping, kExperimentIdPeerStateBasedFraming, @@ -242,6 +246,10 @@ inline bool IsKeepaliveFixEnabled() { inline bool IsKeepaliveServerFixEnabled() { return IsExperimentEnabled(kExperimentIdKeepaliveServerFix); } +#define GRPC_EXPERIMENT_IS_INCLUDED_MAX_PINGS_WO_DATA_THROTTLE +inline bool IsMaxPingsWoDataThrottleEnabled() { + return IsExperimentEnabled(kExperimentIdMaxPingsWoDataThrottle); +} #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return IsExperimentEnabled(kExperimentIdMonitoringExperiment); diff --git a/src/core/lib/experiments/experiments.yaml b/src/core/lib/experiments/experiments.yaml index 4d1a27e4d3c..f26fc02f27e 100644 --- a/src/core/lib/experiments/experiments.yaml +++ b/src/core/lib/experiments/experiments.yaml @@ -120,6 +120,14 @@ owner: yashkt@google.com test_tags: [] allow_in_fuzzing_config: false +- name: max_pings_wo_data_throttle + description: + Experiment to throttle pings to a period of 1 min when + GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA limit has reached (instead of + completely blocking). + expiry: 2024/12/31 + owner: yashkt@google.com + test_tags: [] - name: monitoring_experiment description: Placeholder experiment to prove/disprove our monitoring is working expiry: never-ever diff --git a/src/core/lib/experiments/rollouts.yaml b/src/core/lib/experiments/rollouts.yaml index 65d35a1bd4e..fe679a5718e 100644 --- a/src/core/lib/experiments/rollouts.yaml +++ b/src/core/lib/experiments/rollouts.yaml @@ -79,6 +79,8 @@ default: false - name: keepalive_server_fix default: false +- name: max_pings_wo_data_throttle + default: false - name: monitoring_experiment default: true - name: peer_state_based_framing diff --git a/test/core/end2end/tests/bad_ping.cc b/test/core/end2end/tests/bad_ping.cc index 1aa247e46e3..51a799549b2 100644 --- a/test/core/end2end/tests/bad_ping.cc +++ b/test/core/end2end/tests/bad_ping.cc @@ -87,6 +87,9 @@ CORE_END2END_TEST(RetryHttp2Test, BadPing) { // Try sending more pings than server allows, but server should be fine because // max_pings_without_data should limit pings sent out on wire. CORE_END2END_TEST(RetryHttp2Test, PingsWithoutData) { + if (IsMaxPingsWoDataThrottleEnabled()) { + GTEST_SKIP() << "pings are not limited if this experiment is enabled"; + } // Only allow MAX_PING_STRIKES pings without data (DATA/HEADERS/WINDOW_UPDATE) // so that the transport will throttle the excess pings. InitClient(ChannelArgs() diff --git a/test/core/transport/chttp2/ping_rate_policy_test.cc b/test/core/transport/chttp2/ping_rate_policy_test.cc index be443468bc9..46ddd0501fd 100644 --- a/test/core/transport/chttp2/ping_rate_policy_test.cc +++ b/test/core/transport/chttp2/ping_rate_policy_test.cc @@ -19,6 +19,8 @@ #include "gtest/gtest.h" +#include "src/core/lib/experiments/experiments.h" + namespace grpc_core { namespace { @@ -47,6 +49,10 @@ TEST(PingRatePolicy, ServerCanSendAtStart) { } TEST(PingRatePolicy, ClientBlockedUntilDataSent) { + if (IsMaxPingsWoDataThrottleEnabled()) { + GTEST_SKIP() + << "Pings are not blocked if max_pings_wo_data_throttle is enabled."; + } Chttp2PingRatePolicy policy{ChannelArgs(), true}; EXPECT_EQ(policy.RequestSendPing(Duration::Milliseconds(10), 0), TooManyRecentPings()); @@ -59,6 +65,35 @@ TEST(PingRatePolicy, ClientBlockedUntilDataSent) { EXPECT_EQ(policy.RequestSendPing(Duration::Zero(), 0), TooManyRecentPings()); } +TEST(PingRatePolicy, ClientThrottledUntilDataSent) { + if (!IsMaxPingsWoDataThrottleEnabled()) { + GTEST_SKIP() + << "Throttling behavior is enabled with max_pings_wo_data_throttle."; + } + Chttp2PingRatePolicy policy{ChannelArgs(), true}; + // First ping is allowed. + EXPECT_EQ(policy.RequestSendPing(Duration::Milliseconds(10), 0), + SendGranted()); + policy.SentPing(); + // Second ping is throttled since no data has been sent. + auto result = policy.RequestSendPing(Duration::Zero(), 0); + EXPECT_TRUE(absl::holds_alternative(result)); + EXPECT_EQ(absl::get(result).wait, + Duration::Minutes(1)); + policy.ResetPingsBeforeDataRequired(); + // After resetting pings before data required (data sent), we can send pings + // without being throttled. + EXPECT_EQ(policy.RequestSendPing(Duration::Zero(), 0), SendGranted()); + policy.SentPing(); + EXPECT_EQ(policy.RequestSendPing(Duration::Zero(), 0), SendGranted()); + policy.SentPing(); + // After reaching limit, we are throttled again. + result = policy.RequestSendPing(Duration::Zero(), 0); + EXPECT_TRUE(absl::holds_alternative(result)); + EXPECT_EQ(absl::get(result).wait, + Duration::Minutes(1)); +} + TEST(PingRatePolicy, RateThrottlingWorks) { Chttp2PingRatePolicy policy{ChannelArgs(), false}; // Observe that we can fail if we send in a tight loop