conn_pool: Remove startDrain() and replace it with an argument to drainConnections by RyanTheOptimist · Pull Request #17960 · envoyproxy/envoy

RyanTheOptimist · 2021-09-02T03:56:16Z

conn_pool: Remove startDrain() and replace it with an argument to drainConnections

Add a new DrainBehavior enum argument to drainConnections() and use that to replace startDrain().

Risk Level: Low
Testing: Unit tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist · 2021-09-02T03:56:31Z

/assign @mattklein123

RyanTheOptimist · 2021-09-02T03:56:46Z

/assign @ggreenway

RyanTheOptimist · 2021-09-02T03:57:17Z

Hopefully this is what we discussed! :)

Signed-off-by: Ryan Hamilton <rch@google.com>

ggreenway

Interface changes look good.

/wait

ggreenway · 2021-09-02T15:19:42Z

source/common/conn_pool/conn_pool_base.cc

-void ConnPoolImplBase::drainConnectionsImpl() {
+void ConnPoolImplBase::drainConnectionsImpl(DrainBehavior drain_behavior) {
+  if (drain_behavior == Envoy::ConnectionPool::DrainBehavior::DrainAndDelete) {
+    checkForIdleAndCloseIdleConnsIfDraining();


This function behaves differently depending on the value of is_draining_. You've re-ordered these operations, which changes the behavior. I think the behavior was correct with is_draining_ = true before the function call.

Oh shoot! Thanks. I think I screwed that up during a cleanup. Fixed.

ggreenway · 2021-09-02T15:24:14Z

envoy/common/conn_pool.h

+ * Controls the behavior when draining a connection pool.
+ */
+enum class DrainBehavior {
+  // Starts draining a pool, by gracefully completing all requests and gracefully closing all


Please add that it is invalid to create new requests/streams/connections on this pool after this call.

Good idea. Done.

ggreenway · 2021-09-02T15:24:57Z

envoy/common/conn_pool.h

+enum class DrainBehavior {
+  // Starts draining a pool, by gracefully completing all requests and gracefully closing all
+  // connections, in preparation for deletion.
+  // can be immediately drained.


Make this a proper sentence

Oh whoop. That should just be removed. Done.

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist

Thanks!

RyanTheOptimist · 2021-09-02T15:31:09Z

source/common/conn_pool/conn_pool_base.cc

-void ConnPoolImplBase::drainConnectionsImpl() {
+void ConnPoolImplBase::drainConnectionsImpl(DrainBehavior drain_behavior) {
+  if (drain_behavior == Envoy::ConnectionPool::DrainBehavior::DrainAndDelete) {
+    checkForIdleAndCloseIdleConnsIfDraining();


Oh shoot! Thanks. I think I screwed that up during a cleanup. Fixed.

RyanTheOptimist · 2021-09-02T15:33:22Z

envoy/common/conn_pool.h

+ * Controls the behavior when draining a connection pool.
+ */
+enum class DrainBehavior {
+  // Starts draining a pool, by gracefully completing all requests and gracefully closing all


Good idea. Done.

RyanTheOptimist · 2021-09-02T15:33:52Z

envoy/common/conn_pool.h

+enum class DrainBehavior {
+  // Starts draining a pool, by gracefully completing all requests and gracefully closing all
+  // connections, in preparation for deletion.
+  // can be immediately drained.


Oh whoop. That should just be removed. Done.

ggreenway

LGTM. Thanks!

mattklein123

Awesome, thanks. One question.

/wait-any

mattklein123 · 2021-09-02T16:31:09Z

source/common/conn_pool/conn_pool_base.cc

-void ConnPoolImplBase::drainConnectionsImpl() {
+void ConnPoolImplBase::drainConnectionsImpl(DrainBehavior drain_behavior) {
+  if (drain_behavior == Envoy::ConnectionPool::DrainBehavior::DrainAndDelete) {
+    is_draining_ = true;


I thought we wanted to add more asserts that no new streams are created if is_draining_ is true? Am I missing that?

Ah yes, good point! I've added an ASSERT in ConnPoolImplBase::newStreamImpl. It did make me wonder if we should rename is_draining_ to is_draining_for_deletion_ to match the name of the enum (and sine it's not set on all drain behaviors, just one of them).

Sure renaming SGTM (or just store the enum itself somehow)

Renamed. I notice this matches the description in the .h file nicely now:

// Whether the connection pool is currently in the process of closing // all connections so that it can be gracefully deleted.

It's like we planned it or something :)

Signed-off-by: Ryan Hamilton <rch@google.com>

mattklein123 · 2021-09-03T15:15:25Z

Thanks. Check format?

/wait

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist · 2021-09-03T16:33:42Z

Thanks. Check format?

facepalm Done.

mattklein123

Thanks!

mattklein123 · 2021-09-03T23:32:54Z

Sorry looks like tidy is failing also. Might be pre-existing. Do you mind trying to fix?

/wait

Signed-off-by: Ryan Hamilton <rch@google.com>

…inConnections (envoyproxy#17960) Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist added 4 commits September 2, 2021 03:47

WIP

8844951

Signed-off-by: Ryan Hamilton <rch@google.com>

Tests pass

f618ce6

Signed-off-by: Ryan Hamilton <rch@google.com>

format

f580f4e

Signed-off-by: Ryan Hamilton <rch@google.com>

format

485688c

Signed-off-by: Ryan Hamilton <rch@google.com>

repokitteh-read-only bot assigned mattklein123 Sep 2, 2021

repokitteh-read-only bot assigned ggreenway Sep 2, 2021

Fix rebase issue

2f2a280

Signed-off-by: Ryan Hamilton <rch@google.com>

ggreenway requested changes Sep 2, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 2, 2021

Fix comments from ggreenway

179cdd4

Signed-off-by: Ryan Hamilton <rch@google.com>

repokitteh-read-only bot removed the waiting label Sep 2, 2021

RyanTheOptimist commented Sep 2, 2021

View reviewed changes

ggreenway previously approved these changes Sep 2, 2021

View reviewed changes

mattklein123 reviewed Sep 2, 2021

View reviewed changes

repokitteh-read-only bot added the waiting:any label Sep 2, 2021

Add ASSERT and fix test

8d5f90e

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist dismissed ggreenway’s stale review via 8d5f90e September 2, 2021 18:08

repokitteh-read-only bot removed the waiting:any label Sep 2, 2021

rename is_draining_ => is_draining_for_deletion_

ea6bf0f

Signed-off-by: Ryan Hamilton <rch@google.com>

repokitteh-read-only bot added the waiting label Sep 3, 2021

format

4307372

Signed-off-by: Ryan Hamilton <rch@google.com>

repokitteh-read-only bot removed the waiting label Sep 3, 2021

mattklein123 previously approved these changes Sep 3, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 3, 2021

Tidy

606f3cf

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist dismissed mattklein123’s stale review via 606f3cf September 4, 2021 00:34

repokitteh-read-only bot removed the waiting label Sep 4, 2021

mattklein123 approved these changes Sep 7, 2021

View reviewed changes

mattklein123 merged commit 84efbaf into envoyproxy:main Sep 7, 2021

tyxia pushed a commit to tyxia/envoy that referenced this pull request Sep 21, 2021

conn_pool: Remove startDrain() and replace it with an argument to dra…

77a877f

…inConnections (envoyproxy#17960) Signed-off-by: Ryan Hamilton <rch@google.com>

Conversation

RyanTheOptimist commented Sep 2, 2021

Uh oh!

RyanTheOptimist commented Sep 2, 2021

Uh oh!

RyanTheOptimist commented Sep 2, 2021

Uh oh!

RyanTheOptimist commented Sep 2, 2021

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Sep 3, 2021

Uh oh!

RyanTheOptimist commented Sep 3, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Sep 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants