From 4c6f8ce4a8a4d6d413252a0868d814d7a1beb729 Mon Sep 17 00:00:00 2001 From: Auni Ahsan Date: Thu, 18 Jun 2020 14:21:01 +0000 Subject: [PATCH 01/12] Add ?graceful query parameter to /drain_close Signed-off-by: Auni Ahsan --- source/server/admin/listeners_handler.cc | 12 +++- .../drain_close_integration_test.cc | 57 +++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/source/server/admin/listeners_handler.cc b/source/server/admin/listeners_handler.cc index 3d813ad4b4c8..9accf6c77fc9 100644 --- a/source/server/admin/listeners_handler.cc +++ b/source/server/admin/listeners_handler.cc @@ -16,10 +16,20 @@ ListenersHandler::ListenersHandler(Server::Instance& server) : HandlerContextBas Http::Code ListenersHandler::handlerDrainListeners(absl::string_view url, Http::ResponseHeaderMap&, Buffer::Instance& response, AdminStream&) { const Http::Utility::QueryParams params = Http::Utility::parseQueryString(url); + ListenerManager::StopListenersType stop_listeners_type = params.find("inboundonly") != params.end() ? ListenerManager::StopListenersType::InboundOnly : ListenerManager::StopListenersType::All; - server_.listenerManager().stopListeners(stop_listeners_type); + + const bool graceful = params.find("graceful") != params.end(); + if (graceful) { + server_.drainManager().startDrainSequence([this, stop_listeners_type]() { + server_.listenerManager().stopListeners(stop_listeners_type); + }); + } else { + server_.listenerManager().stopListeners(stop_listeners_type); + } + response.add("OK\n"); return Http::Code::OK; } diff --git a/test/integration/drain_close_integration_test.cc b/test/integration/drain_close_integration_test.cc index cbe58e973ecd..80ebd963b662 100644 --- a/test/integration/drain_close_integration_test.cc +++ b/test/integration/drain_close_integration_test.cc @@ -75,6 +75,63 @@ TEST_P(DrainCloseIntegrationTest, DrainCloseImmediate) { TEST_P(DrainCloseIntegrationTest, AdminDrain) { testAdminDrain(downstreamProtocol()); } +TEST_P(DrainCloseIntegrationTest, AdminGracefulDrain) { + drain_strategy_ = Server::DrainStrategy::Immediate; + drain_time_ = std::chrono::seconds(999); + initialize(); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + uint32_t http_port = lookupPort("http"); + codec_client_ = makeHttpConnection(http_port); + + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + waitForNextUpstreamRequest(0); + upstream_request_->encodeHeaders(default_response_headers_, true); + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_THAT(response->headers(), Http::HttpStatusIs("200")); + // The request is completed but the connection remains open. + EXPECT_TRUE(codec_client_->connected()); + + // Invoke /drain_listeners with graceful drain + BufferingStreamDecoderPtr admin_response = IntegrationUtil::makeSingleRequest( + lookupPort("admin"), "POST", "/drain_listeners?graceful", "", downstreamProtocol(), version_); + EXPECT_EQ(admin_response->headers().Status()->value().getStringView(), "200"); + + // With a 999s graceful drain period, the listener should still be open. + EXPECT_EQ(test_server_->counter("listener_manager.listener_stopped")->value(), 0); + + response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + waitForNextUpstreamRequest(0); + upstream_request_->encodeHeaders(default_response_headers_, true); + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_THAT(response->headers(), Http::HttpStatusIs("200")); + + // Connections will terminate on request complete + ASSERT_TRUE(codec_client_->waitForDisconnect()); + if (downstream_protocol_ == Http::CodecClient::Type::HTTP2) { + EXPECT_TRUE(codec_client_->sawGoAway()); + } else { + EXPECT_EQ("close", response->headers().getConnectionValue()); + } + + // New connections can still be made. + auto second_codec_client_ = makeRawHttpConnection(makeClientConnection(http_port)); + EXPECT_TRUE(second_codec_client_->connected()); + + // Invoke /drain_listeners and shut down listeners. + second_codec_client_->rawConnection().close(Network::ConnectionCloseType::NoFlush); + admin_response = IntegrationUtil::makeSingleRequest( + lookupPort("admin"), "POST", "/drain_listeners", "", downstreamProtocol(), version_); + EXPECT_EQ(admin_response->headers().Status()->value().getStringView(), "200"); + + test_server_->waitForCounterEq("listener_manager.listener_stopped", 1); + EXPECT_NO_THROW(Network::TcpListenSocket( + Network::Utility::getAddressWithPort(*Network::Test::getCanonicalLoopbackAddress(version_), + http_port), + nullptr, true)); +} + INSTANTIATE_TEST_SUITE_P(Protocols, DrainCloseIntegrationTest, testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams( {Http::CodecClient::Type::HTTP1, Http::CodecClient::Type::HTTP2}, From e569e57540172f786f686818f914d569f98300e1 Mon Sep 17 00:00:00 2001 From: Auni Ahsan Date: Mon, 22 Jun 2020 16:29:47 +0000 Subject: [PATCH 02/12] add docs Signed-off-by: Auni Ahsan --- docs/root/intro/arch_overview/operations/draining.rst | 2 ++ docs/root/operations/admin.rst | 3 +++ 2 files changed, 5 insertions(+) diff --git a/docs/root/intro/arch_overview/operations/draining.rst b/docs/root/intro/arch_overview/operations/draining.rst index 0a0932e57a92..a8d4b2afe188 100644 --- a/docs/root/intro/arch_overview/operations/draining.rst +++ b/docs/root/intro/arch_overview/operations/draining.rst @@ -12,6 +12,8 @@ various events. Draining occurs at the following times: * The server is being :ref:`hot restarted `. * Individual listeners are being modified or removed via :ref:`LDS `. +* The server begins shutdown sequence via the :ref:`drain_listeners?graceful + ` admin endpoint. Each :ref:`configured listener ` has a :ref:`drain_type ` setting which controls when draining takes place. The currently diff --git a/docs/root/operations/admin.rst b/docs/root/operations/admin.rst index c4a3cd81ee13..89e382aaaf9c 100644 --- a/docs/root/operations/admin.rst +++ b/docs/root/operations/admin.rst @@ -258,6 +258,9 @@ modify different aspects of the server: :ref:`Listener ` is used to determine whether a listener is inbound or outbound. + .. http:post:: /drain_listeners?graceful + :ref:When draining listeners, enter a graceful drain period prior to closing listeners. This behaviour and duration is determined by server options for the drain manager, and includes sending H1 connection-close, H2 GOAWAYs, and terminating connections on request complete. + .. attention:: This operation directly stops the matched listeners on workers. Once listeners in a given From 4b98fd26923099fd38af8a01f98421fe537247df Mon Sep 17 00:00:00 2001 From: Auni Ahsan Date: Mon, 22 Jun 2020 19:11:20 +0000 Subject: [PATCH 03/12] fix spacing? Signed-off-by: Auni Ahsan --- docs/root/operations/admin.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/root/operations/admin.rst b/docs/root/operations/admin.rst index 89e382aaaf9c..f1b029533ecd 100644 --- a/docs/root/operations/admin.rst +++ b/docs/root/operations/admin.rst @@ -259,7 +259,9 @@ modify different aspects of the server: is inbound or outbound. .. http:post:: /drain_listeners?graceful - :ref:When draining listeners, enter a graceful drain period prior to closing listeners. This behaviour and duration is determined by server options for the drain manager, and includes sending H1 connection-close, H2 GOAWAYs, and terminating connections on request complete. + :ref:When draining listeners, enter a graceful drain period prior to closing listeners. + This behaviour and duration is determined by server options for the drain manager, and + includes sending H1 connection-close, H2 GOAWAYs, and terminating connections on request complete. .. attention:: From b9c32ed11585b082c9da7ab85fe990d9350fcb5c Mon Sep 17 00:00:00 2001 From: Auni Ahsan Date: Mon, 22 Jun 2020 20:07:42 +0000 Subject: [PATCH 04/12] fix reference Signed-off-by: Auni Ahsan --- docs/root/operations/admin.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/operations/admin.rst b/docs/root/operations/admin.rst index f1b029533ecd..b1c25b63d7de 100644 --- a/docs/root/operations/admin.rst +++ b/docs/root/operations/admin.rst @@ -259,7 +259,7 @@ modify different aspects of the server: is inbound or outbound. .. http:post:: /drain_listeners?graceful - :ref:When draining listeners, enter a graceful drain period prior to closing listeners. + When draining listeners, enter a graceful drain period prior to closing listeners. This behaviour and duration is determined by server options for the drain manager, and includes sending H1 connection-close, H2 GOAWAYs, and terminating connections on request complete. From d830849e6cd650e234468d4b26fb25433b935fe4 Mon Sep 17 00:00:00 2001 From: Auni Ahsan Date: Mon, 22 Jun 2020 20:22:03 +0000 Subject: [PATCH 05/12] add newline Signed-off-by: Auni Ahsan --- docs/root/operations/admin.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/operations/admin.rst b/docs/root/operations/admin.rst index b1c25b63d7de..c704f4ee2f36 100644 --- a/docs/root/operations/admin.rst +++ b/docs/root/operations/admin.rst @@ -259,6 +259,7 @@ modify different aspects of the server: is inbound or outbound. .. http:post:: /drain_listeners?graceful + When draining listeners, enter a graceful drain period prior to closing listeners. This behaviour and duration is determined by server options for the drain manager, and includes sending H1 connection-close, H2 GOAWAYs, and terminating connections on request complete. From 648b12163d08590a4c3b680532a1841f147f00d1 Mon Sep 17 00:00:00 2001 From: Auni Ahsan Date: Tue, 23 Jun 2020 13:27:27 +0000 Subject: [PATCH 06/12] Add to docs Signed-off-by: Auni Ahsan --- .../arch_overview/operations/draining.rst | 44 ++++++++++++------- docs/root/operations/admin.rst | 4 +- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/docs/root/intro/arch_overview/operations/draining.rst b/docs/root/intro/arch_overview/operations/draining.rst index a8d4b2afe188..5df5c5589c53 100644 --- a/docs/root/intro/arch_overview/operations/draining.rst +++ b/docs/root/intro/arch_overview/operations/draining.rst @@ -3,17 +3,41 @@ Draining ======== -Draining is the process by which Envoy attempts to gracefully shed connections in response to -various events. Draining occurs at the following times: +In a few different scenarios, Envoy will attempt to gracefully shed connections. For instance, +during server shutdown, existing requests can be discouraged and listeners set to stop accepting, +to reduce the number of open connections when the server shuts down. Draining behaviour is defined +by the server options in addition to indvidiaul listener configs. +Draining occurs at the following times: + +* The server is being :ref:`hot restarted `. +* The server begins the graceful drain sequence via the :ref:`drain_listeners?graceful + ` admin endpoint. * The server has been manually health check failed via the :ref:`healthcheck/fail ` admin endpoint. See the :ref:`health check filter ` architecture overview for more information. -* The server is being :ref:`hot restarted `. * Individual listeners are being modified or removed via :ref:`LDS `. -* The server begins shutdown sequence via the :ref:`drain_listeners?graceful - ` admin endpoint. + +By default, the Envoy server will close listeners immediately on server shutdown. To close listeners +for some duration of time prior to server shutdown, use :ref:`drain_listeners ` +before shutting down the server. The listeners will be directly stopped without any graceful draining behaviour, +and cease accepting new connections immediately. + +To add a graceful drain period prior to listeners being closed, use the query parameter +:ref:`drain_listeners?graceful `. By default, Envoy +will discourage requests for some period of time (as determined by :option:`--drain-time-s`). +The behaviour of request discouraging is determined by the drain manager. + +Note that although draining is a per-listener concept, it must be supported at the network filter +level. Currently the only filters that support graceful draining are +:ref:`Redis `, and +:ref:`Mongo `. +:ref:`HTTP connection manager `, + +By default, the :ref:`HTTP connection manager ` filter will +add "Connection: close" to HTTP1 requests, send HTTP2 GOAWAY, and terminate connections +on request completion (after the delayed close period). Each :ref:`configured listener ` has a :ref:`drain_type ` setting which controls when draining takes place. The currently @@ -29,13 +53,3 @@ modify_only It may be desirable to set *modify_only* on egress listeners so they only drain during modifications while relying on ingress listener draining to perform full server draining when attempting to do a controlled shutdown. - -Note that although draining is a per-listener concept, it must be supported at the network filter -level. Currently the only filters that support graceful draining are -:ref:`HTTP connection manager `, -:ref:`Redis `, and -:ref:`Mongo `. - -Listeners can also be stopped via :ref:`drain_listeners `. In this case, -they are directly stopped (without going through the actual draining process) on worker threads, -so that they will not accept any new requests. diff --git a/docs/root/operations/admin.rst b/docs/root/operations/admin.rst index c704f4ee2f36..b90a1461f415 100644 --- a/docs/root/operations/admin.rst +++ b/docs/root/operations/admin.rst @@ -261,8 +261,8 @@ modify different aspects of the server: .. http:post:: /drain_listeners?graceful When draining listeners, enter a graceful drain period prior to closing listeners. - This behaviour and duration is determined by server options for the drain manager, and - includes sending H1 connection-close, H2 GOAWAYs, and terminating connections on request complete. + This behaviour and duration is configurable via server options or CLI + (:option:`--drain-time-s` and :option:`--drain-strategy`). .. attention:: From 7b4db20d0127b30caf90ffeba406b59849475081 Mon Sep 17 00:00:00 2001 From: Auni Ahsan Date: Tue, 23 Jun 2020 17:29:36 +0000 Subject: [PATCH 07/12] Doc fixes Signed-off-by: Auni Ahsan --- docs/root/intro/arch_overview/operations/draining.rst | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/root/intro/arch_overview/operations/draining.rst b/docs/root/intro/arch_overview/operations/draining.rst index 5df5c5589c53..93c2155f6423 100644 --- a/docs/root/intro/arch_overview/operations/draining.rst +++ b/docs/root/intro/arch_overview/operations/draining.rst @@ -6,7 +6,7 @@ Draining In a few different scenarios, Envoy will attempt to gracefully shed connections. For instance, during server shutdown, existing requests can be discouraged and listeners set to stop accepting, to reduce the number of open connections when the server shuts down. Draining behaviour is defined -by the server options in addition to indvidiaul listener configs. +by the server options in addition to individul listener configs. Draining occurs at the following times: @@ -19,7 +19,7 @@ Draining occurs at the following times: * Individual listeners are being modified or removed via :ref:`LDS `. -By default, the Envoy server will close listeners immediately on server shutdown. To close listeners +By default, the Envoy server will close listeners immediately on server shutdown. To drain listeners for some duration of time prior to server shutdown, use :ref:`drain_listeners ` before shutting down the server. The listeners will be directly stopped without any graceful draining behaviour, and cease accepting new connections immediately. @@ -31,9 +31,9 @@ The behaviour of request discouraging is determined by the drain manager. Note that although draining is a per-listener concept, it must be supported at the network filter level. Currently the only filters that support graceful draining are -:ref:`Redis `, and -:ref:`Mongo `. -:ref:`HTTP connection manager `, +:ref:`Redis `, +:ref:`Mongo `, +and :ref:`HTTP connection manager `. By default, the :ref:`HTTP connection manager ` filter will add "Connection: close" to HTTP1 requests, send HTTP2 GOAWAY, and terminate connections From 403a78d9d6aebae9bec03c5cfee710d885e41107 Mon Sep 17 00:00:00 2001 From: Auni Ahsan Date: Tue, 23 Jun 2020 17:29:41 +0000 Subject: [PATCH 08/12] Support double drain Signed-off-by: Auni Ahsan --- source/server/admin/listeners_handler.cc | 10 +++-- .../drain_close_integration_test.cc | 43 +++++++++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/source/server/admin/listeners_handler.cc b/source/server/admin/listeners_handler.cc index 9accf6c77fc9..c62a6df3fb77 100644 --- a/source/server/admin/listeners_handler.cc +++ b/source/server/admin/listeners_handler.cc @@ -23,9 +23,13 @@ Http::Code ListenersHandler::handlerDrainListeners(absl::string_view url, Http:: const bool graceful = params.find("graceful") != params.end(); if (graceful) { - server_.drainManager().startDrainSequence([this, stop_listeners_type]() { - server_.listenerManager().stopListeners(stop_listeners_type); - }); + if (server_.drainManager().drainClose()) { + // What to do here? + } else { + server_.drainManager().startDrainSequence([this, stop_listeners_type]() { + server_.listenerManager().stopListeners(stop_listeners_type); + }); + } } else { server_.listenerManager().stopListeners(stop_listeners_type); } diff --git a/test/integration/drain_close_integration_test.cc b/test/integration/drain_close_integration_test.cc index 80ebd963b662..0d11519a2d65 100644 --- a/test/integration/drain_close_integration_test.cc +++ b/test/integration/drain_close_integration_test.cc @@ -132,6 +132,49 @@ TEST_P(DrainCloseIntegrationTest, AdminGracefulDrain) { nullptr, true)); } +TEST_P(DrainCloseIntegrationTest, RepeatedAdminGracefulDrain) { + drain_strategy_ = Server::DrainStrategy::Immediate; + drain_time_ = std::chrono::seconds(999); + initialize(); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + uint32_t http_port = lookupPort("http"); + codec_client_ = makeHttpConnection(http_port); + + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + waitForNextUpstreamRequest(0); + upstream_request_->encodeHeaders(default_response_headers_, true); + response->waitForEndStream(); + + // Invoke /drain_listeners with graceful drain + std::cerr << "[AUNI] " << "first drain" << "\n"; + BufferingStreamDecoderPtr admin_response = IntegrationUtil::makeSingleRequest( + lookupPort("admin"), "POST", "/drain_listeners?graceful", "", downstreamProtocol(), version_); + EXPECT_EQ(admin_response->headers().Status()->value().getStringView(), "200"); + EXPECT_EQ(test_server_->counter("listener_manager.listener_stopped")->value(), 0); + + std::cerr << "[AUNI] " << "second drain" << "\n"; + admin_response = IntegrationUtil::makeSingleRequest( + lookupPort("admin"), "POST", "/drain_listeners?graceful", "", downstreamProtocol(), version_); + EXPECT_EQ(admin_response->headers().Status()->value().getStringView(), "200"); + + response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + waitForNextUpstreamRequest(0); + upstream_request_->encodeHeaders(default_response_headers_, true); + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_THAT(response->headers(), Http::HttpStatusIs("200")); + + admin_response = IntegrationUtil::makeSingleRequest( + lookupPort("admin"), "POST", "/drain_listeners", "", downstreamProtocol(), version_); + EXPECT_EQ(admin_response->headers().Status()->value().getStringView(), "200"); + + test_server_->waitForCounterEq("listener_manager.listener_stopped", 1); + EXPECT_NO_THROW(Network::TcpListenSocket( + Network::Utility::getAddressWithPort(*Network::Test::getCanonicalLoopbackAddress(version_), + http_port), + nullptr, true)); +} + INSTANTIATE_TEST_SUITE_P(Protocols, DrainCloseIntegrationTest, testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams( {Http::CodecClient::Type::HTTP1, Http::CodecClient::Type::HTTP2}, From 55c013d8d08a46b0e22ba8db3f81b9811c80bdd5 Mon Sep 17 00:00:00 2001 From: Auni Ahsan Date: Tue, 23 Jun 2020 17:37:04 +0000 Subject: [PATCH 09/12] Fix formatting Signed-off-by: Auni Ahsan --- docs/root/intro/arch_overview/operations/draining.rst | 2 +- test/integration/drain_close_integration_test.cc | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/root/intro/arch_overview/operations/draining.rst b/docs/root/intro/arch_overview/operations/draining.rst index 93c2155f6423..18003197c844 100644 --- a/docs/root/intro/arch_overview/operations/draining.rst +++ b/docs/root/intro/arch_overview/operations/draining.rst @@ -6,7 +6,7 @@ Draining In a few different scenarios, Envoy will attempt to gracefully shed connections. For instance, during server shutdown, existing requests can be discouraged and listeners set to stop accepting, to reduce the number of open connections when the server shuts down. Draining behaviour is defined -by the server options in addition to individul listener configs. +by the server options in addition to individual listener configs. Draining occurs at the following times: diff --git a/test/integration/drain_close_integration_test.cc b/test/integration/drain_close_integration_test.cc index 0d11519a2d65..6b976686871d 100644 --- a/test/integration/drain_close_integration_test.cc +++ b/test/integration/drain_close_integration_test.cc @@ -146,13 +146,11 @@ TEST_P(DrainCloseIntegrationTest, RepeatedAdminGracefulDrain) { response->waitForEndStream(); // Invoke /drain_listeners with graceful drain - std::cerr << "[AUNI] " << "first drain" << "\n"; BufferingStreamDecoderPtr admin_response = IntegrationUtil::makeSingleRequest( lookupPort("admin"), "POST", "/drain_listeners?graceful", "", downstreamProtocol(), version_); EXPECT_EQ(admin_response->headers().Status()->value().getStringView(), "200"); EXPECT_EQ(test_server_->counter("listener_manager.listener_stopped")->value(), 0); - std::cerr << "[AUNI] " << "second drain" << "\n"; admin_response = IntegrationUtil::makeSingleRequest( lookupPort("admin"), "POST", "/drain_listeners?graceful", "", downstreamProtocol(), version_); EXPECT_EQ(admin_response->headers().Status()->value().getStringView(), "200"); @@ -163,6 +161,7 @@ TEST_P(DrainCloseIntegrationTest, RepeatedAdminGracefulDrain) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_THAT(response->headers(), Http::HttpStatusIs("200")); + ASSERT_TRUE(codec_client_->waitForDisconnect()); admin_response = IntegrationUtil::makeSingleRequest( lookupPort("admin"), "POST", "/drain_listeners", "", downstreamProtocol(), version_); From b7405dfe332c2ebbc1cd847c472eedd58e73d27b Mon Sep 17 00:00:00 2001 From: Auni Ahsan Date: Tue, 23 Jun 2020 18:13:04 +0000 Subject: [PATCH 10/12] add draining() Signed-off-by: Auni Ahsan --- include/envoy/server/drain_manager.h | 2 ++ source/server/drain_manager_impl.h | 1 + test/mocks/server/mocks.h | 1 + test/server/drain_manager_impl_test.cc | 3 +++ 4 files changed, 7 insertions(+) diff --git a/include/envoy/server/drain_manager.h b/include/envoy/server/drain_manager.h index 0f29b0cd3eed..040dc1446fbd 100644 --- a/include/envoy/server/drain_manager.h +++ b/include/envoy/server/drain_manager.h @@ -21,6 +21,8 @@ class DrainManager : public Network::DrainDecision { */ virtual void startDrainSequence(std::function drain_complete_cb) PURE; + virtual bool draining() const PURE; + /** * Invoked in the newly launched primary process to begin the parent shutdown sequence. At the end * of the sequence the previous primary process will be terminated. diff --git a/source/server/drain_manager_impl.h b/source/server/drain_manager_impl.h index 38a02465b761..c8056f22396c 100644 --- a/source/server/drain_manager_impl.h +++ b/source/server/drain_manager_impl.h @@ -28,6 +28,7 @@ class DrainManagerImpl : Logger::Loggable, public DrainManager // Server::DrainManager void startDrainSequence(std::function drain_complete_cb) override; + bool draining() const override { return draining_; } void startParentShutdownSequence() override; private: diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 34ffef72e615..6e5060014f72 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -192,6 +192,7 @@ class MockDrainManager : public DrainManager { // Server::DrainManager MOCK_METHOD(bool, drainClose, (), (const)); + MOCK_METHOD(bool, draining, (), (const)); MOCK_METHOD(void, startDrainSequence, (std::function completion)); MOCK_METHOD(void, startParentShutdownSequence, ()); diff --git a/test/server/drain_manager_impl_test.cc b/test/server/drain_manager_impl_test.cc index be09ee0ec7fb..9afeba1b7955 100644 --- a/test/server/drain_manager_impl_test.cc +++ b/test/server/drain_manager_impl_test.cc @@ -126,7 +126,10 @@ TEST_P(DrainManagerImplTest, DrainDeadlineProbability) { EXPECT_TRUE(drain_manager.drainClose()); EXPECT_CALL(server_, healthCheckFailed()).WillRepeatedly(Return(false)); EXPECT_FALSE(drain_manager.drainClose()); + EXPECT_FALSE(drain_manager.draining()); + drain_manager.startDrainSequence([] {}); + EXPECT_TRUE(drain_manager.draining()); if (drain_gradually) { // random() should be called when elapsed time < drain timeout From 0164050bc11f2e846f8bf3a3d30dc972842ffa80 Mon Sep 17 00:00:00 2001 From: Auni Ahsan Date: Tue, 23 Jun 2020 18:17:34 +0000 Subject: [PATCH 11/12] expect 200, change strategy Signed-off-by: Auni Ahsan --- source/server/admin/listeners_handler.cc | 6 +++--- test/integration/drain_close_integration_test.cc | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/source/server/admin/listeners_handler.cc b/source/server/admin/listeners_handler.cc index c62a6df3fb77..93407d9eb6cc 100644 --- a/source/server/admin/listeners_handler.cc +++ b/source/server/admin/listeners_handler.cc @@ -23,9 +23,9 @@ Http::Code ListenersHandler::handlerDrainListeners(absl::string_view url, Http:: const bool graceful = params.find("graceful") != params.end(); if (graceful) { - if (server_.drainManager().drainClose()) { - // What to do here? - } else { + // Ignore calls to /drain_listeners?graceful if the drain sequence has + // already started. + if (!server_.drainManager().draining()) { server_.drainManager().startDrainSequence([this, stop_listeners_type]() { server_.listenerManager().stopListeners(stop_listeners_type); }); diff --git a/test/integration/drain_close_integration_test.cc b/test/integration/drain_close_integration_test.cc index 6b976686871d..aa0afd8d141b 100644 --- a/test/integration/drain_close_integration_test.cc +++ b/test/integration/drain_close_integration_test.cc @@ -133,7 +133,8 @@ TEST_P(DrainCloseIntegrationTest, AdminGracefulDrain) { } TEST_P(DrainCloseIntegrationTest, RepeatedAdminGracefulDrain) { - drain_strategy_ = Server::DrainStrategy::Immediate; + // Use the default gradual probabilistic DrainStrategy so drainClose() + // behaviour isn't conflated with whether the drain sequence has started. drain_time_ = std::chrono::seconds(999); initialize(); fake_upstreams_[0]->set_allow_unexpected_disconnects(true); @@ -154,6 +155,7 @@ TEST_P(DrainCloseIntegrationTest, RepeatedAdminGracefulDrain) { admin_response = IntegrationUtil::makeSingleRequest( lookupPort("admin"), "POST", "/drain_listeners?graceful", "", downstreamProtocol(), version_); EXPECT_EQ(admin_response->headers().Status()->value().getStringView(), "200"); + EXPECT_EQ(admin_response->headers().Status()->value().getStringView(), "200"); response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); waitForNextUpstreamRequest(0); @@ -161,7 +163,6 @@ TEST_P(DrainCloseIntegrationTest, RepeatedAdminGracefulDrain) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_THAT(response->headers(), Http::HttpStatusIs("200")); - ASSERT_TRUE(codec_client_->waitForDisconnect()); admin_response = IntegrationUtil::makeSingleRequest( lookupPort("admin"), "POST", "/drain_listeners", "", downstreamProtocol(), version_); From 5b6d83b63639b1ae5d9b248239289b78c6aad925 Mon Sep 17 00:00:00 2001 From: Auni Ahsan Date: Tue, 23 Jun 2020 19:43:48 +0000 Subject: [PATCH 12/12] add class interface note Signed-off-by: Auni Ahsan --- include/envoy/server/drain_manager.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/envoy/server/drain_manager.h b/include/envoy/server/drain_manager.h index 040dc1446fbd..49ecc194166a 100644 --- a/include/envoy/server/drain_manager.h +++ b/include/envoy/server/drain_manager.h @@ -21,6 +21,9 @@ class DrainManager : public Network::DrainDecision { */ virtual void startDrainSequence(std::function drain_complete_cb) PURE; + /** + * @return whether the drain sequence has started. + */ virtual bool draining() const PURE; /**