drain: give 5s (configurable) time for existing connections to gracefully close #1157
Conversation
howardjohn
added
the
release-notes-none
istio-testing
added
the
size/M
There was a problem hiding this comment.
Since we can't actually signal conn termination in either of these cases, this remains incredibly optimistic and in a lot of cases is probably just a waste of cycles - connection is just as likely to self-terminate in 48 hours as it is in 5 mins, from our perspective.
Doesn't hurt tho.
| @@ -64,21 +66,36 @@ impl InboundPassthrough { | |||
| } | |||
|
|
|||
| pub(super) async fn run(self) { | |||
| let (sub_drain_signal, sub_drain) = drain::channel(); | |||
| let deadline = self.pi.cfg.self_termination_deadline; | |||
There was a problem hiding this comment.
At some point we may want separate vars rather than reusing this for everything - it's overloaded with different meanings already.
Also, we probably should have a more robust form of shutdown signaling with a single channel, rather than creating 2-3 different shutdown channels and using them in concert.
That's probably better handled as part of #899 tho.
There is IMO a huge difference between 0 and 5s: with 0s, even short lived connections are broken. if I just curl httpbin.org, its taking ~1s. Without this change it fails, with it it succeeds |
If that is behavior we care about, it would probably be best to fence it with a unit test so it doesn't get dropped, and you don't have to manually/personally make sure it's still there 6 months from now. |
|
Yep I am working on improving tests here
…On Tue, Jun 18, 2024 at 8:28 PM Ben Leggett ***@***.***> wrote:
Since we can't actually signal conn termination in either of these cases,
this remains incredibly optimistic and in a lot of cases is probably just a
waste of cycles - connection is just as likely to self-terminate in 48
hours as it is in 5 mins, from our perspective.
There is IMO a huge difference between 0 and 5s: with 0s, even short lived
connections are broken.
if I just curl httpbin.org, its taking ~1s. Without this change it fails,
with it it succeeds
If that is behavior we care about, it would probably be best to fence it
with a unit test so it doesn't get dropped, and you don't have to
manually/personally make sure it's still there 6 months from now.
—
Reply to this email directly, view it on GitHub
<#1157 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEYGXOHH7PI4PQL35SNYHDZID3F7AVCNFSM6AAAAABJQOUQMWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZXGQ4DSNZUGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
This was removed in fa7b1aa#diff-22c9338ddb21f85ab7b656447d453c0d0e0644c647054ca18c4d7d6d43fc8716R100-R113