Skip to content

admin: support /drain_listeners?skip_exit#30003

Merged
kyessenov merged 2 commits intoenvoyproxy:mainfrom
howardjohn:admin/skip-exit-drain
Oct 18, 2023
Merged

admin: support /drain_listeners?skip_exit#30003
kyessenov merged 2 commits intoenvoyproxy:mainfrom
howardjohn:admin/skip-exit-drain

Conversation

@howardjohn
Copy link
Copy Markdown
Contributor

Commit Message: admin: support /drain_listeners?skip_exit
Additional Description:
Due to changes in Kubernetes (https://kubernetes.io/blog/2023/08/25/native-sidecar-containers/) around sidecars, there is a more optimal way to manage the lifecycle of Envoy as a sidecar in Kubernetes. However, it is not currently possible with the current knobs.

Our goal:

  1. When the pod starts terminating, start "draining" inbound connections. Accept new ones, but respond with GOAWAY/connection=close to discourage clients
  2. In parallel (outside of envoy), clients should be discourage to making new connections generally
  3. Once the application container shuts down, we have nothing left to do; as a sidecar proxy, we exist only to serve the application container. Shutdown ~immediately

The time between (1) and (3) can be quite large (but not unbounded; Kubernetes has a max time before it will SIGKILL everything).

How we want to implement this:

  1. When pod starts terminating, /drain_listeners?graceful&skip_exit
  2. When application container shutsdown, SIGTERM Envoy.

The reason we need a skip_exit is due to the timing between the two. If we just do this without skip_exit if the application container takes > drain-time-s we will block new connections unexpectedly. While we could just raise the drain-time, this also impacts other draining (notably, LDS update), which may be unexpected.

Risk Level: Low
Testing: Added new integration test based on existing ones around draining
Docs Changes: Add docs to admin.rst

soulxu
soulxu previously approved these changes Oct 13, 2023
Copy link
Copy Markdown
Member

@soulxu soulxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except main merge needed, thanks

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad merge? Can you remove this line?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! done

Copy link
Copy Markdown
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please update the change notes.

@howardjohn howardjohn force-pushed the admin/skip-exit-drain branch from 31adab8 to b80434f Compare October 13, 2023 16:15
kyessenov
kyessenov previously approved these changes Oct 13, 2023
@kyessenov kyessenov enabled auto-merge (squash) October 13, 2023 16:26
@kyessenov
Copy link
Copy Markdown
Contributor

@howardjohn you need to fix the format. Looks like some white space issue by your editor. Can you run bazel run //tools/code_format:check_format -- fix and apply the fixes?

auto-merge was automatically disabled October 16, 2023 17:12

Head branch was pushed to by a user without write access

@howardjohn
Copy link
Copy Markdown
Contributor Author

@howardjohn you need to fix the format. Looks like some white space issue by your editor. Can you run bazel run //tools/code_format:check_format -- fix and apply the fixes?

Ok I don;t really understand why me adding 1 line means the entire file should reformat, but done. Hope that works

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a true compilation problem here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I finally got it... thanks for the patience 🙂

@howardjohn howardjohn force-pushed the admin/skip-exit-drain branch from ae4a409 to e4836d0 Compare October 16, 2023 18:53
Signed-off-by: John Howard <howardjohn@google.com>
@howardjohn howardjohn force-pushed the admin/skip-exit-drain branch from e4836d0 to d1866ef Compare October 16, 2023 22:14
@kyessenov
Copy link
Copy Markdown
Contributor

Can you resolve the conflict? Also, please don't force push.

Signed-off-by: John Howard <howardjohn@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants