server: add --enable-core-dump flag by rgs1 · Pull Request #15245 · envoyproxy/envoy

rgs1 · 2021-03-01T17:24:39Z

This ensures Envoy can core dump when the dumpability bit might have
been unset (e.g.: running inside a container with fewer capabilities
than the ones Envoy itself has).

Fixes #15242.

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com

This ensures Envoy can core dump when the dumpability bit might have been unset (e.g.: running inside a container with fewer capabilities than the ones Envoy itself has). Fixes envoyproxy#15242. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only · 2021-03-01T17:24:45Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15245 was opened by rgs1.

see: more, trace.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2021-03-01T19:08:22Z

I've got --enable-core-dump running internally, works great 👌

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

davinci26 · 2021-03-01T20:41:01Z

@rgs1 can you open an issue or add in the existing issue a Windows label so that we know that we have to implement this feature on windows

rgs1 · 2021-03-01T20:46:08Z

@rgs1 can you open an issue or add in the existing issue a Windows label so that we know that we have to implement this feature on windows

What's the exact problem for Windows? Afaik Windows doesn't have capabilities nor a dumpability bit...

rgs1 · 2021-03-01T20:50:51Z

@rgs1 can you open an issue or add in the existing issue a Windows label so that we know that we have to implement this feature on windows

What's the exact problem for Windows? Afaik Windows doesn't have capabilities nor a dumpability bit...

Which makes me wonder if we should hide via ifdefs the --enable-core-dump command line flag for Windows (and other platforms) without capabilities and this core dumping issue...

cc: @mattklein123 for thoughts

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

davinci26 · 2021-03-01T21:16:22Z

What's the exact problem for Windows? Afaik Windows doesn't have capabilities nor a dumpability bit...

We have flag that is not working for one platform so we should probably document it.

Additionally windows can generate memory dumps from failing applications https://docs.microsoft.com/en-us/windows/client-management/generate-kernel-or-complete-crash-dump (the same applies for user mode applications)

edit: I am not sure if it makes sense to push this functionality to envoy codebase or to the docker image ( see docs). IMHO the latter makes sense more to me but either one should be doable

rgs1 · 2021-03-01T21:35:18Z

What's the exact problem for Windows? Afaik Windows doesn't have capabilities nor a dumpability bit...

We have flag that is not working for one platform so we should probably document it.

Yeah I agree, I actually wonder if we should hide the flag entirely from platforms for which it isn't implemented.

Additionally windows can generate memory dumps from failing applications https://docs.microsoft.com/en-us/windows/client-management/generate-kernel-or-complete-crash-dump (the same applies for user mode applications)

Ok, but is this already working out of the box for Windows or is there anything else to be done? In fairness, this --enable-core-dump is not the entire truth for Linux given that it still won't be enough if you didn't set ulimit correctly and furthermore it isn't really needed if you didn't do anything that won't unset the dumpable bit of the process. Hmm, maybe it should be named --enable-dumpable-bit to be more precise.

edit: I am not sure if it makes sense to push this functionality to envoy codebase or to the docker image ( see docs). IMHO the latter makes sense more to me but either one should be doable

You are talking about the Windows case not this PR for the Linux case?

davinci26 · 2021-03-01T21:37:27Z

You are talking about the Windows case not this PR for the Linux case?

Windows case

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

mattklein123

Yeah I agree that if an option only applies to certain platforms we should document it as such and only expose it on those platforms, optimally without a ton of different ifdefs (e.g. using platform capabilities). Thanks for working on this.

/wait

docs/root/operations/cli.rst

* fix/improve cli doc * introduce linux/platform_impl.cc for Linux specific prctl calls * enabling core dump now happens in MainCommonBase instead of InstanceImpl, because we need access to PlatformImpl * also, move PlatformImpl from MainCommon to MainCommonBase * added some bazel helpers to distinguish Linux from the rest of Posix Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Remove outer calls to envoy_cc_platform_dep, which means we cannot properly pick the Linux platform. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

This should fix the coverage issue and catch any regressions if our kernel friends were to ever break prctl(). Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

* check that enableCoreDump() was called * check the logs for the expected warn entry Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

This probably is enough and we don't need to additionally use `config_settings_group` to group things further. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2021-03-05T14:16:47Z

/retest

repokitteh-read-only · 2021-03-05T14:16:50Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15245 (comment) was created by @rgs1.

see: more, trace.

rgs1 · 2021-03-05T14:17:43Z

/retest

repokitteh-read-only · 2021-03-05T14:17:46Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15245 (comment) was created by @rgs1.

see: more, trace.

Keep ref instead of copying. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

mattklein123

Thanks LGTM with small comment.

/wait

test/exe/main_common_test.cc

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

mattklein123

Thanks!

repokitteh-read-only bot added the api label Mar 1, 2021

repokitteh-read-only bot assigned lizan Mar 1, 2021

Fix docs

82e1ceb

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Fix windows

906aaa5

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Skip prctl for OS X too

1b85c69

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Check prctl's return value and log accordingly

57e810f

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

mattklein123 requested changes Mar 2, 2021

View reviewed changes

docs/root/operations/cli.rst Outdated Show resolved Hide resolved

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

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

Raul Gutierrez Segales added 11 commits March 2, 2021 12:07

Fix bazel helpers

aa46afc

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Fix selectors

193bae6

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Update envoy_cc_posix_without_linux_library

2896729

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Fix CI

7c831e3

Remove outer calls to envoy_cc_platform_dep, which means we cannot properly pick the Linux platform. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Info log when prctl succeeds

d120e3e

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Fix format

fd4f178

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Improve changelog

61a490c

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Merge remote-tracking branch 'upstream/main' into prctl-set-dumpable

9474964

Merge remote-tracking branch 'upstream/main' into prctl-set-dumpable

5373bd2

Add test for enableCoreDump()

2276fce

This should fix the coverage issue and catch any regressions if our kernel friends were to ever break prctl(). Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Merge remote-tracking branch 'upstream/main' into prctl-set-dumpable

803bb02

repokitteh-read-only bot added the waiting label Mar 4, 2021

Raul Gutierrez Segales added 3 commits March 4, 2021 16:40

unique_ptr

d0532e2

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Merge remote-tracking branch 'upstream/main' into prctl-set-dumpable

4f89e2e

Improve test

641e72b

* check that enableCoreDump() was called * check the logs for the expected warn entry Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

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

Raul Gutierrez Segales added 7 commits March 4, 2021 17:38

Fixes

e6abf4b

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Fix format

bdefe7a

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Merge remote-tracking branch 'upstream/main' into prctl-set-dumpable

042277e

Drop the per architecture match, since it's not needed

e9128dd

This probably is enough and we don't need to additionally use `config_settings_group` to group things further. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Fix format

21da012

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Merge remote-tracking branch 'upstream/main' into prctl-set-dumpable

ef02146

Fix

583bf9e

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Raul Gutierrez Segales added 5 commits March 5, 2021 09:20

Fix test

938e4dd

Keep ref instead of copying. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Merge remote-tracking branch 'upstream/main' into prctl-set-dumpable

db5045c

prctl() will only work on Linux, so use an ifdef

fcb514c

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Be more consistent in the use of ifdefs

37f19aa

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Fix clang tidy

b0026f4

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

mattklein123 requested changes Mar 5, 2021

View reviewed changes

test/exe/main_common_test.cc Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Mar 5, 2021

Use a proper mock method

0790c5e

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only bot removed the waiting label Mar 5, 2021

mattklein123 approved these changes Mar 5, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Mar 5, 2021

lizan approved these changes Mar 5, 2021

View reviewed changes

mattklein123 merged commit 4f1ec4f into envoyproxy:main Mar 6, 2021

Conversation

rgs1 commented Mar 1, 2021

Uh oh!

repokitteh-read-only bot commented Mar 1, 2021

Uh oh!

rgs1 commented Mar 1, 2021

Uh oh!

davinci26 commented Mar 1, 2021

Uh oh!

rgs1 commented Mar 1, 2021

Uh oh!

rgs1 commented Mar 1, 2021

Uh oh!

davinci26 commented Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgs1 commented Mar 1, 2021

Uh oh!

davinci26 commented Mar 1, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rgs1 commented Mar 5, 2021

Uh oh!

repokitteh-read-only bot commented Mar 5, 2021

Uh oh!

rgs1 commented Mar 5, 2021

Uh oh!

repokitteh-read-only bot commented Mar 5, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davinci26 commented Mar 1, 2021 •

edited

Loading