config option for upstream-rq-per-try-timeout (#756)

envoyproxy · Apr 13, 2017 · 5b523d2 · 5b523d2
1 parent 842bca3
commit 5b523d2
Show file tree

Hide file tree

Showing 11 changed files with 72 additions and 2 deletions.
diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst
@@ -210,7 +210,8 @@ HTTP retry :ref:`architecture overview <arch_overview_http_routing_retry>`.
 
   {
     "retry_on": "...",
-    "num_retries": "..."
+    "num_retries": "...",
+    "per_try_timeout_ms" : "..."
   }
 
 retry_on
@@ -222,6 +223,17 @@ num_retries
   defaults to 1. These are the same conditions documented for
   :ref:`config_http_filters_router_x-envoy-max-retries`.
 
+per_try_timeout_ms
+  *(optional, integer)* specifies a non-zero timeout per retry attempt. This parameter is optional.
+  The same conditions documented for 
+  :ref:`config_http_filters_router_x-envoy-upstream-rq-per-try-timeout-ms` apply.
+
+  **Note:** If left unspecified, Envoy will use the global
+  `:ref: route timeout <config_http_conn_man_route_table_route_timeout>` for the request. 
+  Consequently, when using a `:ref: 5xx <config_http_filters_router_x-envoy-retry-on>` based 
+  retry policy, a request that times out will not be retried as the total timeout budget 
+  would have been exhausted.
+
 .. _config_http_conn_man_route_table_route_shadow:
 
 Shadow

diff --git a/docs/configuration/http_filters/router_filter.rst b/docs/configuration/http_filters/router_filter.rst
@@ -75,7 +75,7 @@ using a ',' delimited list. The supported policies are:
 
 5xx
   Envoy will attempt a retry if the upstream server responds with any 5xx response code, or does not
-  respond at all (disconnect/reset/etc.). (Includes *connect-failure* and *refused-stream*)
+  respond at all (disconnect/reset/read timeout). (Includes *connect-failure* and *refused-stream*)
 
   * **NOTE:** Envoy will not retry when a request exceeds
     :ref:`config_http_filters_router_x-envoy-upstream-rq-timeout-ms` (resulting in a 504 error

diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h
@@ -36,6 +36,11 @@ class RetryPolicy {
 
   virtual ~RetryPolicy() {}
 
+  /**
+   * @return std::chrono::milliseconds timeout per retry attempt.
+   */
+  virtual std::chrono::milliseconds perTryTimeout() const PURE;
+
   /**
    * @return uint32_t the number of retries to allow against the route.
    */

diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h
@@ -85,6 +85,9 @@ class AsyncStreamImpl : public AsyncClient::Stream,
 
   struct NullRetryPolicy : public Router::RetryPolicy {
     // Router::RetryPolicy
+    std::chrono::milliseconds perTryTimeout() const override {
+      return std::chrono::milliseconds(0);
+    }
     uint32_t numRetries() const override { return 0; }
     uint32_t retryOn() const override { return 0; }
   };

diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc
@@ -567,6 +567,11 @@ const std::string Json::Schema::ROUTE_ENTRY_CONFIGURATION_SCHEMA(R"EOF(
       "retry_policy" : {
         "type" : "object",
         "properties" : {
+          "per_try_timeout_ms" : {
+            "type" : "integer",
+            "minimum" : 0,
+            "exclusiveMinimum" : true
+          },
           "num_retries" : {"type" : "integer"},
           "retry_on" : {"type" : "string"}
         },

diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc
@@ -25,6 +25,8 @@ RetryPolicyImpl::RetryPolicyImpl(const Json::Object& config) {
     return;
   }
 
+  per_try_timeout_ = std::chrono::milliseconds(
+      config.getObject("retry_policy")->getInteger("per_try_timeout_ms", 0));
   num_retries_ = config.getObject("retry_policy")->getInteger("num_retries", 1);
   retry_on_ = RetryStateImpl::parseRetryOn(config.getObject("retry_policy")->getString("retry_on"));
 }

diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h
@@ -120,10 +120,12 @@ class RetryPolicyImpl : public RetryPolicy {
   RetryPolicyImpl(const Json::Object& config);
 
   // Router::RetryPolicy
+  std::chrono::milliseconds perTryTimeout() const override { return per_try_timeout_; }
   uint32_t numRetries() const override { return num_retries_; }
   uint32_t retryOn() const override { return retry_on_; }
 
 private:
+  std::chrono::milliseconds per_try_timeout_{0};
   uint32_t num_retries_{};
   uint32_t retry_on_{};
 };

diff --git a/source/common/router/router.cc b/source/common/router/router.cc
@@ -51,6 +51,7 @@ FilterUtility::TimeoutData FilterUtility::finalTimeout(const RouteEntry& route,
   // otherwise we use the default.
   TimeoutData timeout;
   timeout.global_timeout_ = route.timeout();
+  timeout.per_try_timeout_ = route.retryPolicy().perTryTimeout();
   Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs();
   uint64_t header_timeout;
   if (header_timeout_entry) {

diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc
@@ -1073,6 +1073,7 @@ TEST(RouteMatcherTest, Retry) {
           "prefix": "/",
           "cluster": "www2",
           "retry_policy": {
+            "per_try_timeout_ms" : 1000,
             "num_retries": 3,
             "retry_on": "5xx,connect-failure"
           }
@@ -1090,6 +1091,10 @@ TEST(RouteMatcherTest, Retry) {
 
   EXPECT_FALSE(config.usesRuntime());
 
+  EXPECT_EQ(std::chrono::milliseconds(0), config.route(genHeaders("www.lyft.com", "/foo", "GET"), 0)
+                                              ->routeEntry()
+                                              ->retryPolicy()
+                                              .perTryTimeout());
   EXPECT_EQ(1U, config.route(genHeaders("www.lyft.com", "/foo", "GET"), 0)
                     ->routeEntry()
                     ->retryPolicy()
@@ -1100,6 +1105,10 @@ TEST(RouteMatcherTest, Retry) {
                 ->retryPolicy()
                 .retryOn());
 
+  EXPECT_EQ(std::chrono::milliseconds(0), config.route(genHeaders("www.lyft.com", "/foo", "GET"), 0)
+                                              ->routeEntry()
+                                              ->retryPolicy()
+                                              .perTryTimeout());
   EXPECT_EQ(0U, config.route(genHeaders("www.lyft.com", "/bar", "GET"), 0)
                     ->routeEntry()
                     ->retryPolicy()
@@ -1109,6 +1118,10 @@ TEST(RouteMatcherTest, Retry) {
                     ->retryPolicy()
                     .retryOn());
 
+  EXPECT_EQ(std::chrono::milliseconds(1000), config.route(genHeaders("www.lyft.com", "/", "GET"), 0)
+                                                 ->routeEntry()
+                                                 ->retryPolicy()
+                                                 .perTryTimeout());
   EXPECT_EQ(3U, config.route(genHeaders("www.lyft.com", "/", "GET"), 0)
                     ->routeEntry()
                     ->retryPolicy()

diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc
@@ -864,6 +864,31 @@ TEST(RouterFilterUtilityTest, finalTimeout) {
     EXPECT_FALSE(headers.has("x-envoy-upstream-rq-per-try-timeout-ms"));
     EXPECT_EQ("5", headers.get_("x-envoy-expected-rq-timeout-ms"));
   }
+  {
+    MockRouteEntry route;
+    route.retry_policy_.per_try_timeout_ = std::chrono::milliseconds(7);
+    EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10)));
+    Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "15"}};
+    FilterUtility::TimeoutData timeout = FilterUtility::finalTimeout(route, headers);
+    EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_);
+    EXPECT_EQ(std::chrono::milliseconds(7), timeout.per_try_timeout_);
+    EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms"));
+    EXPECT_FALSE(headers.has("x-envoy-upstream-rq-per-try-timeout-ms"));
+    EXPECT_EQ("7", headers.get_("x-envoy-expected-rq-timeout-ms"));
+  }
+  {
+    MockRouteEntry route;
+    route.retry_policy_.per_try_timeout_ = std::chrono::milliseconds(7);
+    EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10)));
+    Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "15"},
+                                    {"x-envoy-upstream-rq-per-try-timeout-ms", "5"}};
+    FilterUtility::TimeoutData timeout = FilterUtility::finalTimeout(route, headers);
+    EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_);
+    EXPECT_EQ(std::chrono::milliseconds(5), timeout.per_try_timeout_);
+    EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms"));
+    EXPECT_FALSE(headers.has("x-envoy-upstream-rq-per-try-timeout-ms"));
+    EXPECT_EQ("5", headers.get_("x-envoy-expected-rq-timeout-ms"));
+  }
 }
 
 TEST(RouterFilterUtilityTest, setUpstreamScheme) {

diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h
@@ -18,9 +18,11 @@ class MockRedirectEntry : public RedirectEntry {
 class TestRetryPolicy : public RetryPolicy {
 public:
   // Router::RetryPolicy
+  std::chrono::milliseconds perTryTimeout() const override { return per_try_timeout_; }
   uint32_t numRetries() const override { return num_retries_; }
   uint32_t retryOn() const override { return retry_on_; }
 
+  std::chrono::milliseconds per_try_timeout_{0};
   uint32_t num_retries_{};
   uint32_t retry_on_{};
 };